imp(apps): allow one sided fee middleware handshakes to complete

CHANGELOG.md

@@ -57,6 +57,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Improvements
 
 * (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able.
+* (apps/27-interchain-accounts, apps/tranfer, apps/29-fee) [\#6253](https://github.com/cosmos/ibc-go/pull/6253) Allow channel handshake to succeed if fee middleware is wired up on one side, but not the other.
 
 ### Features
 

modules/apps/27-interchain-accounts/host/keeper/handshake.go

@@ -30,13 +30,21 @@ func (k Keeper) OnChanOpenTry(
 	counterparty channeltypes.Counterparty,
 	counterpartyVersion string,
 ) (string, error) {
+	logger := k.Logger(ctx)
 	if portID != icatypes.HostPortID {
 		return "", errorsmod.Wrapf(icatypes.ErrInvalidHostPort, "expected %s, got %s", icatypes.HostPortID, portID)
 	}
 
 	metadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
 	if err != nil {
-		return "", err
+		// Propose the default metadata if the counterparty version is invalid
+		connection, err := k.channelKeeper.GetConnection(ctx, connectionHops[0])
+		if err != nil {
+			return "", errorsmod.Wrapf(err, "failed to retrieve connection %s", connectionHops[0])
+		}
+
+		logger.Debug("counterparty version is invalid, proposing default metadata")
+		metadata = icatypes.NewDefaultMetadata(connection.Counterparty.ConnectionId, connectionHops[0])
 	}
 
 	// set here the HostConnectionId in case the controller did not set it

modules/apps/27-interchain-accounts/host/keeper/handshake_test.go

@@ -198,9 +198,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
 		{
 			"invalid metadata bytestring",
 			func() {
+				// the try step will propose a new valid version
 				path.EndpointA.ChannelConfig.Version = "invalid-metadata-bytestring"
 			},
-			false,
+			true,
 		},
 		{
 			"unsupported encoding format",

modules/apps/29-fee/fee_test.go

@@ -1,13 +1,22 @@
 package fee_test
 
 import (
+	"reflect"
 	"testing"
+	"unsafe"
 
 	testifysuite "github.com/stretchr/testify/suite"
 
+	icacontroller "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller"
+	icacontrollertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
+	icahost "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host"
+	icahosttypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
 	"github.com/cosmos/ibc-go/v8/modules/apps/29-fee/types"
+	"github.com/cosmos/ibc-go/v8/modules/apps/transfer"
+	transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
 	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
+	porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types"
 	ibctesting "github.com/cosmos/ibc-go/v8/testing"
 	ibcmock "github.com/cosmos/ibc-go/v8/testing/mock"
 )
@@ -55,6 +64,49 @@ func (suite *FeeTestSuite) CreateMockPacket() channeltypes.Packet {
 	)
 }
 
+// RemoveFeeMiddleware removes:
+// - Fee middleware from transfer module
+// - Fee middleware from icahost submodule
+// - Fee middleware from icacontroller submodule
+// - The entire mock-fee module
+//
+// It does this by overriding the IBC router with a new router.
+func RemoveFeeMiddleware(chain *ibctesting.TestChain) {
+	channelKeeper := chain.GetSimApp().IBCKeeper.ChannelKeeper
+
+	// Unseal the IBC router by force
+	r := reflect.ValueOf(chain.GetSimApp().IBCKeeper.PortKeeper.Router).Elem()
+
+	f := r.FieldByName("sealed")
+	if !f.IsValid() {
+		panic("field not found")
+	}
+	if !f.CanSet() { // Check if we can set the field
+		f = reflect.NewAt(f.Type(), unsafe.Pointer(f.UnsafeAddr())).Elem()
+	}
+
+	f.SetBool(false) // Use reflection to bypass unexported status
+
+	newRouter := porttypes.NewRouter() // Create a new router
+	// Remove Fee middleware from transfer module
+	chain.GetSimApp().TransferKeeper.WithICS4Wrapper(channelKeeper)
+	transferStack := transfer.NewIBCModule(chain.GetSimApp().TransferKeeper)
+	newRouter.AddRoute(transfertypes.ModuleName, transferStack)
+
+	// Remove Fee middleware from icahost submodule
+	chain.GetSimApp().ICAHostKeeper.WithICS4Wrapper(channelKeeper)
+	icaHostStack := icahost.NewIBCModule(chain.GetSimApp().ICAHostKeeper)
+	newRouter.AddRoute(icahosttypes.SubModuleName, icaHostStack)
+
+	// Remove Fee middleware from icacontroller submodule
+	chain.GetSimApp().ICAControllerKeeper.WithICS4Wrapper(channelKeeper)
+	icaControllerStack := icacontroller.NewIBCMiddleware(nil, chain.GetSimApp().ICAControllerKeeper)
+	newRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack)
+
+	// Override and seal the router
+	chain.GetSimApp().IBCKeeper.SetRouter(newRouter)
+}
+
 // helper function
 func lockFeeModule(chain *ibctesting.TestChain) {
 	ctx := chain.GetContext()

modules/apps/29-fee/ibc_middleware.go

@@ -139,12 +139,13 @@ func (im IBCMiddleware) OnChanOpenAck(
 	counterpartyChannelID string,
 	counterpartyVersion string,
 ) error {
-	// If handshake was initialized with fee enabled it must complete with fee enabled.
-	// If handshake was initialized with fee disabled it must complete with fee disabled.
 	if im.keeper.IsFeeEnabled(ctx, portID, channelID) {
 		versionMetadata, err := types.MetadataFromVersion(counterpartyVersion)
 		if err != nil {
-			return errorsmod.Wrapf(err, "failed to unmarshal ICS29 counterparty version metadata: %s", counterpartyVersion)
+			// 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 {

modules/apps/29-fee/ica_test.go

@@ -181,6 +181,31 @@ func (suite *FeeTestSuite) TestFeeInterchainAccounts() {
 	suite.Require().Equal(preEscrowBalance.SubAmount(defaultRecvFee.AmountOf(sdk.DefaultBondDenom)), postDistBalance)
 }
 
+func (suite *FeeTestSuite) TestOnesidedFeeMiddlewareICAHandshake() {
+	RemoveFeeMiddleware(suite.chainB) // remove fee middleware from chainB
+
+	path := NewIncentivizedICAPath(suite.chainA, suite.chainB)
+
+	path.SetupConnections()
+
+	err := SetupPath(path, defaultOwnerAddress)
+	suite.Require().NoError(err)
+
+	// assert the newly established channel is not fee enabled on chainB
+	interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID)
+	suite.Require().True(found)
+
+	expVersionMetadata, err := icatypes.MetadataFromVersion(defaultICAVersion)
+	suite.Require().NoError(err)
+
+	expVersionMetadata.Address = interchainAccountAddr
+
+	expVersion := string(icatypes.ModuleCdc.MustMarshalJSON(&expVersionMetadata))
+
+	suite.Require().Equal(path.EndpointA.ChannelConfig.Version, expVersion)
+	suite.Require().Equal(path.EndpointB.ChannelConfig.Version, expVersion)
+}
+
 func buildInterchainAccountsPacket(path *ibctesting.Path, data []byte, seq uint64) channeltypes.Packet {
 	packet := channeltypes.NewPacket(
 		data,

modules/apps/29-fee/transfer_test.go

@@ -163,3 +163,19 @@ func (suite *FeeTestSuite) TestTransferFeeUpgrade() {
 		})
 	}
 }
+
+func (suite *FeeTestSuite) TestOnesidedFeeMiddlewareTransferHandshake() {
+	RemoveFeeMiddleware(suite.chainB) // remove fee middleware from chainB
+
+	path := ibctesting.NewPath(suite.chainA, suite.chainB)
+	feeTransferVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version}))
+	path.EndpointA.ChannelConfig.Version = feeTransferVersion // this will be renegotiated by the Try step
+	path.EndpointB.ChannelConfig.Version = ""                 // this will be overwritten by the Try step
+	path.EndpointA.ChannelConfig.PortID = transfertypes.PortID
+	path.EndpointB.ChannelConfig.PortID = transfertypes.PortID
+
+	path.Setup()
+
+	suite.Require().Equal(path.EndpointA.ChannelConfig.Version, transfertypes.Version)
+	suite.Require().Equal(path.EndpointB.ChannelConfig.Version, transfertypes.Version)
+}

modules/apps/transfer/ibc_module.go

@@ -112,19 +112,23 @@ func (im IBCModule) OnChanOpenTry(
 	counterparty channeltypes.Counterparty,
 	counterpartyVersion string,
 ) (string, error) {
+	logger := im.keeper.Logger(ctx)
 	if err := ValidateTransferChannelParams(ctx, im.keeper, order, portID, channelID); err != nil {
 		return "", err
 	}
 
-	if counterpartyVersion != types.Version {
-		return "", errorsmod.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: expected %s, got %s", types.Version, counterpartyVersion)
-	}
-
 	// OpenTry must claim the channelCapability that IBC passes into the callback
 	if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
 		return "", err
 	}
 
+	if counterpartyVersion != types.Version {
+		// Propose the current version
+		logger.Debug(fmt.Sprintf("invalid counterparty version: expected %s, got %s", types.Version, counterpartyVersion))
+		logger.Debug(fmt.Sprintf("proposing the current version: %s", types.Version))
+		return types.Version, nil
+	}
+
 	return types.Version, nil
 }
 

modules/apps/transfer/ibc_module_test.go

@@ -132,31 +132,32 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
 			"success", func() {}, nil,
 		},
 		{
-			"max channels reached", func() {
+			"success: invalid counterparty version", func() {
+				// transfer module will propose the default version
+				counterpartyVersion = "version"
+			}, nil,
+		},
+		{
+			"failure: max channels reached", func() {
 				path.EndpointA.ChannelID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1)
 			}, types.ErrMaxTransferChannels,
 		},
 		{
-			"capability already claimed", func() {
+			"failure: capability already claimed", func() {
 				err := suite.chainA.GetSimApp().ScopedTransferKeeper.ClaimCapability(suite.chainA.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
 				suite.Require().NoError(err)
 			}, capabilitytypes.ErrOwnerClaimed,
 		},
 		{
-			"invalid order - ORDERED", func() {
+			"failure: invalid order - ORDERED", func() {
 				channel.Ordering = channeltypes.ORDERED
 			}, channeltypes.ErrInvalidChannelOrdering,
 		},
 		{
-			"invalid port ID", func() {
+			"failure: invalid port ID", func() {
 				path.EndpointA.ChannelConfig.PortID = ibctesting.MockPort
 			}, porttypes.ErrInvalidPort,
 		},
-		{
-			"invalid counterparty version", func() {
-				counterpartyVersion = "version"
-			}, types.ErrInvalidVersion,
-		},
 	}
 
 	for _, tc := range testCases {

testing/endpoint.go

@@ -342,6 +342,7 @@ func (endpoint *Endpoint) ChanOpenInit() error {
 	// update version to selected app version
 	// NOTE: this update must be performed after SendMsgs()
 	endpoint.ChannelConfig.Version = endpoint.GetChannel().Version
+	endpoint.Counterparty.ChannelConfig.Version = endpoint.GetChannel().Version
 
 	return nil
 }
@@ -374,6 +375,7 @@ func (endpoint *Endpoint) ChanOpenTry() error {
 	// update version to selected app version
 	// NOTE: this update must be performed after the endpoint channelID is set
 	endpoint.ChannelConfig.Version = endpoint.GetChannel().Version
+	endpoint.Counterparty.ChannelConfig.Version = endpoint.GetChannel().Version
 
 	return nil
 }