diff --git a/docs/docs/01-ibc/06-channel-upgrades.md b/docs/docs/01-ibc/06-channel-upgrades.md index 27caaef6cf4..725c7162ebf 100644 --- a/docs/docs/01-ibc/06-channel-upgrades.md +++ b/docs/docs/01-ibc/06-channel-upgrades.md @@ -156,8 +156,6 @@ It is possible for a relayer to cancel an in-progress channel upgrade if the fol Upon cancelling a channel upgrade, an `ErrorReceipt` will be written with the channel's current upgrade sequence, and the channel will move back to `OPEN` state keeping its original parameters. -The application's `OnChanUpgradeRestore` callback method will be invoked. - It will then be possible to re-initiate an upgrade by sending a `MsgChannelOpenInit` message. ## Timing Out a Channel Upgrade @@ -208,8 +206,6 @@ type MsgChannelUpgradeTimeout struct { An `ErrorReceipt` will be written with the channel's current upgrade sequence, and the channel will move back to `OPEN` state keeping its original parameters. -The application's `OnChanUpgradeRestore` callback method will also be invoked. - Note that timing out a channel upgrade will end the upgrade process, and a new `MsgChannelUpgradeInit` will have to be submitted via governance in order to restart the upgrade process. ## Pruning Acknowledgements @@ -256,8 +252,6 @@ should be aware that callbacks will be invoked before any core ibc state changes `OnChanUpgradeOpen` should perform any logic associated with changing of the channel fields. -`OnChanUpgradeRestore` should perform any logic that needs to be executed when an upgrade attempt fails as is reverted. - > IBC applications should not attempt to process any packet data under the new conditions until after `OnChanUpgradeOpen` > has been executed, as up until this point it is still possible for the upgrade handshake to fail and for the channel > to remain in the pre-upgraded state. diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index ae05b7476b2..fe9ef65a026 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -309,23 +309,6 @@ func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID str } } -// OnChanUpgradeRestore implements the IBCModule interface -func (im IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) { - connectionID, err := im.keeper.GetConnectionID(ctx, portID, channelID) - if err != nil { - panic(err) - } - - if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, connectionID) { - // Only cast to UpgradableModule if the application is set. - cbs, ok := im.app.(porttypes.UpgradableModule) - if !ok { - panic(errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack")) - } - cbs.OnChanUpgradeRestore(ctx, portID, channelID) - } -} - // SendPacket implements the ICS4 Wrapper interface func (IBCMiddleware) SendPacket( ctx sdk.Context, diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go index 1c7925e1842..93610b452ff 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -1066,67 +1066,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeOpen() { } } -func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeRestore() { - var ( - path *ibctesting.Path - isNilApp bool - ) - - testCases := []struct { - name string - malleate func() - }{ - { - "success", func() {}, - }, - { - "success: nil app", - func() { - isNilApp = true - }, - }, - { - "middleware disabled", func() { - suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ConnectionID) - suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeAck = func(ctx sdk.Context, portID, channelID string, counterpartyVersion string) error { - return ibcmock.MockApplicationCallbackError - } - }, - }, - } - - for _, tc := range testCases { - tc := tc - - suite.Run(tc.name, func() { - suite.SetupTest() // reset - isNilApp = false - - path = NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) - - err := SetupICAPath(path, TestOwnerAddress) - suite.Require().NoError(err) - - tc.malleate() // malleate mutates test data - - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) - suite.Require().NoError(err) - - app, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) - suite.Require().True(ok) - cbs, ok := app.(porttypes.UpgradableModule) - suite.Require().True(ok) - - if isNilApp { - cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper) - } - - cbs.OnChanUpgradeRestore(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - }) - } -} - func (suite *InterchainAccountsTestSuite) TestSingleHostMultipleControllers() { var ( pathAToB *ibctesting.Path diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index 0ea90d4cddf..622bf284bd9 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module.go @@ -180,9 +180,6 @@ func (IBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpar func (IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, proposedVersion string) { } -// OnChanUpgradeRestore implements the IBCModule interface -func (IBCModule) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {} - // UnmarshalPacketData attempts to unmarshal the provided packet data bytes // into an InterchainAccountPacketData. This function implements the optional // PacketDataUnmarshaler interface required for ADR 008 support. diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index becaa10b2ca..4ac1e7dd989 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -440,16 +440,6 @@ func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID str cbs.OnChanUpgradeOpen(ctx, portID, channelID, proposedOrder, proposedConnectionHops, versionMetadata.AppVersion) } -// OnChanUpgradeRestore implements the IBCModule interface -func (im IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) { - cbs, ok := im.app.(porttypes.UpgradableModule) - if !ok { - panic(errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack")) - } - - cbs.OnChanUpgradeRestore(ctx, portID, channelID) -} - // SendPacket implements the ICS4 Wrapper interface func (im IBCMiddleware) SendPacket( ctx sdk.Context, diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go index d43ab06cff6..1f1da0b6cde 100644 --- a/modules/apps/callbacks/ibc_middleware.go +++ b/modules/apps/callbacks/ibc_middleware.go @@ -404,16 +404,6 @@ func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID str cbs.OnChanUpgradeOpen(ctx, portID, channelID, proposedOrder, proposedConnectionHops, proposedVersion) } -// OnChanUpgradeRestore implements the IBCModule interface -func (im IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) { - cbs, ok := im.app.(porttypes.UpgradableModule) - if !ok { - panic(errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack")) - } - - cbs.OnChanUpgradeRestore(ctx, portID, channelID) -} - // GetAppVersion implements the ICS4Wrapper interface. Callbacks has no version, // so the call is deferred to the underlying application. func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool) { diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 28ae28d8f65..b73dc6e8edb 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -347,9 +347,6 @@ func (IBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpar func (IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, proposedVersion string) { } -// OnChanUpgradeRestore implements the IBCModule interface -func (IBCModule) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {} - // UnmarshalPacketData attempts to unmarshal the provided packet data bytes // into a FungibleTokenPacketData. This function implements the optional // PacketDataUnmarshaler interface required for ADR 008 support. diff --git a/modules/core/05-port/types/module.go b/modules/core/05-port/types/module.go index 77b6ae6fad7..23fc5705233 100644 --- a/modules/core/05-port/types/module.go +++ b/modules/core/05-port/types/module.go @@ -112,7 +112,8 @@ type IBCModule interface { type UpgradableModule interface { // OnChanUpgradeInit enables additional custom logic to be executed when the channel upgrade is initialized. // It must validate the proposed version, order, and connection hops. - // Note: in the case of crossing hellos, this callback may be executed on both chains. + // NOTE: in the case of crossing hellos, this callback may be executed on both chains. + // NOTE: Any IBC application state changes made in this callback handler are not committed. OnChanUpgradeInit( ctx sdk.Context, portID, channelID string, @@ -124,6 +125,7 @@ type UpgradableModule interface { // OnChanUpgradeTry enables additional custom logic to be executed in the ChannelUpgradeTry step of the // channel upgrade handshake. It must validate the proposed version (provided by the counterparty), order, // and connection hops. + // NOTE: Any IBC application state changes made in this callback handler are not committed. OnChanUpgradeTry( ctx sdk.Context, portID, channelID string, @@ -134,6 +136,7 @@ type UpgradableModule interface { // OnChanUpgradeAck enables additional custom logic to be executed in the ChannelUpgradeAck step of the // channel upgrade handshake. It must validate the version proposed by the counterparty. + // NOTE: Any IBC application state changes made in this callback handler are not committed. OnChanUpgradeAck( ctx sdk.Context, portID, @@ -152,17 +155,6 @@ type UpgradableModule interface { proposedConnectionHops []string, proposedVersion string, ) - - // OnChanUpgradeRestore enables additional custom logic to be executed when any of the following occur: - // - the channel upgrade is aborted. - // - the channel upgrade is cancelled. - // - the channel upgrade times out. - // Any logic set due to an upgrade attempt should be reverted in this callback. - OnChanUpgradeRestore( - ctx sdk.Context, - portID, - channelID string, - ) } // ICS4Wrapper implements the ICS4 interfaces that IBC applications use to send packets and acknowledgements. diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 202411c7fcc..f99ce0b3a82 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -769,14 +769,10 @@ func (k Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.MsgC return nil, errorsmod.Wrap(err, "channel upgrade init failed") } - upgradeVersion, err := cbs.OnChanUpgradeInit( - ctx, - msg.PortId, - msg.ChannelId, - upgrade.Fields.Ordering, - upgrade.Fields.ConnectionHops, - upgrade.Fields.Version, - ) + // NOTE: a cached context is used to discard ibc application state changes and events. + // IBC applications must flush in-flight packets using the pre-upgrade channel parameters. + cacheCtx, _ := ctx.CacheContext() + upgradeVersion, err := cbs.OnChanUpgradeInit(cacheCtx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version) if err != nil { ctx.Logger().Error("channel upgrade init callback failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", err.Error()) return nil, errorsmod.Wrapf(err, "channel upgrade init callback failed for port ID: %s, channel ID: %s", msg.PortId, msg.ChannelId) @@ -829,7 +825,10 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh return nil, errorsmod.Wrap(err, "channel upgrade try failed") } - upgradeVersion, err := cbs.OnChanUpgradeTry(ctx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version) + // NOTE: a cached context is used to discard ibc application state changes and events. + // IBC applications must flush in-flight packets using the pre-upgrade channel parameters. + cacheCtx, _ := ctx.CacheContext() + upgradeVersion, err := cbs.OnChanUpgradeTry(cacheCtx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version) if err != nil { ctx.Logger().Error("channel upgrade try callback failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", err.Error()) return nil, errorsmod.Wrapf(err, "channel upgrade try callback failed for port ID: %s, channel ID: %s", msg.PortId, msg.ChannelId) @@ -875,7 +874,6 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh if err != nil { ctx.Logger().Error("channel upgrade ack failed", "error", errorsmod.Wrap(err, "channel upgrade ack failed")) if channeltypes.IsUpgradeError(err) { - cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId) k.ChannelKeeper.MustAbortUpgrade(ctx, msg.PortId, msg.ChannelId, err) // NOTE: a FAILURE result is returned to the client and an error receipt is written to state. @@ -887,7 +885,9 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh return nil, errorsmod.Wrap(err, "channel upgrade ack failed") } - cacheCtx, writeFn := ctx.CacheContext() + // NOTE: a cached context is used to discard ibc application state changes and events. + // IBC applications must flush in-flight packets using the pre-upgrade channel parameters. + cacheCtx, _ := ctx.CacheContext() err = cbs.OnChanUpgradeAck(cacheCtx, msg.PortId, msg.ChannelId, msg.CounterpartyUpgrade.Fields.Version) if err != nil { channel, found := k.ChannelKeeper.GetChannel(ctx, msg.PortId, msg.ChannelId) @@ -896,7 +896,6 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh } ctx.Logger().Error("channel upgrade ack callback failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", err.Error()) - cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId) // explicitly wrap the application callback in an upgrade error with the correct upgrade sequence. // this prevents any errors caused from the application returning an UpgradeError with an incorrect sequence. @@ -905,8 +904,6 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh return &channeltypes.MsgChannelUpgradeAckResponse{Result: channeltypes.FAILURE}, nil } - writeFn() - channel, upgrade := k.ChannelKeeper.WriteUpgradeAckChannel(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyUpgrade) ctx.Logger().Info("channel upgrade ack succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId) @@ -943,7 +940,6 @@ func (k Keeper) ChannelUpgradeConfirm(goCtx context.Context, msg *channeltypes.M if err != nil { ctx.Logger().Error("channel upgrade confirm failed", "error", errorsmod.Wrap(err, "channel upgrade confirm failed")) if channeltypes.IsUpgradeError(err) { - cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId) k.ChannelKeeper.MustAbortUpgrade(ctx, msg.PortId, msg.ChannelId, err) // NOTE: a FAILURE result is returned to the client and an error receipt is written to state. @@ -1023,32 +1019,11 @@ func (k Keeper) ChannelUpgradeOpen(goCtx context.Context, msg *channeltypes.MsgC // ChannelUpgradeTimeout defines a rpc handler method for MsgChannelUpgradeTimeout. func (k Keeper) ChannelUpgradeTimeout(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeTimeout) (*channeltypes.MsgChannelUpgradeTimeoutResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) - module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.PortId, msg.ChannelId) - if err != nil { - ctx.Logger().Error("channel upgrade timeout failed", "port-id", msg.PortId, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) - return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") - } - - app, ok := k.Router.GetRoute(module) - if !ok { - err = errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) - ctx.Logger().Error("channel upgrade timeout failed", "port-id", msg.PortId, "error", err) - return nil, err - } - - cbs, ok := app.(porttypes.UpgradableModule) - if !ok { - err = errorsmod.Wrapf(porttypes.ErrInvalidRoute, "upgrade route not found to module: %s", module) - ctx.Logger().Error("channel upgrade timeout failed", "port-id", msg.PortId, "error", err) - return nil, err - } - err = k.ChannelKeeper.ChanUpgradeTimeout(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyChannel, msg.ProofChannel, msg.ProofHeight) - if err != nil { + if err := k.ChannelKeeper.ChanUpgradeTimeout(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyChannel, msg.ProofChannel, msg.ProofHeight); err != nil { return nil, errorsmod.Wrapf(err, "could not timeout upgrade for channel: %s", msg.ChannelId) } - cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId) channel, upgrade := k.ChannelKeeper.WriteUpgradeTimeoutChannel(ctx, msg.PortId, msg.ChannelId) ctx.Logger().Info("channel upgrade timeout callback succeeded: portID %s, channelID %s", msg.PortId, msg.ChannelId) @@ -1060,25 +1035,6 @@ func (k Keeper) ChannelUpgradeTimeout(goCtx context.Context, msg *channeltypes.M // ChannelUpgradeCancel defines a rpc handler method for MsgChannelUpgradeCancel. func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeCancel) (*channeltypes.MsgChannelUpgradeCancelResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) - module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.PortId, msg.ChannelId) - if err != nil { - ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) - return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") - } - - app, ok := k.Router.GetRoute(module) - if !ok { - err = errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) - ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", err) - return nil, err - } - - cbs, ok := app.(porttypes.UpgradableModule) - if !ok { - err = errorsmod.Wrapf(porttypes.ErrInvalidRoute, "upgrade route not found to module: %s", module) - ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, err) - return nil, err - } channel, found := k.ChannelKeeper.GetChannel(ctx, msg.PortId, msg.ChannelId) if !found { @@ -1094,7 +1050,6 @@ func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.Ms return nil, errorsmod.Wrapf(channeltypes.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", msg.PortId, msg.ChannelId) } - cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId) k.ChannelKeeper.WriteUpgradeCancelChannel(ctx, msg.PortId, msg.ChannelId, channel.UpgradeSequence) ctx.Logger().Info("channel upgrade cancel succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId) @@ -1115,7 +1070,6 @@ func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.Ms return nil, errorsmod.Wrapf(channeltypes.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", msg.PortId, msg.ChannelId) } - cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId) k.ChannelKeeper.WriteUpgradeCancelChannel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt.Sequence) ctx.Logger().Info("channel upgrade cancel succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index f883faad130..50df87d4967 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -990,6 +990,41 @@ func (suite *KeeperTestSuite) TestChannelUpgradeInit() { suite.Require().Empty(events) }, }, + { + "ibc application does not commit state changes in callback", + func() { + msg = channeltypes.NewMsgChannelUpgradeInit( + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + path.EndpointA.GetProposedUpgrade().Fields, + path.EndpointA.Chain.GetSimApp().IBCKeeper.GetAuthority(), + ) + + suite.chainA.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeInit = func(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) { + storeKey := suite.chainA.GetSimApp().GetKey(exported.ModuleName) + store := ctx.KVStore(storeKey) + store.Set(ibcmock.TestKey, ibcmock.TestValue) + + ctx.EventManager().EmitEvent(sdk.NewEvent(ibcmock.MockEventType)) + return ibcmock.UpgradeVersion, nil + } + }, + func(res *channeltypes.MsgChannelUpgradeInitResponse, events []abci.Event, err error) { + suite.Require().NoError(err) + suite.Require().NotNil(res) + suite.Require().Equal(uint64(1), res.UpgradeSequence) + + storeKey := suite.chainA.GetSimApp().GetKey(exported.ModuleName) + store := suite.chainA.GetContext().KVStore(storeKey) + suite.Require().Nil(store.Get(ibcmock.TestKey)) + + for _, event := range events { + if event.GetType() == ibcmock.MockEventType { + suite.Fail("expected application callback events to be discarded") + } + } + }, + }, } for _, tc := range cases { @@ -1128,6 +1163,34 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() { suite.Require().Empty(events) }, }, + { + "ibc application does not commit state changes in callback", + func() { + suite.chainA.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeTry = func(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, counterpartyVersion string) (string, error) { + storeKey := suite.chainA.GetSimApp().GetKey(exported.ModuleName) + store := ctx.KVStore(storeKey) + store.Set(ibcmock.TestKey, ibcmock.TestValue) + + ctx.EventManager().EmitEvent(sdk.NewEvent(ibcmock.MockEventType)) + return ibcmock.UpgradeVersion, nil + } + }, + func(res *channeltypes.MsgChannelUpgradeTryResponse, events []abci.Event, err error) { + suite.Require().NoError(err) + suite.Require().NotNil(res) + suite.Require().Equal(uint64(1), res.UpgradeSequence) + + storeKey := suite.chainA.GetSimApp().GetKey(exported.ModuleName) + store := suite.chainA.GetContext().KVStore(storeKey) + suite.Require().Nil(store.Get(ibcmock.TestKey)) + + for _, event := range events { + if event.GetType() == ibcmock.MockEventType { + suite.Fail("expected application callback events to be discarded") + } + } + }, + }, } for _, tc := range cases { @@ -1405,6 +1468,33 @@ func (suite *KeeperTestSuite) TestChannelUpgradeAck() { suite.Require().Equal(uint64(1), path.EndpointA.GetChannel().UpgradeSequence, "application callback upgrade sequence should not be used") }, }, + { + "ibc application does not commit state changes in callback", + func() { + suite.chainA.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeAck = func(ctx sdk.Context, portID, channelID, counterpartyVersion string) error { + storeKey := suite.chainA.GetSimApp().GetKey(exported.ModuleName) + store := ctx.KVStore(storeKey) + store.Set(ibcmock.TestKey, ibcmock.TestValue) + + ctx.EventManager().EmitEvent(sdk.NewEvent(ibcmock.MockEventType)) + return nil + } + }, + func(res *channeltypes.MsgChannelUpgradeAckResponse, events []abci.Event, err error) { + suite.Require().NoError(err) + suite.Require().NotNil(res) + + storeKey := suite.chainA.GetSimApp().GetKey(exported.ModuleName) + store := suite.chainA.GetContext().KVStore(storeKey) + suite.Require().Nil(store.Get(ibcmock.TestKey)) + + for _, event := range events { + if event.GetType() == ibcmock.MockEventType { + suite.Fail("expected application callback events to be discarded") + } + } + }, + }, } for _, tc := range cases { @@ -2217,24 +2307,6 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { suite.Require().Equal(uint64(1), channel.UpgradeSequence) }, }, - { - "capability not found", - func() { - msg.ChannelId = ibctesting.InvalidID - }, - func(res *channeltypes.MsgChannelUpgradeCancelResponse, events []abci.Event, err error) { - suite.Require().Error(err) - suite.Require().Nil(res) - - channel := path.EndpointA.GetChannel() - suite.Require().ErrorIs(err, capabilitytypes.ErrCapabilityNotFound) - suite.Require().Equal(channeltypes.OPEN, channel.State) - // Upgrade sequence should not be changed. - suite.Require().Equal(uint64(1), channel.UpgradeSequence) - - suite.Require().Empty(events) - }, - }, { "core handler fails: invalid proof", func() { @@ -2257,29 +2329,11 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { // Upgrade sequence should not be changed. suite.Require().Equal(uint64(2), channel.UpgradeSequence) - suite.Require().Empty(events) - }, - }, - { - "ibc application does not implement the UpgradeableModule interface", - func() { - path = ibctesting.NewPath(suite.chainA, suite.chainB) - path.EndpointA.ChannelConfig.PortID = ibcmock.MockBlockUpgrade - path.EndpointB.ChannelConfig.PortID = ibcmock.MockBlockUpgrade - - suite.coordinator.Setup(path) - - msg.PortId = path.EndpointB.ChannelConfig.PortID - msg.ChannelId = path.EndpointB.ChannelID - }, - func(res *channeltypes.MsgChannelUpgradeCancelResponse, events []abci.Event, err error) { - suite.Require().ErrorIs(err, porttypes.ErrInvalidRoute) - suite.Require().Nil(res) - suite.Require().Empty(events) }, }, } + for _, tc := range cases { tc := tc suite.Run(tc.name, func() { @@ -2418,25 +2472,6 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTimeout() { ibctesting.AssertEventsLegacy(&suite.Suite, expEvents, events) }, }, - { - "capability not found", - func() { - msg.ChannelId = ibctesting.InvalidID - }, - func(res *channeltypes.MsgChannelUpgradeTimeoutResponse, events []abci.Event, err error) { - suite.Require().Error(err) - suite.Require().ErrorIs(err, capabilitytypes.ErrCapabilityNotFound) - - channel := path.EndpointA.GetChannel() - suite.Require().Equalf(channeltypes.FLUSHCOMPLETE, channel.State, "channel state should be %s", channeltypes.OPEN) - suite.Require().Equal(uint64(1), channel.UpgradeSequence, "channel upgrade sequence should not incremented") - - _, found := path.EndpointA.Chain.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - suite.Require().True(found, "channel upgrade should not be nil") - - suite.Require().Empty(events) - }, - }, { "core handler fails: invalid proof", func() { @@ -2460,25 +2495,6 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTimeout() { _, found := path.EndpointA.Chain.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().True(found, "channel upgrade should not be nil") - suite.Require().Empty(events) - }, - }, - { - "ibc application does not implement the UpgradeableModule interface", - func() { - path = ibctesting.NewPath(suite.chainA, suite.chainB) - path.EndpointA.ChannelConfig.PortID = ibcmock.MockBlockUpgrade - path.EndpointB.ChannelConfig.PortID = ibcmock.MockBlockUpgrade - - suite.coordinator.Setup(path) - - msg.PortId = path.EndpointB.ChannelConfig.PortID - msg.ChannelId = path.EndpointB.ChannelID - }, - func(res *channeltypes.MsgChannelUpgradeTimeoutResponse, events []abci.Event, err error) { - suite.Require().ErrorIs(err, porttypes.ErrInvalidRoute) - suite.Require().Nil(res) - suite.Require().Empty(events) }, }, diff --git a/testing/mock/events.go b/testing/mock/events.go index e92592c0326..124705962a5 100644 --- a/testing/mock/events.go +++ b/testing/mock/events.go @@ -3,6 +3,7 @@ package mock import sdk "github.com/cosmos/cosmos-sdk/types" const ( + MockEventType = "mock-event-type" MockEventTypeRecvPacket = "mock-recv-packet" MockEventTypeAckPacket = "mock-ack-packet" MockEventTypeTimeoutPacket = "mock-timeout" diff --git a/testing/mock/ibc_app.go b/testing/mock/ibc_app.go index a49a834ef96..026a5d0873d 100644 --- a/testing/mock/ibc_app.go +++ b/testing/mock/ibc_app.go @@ -117,12 +117,6 @@ type IBCApp struct { connectionHops []string, version string, ) - - OnChanUpgradeRestore func( - ctx sdk.Context, - portID, - channelID string, - ) } // NewIBCApp returns a IBCApp. An empty PortID indicates the mock app doesn't bind/claim ports. diff --git a/testing/mock/ibc_module.go b/testing/mock/ibc_module.go index 9ca2b23c6d8..b3bd2432de3 100644 --- a/testing/mock/ibc_module.go +++ b/testing/mock/ibc_module.go @@ -217,13 +217,6 @@ func (im IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, } } -// OnChanUpgradeRestore implements the IBCModule interface -func (im IBCModule) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) { - if im.IBCApp.OnChanUpgradeRestore != nil { - im.IBCApp.OnChanUpgradeRestore(ctx, portID, channelID) - } -} - // UnmarshalPacketData returns the MockPacketData. This function implements the optional // PacketDataUnmarshaler interface required for ADR 008 support. func (IBCModule) UnmarshalPacketData(bz []byte) (interface{}, error) { diff --git a/testing/mock/mock.go b/testing/mock/mock.go index 86de269f48b..1d90491f43e 100644 --- a/testing/mock/mock.go +++ b/testing/mock/mock.go @@ -49,6 +49,11 @@ var ( MockApplicationCallbackError error = &applicationCallbackError{} ) +var ( + TestKey = []byte("test-key") + TestValue = []byte("test-value") +) + var ( _ module.AppModuleBasic = (*AppModuleBasic)(nil) _ appmodule.AppModule = (*AppModule)(nil)