feat: adding fee middleware support to ics27 interchain accounts (#1432)

* updating simapp to include ics29 fee in ica stacks * updating RegisterInterchainAccount to pass through version arg and support fee middleware functionality * updating tests to support additional version arg in RegisterInterchainAccount * adding migration docs for ICS27 fee middleware support * remove unnecessary spacing * fixing typo in godoc * adding changelog entry * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>
cosmos · Jun 8, 2022 · 15a3280 · 15a3280
1 parent 042d818
commit 15a3280
Show file tree

Hide file tree

Showing 9 changed files with 111 additions and 47 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -46,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) The `OnChanOpenInit` application callback now returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629).  
 * (modules/29-fee)[\#1338](https://github.com/cosmos/ibc-go/pull/1338) Renaming `Result` field in `IncentivizedAcknowledgement` to `AppAcknowledgement`.
 * (modules/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1343) Renaming `KeyForwardRelayerAddress` to `KeyRelayerAddressForAsyncAck`, and `ParseKeyForwardRelayerAddress` to `ParseKeyRelayerAddressForAsyncAck`.
+* (apps/27-interchain-accounts)[\#1432](https://github.com/cosmos/ibc-go/pull/1432) Updating `RegisterInterchainAccount` to include an additional `version` argument, supporting ICS29 fee middleware functionality in ICS27 interchain accounts.
 
 ### State Machine Breaking
 

diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md
@@ -18,14 +18,72 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g
 
 ## Chains
 
-### IS04 - Channel 
+### ICS04 - Channel 
 
 The `WriteAcknowledgement` API now takes the `exported.Acknowledgement` type instead of passing in the acknowledgement byte array directly. 
 This is an API breaking change and as such IBC application developers will have to update any calls to `WriteAcknowledgement`. 
 
 The `OnChanOpenInit` application callback has been modified.
 The return signature now includes the application version as detailed in the latest IBC [spec changes](https://github.com/cosmos/ibc/pull/629).
 
+### ICS27 - Interchain Accounts
+
+The `RegisterInterchainAccount` API has been modified to include an additional `version` argument. This change has been made in order to support ICS29 fee middelware, for relayer incentivization of ICS27 packets.
+Consumers of the `RegisterInterchainAccount` are now expected to build the appropriate JSON encoded version string themselves and pass it accordingly. 
+This should be constructed within the interchain accounts authentication module which leverages the APIs exposed via the interchain accounts `controllerKeeper`. 
+
+The following code snippet illustrates how to construct an appropriate interchain accounts `Metadata` and encode it as a JSON bytestring:
+
+```go
+icaMetadata := icatypes.Metadata{
+    Version:                icatypes.Version,
+    ControllerConnectionId: controllerConnectionID,
+    HostConnectionId:       hostConnectionID,
+    Encoding:               icatypes.EncodingProtobuf,
+    TxType:                 icatypes.TxTypeSDKMultiMsg,
+}
+
+appVersion, err := icatypes.ModuleCdc.MarshalJSON(&icaMetadata)
+if err != nil {
+    return err
+}
+
+if err := k.icaControllerKeeper.RegisterInterchainAccount(ctx, msg.ConnectionId, msg.Owner, string(appVersion)); err != nil {
+    return err
+}
+```
+
+Similarly, if the application stack is configured to route through ICS29 fee middleware and a fee enabled channel is desired, construct the appropriate ICS29 `Metadata` type:
+
+```go
+icaMetadata := icatypes.Metadata{
+    Version:                icatypes.Version,
+    ControllerConnectionId: controllerConnectionID,
+    HostConnectionId:       hostConnectionID,
+    Encoding:               icatypes.EncodingProtobuf,
+    TxType:                 icatypes.TxTypeSDKMultiMsg,
+}
+
+appVersion, err := icatypes.ModuleCdc.MarshalJSON(&icaMetadata)
+if err != nil {
+    return err
+}
+
+feeMetadata := feetypes.Metadata{
+    AppVersion: string(appVersion),
+    FeeVersion: feetypes.Version,
+}
+
+feeEnabledVersion, err := feetypes.ModuleCdc.MarshalJSON(&feeMetadata)
+if err != nil {
+    return err
+}
+
+if err := k.icaControllerKeeper.RegisterInterchainAccount(ctx, msg.ConnectionId, msg.Owner, string(feeEnabledVersion)); err != nil {
+    return err
+}
+```
+
 ## Relayers
 
 When using the `DenomTrace` gRPC, the full IBC denomination with the `ibc/` prefix may now be passed in.
diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go
@@ -9,9 +9,9 @@ import (
 	"github.com/stretchr/testify/suite"
 	"github.com/tendermint/tendermint/crypto"
 
-	icacontroller "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller"
 	"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/types"
 	icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
+	fee "github.com/cosmos/ibc-go/v3/modules/apps/29-fee"
 	clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 	host "github.com/cosmos/ibc-go/v3/modules/core/24-host"
@@ -83,7 +83,7 @@ func RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) erro
 
 	channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext())
 
-	if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner); err != nil {
+	if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion); err != nil {
 		return err
 	}
 
@@ -714,9 +714,8 @@ func (suite *InterchainAccountsTestSuite) TestGetAppVersion() {
 	cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
 	suite.Require().True(ok)
 
-	controllerModule := cbs.(icacontroller.IBCMiddleware)
-
-	appVersion, found := controllerModule.GetAppVersion(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+	controllerStack := cbs.(fee.IBCMiddleware)
+	appVersion, found := controllerStack.GetAppVersion(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
 	suite.Require().True(found)
 	suite.Require().Equal(path.EndpointA.ChannelConfig.Version, appVersion)
 }
diff --git a/modules/apps/27-interchain-accounts/controller/keeper/account.go b/modules/apps/27-interchain-accounts/controller/keeper/account.go
@@ -9,12 +9,14 @@ import (
 	host "github.com/cosmos/ibc-go/v3/modules/core/24-host"
 )
 
-// RegisterInterchainAccount is the entry point to registering an interchain account.
-// It generates a new port identifier using the owner address. It will bind to the
-// port identifier and call 04-channel 'ChanOpenInit'. An error is returned if the port
-// identifier is already in use. Gaining access to interchain accounts whose channels
-// have closed cannot be done with this function. A regular MsgChanOpenInit must be used.
-func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, owner string) error {
+// RegisterInterchainAccount is the entry point to registering an interchain account:
+// - It generates a new port identifier using the provided owner string, binds to the port identifier and claims the associated capability.
+// - Callers are expected to provide the appropriate application version string.
+// - For example, this could be an ICS27 encoded metadata type or an ICS29 encoded metadata type with a nested application version.
+// - A new MsgChannelOpenInit is routed through the MsgServiceRouter, executing the OnOpenChanInit callback stack as configured.
+// - An error is returned if the port identifier is already in use. Gaining access to interchain accounts whose channels
+// have closed cannot be done with this function. A regular MsgChannelOpenInit must be used.
+func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, owner, version string) error {
 	portID, err := icatypes.NewControllerPortID(owner)
 	if err != nil {
 		return err
@@ -36,27 +38,7 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, owner s
 		}
 	}
 
-	connectionEnd, err := k.channelKeeper.GetConnection(ctx, connectionID)
-	if err != nil {
-		return err
-	}
-
-	// NOTE: An empty string is provided for accAddress, to be fulfilled upon OnChanOpenTry handshake step
-	metadata := icatypes.NewMetadata(
-		icatypes.Version,
-		connectionID,
-		connectionEnd.GetCounterparty().GetConnectionID(),
-		"",
-		icatypes.EncodingProtobuf,
-		icatypes.TxTypeSDKMultiMsg,
-	)
-
-	versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
-	if err != nil {
-		return err
-	}
-
-	msg := channeltypes.NewMsgChannelOpenInit(portID, string(versionBytes), channeltypes.ORDERED, []string{connectionID}, icatypes.PortID, icatypes.ModuleName)
+	msg := channeltypes.NewMsgChannelOpenInit(portID, version, channeltypes.ORDERED, []string{connectionID}, icatypes.PortID, icatypes.ModuleName)
 	handler := k.msgRouter.Handler(msg)
 
 	res, err := handler(ctx, msg)

diff --git a/modules/apps/27-interchain-accounts/controller/keeper/account_test.go b/modules/apps/27-interchain-accounts/controller/keeper/account_test.go
@@ -72,7 +72,7 @@ func (suite *KeeperTestSuite) TestRegisterInterchainAccount() {
 
 			tc.malleate() // malleate mutates test data
 
-			err = suite.chainA.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(suite.chainA.GetContext(), path.EndpointA.ConnectionID, owner)
+			err = suite.chainA.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(suite.chainA.GetContext(), path.EndpointA.ConnectionID, owner, TestVersion)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
@@ -88,15 +88,33 @@ func (suite *KeeperTestSuite) TestRegisterSameOwnerMultipleConnections() {
 
 	owner := TestOwnerAddress
 
-	path := NewICAPath(suite.chainA, suite.chainB)
-	suite.coordinator.SetupConnections(path)
+	pathAToB := NewICAPath(suite.chainA, suite.chainB)
+	suite.coordinator.SetupConnections(pathAToB)
 
-	path2 := NewICAPath(suite.chainA, suite.chainC)
-	suite.coordinator.SetupConnections(path2)
+	pathAToC := NewICAPath(suite.chainA, suite.chainC)
+	suite.coordinator.SetupConnections(pathAToC)
 
-	err := suite.chainA.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(suite.chainA.GetContext(), path.EndpointA.ConnectionID, owner)
+	// build ICS27 metadata with connection identifiers for path A->B
+	metadata := &icatypes.Metadata{
+		Version:                icatypes.Version,
+		ControllerConnectionId: pathAToB.EndpointA.ConnectionID,
+		HostConnectionId:       pathAToB.EndpointB.ConnectionID,
+		Encoding:               icatypes.EncodingProtobuf,
+		TxType:                 icatypes.TxTypeSDKMultiMsg,
+	}
+
+	err := suite.chainA.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(suite.chainA.GetContext(), pathAToB.EndpointA.ConnectionID, owner, string(icatypes.ModuleCdc.MustMarshalJSON(metadata)))
 	suite.Require().NoError(err)
 
-	err = suite.chainA.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(suite.chainA.GetContext(), path2.EndpointA.ConnectionID, owner)
+	// build ICS27 metadata with connection identifiers for path A->C
+	metadata = &icatypes.Metadata{
+		Version:                icatypes.Version,
+		ControllerConnectionId: pathAToC.EndpointA.ConnectionID,
+		HostConnectionId:       pathAToC.EndpointB.ConnectionID,
+		Encoding:               icatypes.EncodingProtobuf,
+		TxType:                 icatypes.TxTypeSDKMultiMsg,
+	}
+
+	err = suite.chainA.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(suite.chainA.GetContext(), pathAToC.EndpointA.ConnectionID, owner, string(icatypes.ModuleCdc.MustMarshalJSON(metadata)))
 	suite.Require().NoError(err)
 }
diff --git a/modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go
@@ -95,7 +95,7 @@ func RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) erro
 
 	channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext())
 
-	if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner); err != nil {
+	if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion); err != nil {
 		return err
 	}
 

diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go
@@ -86,7 +86,7 @@ func RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) erro
 
 	channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext())
 
-	if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner); err != nil {
+	if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion); err != nil {
 		return err
 	}
 

diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go
@@ -95,7 +95,7 @@ func RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) erro
 
 	channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext())
 
-	if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner); err != nil {
+	if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion); err != nil {
 		return err
 	}
 

diff --git a/testing/simapp/app.go b/testing/simapp/app.go
@@ -366,7 +366,7 @@ func NewSimApp(
 	// ICA Controller keeper
 	app.ICAControllerKeeper = icacontrollerkeeper.NewKeeper(
 		appCodec, keys[icacontrollertypes.StoreKey], app.GetSubspace(icacontrollertypes.SubModuleName),
-		app.IBCKeeper.ChannelKeeper, // may be replaced with middleware such as ics29 fee
+		app.IBCFeeKeeper, // use ics29 fee as ics4Wrapper in middleware stack
 		app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper,
 		scopedICAControllerKeeper, app.MsgServiceRouter(),
 	)
@@ -424,23 +424,29 @@ func NewSimApp(
 
 	// Create Interchain Accounts Stack
 	// SendPacket, since it is originating from the application to core IBC:
-	// icaAuthModuleKeeper.SendTx -> icaControllerKeeper.SendPacket -> channel.SendPacket
+	// icaAuthModuleKeeper.SendTx -> icaController.SendPacket -> fee.SendPacket -> channel.SendPacket
 
 	// initialize ICA module with mock module as the authentication module on the controller side
 	var icaControllerStack porttypes.IBCModule
 	icaControllerStack = ibcmock.NewIBCModule(&mockModule, ibcmock.NewMockIBCApp("", scopedICAMockKeeper))
 	app.ICAAuthModule = icaControllerStack.(ibcmock.IBCModule)
 	icaControllerStack = icacontroller.NewIBCMiddleware(icaControllerStack, app.ICAControllerKeeper)
+	icaControllerStack = ibcfee.NewIBCMiddleware(icaControllerStack, app.IBCFeeKeeper)
 
-	icaHostIBCModule := icahost.NewIBCModule(app.ICAHostKeeper)
+	// RecvPacket, message that originates from core IBC and goes down to app, the flow is:
+	// channel.RecvPacket -> fee.OnRecvPacket -> icaHost.OnRecvPacket
+
+	var icaHostStack porttypes.IBCModule
+	icaHostStack = icahost.NewIBCModule(app.ICAHostKeeper)
+	icaHostStack = ibcfee.NewIBCMiddleware(icaHostStack, app.IBCFeeKeeper)
 
 	// Add host, controller & ica auth modules to IBC router
 	ibcRouter.
 		// the ICA Controller middleware needs to be explicitly added to the IBC Router because the
 		// ICA controller module owns the port capability for ICA. The ICA authentication module
 		// owns the channel capability.
 		AddRoute(icacontrollertypes.SubModuleName, icaControllerStack).
-		AddRoute(icahosttypes.SubModuleName, icaHostIBCModule).
+		AddRoute(icahosttypes.SubModuleName, icaHostStack).
 		AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack) // ica with mock auth module stack route to ica (top level of middleware stack)
 
 	// Create Mock IBC Fee module stack for testing