Skip to content

Commit

Permalink
chore (api)!: remove capabilities from channel handshakes (#7232)
Browse files Browse the repository at this point in the history
* almost all tests working

* fix last tests

* solve merge error

* remove newline

* fix comment

* docstring

* docs

* Update modules/core/keeper/msg_server.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* pr feedback

* change error message

* forgot to save a file...

* merge error

---------

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
  • Loading branch information
bznein and colin-axner authored Sep 2, 2024
1 parent eb51dff commit eb9bcc3
Show file tree
Hide file tree
Showing 31 changed files with 184 additions and 972 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

* (core, apps) [\#7213](https://github.com/cosmos/ibc-go/pull/7213) Remove capabilities from `SendPacket`.
* (core, apps) [\#7213](https://github.com/cosmos/ibc-go/pull/7225) Remove capabilities from `WriteAcknowledgement`.
* (core, apps) [\#7232](https://github.com/cosmos/ibc-go/pull/7232) Remove capabilities from channel handshake methods.

### State Machine Breaking

Expand Down
1 change: 1 addition & 0 deletions docs/docs/05-migrations/14-v9-to-v10.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ There are four sections based on the four potential user groups of this document

- (TODO: expand later) Removal of capabilities in `SendPacket` [\#7213](https://github.com/cosmos/ibc-go/pull/7213).
- (TODO: expand later) Removal of capabilities in `WriteAcknowledgement` [\#7225](https://github.com/cosmos/ibc-go/pull/7213).
- (TODO: expand later) Removal of capabilities in channel handshake methods [\#7232](https://github.com/cosmos/ibc-go/pull/7232).

### ICS27 - Interchain Accounts

Expand Down
12 changes: 2 additions & 10 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,12 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"

capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
"github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/controller/keeper"
"github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/controller/types"
icatypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/types"
clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v9/modules/core/05-port/types"
host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors"
ibcexported "github.com/cosmos/ibc-go/v9/modules/core/exported"
)
Expand Down Expand Up @@ -63,28 +61,23 @@ func (im IBCMiddleware) OnChanOpenInit(
connectionHops []string,
portID string,
channelID string,
chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
) (string, error) {
if !im.keeper.GetParams(ctx).ControllerEnabled {
return "", types.ErrControllerSubModuleDisabled
}

version, err := im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version)
version, err := im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, counterparty, version)
if err != nil {
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 && im.keeper.IsMiddlewareEnabled(ctx, portID, connectionHops[0]) {
if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, nil, counterparty, version); err != nil {
if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, counterparty, version); err != nil {
return "", err
}
}
Expand All @@ -99,7 +92,6 @@ func (IBCMiddleware) OnChanOpenTry(
connectionHops []string,
portID,
channelID string,
chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
counterpartyVersion string,
) (string, error) {
Expand Down
104 changes: 19 additions & 85 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"

capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
"github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/controller"
controllerkeeper "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/controller/keeper"
"github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/controller/types"
Expand Down Expand Up @@ -128,26 +127,12 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
{
"success", func() {}, nil,
},
{
"ICA auth module does not claim channel capability", func() {
suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx context.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
}
}, nil,
},
{
"ICA auth module modification of channel version is ignored", func() {
// NOTE: explicitly modify the channel version via the auth module callback,
// ensuring the expected JSON encoded metadata is not modified upon return
suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx context.Context, order channeltypes.Order, connectionHops []string,
portID, channelID string, chanCap *capabilitytypes.Capability,
portID, channelID string,
counterparty channeltypes.Counterparty, version string,
) (string, error) {
return "invalid-version", nil
Expand All @@ -162,7 +147,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
{
"ICA auth module callback fails", func() {
suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx context.Context, order channeltypes.Order, connectionHops []string,
portID, channelID string, chanCap *capabilitytypes.Capability,
portID, channelID string,
counterparty channeltypes.Counterparty, version string,
) (string, error) {
return "", fmt.Errorf("mock ica auth fails")
Expand All @@ -179,7 +164,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ConnectionID)

suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx context.Context, order channeltypes.Order, connectionHops []string,
portID, channelID string, chanCap *capabilitytypes.Capability,
portID, channelID string,
counterparty channeltypes.Counterparty, version string,
) (string, error) {
return "", fmt.Errorf("error should be unreachable")
Expand All @@ -203,9 +188,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
portID, err := icatypes.NewControllerPortID(TestOwnerAddress)
suite.Require().NoError(err)

portCap := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), portID)
suite.chainA.GetSimApp().ICAControllerKeeper.ClaimCapability(suite.chainA.GetContext(), portCap, host.PortPath(portID)) //nolint:errcheck // checking this error isn't needed for the test

path.EndpointA.ChannelConfig.PortID = portID
path.EndpointA.ChannelID = ibctesting.FirstChannelID

Expand All @@ -226,21 +208,15 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
// ensure channel on chainA is set in state
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.SetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, *channel)

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

chanCap, err := suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)

if isNilApp {
cbs = controller.NewIBCMiddleware(suite.chainA.GetSimApp().ICAControllerKeeper)
}

version, err := cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.ConnectionHops,
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.Version,
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, channel.Counterparty, channel.Version,
)

if tc.expErr == nil {
Expand Down Expand Up @@ -286,20 +262,14 @@ func (suite *InterchainAccountsTestSuite) TestChanOpenTry() {

suite.Require().Error(err)

// call application callback directly
module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)

counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
chanCap, found := suite.chainA.App.GetScopedIBCKeeper().GetCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
suite.Require().True(found)

version, err := cbs.OnChanOpenTry(
suite.chainA.GetContext(), path.EndpointA.ChannelConfig.Order, []string{path.EndpointA.ConnectionID},
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap,
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID,
counterparty, path.EndpointB.ChannelConfig.Version,
)
suite.Require().Error(err)
Expand Down Expand Up @@ -377,10 +347,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {

tc.malleate() // malleate mutates test data

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)

err = cbs.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelID, path.EndpointB.ChannelConfig.Version)
Expand Down Expand Up @@ -437,11 +404,7 @@ func (suite *InterchainAccountsTestSuite) TestChanOpenConfirm() {

suite.Require().Error(err)

// call application callback directly
module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)

err = cbs.OnChanOpenConfirm(
Expand All @@ -462,10 +425,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanCloseInit() {
err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)

err = cbs.OnChanCloseInit(
Expand Down Expand Up @@ -512,10 +472,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanCloseConfirm() {
suite.Require().NoError(err)

tc.malleate() // malleate mutates test data
module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)

if isNilApp {
Expand Down Expand Up @@ -561,10 +519,7 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {

tc.malleate() // malleate mutates test data

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)

packet := channeltypes.NewPacket(
Expand Down Expand Up @@ -674,10 +629,7 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {

tc.malleate() // malleate mutates test data

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)

if isNilApp {
Expand Down Expand Up @@ -771,10 +723,7 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {

tc.malleate() // malleate mutates test data

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)

if isNilApp {
Expand Down Expand Up @@ -860,10 +809,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() {

tc.malleate() // malleate mutates test data

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)
cbs, ok := app.(porttypes.UpgradableModule)
suite.Require().True(ok)
Expand Down Expand Up @@ -903,10 +849,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() {
suite.Require().NoError(err)

// call application callback directly
module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)
cbs, ok := app.(porttypes.UpgradableModule)
suite.Require().True(ok)
Expand Down Expand Up @@ -988,10 +931,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() {

tc.malleate() // malleate mutates test data

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)
cbs, ok := app.(porttypes.UpgradableModule)
suite.Require().True(ok)
Expand Down Expand Up @@ -1078,10 +1018,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeOpen() {

tc.malleate() // malleate mutates test data

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)
cbs, ok := app.(porttypes.UpgradableModule)
suite.Require().True(ok)
Expand Down Expand Up @@ -1196,10 +1133,7 @@ func (suite *InterchainAccountsTestSuite) TestGetAppVersion() {
err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)

controllerStack, ok := cbs.(porttypes.ICS4Wrapper)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (

errorsmod "cosmossdk.io/errors"

capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
icatypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/types"
connectiontypes "github.com/cosmos/ibc-go/v9/modules/core/03-connection/types"
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
Expand All @@ -25,7 +24,6 @@ func (k Keeper) OnChanOpenInit(
connectionHops []string,
portID string,
channelID string,
chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
) (string, error) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
package keeper_test

import (
capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
icatypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/types"
connectiontypes "github.com/cosmos/ibc-go/v9/modules/core/03-connection/types"
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors"
ibctesting "github.com/cosmos/ibc-go/v9/testing"
)
Expand All @@ -19,7 +17,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
var (
channel *channeltypes.Channel
path *ibctesting.Path
chanCap *capabilitytypes.Capability
metadata icatypes.Metadata
expectedVersion string
)
Expand Down Expand Up @@ -274,8 +271,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
portID, err := icatypes.NewControllerPortID(TestOwnerAddress)
suite.Require().NoError(err)

portCap := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), portID)
suite.chainA.GetSimApp().ICAControllerKeeper.ClaimCapability(suite.chainA.GetContext(), portCap, host.PortPath(portID)) //nolint:errcheck // this error check isn't needed for tests
path.EndpointA.ChannelConfig.PortID = portID

// default values
Expand All @@ -299,11 +294,8 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {

tc.malleate() // malleate mutates test data

chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
suite.Require().NoError(err)

version, err := suite.chainA.GetSimApp().ICAControllerKeeper.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.ConnectionHops,
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.Version,
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, channel.Counterparty, channel.Version,
)

expPass := tc.expError == nil
Expand Down
Loading

0 comments on commit eb9bcc3

Please sign in to comment.