From 9a178d8e1c0516f8a3f16dcc6d40711886f614af Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 8 Aug 2024 10:49:51 +0100 Subject: [PATCH 1/3] chore: implement OnChanUpgradeTry --- .../controller/ibc_middleware_test.go | 7 +-- .../host/ibc_module_test.go | 6 +-- modules/apps/29-fee/ibc_middleware.go | 32 ++----------- modules/apps/callbacks/ibc_middleware.go | 9 +--- modules/apps/transfer/ibc_module_test.go | 5 +- .../core/05-port/types/ibc_legacy_module.go | 41 +++++++++++++++- modules/core/keeper/msg_server.go | 2 +- modules/core/keeper/msg_server_test.go | 47 ------------------- 8 files changed, 51 insertions(+), 98 deletions(-) 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 9b6a9f6f50f..8872433b338 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -806,12 +806,9 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() { err := SetupICAPath(path, TestOwnerAddress) suite.Require().NoError(err) - // call application callback directly - 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) 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 7be9f6c431b..47d91b1d50f 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -716,11 +716,9 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() { tc.malleate() // malleate mutates test data - 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 060977d1e62..ed8f8244e12 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -309,35 +309,11 @@ func (IBCMiddleware) OnChanUpgradeInit( } // OnChanUpgradeTry implements the IBCModule interface -func (im IBCMiddleware) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, counterpartyVersion string) (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.OnChanUpgradeTry(ctx, portID, channelID, proposedOrder, proposedConnectionHops, counterpartyVersion) - } - - if versionMetadata.FeeVersion != types.Version { - return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion) - } - - appVersion, err := cbs.OnChanUpgradeTry(ctx, portID, channelID, proposedOrder, proposedConnectionHops, versionMetadata.AppVersion) - if err != nil { - return "", err - } - - versionMetadata.AppVersion = appVersion - versionBz, err := types.ModuleCdc.MarshalJSON(&versionMetadata) - if err != nil { - return "", err +func (IBCMiddleware) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, counterpartyVersion string) (string, error) { + if counterpartyVersion != types.Version { + return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, counterpartyVersion) } - - return string(versionBz), nil + return counterpartyVersion, nil } // OnChanUpgradeAck implements the IBCModule interface diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go index a5935e1b177..88cf5efee06 100644 --- a/modules/apps/callbacks/ibc_middleware.go +++ b/modules/apps/callbacks/ibc_middleware.go @@ -363,13 +363,8 @@ func (IBCMiddleware) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string } // OnChanUpgradeTry implements the IBCModule interface -func (im IBCMiddleware) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, counterpartyVersion string) (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.OnChanUpgradeTry(ctx, portID, channelID, proposedOrder, proposedConnectionHops, counterpartyVersion) +func (IBCMiddleware) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, counterpartyVersion string) (string, error) { + return "", nil } // OnChanUpgradeAck implements the IBCModule interface diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index aa09bdf15f6..39b23e0945c 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -621,10 +621,7 @@ func (suite *TransferTestSuite) TestOnChanUpgradeTry() { tc.malleate() - module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), types.PortID) - suite.Require().NoError(err) - - app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module) + app, ok := suite.chainB.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 43415290a8e..aea0ab2412f 100644 --- a/modules/core/05-port/types/ibc_legacy_module.go +++ b/modules/core/05-port/types/ibc_legacy_module.go @@ -286,8 +286,45 @@ func (im *LegacyIBCModule) OnChanUpgradeInit(ctx sdk.Context, portID, channelID } // OnChanUpgradeTry implements the IBCModule interface -func (LegacyIBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, counterpartyVersion string) (string, error) { - return "", nil +func (im *LegacyIBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, counterpartyVersion string) (string, error) { + negotiatedVersions := make([]string, len(im.cbs)) + + 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 + negotiatedVersions[i] = "" + 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") + } + + negotiatedVersion, err := upgradableModule.OnChanUpgradeTry(ctx, portID, channelID, proposedOrder, proposedConnectionHops, cbVersion) + if err != nil { + return "", errorsmod.Wrapf(err, "channel open init callback failed for port ID: %s, channel ID: %s", portID, channelID) + } + negotiatedVersions[i] = negotiatedVersion + } + + return reconstructVersion(im.cbs, negotiatedVersions) } // OnChanUpgradeAck implements the IBCModule interface diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 6af62cdc465..0fb4dce3a7c 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -703,7 +703,7 @@ func (k *Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.Msg func (k *Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeTry) (*channeltypes.MsgChannelUpgradeTryResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) - app, ok := k.PortKeeper.Route(msg.PortId) + app, ok := k.PortKeeper.AppRouter.HandshakeRoute(msg.PortId) if !ok { ctx.Logger().Error("channel upgrade try failed", "port-id", msg.PortId, "error", errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", msg.PortId)) return nil, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", msg.PortId) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index 9824358cef4..205683b8d2c 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -1109,53 +1109,6 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() { 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.EndpointB.ChannelConfig.PortID - msg.ChannelId = path.EndpointB.ChannelID - }, - func(res *channeltypes.MsgChannelUpgradeTryResponse, events []abci.Event, err error) { - suite.Require().ErrorIs(err, porttypes.ErrInvalidRoute) - suite.Require().Nil(res) - - 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 { From 616c3f62aa9a2c8c7910408f28a393c16bf19ebf Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 8 Aug 2024 11:35:15 +0100 Subject: [PATCH 2/3] wip: partial implementation for OnChanUpgradeAck --- .../controller/ibc_middleware.go | 20 +------------------ modules/apps/29-fee/ibc_middleware.go | 20 +++---------------- modules/apps/callbacks/ibc_middleware.go | 7 +------ modules/core/keeper/msg_server.go | 2 +- 4 files changed, 6 insertions(+), 43 deletions(-) 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/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index ed8f8244e12..4c9a7220a07 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -318,24 +318,10 @@ 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) + 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/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go index 88cf5efee06..a568642ca67 100644 --- a/modules/apps/callbacks/ibc_middleware.go +++ b/modules/apps/callbacks/ibc_middleware.go @@ -369,12 +369,7 @@ 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) + return nil } // OnChanUpgradeOpen implements the IBCModule interface diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 0fb4dce3a7c..048ad889158 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) From 592eecf0e419ceb9cc2b9a17a1bfdffc143c86f6 Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 8 Aug 2024 11:56:04 +0100 Subject: [PATCH 3/3] chore: updated tests to work with new OnChanUpgradeAck implementation --- .../controller/ibc_middleware_test.go | 24 +------ .../host/ibc_module_test.go | 5 +- modules/apps/29-fee/ibc_middleware.go | 2 +- modules/apps/29-fee/ibc_middleware_test.go | 5 +- modules/apps/callbacks/ibc_middleware.go | 2 +- modules/apps/transfer/ibc_module_test.go | 5 +- .../core/05-port/types/ibc_legacy_module.go | 34 +++++++++- modules/core/keeper/msg_server_test.go | 64 ------------------- 8 files changed, 41 insertions(+), 100 deletions(-) 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 1fd0d884b5d..95065e9f4aa 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -828,7 +828,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() { func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { var ( path *ibctesting.Path - isNilApp bool counterpartyVersion string ) @@ -842,9 +841,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { }, { "success: nil underlying app", - func() { - isNilApp = true - }, + func() {}, nil, }, { @@ -864,14 +861,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} { @@ -880,7 +869,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { suite.Run(tc.name, func() { suite.SetupTest() // reset - isNilApp = false path = NewICAPath(suite.chainA, suite.chainB, ordering) path.SetupConnections() @@ -892,18 +880,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 cee5d45d9f1..6ebb4e222e2 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -748,10 +748,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 2b304d61613..1382337aca8 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -316,7 +316,7 @@ 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 { +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) } diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 107b736f6d1..2598bc2c27d 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -1283,10 +1283,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 f49960665b2..25a47b37e04 100644 --- a/modules/apps/callbacks/ibc_middleware.go +++ b/modules/apps/callbacks/ibc_middleware.go @@ -368,7 +368,7 @@ 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 { +func (IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error { return nil } diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index 39b23e0945c..6454b1a923f 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -689,10 +689,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 0c36d557728..6aff38d9e7b 100644 --- a/modules/core/05-port/types/ibc_legacy_module.go +++ b/modules/core/05-port/types/ibc_legacy_module.go @@ -334,7 +334,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_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() {