Use new port router for OnChanOpenAck (#7084)

* refactor: use new port router for OnChanOpenAck

* fix: transfer test

* fix: controller tests

* fix: legacy ibc module handler on disabling

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

@@ -108,16 +108,6 @@ func (im IBCMiddleware) OnChanOpenAck(
 		return err
 	}
 
-	connectionID, err := im.keeper.GetConnectionID(ctx, portID, channelID)
-	if err != nil {
-		return err
-	}
-
-	// call underlying app's OnChanOpenAck callback with the counterparty app version.
-	if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, connectionID) {
-		return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion)
-	}
-
 	return nil
 }
 

modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go

@@ -252,53 +252,25 @@ func (suite *InterchainAccountsTestSuite) TestChanOpenTry() {
 }
 
 func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {
-	var (
-		path     *ibctesting.Path
-		isNilApp bool
-	)
+	var path *ibctesting.Path
 
 	testCases := []struct {
 		name     string
 		malleate func()
-		expPass  bool
+		expError error
 	}{
 		{
-			"success", func() {}, true,
+			"success", func() {}, nil,
 		},
 		{
 			"controller submodule disabled", func() {
 				suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(suite.chainA.GetContext(), types.NewParams(false))
-			}, false,
+			}, types.ErrControllerSubModuleDisabled,
 		},
 		{
 			"ICA OnChanOpenACK fails - invalid version", func() {
 				path.EndpointB.ChannelConfig.Version = invalidVersion
-			}, false,
-		},
-		{
-			"ICA auth module callback fails", func() {
-				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenAck = func(
-					ctx sdk.Context, portID, channelID string, counterpartyChannelID string, counterpartyVersion string,
-				) error {
-					return fmt.Errorf("mock ica auth fails")
-				}
-			}, false,
-		},
-		{
-			"nil underlying app", func() {
-				isNilApp = true
-			}, true,
-		},
-		{
-			"middleware disabled", func() {
-				suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ConnectionID)
-
-				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenAck = func(
-					ctx sdk.Context, portID, channelID string, counterpartyChannelID string, counterpartyVersion string,
-				) error {
-					return fmt.Errorf("error should be unreachable")
-				}
-			}, true,
+			}, ibcerrors.ErrInvalidType,
 		},
 	}
 
@@ -308,7 +280,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {
 
 			suite.Run(tc.name, func() {
 				suite.SetupTest() // reset
-				isNilApp = false
 
 				path = NewICAPath(suite.chainA, suite.chainB, ordering)
 				path.SetupConnections()
@@ -321,22 +292,22 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {
 
 				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)
-
-				cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
+				cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(path.EndpointA.ChannelConfig.PortID)
 				suite.Require().True(ok)
 
-				err = cbs.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelID, path.EndpointB.ChannelConfig.Version)
+				legacyModule, ok := cbs.(*porttypes.LegacyIBCModule)
+				suite.Require().True(ok, "expected there to be a single legacy ibc module")
 
-				if isNilApp {
-					cbs = controller.NewIBCMiddleware(suite.chainA.GetSimApp().ICAControllerKeeper)
-				}
+				legacyModuleCbs := legacyModule.GetCallbacks()
+				controllerModule, ok := legacyModuleCbs[1].(controller.IBCMiddleware) // fee module is routed second
+				suite.Require().True(ok)
 
-				if tc.expPass {
+				err = controllerModule.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelID, path.EndpointB.ChannelConfig.Version)
+
+				if tc.expError == nil {
 					suite.Require().NoError(err)
 				} else {
-					suite.Require().Error(err)
+					suite.Require().ErrorIs(err, tc.expError)
 				}
 			})
 		}

modules/apps/29-fee/ibc_middleware.go

@@ -85,25 +85,18 @@ func (im IBCMiddleware) OnChanOpenAck(
 	counterpartyChannelID string,
 	counterpartyVersion string,
 ) error {
-	if im.keeper.IsFeeEnabled(ctx, portID, channelID) {
-		versionMetadata, err := types.MetadataFromVersion(counterpartyVersion)
-		if err != nil {
-			// we pass the entire version string onto the underlying application.
-			// and disable fees for this channel
-			im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
-			return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion)
-		}
-
-		if versionMetadata.FeeVersion != types.Version {
-			return errorsmod.Wrapf(types.ErrInvalidVersion, "expected counterparty fee version: %s, got: %s", types.Version, versionMetadata.FeeVersion)
-		}
+	if strings.TrimSpace(counterpartyVersion) == "" {
+		// disable fees for this channel
+		im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
+		return nil
+	}
 
-		// call underlying app's OnChanOpenAck callback with the counterparty app version.
-		return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, versionMetadata.AppVersion)
+	if counterpartyVersion != types.Version {
+		return errorsmod.Wrapf(types.ErrInvalidVersion, "expected counterparty fee version: %s, got: %s", types.Version, counterpartyVersion)
 	}
 
-	// call underlying app's OnChanOpenAck callback with the counterparty app version.
-	return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion)
+	im.keeper.SetFeeEnabled(ctx, portID, channelID)
+	return nil
 }
 
 // OnChanOpenConfirm implements the IBCMiddleware interface

modules/apps/29-fee/ibc_middleware_test.go

@@ -181,41 +181,26 @@ func (suite *FeeTestSuite) TestOnChanOpenAck() {
 		name      string
 		cpVersion string
 		malleate  func(suite *FeeTestSuite)
-		expPass   bool
+		expError  error
 	}{
 		{
 			"success",
-			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version})),
-			func(suite *FeeTestSuite) {},
-			true,
-		},
-		{
-			"invalid fee version",
-			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: "invalid-ics29-1", AppVersion: ibcmock.Version})),
+			types.Version,
 			func(suite *FeeTestSuite) {},
-			false,
+			nil,
 		},
 		{
-			"invalid mock version",
-			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: "invalid-mock-version"})),
+			"success, empty version string (disable middleware)",
+			"",
 			func(suite *FeeTestSuite) {},
-			false,
+			nil,
 		},
+
 		{
-			"invalid version fails to unmarshal metadata",
-			ibctesting.InvalidID,
+			"invalid fee version",
+			"invalid-ics29-1",
 			func(suite *FeeTestSuite) {},
-			false,
-		},
-		{
-			"previous INIT set without fee, however counterparty set fee version", // note this can only happen with incompetent or malicious counterparty chain
-			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version})),
-			func(suite *FeeTestSuite) {
-				// do the first steps without fee version, then pass the fee version as counterparty version in ChanOpenACK
-				suite.path.EndpointA.ChannelConfig.Version = ibcmock.Version
-				suite.path.EndpointB.ChannelConfig.Version = ibcmock.Version
-			},
-			false,
+			types.ErrInvalidVersion,
 		},
 	}
 
@@ -225,16 +210,6 @@ func (suite *FeeTestSuite) TestOnChanOpenAck() {
 			suite.SetupTest()
 			suite.path.SetupConnections()
 
-			// setup mock callback
-			suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanOpenAck = func(
-				ctx sdk.Context, portID, channelID string, counterpartyChannelID string, counterpartyVersion string,
-			) error {
-				if counterpartyVersion != ibcmock.Version {
-					return fmt.Errorf("incorrect mock version")
-				}
-				return nil
-			}
-
 			// malleate test case
 			tc.malleate(suite)
 
@@ -243,17 +218,21 @@ func (suite *FeeTestSuite) TestOnChanOpenAck() {
 			err = suite.path.EndpointB.ChanOpenTry()
 			suite.Require().NoError(err)
 
-			module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort)
-			suite.Require().NoError(err)
+			cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(ibctesting.MockFeePort)
+			suite.Require().True(ok)
 
-			cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
+			legacyModule, ok := cbs.(*porttypes.LegacyIBCModule)
+			suite.Require().True(ok, "expected there to be a single legacy ibc module")
+
+			legacyModuleCbs := legacyModule.GetCallbacks()
+			feeModule, ok := legacyModuleCbs[1].(ibcfee.IBCMiddleware) // fee module is routed second
 			suite.Require().True(ok)
 
-			err = cbs.OnChanOpenAck(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, suite.path.EndpointA.Counterparty.ChannelID, tc.cpVersion)
-			if tc.expPass {
+			err = feeModule.OnChanOpenAck(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, suite.path.EndpointA.Counterparty.ChannelID, tc.cpVersion)
+			if tc.expError == nil {
 				suite.Require().NoError(err, "unexpected error for case: %s", tc.name)
 			} else {
-				suite.Require().Error(err, "%s expected error but returned none", tc.name)
+				suite.Require().ErrorIs(err, tc.expError, "%s expected error but returned none", tc.name)
 			}
 		})
 	}

modules/apps/callbacks/ibc_middleware.go

@@ -331,15 +331,15 @@ func (IBCMiddleware) OnChanOpenTry(
 	return "", nil
 }
 
-// OnChanOpenAck defers to the underlying application
-func (im IBCMiddleware) OnChanOpenAck(
+// OnChanOpenAck is a no-op for the callbacks middleware.
+func (IBCMiddleware) OnChanOpenAck(
 	ctx sdk.Context,
 	portID,
 	channelID,
 	counterpartyChannelID,
 	counterpartyVersion string,
 ) error {
-	return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion)
+	return nil
 }
 
 // OnChanOpenConfirm defers to the underlying application

modules/apps/transfer/ibc_module_test.go

@@ -231,15 +231,10 @@ func (suite *TransferTestSuite) TestOnChanOpenAck() {
 			path.EndpointA.ChannelID = ibctesting.FirstChannelID
 			counterpartyVersion = types.V2
 
-			module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
-			suite.Require().NoError(err)
-
-			cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
-			suite.Require().True(ok)
-
 			tc.malleate() // explicitly change fields in channel and testChannel
 
-			err = cbs.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointA.Counterparty.ChannelID, counterpartyVersion)
+			transferModule := transfer.NewIBCModule(suite.chainA.GetSimApp().TransferKeeper)
+			err := transferModule.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointA.Counterparty.ChannelID, counterpartyVersion)
 
 			expPass := tc.expError == nil
 			if expPass {

modules/core/05-port/types/ibc_legacy_module.go

@@ -31,6 +31,10 @@ func NewLegacyIBCModule(cbs ...ClassicIBCModule) ClassicIBCModule {
 }
 
 // OnChanOpenInit implements the IBCModule interface.
+// NOTE: The application callback is skipped if all the following are true:
+// - the relayer provided channel version is not empty
+// - the callback application is a VersionWrapper
+// - the application cannot unwrap the version
 func (im *LegacyIBCModule) OnChanOpenInit(
 	ctx sdk.Context,
 	order channeltypes.Order,
@@ -75,6 +79,10 @@ func (im *LegacyIBCModule) OnChanOpenInit(
 }
 
 // OnChanOpenTry implements the IBCModule interface.
+// NOTE: The application callback is skipped if all the following are true:
+// - the relayer provided channel version is not empty
+// - the callback application is a VersionWrapper
+// - the application cannot unwrap the version
 func (im *LegacyIBCModule) OnChanOpenTry(
 	ctx sdk.Context,
 	order channeltypes.Order,
@@ -134,14 +142,39 @@ func reconstructVersion(cbs []ClassicIBCModule, negotiatedVersions []string) (st
 	return version, nil
 }
 
-// OnChanOpenAck implements the IBCModule interface
-func (LegacyIBCModule) OnChanOpenAck(
+// OnChanOpenAck implements the IBCModule interface.
+// NOTE: The callback will occur for all applications in the callback list.
+// If the application is provided an empty string for the counterparty version,
+// this indicates the module should be disabled for this portID and channelID.
+func (im *LegacyIBCModule) OnChanOpenAck(
 	ctx sdk.Context,
 	portID,
 	channelID string,
 	counterpartyChannelID string,
 	counterpartyVersion string,
 ) error {
+	for i := len(im.cbs) - 1; i >= 0; i-- {
+		cbVersion := counterpartyVersion
+
+		// To maintain backwards compatibility, we must handle counterparty version negotiation.
+		// This means the version may have changed, and applications must be allowed to be disabled.
+		// Applications should be disabled when receiving an empty counterparty version. Callbacks
+		// for all applications must occur to allow disabling.
+		if wrapper, ok := im.cbs[i].(VersionWrapper); ok {
+			appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(counterpartyVersion)
+			if err != nil {
+				cbVersion = "" // disable application
+			} else {
+				cbVersion, counterpartyVersion = appVersion, underlyingAppVersion
+			}
+		}
+
+		err := im.cbs[i].OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, cbVersion)
+		if err != nil {
+			return errorsmod.Wrapf(err, "channel open ack callback failed for port ID: %s, channel ID: %s", portID, channelID)
+		}
+	}
+
 	return nil
 }
 

modules/core/keeper/msg_server.go

@@ -275,7 +275,7 @@ func (k *Keeper) ChannelOpenAck(goCtx context.Context, msg *channeltypes.MsgChan
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
 	// Retrieve application callbacks from router
-	cbs, ok := k.PortKeeper.Route(msg.PortId)
+	cbs, ok := k.PortKeeper.AppRouter.HandshakeRoute(msg.PortId)
 	if !ok {
 		ctx.Logger().Error("channel open ack 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)