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: adding assertion of channel capabilities migration #2140

Merged
merged 22 commits into from
Sep 7, 2022
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a6575eb
wip initial commit
damiannolan Aug 26, 2022
94091cf
draft migration completed
damiannolan Aug 26, 2022
3c95389
removing unnecessary storekey arg
damiannolan Aug 26, 2022
2bb7c3f
additional cleanup
damiannolan Aug 26, 2022
d546d20
adding updates to migrations and tests additional assertions
damiannolan Aug 26, 2022
a79d10e
updating and moving migrations code
damiannolan Aug 26, 2022
c2da8b2
adding additional checks to tests
damiannolan Aug 26, 2022
6ea2014
cleaning up tests
damiannolan Aug 26, 2022
c981bcc
cleaning up tests
damiannolan Aug 26, 2022
88124f4
updating inline doc comments, checking err return
damiannolan Aug 26, 2022
cf3bfaf
Merge branch 'main' into damian/ics27-chan-capability-migrations
damiannolan Aug 26, 2022
443b548
using InitMemStore in favour of InitializeCapability, adjusting tests
damiannolan Aug 29, 2022
bdec1bc
Merge branch 'damian/ics27-chan-capability-migrations' of github.com:…
damiannolan Aug 29, 2022
5229159
adding assertion of channel capabilities migration
damiannolan Aug 29, 2022
478521e
Merge branch 'main' into damian/ics27-assert-chan-capabilities
damiannolan Sep 1, 2022
bf88e6d
adapting migration code to use get/authenticate capability
damiannolan Sep 1, 2022
8949fa9
adding changelog
damiannolan Sep 1, 2022
0bd27e9
updating tests
damiannolan Sep 1, 2022
dda0398
set middleware enabled for existing channels
damiannolan Sep 1, 2022
49df7dd
assert middleware enabled in tests
damiannolan Sep 6, 2022
4fc9aae
updating changelog with middleware enabled flag
damiannolan Sep 6, 2022
8cd3d49
Merge branch 'main' into damian/ics27-assert-chan-capabilities
damiannolan Sep 7, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (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.
* (apps/27-interchain-accounts) [\#2134](https://github.com/cosmos/ibc-go/pull/2134) Adding upgrade handler to ICS27 `controller` submodule for migration of channel capabilities. This upgrade handler migrates ownership of channel capabilities from the underlying application to the ICS27 `controller` submodule.
* (apps/27-interchain-accounts) [\#2157](https://github.com/cosmos/ibc-go/pull/2157) Adding `IsMiddlewareEnabled` functionality to enforce calls to ICS27 msg server to *not* route to the underlying application.
* (apps/27-interchain-accounts) [\#2140](https://github.com/cosmos/ibc-go/pull/2140) Adding migration handler to ICS27 `controller` submodule to assert ownership of channel capabilities and set middleware enabled flag for existing channels. The ICS27 module consensus version has been bumped from 1 to 2.

### Features

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package keeper

import (
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

"github.com/cosmos/ibc-go/v5/modules/core/23-commitment/types"
host "github.com/cosmos/ibc-go/v5/modules/core/24-host"
)

// Migrator is a struct for handling in-place store migrations.
type Migrator struct {
keeper *Keeper
}

// NewMigrator returns a new Migrator.
func NewMigrator(keeper *Keeper) Migrator {
return Migrator{keeper: keeper}
}

// AssertChannelCapabilityMigrations checks that all channel capabilities generated using the interchain accounts controller port prefix
// are owned by the controller submodule and ibc.
func (m Migrator) AssertChannelCapabilityMigrations(ctx sdk.Context) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to suggestions on improving the name of this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the name

for _, ch := range m.keeper.GetAllActiveChannels(ctx) {
name := host.ChannelCapabilityPath(ch.PortId, ch.ChannelId)
cap, found := m.keeper.scopedKeeper.GetCapability(ctx, name)
if !found {
return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotFound, "failed to find capability: %s", name)
}

isAuthenticated := m.keeper.scopedKeeper.AuthenticateCapability(ctx, cap, name)
if !isAuthenticated {
return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", types.SubModuleName)
}

m.keeper.SetMiddlewareEnabled(ctx, ch.PortId, ch.ChannelId)
}

return nil
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package keeper_test

import (
"github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/keeper"
ibctesting "github.com/cosmos/ibc-go/v5/testing"
)

func (suite *KeeperTestSuite) TestAssertChannelCapabilityMigrations() {
testCases := []struct {
name string
malleate func()
expPass bool
}{
{
"success",
func() {},
true,
},
{
"capability not found",
func() {
suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(
suite.chainA.GetContext(),
ibctesting.FirstConnectionID,
ibctesting.MockPort,
ibctesting.FirstChannelID,
)
},
false,
},
}

for _, tc := range testCases {
suite.Run(tc.name, func() {
suite.SetupTest()

path := NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

err := SetupICAPath(path, ibctesting.TestAccAddress)
suite.Require().NoError(err)

tc.malleate()

migrator := keeper.NewMigrator(&suite.chainA.GetSimApp().ICAControllerKeeper)
err = migrator.AssertChannelCapabilityMigrations(suite.chainA.GetContext())

if tc.expPass {
suite.Require().NoError(err)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved

isMiddlewareEnabled := suite.chainA.GetSimApp().ICAControllerKeeper.IsMiddlewareEnabled(
suite.chainA.GetContext(),
path.EndpointA.ChannelConfig.PortID,
path.EndpointA.ChannelID,
)

suite.Require().True(isMiddlewareEnabled)
} else {
suite.Require().Error(err)
}
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
sdktypes "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

"github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types"
controllertypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/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"
Expand Down Expand Up @@ -180,7 +179,7 @@ func (suite *KeeperTestSuite) TestSubmitTx() {
timeoutTimestamp := uint64(suite.chainA.GetContext().BlockTime().Add(time.Minute).UnixNano())
connectionID := path.EndpointA.ConnectionID

msg = types.NewMsgSubmitTx(owner, connectionID, clienttypes.ZeroHeight(), timeoutTimestamp, packetData)
msg = controllertypes.NewMsgSubmitTx(owner, connectionID, clienttypes.ZeroHeight(), timeoutTimestamp, packetData)

tc.malleate() // malleate mutates test data
res, err := suite.chainA.GetSimApp().ICAControllerKeeper.SubmitTx(sdk.WrapSDKContext(suite.chainA.GetContext()), msg)
Expand Down
7 changes: 6 additions & 1 deletion modules/apps/27-interchain-accounts/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,11 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {
controllertypes.RegisterMsgServer(cfg.MsgServer(), am.controllerKeeper)
controllertypes.RegisterQueryServer(cfg.QueryServer(), am.controllerKeeper)
hosttypes.RegisterQueryServer(cfg.QueryServer(), am.hostKeeper)

m := controllerkeeper.NewMigrator(am.controllerKeeper)
if err := cfg.RegisterMigration(types.ModuleName, 1, m.AssertChannelCapabilityMigrations); err != nil {
panic(fmt.Sprintf("failed to migrate interchainaccounts app from version 1 to 2: %v", err))
}
}

// InitGenesis performs genesis initialization for the interchain accounts module.
Expand Down Expand Up @@ -191,7 +196,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw
}

// ConsensusVersion implements AppModule/ConsensusVersion.
func (AppModule) ConsensusVersion() uint64 { return 1 }
func (AppModule) ConsensusVersion() uint64 { return 2 }

// BeginBlock implements the AppModule interface
func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {
Expand Down