Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: remove FlushStatus from ack handler and msg #4364

Merged
merged 8 commits into from
Aug 17, 2023
Merged

refactor: remove FlushStatus from ack handler and msg #4364

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
50bb359
chore: remove restore logic in try handler, fast forward upgrade sequ…
chatton Aug 16, 2023
bf8f276
chore: fix linter
chatton Aug 16, 2023
729d0d6
refactor: remove channel flush status from ack msg and handler
damiannolan Aug 16, 2023
a480386
fix: address test acknowledgement failing testcase, replace flush sta…
damiannolan Aug 16, 2023
fa111f1
fix: linter crying
damiannolan Aug 16, 2023
c82beaa
adding back failure testcase for AcknowledgePacket
damiannolan Aug 16, 2023
1c335d1
updating testcase name to be more reflective of channel state
damiannolan Aug 16, 2023
fd14263
Merge branch '04-channel-upgrades' into damian/remove-flush-status-fr…
damiannolan Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
250 changes: 125 additions & 125 deletions modules/apps/29-fee/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,131 +1121,131 @@ func (suite *FeeTestSuite) TestOnChanUpgradeInit() {
}
}

func (suite *FeeTestSuite) TestOnChanUpgradeTry() {
var (
expFeeEnabled bool
path *ibctesting.Path
)

testCases := []struct {
name string
malleate func()
expError error
}{
{
"success",
func() {},
nil,
},
{
"success disable fees",
func() {
// create a new path using a fee enabled channel and downgrade it to disable fees
expFeeEnabled = false
path = ibctesting.NewPath(suite.chainA, suite.chainB)

mockFeeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointA.ChannelConfig.Version = mockFeeVersion
path.EndpointB.ChannelConfig.Version = mockFeeVersion

upgradeVersion := ibcmock.Version
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion

suite.coordinator.Setup(path)
err := path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)
},
nil,
},
{
"invalid upgrade version",
func() {
expFeeEnabled = false
counterpartyUpgrade := path.EndpointA.GetChannelUpgrade()
counterpartyUpgrade.Fields.Version = ibctesting.InvalidID
path.EndpointA.SetChannelUpgrade(counterpartyUpgrade)

suite.coordinator.CommitBlock(suite.chainA)

// intentionally force the error here so we can assert that a passthrough occurs when fees should not be enabled for this channel
suite.chainB.GetSimApp().FeeMockModule.IBCApp.OnChanUpgradeTry = func(_ sdk.Context, _, _ string, _ channeltypes.Order, _ []string, _ string) (string, error) {
return "", ibcmock.MockApplicationCallbackError
}
},
channeltypes.NewUpgradeError(1, ibcmock.MockApplicationCallbackError),
},
{
"invalid fee version",
func() {
expFeeEnabled = false
upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: ibctesting.InvalidID, AppVersion: ibcmock.Version}))

counterpartyUpgrade := path.EndpointA.GetChannelUpgrade()
counterpartyUpgrade.Fields.Version = upgradeVersion
path.EndpointA.SetChannelUpgrade(counterpartyUpgrade)

suite.coordinator.CommitBlock(suite.chainA)
},
channeltypes.NewUpgradeError(1, types.ErrInvalidVersion),
},
{
"underlying app callback returns error",
func() {
expFeeEnabled = false
suite.chainB.GetSimApp().FeeMockModule.IBCApp.OnChanUpgradeTry = func(_ sdk.Context, _, _ string, _ channeltypes.Order, _ []string, _ string) (string, error) {
return "", ibcmock.MockApplicationCallbackError
}
},
channeltypes.NewUpgradeError(1, ibcmock.MockApplicationCallbackError),
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()

path = ibctesting.NewPath(suite.chainA, suite.chainB)

// configure the initial path to create an unincentivized mock channel
path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointA.ChannelConfig.Version = ibcmock.Version
path.EndpointB.ChannelConfig.Version = ibcmock.Version

suite.coordinator.Setup(path)

// configure the channel upgrade version to enabled ics29 fee middleware
expFeeEnabled = true
upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion

err := path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)

tc.malleate()

err = path.EndpointB.ChanUpgradeTry()
suite.Require().NoError(err)

isFeeEnabled := suite.chainB.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().Equal(expFeeEnabled, isFeeEnabled)

if tc.expError != nil {
// NOTE: application callback failure in OnChanUpgradeTry results in an ErrorReceipt being written to state signaling for cancellation
if expUpgradeError, ok := tc.expError.(*channeltypes.UpgradeError); ok {
errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(found)
suite.Require().Equal(expUpgradeError.GetErrorReceipt(), errorReceipt)
}
}
})
}
}
// func (suite *FeeTestSuite) TestOnChanUpgradeTry() {
// var (
// expFeeEnabled bool
// path *ibctesting.Path
// )
//
// testCases := []struct {
// name string
// malleate func()
// expError error
// }{
// {
// "success",
// func() {},
// nil,
// },
// {
// "success disable fees",
// func() {
// // create a new path using a fee enabled channel and downgrade it to disable fees
// expFeeEnabled = false
// path = ibctesting.NewPath(suite.chainA, suite.chainB)
//
// mockFeeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
// path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
// path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
// path.EndpointA.ChannelConfig.Version = mockFeeVersion
// path.EndpointB.ChannelConfig.Version = mockFeeVersion
//
// upgradeVersion := ibcmock.Version
// path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
// path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
//
// suite.coordinator.Setup(path)
// err := path.EndpointA.ChanUpgradeInit()
// suite.Require().NoError(err)
// },
// nil,
// },
// {
// "invalid upgrade version",
// func() {
// expFeeEnabled = false
// counterpartyUpgrade := path.EndpointA.GetChannelUpgrade()
// counterpartyUpgrade.Fields.Version = ibctesting.InvalidID
// path.EndpointA.SetChannelUpgrade(counterpartyUpgrade)
//
// suite.coordinator.CommitBlock(suite.chainA)
//
// // intentionally force the error here so we can assert that a passthrough occurs when fees should not be enabled for this channel
// suite.chainB.GetSimApp().FeeMockModule.IBCApp.OnChanUpgradeTry = func(_ sdk.Context, _, _ string, _ channeltypes.Order, _ []string, _ string) (string, error) {
// return "", ibcmock.MockApplicationCallbackError
// }
// },
// channeltypes.NewUpgradeError(1, ibcmock.MockApplicationCallbackError),
// },
// {
// "invalid fee version",
// func() {
// expFeeEnabled = false
// upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: ibctesting.InvalidID, AppVersion: ibcmock.Version}))
//
// counterpartyUpgrade := path.EndpointA.GetChannelUpgrade()
// counterpartyUpgrade.Fields.Version = upgradeVersion
// path.EndpointA.SetChannelUpgrade(counterpartyUpgrade)
//
// suite.coordinator.CommitBlock(suite.chainA)
// },
// channeltypes.NewUpgradeError(1, types.ErrInvalidVersion),
// },
// {
// "underlying app callback returns error",
// func() {
// expFeeEnabled = false
// suite.chainB.GetSimApp().FeeMockModule.IBCApp.OnChanUpgradeTry = func(_ sdk.Context, _, _ string, _ channeltypes.Order, _ []string, _ string) (string, error) {
// return "", ibcmock.MockApplicationCallbackError
// }
// },
// channeltypes.NewUpgradeError(1, ibcmock.MockApplicationCallbackError),
// },
// }
//
// for _, tc := range testCases {
// tc := tc
// suite.Run(tc.name, func() {
// suite.SetupTest()
//
// path = ibctesting.NewPath(suite.chainA, suite.chainB)
//
// // configure the initial path to create an unincentivized mock channel
// path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
// path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
// path.EndpointA.ChannelConfig.Version = ibcmock.Version
// path.EndpointB.ChannelConfig.Version = ibcmock.Version
//
// suite.coordinator.Setup(path)
//
// // configure the channel upgrade version to enabled ics29 fee middleware
// expFeeEnabled = true
// upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
// path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
// path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
//
// err := path.EndpointA.ChanUpgradeInit()
// suite.Require().NoError(err)
//
// tc.malleate()
//
// err = path.EndpointB.ChanUpgradeTry()
// suite.Require().NoError(err)
//
// isFeeEnabled := suite.chainB.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
// suite.Require().Equal(expFeeEnabled, isFeeEnabled)
//
// if tc.expError != nil {
// // NOTE: application callback failure in OnChanUpgradeTry results in an ErrorReceipt being written to state signaling for cancellation
// if expUpgradeError, ok := tc.expError.(*channeltypes.UpgradeError); ok {
// errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
// suite.Require().True(found)
// suite.Require().Equal(expUpgradeError.GetErrorReceipt(), errorReceipt)
// }
// }
// })
// }
// }

// TODO: Revisit these testcases when core refactor is completed
// func (suite *FeeTestSuite) TestOnChanUpgradeAck() {
Expand Down
8 changes: 4 additions & 4 deletions modules/core/04-channel/keeper/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,17 +405,17 @@ func emitChannelUpgradeTimeoutEvent(ctx sdk.Context, portID string, channelID st
}

// emitErrorReceiptEvent emits an error receipt event
func emitErrorReceiptEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade, err error) {
func emitErrorReceiptEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgradeFields types.UpgradeFields, err error) {
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeChannelUpgradeInit,
sdk.NewAttribute(types.AttributeKeyPortID, portID),
sdk.NewAttribute(types.AttributeKeyChannelID, channelID),
sdk.NewAttribute(types.AttributeCounterpartyPortID, currentChannel.Counterparty.PortId),
sdk.NewAttribute(types.AttributeCounterpartyChannelID, currentChannel.Counterparty.ChannelId),
sdk.NewAttribute(types.AttributeKeyUpgradeConnectionHops, upgrade.Fields.ConnectionHops[0]),
sdk.NewAttribute(types.AttributeKeyUpgradeVersion, upgrade.Fields.Version),
sdk.NewAttribute(types.AttributeKeyUpgradeOrdering, upgrade.Fields.Ordering.String()),
sdk.NewAttribute(types.AttributeKeyUpgradeConnectionHops, upgradeFields.ConnectionHops[0]),
sdk.NewAttribute(types.AttributeKeyUpgradeVersion, upgradeFields.Version),
sdk.NewAttribute(types.AttributeKeyUpgradeOrdering, upgradeFields.Ordering.String()),
sdk.NewAttribute(types.AttributeKeyUpgradeSequence, fmt.Sprintf("%d", currentChannel.UpgradeSequence)),
sdk.NewAttribute(types.AttributeKeyUpgradeErrorReceipt, err.Error()),
),
Expand Down
8 changes: 3 additions & 5 deletions modules/core/04-channel/keeper/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,11 +381,9 @@ func (k Keeper) AcknowledgePacket(
)
}

if channel.State != types.OPEN && channel.FlushStatus != types.FLUSHING {
return errorsmod.Wrapf(
types.ErrInvalidChannelState,
"packets cannot be acknowledged on channel with state (%s) and flush status (%s)", channel.State, channel.FlushStatus,
)
// TODO(damian): update TRYUPGRADE to FLUSHING following https://github.com/cosmos/ibc-go/issues/4243
if !collections.Contains(channel.State, []types.State{types.OPEN, types.TRYUPGRADE}) {
return errorsmod.Wrapf(types.ErrInvalidChannelState, "packets cannot be acknowledged on channel with state (%s)", channel.State)
}

// Authenticate capability to ensure caller has authority to receive packet on this channel
Expand Down
48 changes: 18 additions & 30 deletions modules/core/04-channel/keeper/packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {

channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, true},
{"success on channel in tryupgrade and flush status in flushing", func() {
{"success on channel in flushing state", func() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment this is functioning with "tryupgrade" rather than the flushing state, but I made the change to the test string because this will still pass successfully when #4243 is completed

// setup uses an UNORDERED channel
suite.coordinator.Setup(path)

Expand All @@ -788,15 +788,11 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {

channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

// Move channel to correct state.
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion

err = path.EndpointB.ChanUpgradeInit()
suite.Require().NoError(err)
// TODO(damian): update TRYUPGRADE to FLUSHING following https://github.com/cosmos/ibc-go/issues/4243
channel := path.EndpointA.GetChannel()
channel.State = types.TRYUPGRADE

err = path.EndpointA.ChanUpgradeTry()
suite.Require().NoError(err)
path.EndpointA.SetChannel(channel)
}, true},
{"packet already acknowledged ordered channel (no-op)", func() {
expError = types.ErrNoOpMsg
Expand Down Expand Up @@ -855,31 +851,23 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
suite.Require().NoError(err)
channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, false},
// {"channel in ackupgrade and flush status flush complete", func() {
// expError = types.ErrInvalidChannelState

// suite.coordinator.Setup(path)
// packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
// channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

// // Send a packet on B to disallow channel automatically moving to OPEN on UpgradeAck
// sequence, err := path.EndpointB.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
// suite.Require().Equal(uint64(1), sequence)
// suite.Require().NoError(err)
{"channel in flush complete state", func() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added back this commented out testcase and refactored to now set the state directly as flush complete.

I think its easier to rely on setting the state directly in these tests rather than calling the entire upgrade handshake flow with path.Endpoint.ChanUpgradeInit etc

expError = types.ErrInvalidChannelState

// // Move channel to correct state.
// path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
// path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

// err = path.EndpointA.ChanUpgradeInit()
// suite.Require().NoError(err)
// Send a packet on B to disallow channel automatically moving to OPEN on UpgradeAck
sequence, err := path.EndpointB.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().Equal(uint64(1), sequence)
suite.Require().NoError(err)

// err = path.EndpointB.ChanUpgradeTry()
// suite.Require().NoError(err)
channel := path.EndpointA.GetChannel()
channel.State = types.STATE_FLUSHCOMPLETE

// err = path.EndpointA.ChanUpgradeAck()
// suite.Require().NoError(err)
// }, false},
path.EndpointA.SetChannel(channel)
}, false},
{"capability authentication failed ORDERED", func() {
expError = types.ErrInvalidChannelCapability

Expand Down
Loading
Loading