diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index cf9905781f6..b36cf9736d1 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -287,25 +287,7 @@ func (im IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, cou return types.ErrControllerSubModuleDisabled } - if err := im.keeper.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion); err != nil { - return err - } - - connectionID, err := im.keeper.GetConnectionID(ctx, portID, channelID) - if err != nil { - return 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 { - return errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack") - } - return cbs.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion) - } - - return nil + return im.keeper.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion) } // OnChanUpgradeOpen implements the IBCModule interface 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 a136f3c756b..70562c1216d 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -825,7 +825,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() { func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { var ( path *ibctesting.Path - isNilApp bool counterpartyVersion string ) @@ -839,9 +838,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { }, { "success: nil underlying app", - func() { - isNilApp = true - }, + func() {}, nil, }, { @@ -861,14 +858,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { } }, ibcmock.MockApplicationCallbackError, }, - { - "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 - } - }, nil, - }, } for _, ordering := range []channeltypes.Order{channeltypes.UNORDERED, channeltypes.ORDERED} { @@ -877,7 +866,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { suite.Run(tc.name, func() { suite.SetupTest() // reset - isNilApp = false path = NewICAPath(suite.chainA, suite.chainB, ordering) path.SetupConnections() @@ -889,18 +877,12 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { 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().PortKeeper.Route(module) + app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(path.EndpointA.ChannelConfig.PortID) suite.Require().True(ok) + cbs, ok := app.(porttypes.UpgradableModule) suite.Require().True(ok) - if isNilApp { - cbs = controller.NewIBCMiddleware(suite.chainA.GetSimApp().ICAControllerKeeper) - } - err = cbs.OnChanUpgradeAck( suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index 71b00cd64d5..dfdaedcf3ea 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -745,10 +745,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { suite.Require().NoError(err) // call application callback directly - module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) - suite.Require().NoError(err) - - app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module) + app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(path.EndpointB.ChannelConfig.PortID) suite.Require().True(ok) cbs, ok := app.(porttypes.UpgradableModule) suite.Require().True(ok) diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index 5a2acfd2a4b..e0c3f96599a 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -312,25 +312,11 @@ func (IBCMiddleware) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, } // OnChanUpgradeAck implements the IBCModule interface -func (im IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error { - cbs, ok := im.app.(porttypes.UpgradableModule) - if !ok { - return errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack") - } - - versionMetadata, err := types.MetadataFromVersion(counterpartyVersion) - if err != nil { - // since it is valid for fee version to not be specified, the counterparty upgrade version may be for a middleware - // or application further down in the stack. Thus, pass through to next middleware or application in callstack. - return cbs.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion) - } - - if versionMetadata.FeeVersion != types.Version { - return errorsmod.Wrapf(types.ErrInvalidVersion, "expected counterparty fee version: %s, got: %s", types.Version, versionMetadata.FeeVersion) +func (IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error { + if counterpartyVersion != types.Version { + return errorsmod.Wrapf(types.ErrInvalidVersion, "expected counterparty fee version: %s, got: %s", types.Version, counterpartyVersion) } - - // call underlying app's OnChanUpgradeAck callback with the counterparty app version. - return cbs.OnChanUpgradeAck(ctx, portID, channelID, versionMetadata.AppVersion) + return nil } // OnChanUpgradeOpen implements the IBCModule interface diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 64f494cbbca..39199d58d10 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -1274,10 +1274,7 @@ func (suite *FeeTestSuite) TestOnChanUpgradeAck() { counterpartyUpgrade := path.EndpointB.GetChannelUpgrade() - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort) - suite.Require().NoError(err) - - app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module) + app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(ibctesting.MockFeePort) suite.Require().True(ok) cbs, ok := app.(porttypes.UpgradableModule) diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go index 69876944e94..19cccab81ed 100644 --- a/modules/apps/callbacks/ibc_middleware.go +++ b/modules/apps/callbacks/ibc_middleware.go @@ -368,13 +368,8 @@ func (IBCMiddleware) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, } // OnChanUpgradeAck implements the IBCModule interface -func (im IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error { - cbs, ok := im.app.(porttypes.UpgradableModule) - if !ok { - return errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack") - } - - return cbs.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion) +func (IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error { + return nil } // OnChanUpgradeOpen implements the IBCModule interface diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index 7162cebd967..c610d6d9503 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -690,10 +690,7 @@ func (suite *TransferTestSuite) TestOnChanUpgradeAck() { tc.malleate() - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) - suite.Require().NoError(err) - - app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module) + app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(types.PortID) suite.Require().True(ok) cbs, ok := app.(porttypes.UpgradableModule) diff --git a/modules/core/05-port/types/ibc_legacy_module.go b/modules/core/05-port/types/ibc_legacy_module.go index f9291c68ab8..e58d18daefa 100644 --- a/modules/core/05-port/types/ibc_legacy_module.go +++ b/modules/core/05-port/types/ibc_legacy_module.go @@ -339,7 +339,39 @@ func (im *LegacyIBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID s } // OnChanUpgradeAck implements the IBCModule interface -func (LegacyIBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error { +func (im *LegacyIBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error { + for i := len(im.cbs) - 1; i >= 0; i-- { + cbVersion := counterpartyVersion + + // To maintain backwards compatibility, we must handle two cases: + // - relayer provides empty version (use default versions) + // - relayer provides version which chooses to not enable a middleware + // + // If an application is a VersionWrapper which means it modifies the version string + // and the version string is non-empty (don't use default), then the application must + // attempt to unmarshal the version using the UnwrapVersionUnsafe interface function. + // If it is unsuccessful, no callback will occur to this application as the version + // indicates it should be disabled. + if wrapper, ok := im.cbs[i].(VersionWrapper); ok && strings.TrimSpace(counterpartyVersion) != "" { + appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(counterpartyVersion) + if err != nil { + // middleware disabled + continue + } + cbVersion, counterpartyVersion = appVersion, underlyingAppVersion + } + + // in order to maintain backwards compatibility, every callback in the stack must implement the UpgradableModule interface. + upgradableModule, ok := im.cbs[i].(UpgradableModule) + if !ok { + return errorsmod.Wrap(ErrInvalidRoute, "upgrade route not found to module in application callstack") + } + + err := upgradableModule.OnChanUpgradeAck(ctx, portID, channelID, cbVersion) + if err != nil { + return errorsmod.Wrapf(err, "channel open init callback failed for port ID: %s, channel ID: %s", portID, channelID) + } + } return nil } diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index efb200dfd3e..dd1372c7d43 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -758,7 +758,7 @@ func (k *Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgC func (k *Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeAck) (*channeltypes.MsgChannelUpgradeAckResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) - app, ok := k.PortKeeper.Route(msg.PortId) + app, ok := k.PortKeeper.AppRouter.HandshakeRoute(msg.PortId) if !ok { err := errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", msg.PortId) ctx.Logger().Error("channel upgrade ack failed", "port-id", msg.PortId, "error", err) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index 205683b8d2c..02c1592e3ad 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -1285,70 +1285,6 @@ func (suite *KeeperTestSuite) TestChannelUpgradeAck() { ibctesting.AssertEvents(&suite.Suite, expEvents, events) }, }, - { - "application callback returns error and error receipt is written", - func() { - suite.chainA.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeAck = func( - ctx sdk.Context, portID, channelID, counterpartyVersion string, - ) error { - // set arbitrary value in store to mock application state changes - store := ctx.KVStore(suite.chainA.GetSimApp().GetKey(exported.ModuleName)) - store.Set([]byte("foo"), []byte("bar")) - return fmt.Errorf("mock app callback failed") - } - }, - func(res *channeltypes.MsgChannelUpgradeAckResponse, events []abci.Event, err error) { - suite.Require().NoError(err) - - suite.Require().NotNil(res) - suite.Require().Equal(channeltypes.FAILURE, res.Result) - - errorReceipt, found := suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - suite.Require().True(found) - suite.Require().Equal(uint64(1), errorReceipt.Sequence) - - // assert application state changes are not committed - store := suite.chainA.GetContext().KVStore(suite.chainA.GetSimApp().GetKey(exported.ModuleName)) - suite.Require().False(store.Has([]byte("foo"))) - - channel := path.EndpointB.GetChannel() - expEvents := sdk.Events{ - sdk.NewEvent( - channeltypes.EventTypeChannelUpgradeError, - sdk.NewAttribute(channeltypes.AttributeKeyPortID, path.EndpointA.ChannelConfig.PortID), - sdk.NewAttribute(channeltypes.AttributeKeyChannelID, path.EndpointA.ChannelID), - sdk.NewAttribute(channeltypes.AttributeCounterpartyPortID, path.EndpointB.ChannelConfig.PortID), - sdk.NewAttribute(channeltypes.AttributeCounterpartyChannelID, path.EndpointB.ChannelID), - sdk.NewAttribute(channeltypes.AttributeKeyUpgradeSequence, fmt.Sprintf("%d", channel.UpgradeSequence)), - // need to manually insert this because the errorReceipt is a string constant as it is written into state - sdk.NewAttribute(channeltypes.AttributeKeyErrorReceipt, "mock app callback failed"), - ), - sdk.NewEvent( - sdk.EventTypeMessage, - sdk.NewAttribute(sdk.AttributeKeyModule, channeltypes.AttributeValueCategory), - ), - }.ToABCIEvents() - ibctesting.AssertEvents(&suite.Suite, expEvents, 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 - - path.Setup() - - msg.PortId = path.EndpointA.ChannelConfig.PortID - msg.ChannelId = path.EndpointA.ChannelID - }, - func(res *channeltypes.MsgChannelUpgradeAckResponse, events []abci.Event, err error) { - suite.Require().ErrorIs(err, porttypes.ErrInvalidRoute) - suite.Require().Nil(res) - suite.Require().Empty(events) - }, - }, { "application callback returns an upgrade error", func() {