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

* imp: added counter version proposal to try step * test: fixed tests * test: tested removing fee middleware * test: added ica onesided test * add changelog * imp: improved RemoveFeeMiddleware * nit: damian --------- Co-authored-by: Carlos Rodriguez <[email protected]> (cherry picked from commit 3b3ecc5) # Conflicts: # CHANGELOG.md # modules/apps/transfer/ibc_module_test.go
cosmos · May 7, 2024 · 5503562 · 5503562
1 parent 0b77b98
commit 5503562
Show file tree

Hide file tree

Showing 10 changed files with 123 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -44,6 +44,12 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 
+<<<<<<< HEAD
+=======
+* (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.
+
+>>>>>>> 3b3ecc5a (imp(apps): allow one sided fee middleware handshakes to complete (#6253))
 ### Features
 
 * (core) [\#6055](https://github.com/cosmos/ibc-go/pull/6055) Introduce a new interface `ConsensusHost` used to validate an IBC `ClientState` and `ConsensusState` against the host chain's underlying consensus parameters.

diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake.go b/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])
 	}
 
 	if err = icatypes.ValidateHostMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil {

diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go
@@ -185,9 +185,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",

diff --git a/modules/apps/29-fee/fee_test.go b/modules/apps/29-fee/fee_test.go
@@ -5,9 +5,16 @@ import (
 
 	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"
 )
@@ -64,6 +71,39 @@ 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
+	chain.GetSimApp().IBCKeeper.PortKeeper.Router = nil
+
+	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()

diff --git a/modules/apps/29-fee/ibc_middleware.go b/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 {

diff --git a/modules/apps/29-fee/ica_test.go b/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,

diff --git a/modules/apps/29-fee/transfer_test.go b/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)
+}
diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go
@@ -115,15 +115,17 @@ func (im IBCModule) OnChanOpenTry(
 		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
+		im.keeper.Logger(ctx).Debug("invalid counterparty version, proposing current app version", "counterpartyVersion", counterpartyVersion, "version", types.Version)
+		return types.Version, nil
+	}
+
 	return types.Version, nil
 }
 

diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go
@@ -130,31 +130,40 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
 			"success", func() {}, true,
 		},
 		{
-			"max channels reached", func() {
+			"success: invalid counterparty version proposes new 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)
 			}, false,
 		},
 		{
-			"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)
 			}, false,
 		},
 		{
-			"invalid order - ORDERED", func() {
+			"failure: invalid order - ORDERED", func() {
 				channel.Ordering = channeltypes.ORDERED
 			}, false,
 		},
 		{
-			"invalid port ID", func() {
+			"failure: invalid port ID", func() {
 				path.EndpointA.ChannelConfig.PortID = ibctesting.MockPort
 			}, false,
 		},
+<<<<<<< HEAD
 		{
 			"invalid counterparty version", func() {
 				counterpartyVersion = "version"
 			}, false,
 		},
+=======
+>>>>>>> 3b3ecc5a (imp(apps): allow one sided fee middleware handshakes to complete (#6253))
 	}
 
 	for _, tc := range testCases {

diff --git a/testing/endpoint.go b/testing/endpoint.go
@@ -336,6 +336,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
 }
@@ -368,6 +369,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
 }