From 15a328091bc2b6760b88a14c89e7f77708183f69 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 8 Jun 2022 11:28:08 +0200 Subject: [PATCH] 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 Co-authored-by: Carlos Rodriguez --- CHANGELOG.md | 1 + docs/migrations/v3-to-v4.md | 60 ++++++++++++++++++- .../controller/ibc_middleware_test.go | 9 ++- .../controller/keeper/account.go | 36 +++-------- .../controller/keeper/account_test.go | 32 +++++++--- .../controller/keeper/keeper_test.go | 2 +- .../host/ibc_module_test.go | 2 +- .../host/keeper/keeper_test.go | 2 +- testing/simapp/app.go | 14 +++-- 9 files changed, 111 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 104c9bb142e..bb092156aad 100644 --- 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 index 5f684581ab2..a6a90055a69 100644 --- a/docs/migrations/v3-to-v4.md +++ b/docs/migrations/v3-to-v4.md @@ -18,7 +18,7 @@ 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`. @@ -26,6 +26,64 @@ This is an API breaking change and as such IBC application developers will have 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 index adaffcb83f9..1c3660c54b5 100644 --- 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 index 282b6e2e77a..2cb5e023e22 100644 --- 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 index 9387baa3936..0f8c89c51a8 100644 --- 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 index a0772f1e648..76020e17c87 100644 --- 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 index db220f86026..78a21cfee01 100644 --- 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 index 609fb06884d..98f9c06bf25 100644 --- 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 index b09cf6dffb7..94d5640aa17 100644 --- 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,15 +424,21 @@ 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. @@ -440,7 +446,7 @@ func NewSimApp( // 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