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/27-interchain-accounts/host/keeper/handshake.go
#	modules/apps/29-fee/fee_test.go
#	modules/apps/29-fee/ibc_middleware.go
#	modules/apps/29-fee/transfer_test.go
#	modules/apps/transfer/ibc_module.go
#	modules/apps/transfer/ibc_module_test.go

CHANGELOG.md

@@ -46,8 +46,13 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 
+<<<<<<< HEAD
 * (apps/27-interchain-accounts) [\#6147](https://github.com/cosmos/ibc-go/pull/6147) Emit an event signalling that the host submodule is disabled.
 * (testing) [\#6180](https://github.com/cosmos/ibc-go/pull/6180) Add version to tm abci headers in ibctesting. 
+=======
+* (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
 

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

@@ -27,13 +27,27 @@ func (k Keeper) OnChanOpenTry(
 	counterparty channeltypes.Counterparty,
 	counterpartyVersion string,
 ) (string, error) {
+	logger := k.Logger(ctx)
 	if portID != icatypes.HostPortID {
 		return "", sdkerrors.Wrapf(icatypes.ErrInvalidHostPort, "expected %s, got %s", icatypes.HostPortID, portID)
 	}
 
+<<<<<<< HEAD
 	var metadata icatypes.Metadata
 	if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(counterpartyVersion), &metadata); err != nil {
 		return "", sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
+=======
+	metadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
+	if err != nil {
+		// 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])
+>>>>>>> 3b3ecc5a (imp(apps): allow one sided fee middleware handshakes to complete (#6253))
 	}
 
 	if err := icatypes.ValidateHostMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil {

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

@@ -173,9 +173,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

@@ -5,11 +5,26 @@ import (
 
 	"github.com/stretchr/testify/suite"
 
+<<<<<<< HEAD
 	"github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
 	clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
 	ibctesting "github.com/cosmos/ibc-go/v7/testing"
 	ibcmock "github.com/cosmos/ibc-go/v7/testing/mock"
+=======
+	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"
+>>>>>>> 3b3ecc5a (imp(apps): allow one sided fee middleware handshakes to complete (#6253))
 )
 
 type FeeTestSuite struct {
@@ -64,6 +79,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()

modules/apps/29-fee/ibc_middleware.go

@@ -135,12 +135,19 @@ 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) {
+<<<<<<< HEAD
 		var versionMetadata types.Metadata
 		if err := types.ModuleCdc.UnmarshalJSON([]byte(counterpartyVersion), &versionMetadata); err != nil {
 			return sdkerrors.Wrapf(err, "failed to unmarshal ICS29 counterparty version metadata: %s", counterpartyVersion)
+=======
+		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)
+>>>>>>> 3b3ecc5a (imp(apps): allow one sided fee middleware handshakes to complete (#6253))
 		}
 
 		if versionMetadata.FeeVersion != types.Version {

modules/apps/29-fee/ica_test.go

@@ -182,6 +182,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

@@ -68,3 +68,117 @@ func (suite *FeeTestSuite) TestFeeTransfer() {
 		fee.AckFee.Add(fee.TimeoutFee...), // ack fee paid, timeout fee refunded
 		sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)).Sub(originalChainASenderAccountBalance[0]))
 }
+<<<<<<< HEAD
+=======
+
+func (suite *FeeTestSuite) TestTransferFeeUpgrade() {
+	var path *ibctesting.Path
+
+	testCases := []struct {
+		name     string
+		malleate func()
+		expError error
+	}{
+		{
+			"success",
+			func() {},
+			nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		suite.Run(tc.name, func() {
+			suite.SetupTest()
+
+			path = ibctesting.NewPath(suite.chainA, suite.chainB)
+
+			// configure the initial path to create a regular transfer channel
+			path.EndpointA.ChannelConfig.PortID = transfertypes.PortID
+			path.EndpointB.ChannelConfig.PortID = transfertypes.PortID
+			path.EndpointA.ChannelConfig.Version = transfertypes.Version
+			path.EndpointB.ChannelConfig.Version = transfertypes.Version
+
+			path.Setup()
+
+			// configure the channel upgrade to upgrade to an incentivized fee enabled transfer channel
+			upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version}))
+			path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
+			path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
+
+			tc.malleate()
+
+			err := path.EndpointA.ChanUpgradeInit()
+			suite.Require().NoError(err)
+
+			err = path.EndpointB.ChanUpgradeTry()
+			suite.Require().NoError(err)
+
+			err = path.EndpointA.ChanUpgradeAck()
+			suite.Require().NoError(err)
+
+			err = path.EndpointB.ChanUpgradeConfirm()
+			suite.Require().NoError(err)
+
+			err = path.EndpointA.ChanUpgradeOpen()
+			suite.Require().NoError(err)
+
+			expPass := tc.expError == nil
+			if expPass {
+				channelA := path.EndpointA.GetChannel()
+				suite.Require().Equal(upgradeVersion, channelA.Version)
+
+				channelB := path.EndpointB.GetChannel()
+				suite.Require().Equal(upgradeVersion, channelB.Version)
+
+				isFeeEnabled := suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+				suite.Require().True(isFeeEnabled)
+
+				isFeeEnabled = suite.chainB.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
+				suite.Require().True(isFeeEnabled)
+
+				fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee)
+				msgs := []sdk.Msg{
+					types.NewMsgPayPacketFee(fee, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, suite.chainA.SenderAccount.GetAddress().String(), nil),
+					transfertypes.NewMsgTransfer(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, ibctesting.TestCoin, suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), clienttypes.NewHeight(1, 100), 0, ""),
+				}
+
+				res, err := suite.chainA.SendMsgs(msgs...)
+				suite.Require().NoError(err) // message committed
+
+				feeEscrowAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(types.ModuleName)
+				escrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), feeEscrowAddr, sdk.DefaultBondDenom)
+				suite.Require().Equal(escrowBalance.Amount, fee.Total().AmountOf(sdk.DefaultBondDenom))
+
+				packet, err := ibctesting.ParsePacketFromEvents(res.Events)
+				suite.Require().NoError(err)
+
+				err = path.RelayPacket(packet)
+				suite.Require().NoError(err) // relay committed
+
+				escrowBalance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), feeEscrowAddr, sdk.DefaultBondDenom)
+				suite.Require().True(escrowBalance.IsZero())
+			} else {
+				suite.Require().Error(err)
+				suite.Require().ErrorIs(err, tc.expError)
+			}
+		})
+	}
+}
+
+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)
+}
+>>>>>>> 3b3ecc5a (imp(apps): allow one sided fee middleware handshakes to complete (#6253))

modules/apps/transfer/ibc_module.go

@@ -112,15 +112,24 @@ func (im IBCModule) OnChanOpenTry(
 		return "", err
 	}
 
+<<<<<<< HEAD
 	if counterpartyVersion != types.Version {
 		return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: got: %s, expected %s", counterpartyVersion, types.Version)
 	}
 
+=======
+>>>>>>> 3b3ecc5a (imp(apps): allow one sided fee middleware handshakes to complete (#6253))
 	// 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
 }
 

modules/apps/transfer/ibc_module_test.go

@@ -121,31 +121,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 {

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
 }