Add Wrap/Unwrap Interfaces and implement OnChanOpenInit

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

@@ -71,16 +71,6 @@ func (im IBCMiddleware) OnChanOpenInit(
 	if err != nil {
 		return "", err
 	}
-
-	// call underlying app's OnChanOpenInit callback with the passed in version
-	// the version returned is discarded as the ica-auth module does not have permission to edit the version string.
-	// ics27 will always return the version string containing the Metadata struct which is created during the `RegisterInterchainAccount` call.
-	if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, connectionHops[0]) {
-		if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, counterparty, version); err != nil {
-			return "", err
-		}
-	}
-
 	return version, nil
 }
 
@@ -395,3 +385,17 @@ func (im IBCMiddleware) UnmarshalPacketData(ctx sdk.Context, portID string, chan
 
 	return data, version, nil
 }
+
+// WrapVersion returns the wrapped version based on the provided version string and underlying application version.
+// TODO: decide how we want to handle the underlying app. For now I made it backwards compatible.
+// https://github.com/cosmos/ibc-go/issues/7063
+func (IBCMiddleware) WrapVersion(cbVersion, underlyingAppVersion string) string {
+	// ignore underlying app version
+	return cbVersion
+}
+
+// UnwrapVersionUnsafe returns the version. Interchain accounts does not wrap versions.
+func (IBCMiddleware) UnwrapVersionUnsafe(version string) (string, string, error) {
+	// ignore underlying app version
+	return version, "", nil
+}

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

@@ -113,72 +113,28 @@ func SetupICAPath(path *ibctesting.Path, owner string) error {
 
 func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 	var (
-		channel  *channeltypes.Channel
-		isNilApp bool
-		path     *ibctesting.Path
+		channel      *channeltypes.Channel
+		counterparty channeltypes.Counterparty
+		path         *ibctesting.Path
 	)
 
 	testCases := []struct {
 		name     string
 		malleate func()
-		expPass  bool
+		expError error
 	}{
 		{
-			"success", func() {}, true,
-		},
-		{
-			"ICA auth module does not claim channel capability", func() {
-				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
-					portID, channelID string,
-					counterparty channeltypes.Counterparty, version string,
-				) (string, error) {
-					return version, nil
-				}
-			}, true,
-		},
-		{
-			"ICA auth module modification of channel version is ignored", func() {
-				// NOTE: explicitly modify the channel version via the auth module callback,
-				// ensuring the expected JSON encoded metadata is not modified upon return
-				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
-					portID, channelID string,
-					counterparty channeltypes.Counterparty, version string,
-				) (string, error) {
-					return "invalid-version", nil
-				}
-			}, true,
+			"success", func() {}, nil,
 		},
 		{
 			"controller submodule disabled", func() {
 				suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(suite.chainA.GetContext(), types.NewParams(false))
-			}, false,
-		},
-		{
-			"ICA auth module callback fails", func() {
-				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
-					portID, channelID string,
-					counterparty channeltypes.Counterparty, version string,
-				) (string, error) {
-					return "", fmt.Errorf("mock ica auth fails")
-				}
-			}, false,
-		},
-		{
-			"nil underlying app", func() {
-				isNilApp = true
-			}, true,
+			}, types.ErrControllerSubModuleDisabled,
 		},
 		{
-			"middleware disabled", func() {
-				suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ConnectionID)
-
-				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
-					portID, channelID string,
-					counterparty channeltypes.Counterparty, version string,
-				) (string, error) {
-					return "", fmt.Errorf("error should be unreachable")
-				}
-			}, true,
+			"keeper call fails - counterparty portID incorrect", func() {
+				counterparty.PortId = "invalid-portID"
+			}, icatypes.ErrInvalidHostPort,
 		},
 	}
 
@@ -188,7 +144,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 
 			suite.Run(tc.name, func() {
 				suite.SetupTest() // reset
-				isNilApp = false
 
 				path = NewICAPath(suite.chainA, suite.chainB, ordering)
 				path.SetupConnections()
@@ -206,7 +161,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 				suite.chainA.GetSimApp().ICAControllerKeeper.SetMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ConnectionID)
 
 				// default values
-				counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
+				counterparty = channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
 				channel = &channeltypes.Channel{
 					State:          channeltypes.INIT,
 					Ordering:       ordering,
@@ -220,21 +175,21 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 				// ensure channel on chainA is set in state
 				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.SetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, *channel)
 
-				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)
 
-				if isNilApp {
-					cbs = controller.NewIBCMiddleware(suite.chainA.GetSimApp().ICAControllerKeeper)
-				}
+				legacyModule, ok := cbs.(*porttypes.LegacyIBCModule)
+				suite.Require().True(ok, "expected there to be a single legacy ibc module")
+
+				legacyModuleCbs := legacyModule.GetCallbacks()
+				controllerModule, ok := legacyModuleCbs[1].(controller.IBCMiddleware) // fee module is routed second
+				suite.Require().True(ok)
 
-				version, err := cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.ConnectionHops,
-					path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, channel.Counterparty, channel.Version,
+				version, err := controllerModule.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.ConnectionHops,
+					path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, counterparty, channel.Version,
 				)
 
-				if tc.expPass {
+				if tc.expError == nil {
 					suite.Require().Equal(TestVersion, version)
 					suite.Require().NoError(err)
 				} else {

modules/apps/29-fee/ibc_middleware.go

@@ -2,6 +2,7 @@ package fee
 
 import (
 	"encoding/json"
+	"fmt"
 	"strings"
 
 	errorsmod "cosmossdk.io/errors"
@@ -21,6 +22,7 @@ var (
 	_ porttypes.Middleware            = (*IBCMiddleware)(nil)
 	_ porttypes.PacketDataUnmarshaler = (*IBCMiddleware)(nil)
 	_ porttypes.UpgradableModule      = (*IBCMiddleware)(nil)
+	_ porttypes.VersionWrapper        = (*IBCMiddleware)(nil)
 )
 
 // IBCMiddleware implements the ICS26 callbacks for the fee middleware given the
@@ -48,45 +50,12 @@ func (im IBCMiddleware) OnChanOpenInit(
 	counterparty channeltypes.Counterparty,
 	version string,
 ) (string, error) {
-	var versionMetadata types.Metadata
-
-	if strings.TrimSpace(version) == "" {
-		// default version
-		versionMetadata = types.Metadata{
-			FeeVersion: types.Version,
-			AppVersion: "",
-		}
-	} else {
-		metadata, err := types.MetadataFromVersion(version)
-		if err != nil {
-			// Since it is valid for fee version to not be specified, the above middleware version may be for a middleware
-			// lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying
-			// application.
-			return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
-				counterparty, version)
-		}
-		versionMetadata = metadata
-	}
-
-	if versionMetadata.FeeVersion != types.Version {
-		return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion)
-	}
-
-	appVersion, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, counterparty, versionMetadata.AppVersion)
-	if err != nil {
-		return "", err
-	}
-
-	versionMetadata.AppVersion = appVersion
-	versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata)
-	if err != nil {
-		return "", err
+	if strings.TrimSpace(version) != "" && version != types.Version {
+		return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, version)
 	}
 
 	im.keeper.SetFeeEnabled(ctx, portID, channelID)
-
-	// call underlying app's OnChanOpenInit callback with the appVersion
-	return string(versionBytes), nil
+	return types.Version, nil
 }
 
 // OnChanOpenTry implements the IBCMiddleware interface
@@ -501,3 +470,30 @@ func unwrapAppVersion(channelVersion string) string {
 
 	return metadata.AppVersion
 }
+
+// WrapVersion returns the wrapped ics29 version based on the provided ics29 version and the underlying application version.
+func (IBCMiddleware) WrapVersion(cbVersion, underlyingAppVersion string) string {
+	if cbVersion != types.Version {
+		panic(fmt.Errorf("invalid ics29 version provided. expected: %s, got: %s", types.Version, cbVersion))
+	}
+
+	metadata := types.Metadata{
+		FeeVersion: cbVersion,
+		AppVersion: underlyingAppVersion,
+	}
+
+	versionBytes := types.ModuleCdc.MustMarshalJSON(&metadata)
+
+	return string(versionBytes)
+}
+
+// UnwrapVersionUnsafe attempts to unmarshal the version string into a ics29 version. An error is returned if unsuccessful. 
+func (IBCMiddleware) UnwrapVersionUnsafe(version string) (string, string, error) {
+	metadata, err := types.MetadataFromVersion(version)
+	if err != nil {
+		// not an ics29 version
+		return "", version, err
+	}
+
+	return metadata.FeeVersion, metadata.AppVersion, nil
+}

modules/apps/29-fee/ibc_middleware_test.go

@@ -29,46 +29,24 @@ var (
 // Tests OnChanOpenInit on ChainA
 func (suite *FeeTestSuite) TestOnChanOpenInit() {
 	testCases := []struct {
-		name         string
-		version      string
-		expPass      bool
-		isFeeEnabled bool
+		name     string
+		version  string
+		expError error
 	}{
 		{
-			"success - valid fee middleware and mock version",
-			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version})),
-			true,
-			true,
+			"success - valid fee version",
+			types.Version,
+			nil,
 		},
 		{
-			"success - fee version not included, only perform mock logic",
-			ibcmock.Version,
-			true,
-			false,
+			"success - empty version",
+			"",
+			nil,
 		},
 		{
 			"invalid fee middleware version",
-			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: "invalid-ics29-1", AppVersion: ibcmock.Version})),
-			false,
-			false,
-		},
-		{
-			"invalid mock version",
-			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: "invalid-mock-version"})),
-			false,
-			false,
-		},
-		{
-			"mock version not wrapped",
-			types.Version,
-			false,
-			false,
-		},
-		{
-			"passing an empty string returns default version",
-			"",
-			true,
-			true,
+			"invalid-ics29-1",
+			types.ErrInvalidVersion,
 		},
 	}
 
@@ -81,17 +59,6 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() {
 				suite.SetupTest()
 				suite.path.SetupConnections()
 
-				// setup mock callback
-				suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
-					portID, channelID string,
-					counterparty channeltypes.Counterparty, version string,
-				) (string, error) {
-					if version != ibcmock.Version {
-						return "", fmt.Errorf("incorrect mock version")
-					}
-					return ibcmock.Version, nil
-				}
-
 				suite.path.EndpointA.ChannelID = ibctesting.FirstChannelID
 
 				counterparty := channeltypes.NewCounterparty(suite.path.EndpointB.ChannelConfig.PortID, suite.path.EndpointB.ChannelID)
@@ -103,34 +70,24 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() {
 					Version:        tc.version,
 				}
 
-				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)
 
-				version, err := cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.ConnectionHops,
+				version, err := feeModule.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.ConnectionHops,
 					suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, counterparty, channel.Version)
 
-				if tc.expPass {
-					// check if the channel is fee enabled. If so version string should include metaData
-					if tc.isFeeEnabled {
-						versionMetadata := types.Metadata{
-							FeeVersion: types.Version,
-							AppVersion: ibcmock.Version,
-						}
-
-						versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata)
-						suite.Require().NoError(err)
-
-						suite.Require().Equal(version, string(versionBytes))
-					} else {
-						suite.Require().Equal(ibcmock.Version, version)
-					}
-
+				if tc.expError == nil {
+					suite.Require().Equal(types.Version, version)
 					suite.Require().NoError(err, "unexpected error from version: %s", tc.version)
 				} else {
-					suite.Require().Error(err, "error not returned for version: %s", tc.version)
+					suite.Require().ErrorIs(err, tc.expError, "error not returned for version: %s", tc.version)
 					suite.Require().Equal("", version)
 				}
 			})