Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore (api)!: remove capabilities from channel handshakes #7232

Merged
merged 14 commits into from
Sep 2, 2024
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this Path func in host in use anymore after this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undirectly in a lot of test cases. I plan to remove them as followup

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
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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capability-related test. No longer needed

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not from your changes, but I think the test might have been incorrect. I feel like this should be EndpointB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm, it is EndpointB. Did you mean A?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes sorry!

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
Loading