From 203dc5fa264eec60a67626d19450d698138a7af2 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 30 Aug 2022 13:37:47 +0200 Subject: [PATCH 1/5] [WIP] adding current code, test failures due to removal of claim capaiblities in mock IBCApp --- .../controller/ibc_middleware.go | 7 +++- .../controller/ibc_middleware_test.go | 14 +++++++ .../controller/keeper/relay.go | 8 +++- .../controller/keeper/relay_test.go | 16 +------- modules/apps/29-fee/ibc_middleware_test.go | 37 +++++++++++++++++++ testing/mock/ibc_module.go | 11 ------ 6 files changed, 65 insertions(+), 28 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index cd09386f1e5..2da54dd3eda 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -10,6 +10,7 @@ import ( icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types" channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types" porttypes "github.com/cosmos/ibc-go/v5/modules/core/05-port/types" + host "github.com/cosmos/ibc-go/v5/modules/core/24-host" ibcexported "github.com/cosmos/ibc-go/v5/modules/core/exported" ) @@ -55,11 +56,15 @@ func (im IBCMiddleware) OnChanOpenInit( return "", err } + if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { + return "", err + } + // call underlying app's OnChanOpenInit callback with the passed in version // the version returned is discarded as the ica-auth module does not have permission to edit the version string. // ics27 will always return the version string containing the Metadata struct which is created during the `RegisterInterchainAccount` call. if im.app != nil { - if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil { + if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, nil, counterparty, version); err != nil { return "", err } } 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 f9c44a7cefe..6a4680a4dfc 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -134,6 +134,20 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { { "success", func() {}, true, }, + { + "ICA auth module does not claim channel capability", func() { + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, + portID, channelID string, chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, version string, + ) (string, error) { + if chanCap != nil { + return "", fmt.Errorf("channel capability should be nil") + } + + return version, nil + } + }, true, + }, { "ICA auth module modification of channel version is ignored", func() { // NOTE: explicitly modify the channel version via the auth module callback, diff --git a/modules/apps/27-interchain-accounts/controller/keeper/relay.go b/modules/apps/27-interchain-accounts/controller/keeper/relay.go index 57d92ebef99..fc8bc4ea005 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/relay.go @@ -8,6 +8,7 @@ import ( icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types" clienttypes "github.com/cosmos/ibc-go/v5/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types" + host "github.com/cosmos/ibc-go/v5/modules/core/24-host" ) // SendTx takes pre-built packet data containing messages to be executed on the host chain from an authentication module and attempts to send the packet. @@ -15,7 +16,7 @@ import ( // If the base application has the capability to send on the provided portID. An appropriate // absolute timeoutTimestamp must be provided. If the packet is timed out, the channel will be closed. // In the case of channel closure, a new channel may be reopened to reconnect to the host chain. -func (k Keeper) SendTx(ctx sdk.Context, chanCap *capabilitytypes.Capability, connectionID, portID string, icaPacketData icatypes.InterchainAccountPacketData, timeoutTimestamp uint64) (uint64, error) { +func (k Keeper) SendTx(ctx sdk.Context, _ *capabilitytypes.Capability, connectionID, portID string, icaPacketData icatypes.InterchainAccountPacketData, timeoutTimestamp uint64) (uint64, error) { activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionID, portID) if !found { return 0, sdkerrors.Wrapf(icatypes.ErrActiveChannelNotFound, "failed to retrieve active channel on connection %s for port %s", connectionID, portID) @@ -29,6 +30,11 @@ func (k Keeper) SendTx(ctx sdk.Context, chanCap *capabilitytypes.Capability, con destinationPort := sourceChannelEnd.GetCounterparty().GetPortID() destinationChannel := sourceChannelEnd.GetCounterparty().GetChannelID() + chanCap, found := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(portID, activeChannelID)) + if !found { + return 0, sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotFound, "failed to find capability: %s", host.ChannelCapabilityPath(portID, activeChannelID)) + } + if uint64(ctx.BlockTime().UnixNano()) >= timeoutTimestamp { return 0, icatypes.ErrInvalidTimeoutTimestamp } diff --git a/modules/apps/27-interchain-accounts/controller/keeper/relay_test.go b/modules/apps/27-interchain-accounts/controller/keeper/relay_test.go index 96ce41b3e23..58afd075bd4 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/relay_test.go @@ -3,12 +3,10 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" - capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types" clienttypes "github.com/cosmos/ibc-go/v5/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types" - host "github.com/cosmos/ibc-go/v5/modules/core/24-host" ibctesting "github.com/cosmos/ibc-go/v5/testing" ) @@ -16,7 +14,6 @@ func (suite *KeeperTestSuite) TestSendTx() { var ( path *ibctesting.Path packetData icatypes.InterchainAccountPacketData - chanCap *capabilitytypes.Capability timeoutTimestamp uint64 ) @@ -119,13 +116,6 @@ func (suite *KeeperTestSuite) TestSendTx() { }, false, }, - { - "invalid channel capability provided", - func() { - chanCap = nil - }, - false, - }, { "timeout timestamp is not in the future", func() { @@ -165,13 +155,9 @@ func (suite *KeeperTestSuite) TestSendTx() { err := SetupICAPath(path, TestOwnerAddress) suite.Require().NoError(err) - var ok bool - chanCap, ok = suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(path.EndpointA.Chain.GetContext(), host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) - suite.Require().True(ok) - tc.malleate() // malleate mutates test data - _, err = suite.chainA.GetSimApp().ICAControllerKeeper.SendTx(suite.chainA.GetContext(), chanCap, ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, packetData, timeoutTimestamp) + _, err = suite.chainA.GetSimApp().ICAControllerKeeper.SendTx(suite.chainA.GetContext(), nil, ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, packetData, timeoutTimestamp) if tc.expPass { suite.Require().NoError(err) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index dd1ec0a6157..538a1addf3a 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -85,6 +85,15 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() { if version != ibcmock.Version { return "", fmt.Errorf("incorrect mock version") } + + if err := suite.chainA.GetSimApp().FeeMockModule.IBCApp.ScopedKeeper.ClaimCapability( + suite.chainA.GetContext(), + chanCap, + host.ChannelCapabilityPath(portID, channelID), + ); err != nil { + return "", err + } + return ibcmock.Version, nil } @@ -182,6 +191,15 @@ func (suite *FeeTestSuite) TestOnChanOpenTry() { if counterpartyVersion != ibcmock.Version { return "", fmt.Errorf("incorrect mock version") } + + if err := suite.chainA.GetSimApp().FeeMockModule.IBCApp.ScopedKeeper.ClaimCapability( + suite.chainA.GetContext(), + chanCap, + host.ChannelCapabilityPath(portID, channelID), + ); err != nil { + return "", err + } + return ibcmock.Version, nil } @@ -360,6 +378,25 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { suite.SetupTest() suite.coordinator.Setup(suite.path) // setup channel + suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, + portID, channelID string, chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, version string, + ) (string, error) { + if version != ibcmock.Version { + return "", fmt.Errorf("incorrect mock version") + } + + if err := suite.chainA.GetSimApp().FeeMockModule.IBCApp.ScopedKeeper.ClaimCapability( + suite.chainA.GetContext(), + chanCap, + host.ChannelCapabilityPath(portID, channelID), + ); err != nil { + return "", err + } + + return ibcmock.Version, nil + } + packetID := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) fee = types.Fee{ RecvFee: defaultRecvFee, diff --git a/testing/mock/ibc_module.go b/testing/mock/ibc_module.go index 7687be61a09..22157d23cb5 100644 --- a/testing/mock/ibc_module.go +++ b/testing/mock/ibc_module.go @@ -10,7 +10,6 @@ import ( capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types" - host "github.com/cosmos/ibc-go/v5/modules/core/24-host" "github.com/cosmos/ibc-go/v5/modules/core/exported" ) @@ -42,11 +41,6 @@ func (im IBCModule) OnChanOpenInit( return im.IBCApp.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version) } - // Claim channel capability passed back by IBC module - if err := im.IBCApp.ScopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { - return "", err - } - return version, nil } @@ -59,11 +53,6 @@ func (im IBCModule) OnChanOpenTry( return im.IBCApp.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, counterpartyVersion) } - // Claim channel capability passed back by IBC module - if err := im.IBCApp.ScopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { - return "", err - } - return Version, nil } From e80cc2ab2cf168a839b49c5b911057e50baaef41 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 30 Aug 2022 13:40:45 +0200 Subject: [PATCH 2/5] [WIP] removing changes to fee tests --- modules/apps/29-fee/ibc_middleware_test.go | 35 ---------------------- 1 file changed, 35 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 538a1addf3a..3ceec6e3ef9 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -86,14 +86,6 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() { return "", fmt.Errorf("incorrect mock version") } - if err := suite.chainA.GetSimApp().FeeMockModule.IBCApp.ScopedKeeper.ClaimCapability( - suite.chainA.GetContext(), - chanCap, - host.ChannelCapabilityPath(portID, channelID), - ); err != nil { - return "", err - } - return ibcmock.Version, nil } @@ -192,14 +184,6 @@ func (suite *FeeTestSuite) TestOnChanOpenTry() { return "", fmt.Errorf("incorrect mock version") } - if err := suite.chainA.GetSimApp().FeeMockModule.IBCApp.ScopedKeeper.ClaimCapability( - suite.chainA.GetContext(), - chanCap, - host.ChannelCapabilityPath(portID, channelID), - ); err != nil { - return "", err - } - return ibcmock.Version, nil } @@ -378,25 +362,6 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { suite.SetupTest() suite.coordinator.Setup(suite.path) // setup channel - suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, - portID, channelID string, chanCap *capabilitytypes.Capability, - counterparty channeltypes.Counterparty, version string, - ) (string, error) { - if version != ibcmock.Version { - return "", fmt.Errorf("incorrect mock version") - } - - if err := suite.chainA.GetSimApp().FeeMockModule.IBCApp.ScopedKeeper.ClaimCapability( - suite.chainA.GetContext(), - chanCap, - host.ChannelCapabilityPath(portID, channelID), - ); err != nil { - return "", err - } - - return ibcmock.Version, nil - } - packetID := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) fee = types.Fee{ RecvFee: defaultRecvFee, From da91a9a5010ad00828765b8841796a273bc207b3 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 30 Aug 2022 13:41:52 +0200 Subject: [PATCH 3/5] [WIP] removing whitespaces --- modules/apps/29-fee/ibc_middleware_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 3ceec6e3ef9..dd1ec0a6157 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -85,7 +85,6 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() { if version != ibcmock.Version { return "", fmt.Errorf("incorrect mock version") } - return ibcmock.Version, nil } @@ -183,7 +182,6 @@ func (suite *FeeTestSuite) TestOnChanOpenTry() { if counterpartyVersion != ibcmock.Version { return "", fmt.Errorf("incorrect mock version") } - return ibcmock.Version, nil } From 3e11171213a92ea24ade5716d15b51277c281eac Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 30 Aug 2022 13:56:01 +0200 Subject: [PATCH 4/5] adding nil check on chan caps in mock ibc module --- .../host/ibc_module_test.go | 11 ++--------- testing/mock/ibc_module.go | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 9 deletions(-) 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 5de70cc3fab..0dd7c71f8ed 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -638,10 +638,7 @@ func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose() params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) - chanCap, ok := suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(path.EndpointA.Chain.GetContext(), host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) - suite.Require().True(ok) - - _, err = suite.chainA.GetSimApp().ICAControllerKeeper.SendTx(suite.chainA.GetContext(), chanCap, ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, icaPacketData, ^uint64(0)) + _, err = suite.chainA.GetSimApp().ICAControllerKeeper.SendTx(suite.chainA.GetContext(), nil, ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, icaPacketData, ^uint64(0)) suite.Require().NoError(err) path.EndpointB.UpdateClient() @@ -668,11 +665,7 @@ func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose() path.EndpointB.ChannelID = "" suite.coordinator.CreateChannels(path) - // try to control the interchain account again - chanCap, ok = suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(path.EndpointA.Chain.GetContext(), host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) - suite.Require().True(ok) - - _, err = suite.chainA.GetSimApp().ICAControllerKeeper.SendTx(suite.chainA.GetContext(), chanCap, ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, icaPacketData, ^uint64(0)) + _, err = suite.chainA.GetSimApp().ICAControllerKeeper.SendTx(suite.chainA.GetContext(), nil, ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, icaPacketData, ^uint64(0)) suite.Require().NoError(err) path.EndpointB.UpdateClient() diff --git a/testing/mock/ibc_module.go b/testing/mock/ibc_module.go index 22157d23cb5..f94e6a90361 100644 --- a/testing/mock/ibc_module.go +++ b/testing/mock/ibc_module.go @@ -10,6 +10,7 @@ import ( capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types" + host "github.com/cosmos/ibc-go/v5/modules/core/24-host" "github.com/cosmos/ibc-go/v5/modules/core/exported" ) @@ -41,6 +42,13 @@ func (im IBCModule) OnChanOpenInit( return im.IBCApp.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version) } + if chanCap != nil { + // Claim channel capability passed back by IBC module + if err := im.IBCApp.ScopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { + return "", err + } + } + return version, nil } @@ -53,6 +61,13 @@ func (im IBCModule) OnChanOpenTry( return im.IBCApp.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, counterpartyVersion) } + if chanCap != nil { + // Claim channel capability passed back by IBC module + if err := im.IBCApp.ScopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { + return "", err + } + } + return Version, nil } From 7f1293eef84d2e32ebb35994bda6888fd8f5305a Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 30 Aug 2022 20:24:33 +0200 Subject: [PATCH 5/5] adding changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae3378bfe46..d73d2fbfc92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (02-client/cli) [\#897](https://github.com/cosmos/ibc-go/pull/897) Remove `GetClientID()` from `Misbehaviour` interface. Submit client misbehaviour cli command requires an explicit client id now. * (06-solomachine) [\#1972](https://github.com/cosmos/ibc-go/pull/1972) Solo machine implementation of `ZeroCustomFields` fn now panics as the fn is only used for upgrades which solo machine does not support. * (apps/27-interchain-accounts) [\#2102](https://github.com/cosmos/ibc-go/pull/2102) ICS27 controller middleware now supports a nil underlying application. This allows chains to make use of interchain accounts with existing auth mechanisms such as x/group and x/gov. +* (apps/27-interchain-accounts) [\#2146](https://github.com/cosmos/ibc-go/pull/2146) ICS27 controller now claims the channel capability passed via ibc core, and passes `nil` to the underlying app callback. The channel capability arg in `SendTx` is now ignored and looked up internally. ### Features