From a6575ebe278310195c861a979e0d812ef724b4bd Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 26 Aug 2022 12:00:48 +0200 Subject: [PATCH 01/18] wip initial commit --- .../controller/keeper/migrations.go | 37 +++++++++++++++++++ .../controller/keeper/migrations_test.go | 12 ++++++ 2 files changed, 49 insertions(+) create mode 100644 modules/apps/27-interchain-accounts/controller/keeper/migrations.go create mode 100644 modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go new file mode 100644 index 00000000000..83fbd61d155 --- /dev/null +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -0,0 +1,37 @@ +package keeper + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper" +) + +// MigrateChannelCapability takes in global capability keeper and auth module scoped keeper name, +// iterates through all capabilities and checks if one of the owners is the auth module, +// if so, replaces the capabilities owner with the controller module scoped keeper +func MigrateChannelCapability(ctx sdk.Context, capabilityKeeper capabilitykeeper.Keeper, authModule string) error { + + // iterate controller port prefix to get name to construct owner + + latestIndex := capabilityKeeper.GetLatestIndex(ctx) + + for i := 1; i <= int(latestIndex); i++ { + capOwners, found := capabilityKeeper.GetOwners(ctx, uint64(i)) + if !found { + continue + } + + // index, found := capOwners.Get(owner) + + // for _, owner := range capOwners.GetOwners() { + // if owner.Module == authModule { + // newOwner := capabilitytypes.NewOwner(types.SubModuleName, "todo") + + // capOwners := capabilitytypes.NewCapabilityOwners() + + // capabilityKeeper.SetOwners(ctx, uint64(i), *capOwners) + // } + // } + } + + return nil +} diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go new file mode 100644 index 00000000000..78a9ea52075 --- /dev/null +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go @@ -0,0 +1,12 @@ +package keeper_test + +func (suite *KeeperTestSuite) TestMigrateChannelCapability() { + suite.SetupTest() + + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + +} From 94091cf7a8363156ef563e1cc9f9473f3964ae06 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 26 Aug 2022 14:11:23 +0200 Subject: [PATCH 02/18] draft migration completed --- .../controller/keeper/migrations.go | 57 ++++++++++++------- .../controller/keeper/migrations_test.go | 18 ++++++ 2 files changed, 56 insertions(+), 19 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go index 83fbd61d155..9a3b00768da 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -1,36 +1,55 @@ package keeper import ( + "fmt" + + "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/store/prefix" + storetypes "github.com/cosmos/cosmos-sdk/store/types" sdk "github.com/cosmos/cosmos-sdk/types" capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper" + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + + "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types" + icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types" + host "github.com/cosmos/ibc-go/v5/modules/core/24-host" ) // MigrateChannelCapability takes in global capability keeper and auth module scoped keeper name, // iterates through all capabilities and checks if one of the owners is the auth module, // if so, replaces the capabilities owner with the controller module scoped keeper -func MigrateChannelCapability(ctx sdk.Context, capabilityKeeper capabilitykeeper.Keeper, authModule string) error { - - // iterate controller port prefix to get name to construct owner - - latestIndex := capabilityKeeper.GetLatestIndex(ctx) - - for i := 1; i <= int(latestIndex); i++ { - capOwners, found := capabilityKeeper.GetOwners(ctx, uint64(i)) +func MigrateChannelCapability( + ctx sdk.Context, + cdc codec.BinaryCodec, + storeKey storetypes.StoreKey, + memStoreKey storetypes.StoreKey, + capabilityKeeper capabilitykeeper.Keeper, + authModule string, +) error { + keyPrefix := capabilitytypes.RevCapabilityKey(authModule, fmt.Sprintf("%s/%s/%s", host.KeyChannelCapabilityPrefix, host.KeyPortPrefix, icatypes.PortPrefix)) + prefixStore := prefix.NewStore(ctx.KVStore(memStoreKey), keyPrefix) + iterator := sdk.KVStorePrefixIterator(prefixStore, nil) + + defer iterator.Close() + for ; iterator.Valid(); iterator.Next() { + key := string(iterator.Key()) + + name := fmt.Sprintf("%s/%s/%s%s", host.KeyChannelCapabilityPrefix, host.KeyPortPrefix, types.PortPrefix, key) + capOwner := capabilitytypes.NewOwner(authModule, name) + + ctx.Logger().Info("migrating ibc channel capability", "owner", capOwner.String()) + + index := sdk.BigEndianToUint64(iterator.Value()) + + capOwners, found := capabilityKeeper.GetOwners(ctx, index) if !found { - continue + panic(fmt.Sprintf("no owners for capability at index: %d", index)) } - // index, found := capOwners.Get(owner) - - // for _, owner := range capOwners.GetOwners() { - // if owner.Module == authModule { - // newOwner := capabilitytypes.NewOwner(types.SubModuleName, "todo") - - // capOwners := capabilitytypes.NewCapabilityOwners() + capOwners.Remove(capOwner) + capOwners.Set(capabilitytypes.NewOwner(types.ModuleName, name)) - // capabilityKeeper.SetOwners(ctx, uint64(i), *capOwners) - // } - // } + capabilityKeeper.SetOwners(ctx, index, capOwners) } return nil diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go index 78a9ea52075..3330b49cae8 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go @@ -1,5 +1,11 @@ package keeper_test +import ( + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + + "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/keeper" +) + func (suite *KeeperTestSuite) TestMigrateChannelCapability() { suite.SetupTest() @@ -9,4 +15,16 @@ func (suite *KeeperTestSuite) TestMigrateChannelCapability() { err := SetupICAPath(path, TestOwnerAddress) suite.Require().NoError(err) + err = keeper.MigrateChannelCapability( + suite.chainA.GetContext(), + suite.chainA.Codec, + suite.chainA.GetSimApp().GetKey(capabilitytypes.StoreKey), + suite.chainA.GetSimApp().GetMemKey(capabilitytypes.MemStoreKey), + *suite.chainA.GetSimApp().CapabilityKeeper, + "mockicacontroller", + ) + + suite.Require().NoError(err) + + // TODO: follow up assertions } From 3c95389ebc11d3564cf6e6c7c8862aa6a4dbf944 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 26 Aug 2022 14:12:34 +0200 Subject: [PATCH 03/18] removing unnecessary storekey arg --- .../apps/27-interchain-accounts/controller/keeper/migrations.go | 1 - .../27-interchain-accounts/controller/keeper/migrations_test.go | 1 - 2 files changed, 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go index 9a3b00768da..5ee155fbac1 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -21,7 +21,6 @@ import ( func MigrateChannelCapability( ctx sdk.Context, cdc codec.BinaryCodec, - storeKey storetypes.StoreKey, memStoreKey storetypes.StoreKey, capabilityKeeper capabilitykeeper.Keeper, authModule string, diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go index 3330b49cae8..76577fb55d9 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go @@ -18,7 +18,6 @@ func (suite *KeeperTestSuite) TestMigrateChannelCapability() { err = keeper.MigrateChannelCapability( suite.chainA.GetContext(), suite.chainA.Codec, - suite.chainA.GetSimApp().GetKey(capabilitytypes.StoreKey), suite.chainA.GetSimApp().GetMemKey(capabilitytypes.MemStoreKey), *suite.chainA.GetSimApp().CapabilityKeeper, "mockicacontroller", From 2bb7c3fdded2bf9f385dce9a7fe832b33133031e Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 26 Aug 2022 14:19:31 +0200 Subject: [PATCH 04/18] additional cleanup --- .../controller/keeper/migrations.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go index 5ee155fbac1..f411e3e2888 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -10,14 +10,13 @@ import ( capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" - "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types" + "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" host "github.com/cosmos/ibc-go/v5/modules/core/24-host" ) -// MigrateChannelCapability takes in global capability keeper and auth module scoped keeper name, -// iterates through all capabilities and checks if one of the owners is the auth module, -// if so, replaces the capabilities owner with the controller module scoped keeper +// MigrateChannelCapability performs a search on a prefix store using the provided authModule name, +// retrieves the associated capability index and reassigns ownership to the ICS27 controller submodule func MigrateChannelCapability( ctx sdk.Context, cdc codec.BinaryCodec, @@ -31,22 +30,25 @@ func MigrateChannelCapability( defer iterator.Close() for ; iterator.Valid(); iterator.Next() { - key := string(iterator.Key()) + key := string(iterator.Key()) // search prefix is omitted - name := fmt.Sprintf("%s/%s/%s%s", host.KeyChannelCapabilityPrefix, host.KeyPortPrefix, types.PortPrefix, key) + // reconstruct the capability name using the prefix and iterator key + name := fmt.Sprintf("%s/%s/%s%s", host.KeyChannelCapabilityPrefix, host.KeyPortPrefix, icatypes.PortPrefix, key) capOwner := capabilitytypes.NewOwner(authModule, name) ctx.Logger().Info("migrating ibc channel capability", "owner", capOwner.String()) index := sdk.BigEndianToUint64(iterator.Value()) - capOwners, found := capabilityKeeper.GetOwners(ctx, index) if !found { panic(fmt.Sprintf("no owners for capability at index: %d", index)) } + // remove the existing auth module owner capOwners.Remove(capOwner) - capOwners.Set(capabilitytypes.NewOwner(types.ModuleName, name)) + + // add the controller submodule as a new capability owner + capOwners.Set(capabilitytypes.NewOwner(types.SubModuleName, name)) capabilityKeeper.SetOwners(ctx, index, capOwners) } From d546d2006ef0cf53687bc906aeec35503fc7f7ff Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 26 Aug 2022 15:37:09 +0200 Subject: [PATCH 05/18] adding updates to migrations and tests additional assertions --- .../controller/keeper/migrations.go | 27 ++++++++++--------- .../controller/keeper/migrations_test.go | 27 ++++++++++++++++--- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go index f411e3e2888..61f5da3a0eb 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -15,16 +15,16 @@ import ( host "github.com/cosmos/ibc-go/v5/modules/core/24-host" ) -// MigrateChannelCapability performs a search on a prefix store using the provided authModule name, -// retrieves the associated capability index and reassigns ownership to the ICS27 controller submodule +// MigrateChannelCapability performs a search on a prefix store using the provided store key and module name. +// It retrieves the associated channel capability index and reassigns ownership to the ICS27 controller submodule. func MigrateChannelCapability( ctx sdk.Context, cdc codec.BinaryCodec, memStoreKey storetypes.StoreKey, - capabilityKeeper capabilitykeeper.Keeper, - authModule string, + capabilityKeeper *capabilitykeeper.Keeper, + module string, ) error { - keyPrefix := capabilitytypes.RevCapabilityKey(authModule, fmt.Sprintf("%s/%s/%s", host.KeyChannelCapabilityPrefix, host.KeyPortPrefix, icatypes.PortPrefix)) + keyPrefix := capabilitytypes.RevCapabilityKey(module, fmt.Sprintf("%s/%s/%s", host.KeyChannelCapabilityPrefix, host.KeyPortPrefix, icatypes.PortPrefix)) prefixStore := prefix.NewStore(ctx.KVStore(memStoreKey), keyPrefix) iterator := sdk.KVStorePrefixIterator(prefixStore, nil) @@ -34,23 +34,24 @@ func MigrateChannelCapability( // reconstruct the capability name using the prefix and iterator key name := fmt.Sprintf("%s/%s/%s%s", host.KeyChannelCapabilityPrefix, host.KeyPortPrefix, icatypes.PortPrefix, key) - capOwner := capabilitytypes.NewOwner(authModule, name) + owner := capabilitytypes.NewOwner(module, name) - ctx.Logger().Info("migrating ibc channel capability", "owner", capOwner.String()) + ctx.Logger().Info("migrating ibc channel capability", "owner", owner.String()) index := sdk.BigEndianToUint64(iterator.Value()) - capOwners, found := capabilityKeeper.GetOwners(ctx, index) + owners, found := capabilityKeeper.GetOwners(ctx, index) if !found { panic(fmt.Sprintf("no owners for capability at index: %d", index)) } - // remove the existing auth module owner - capOwners.Remove(capOwner) + // remove the existing module owner + owners.Remove(owner) + prefixStore.Delete(iterator.Key()) // add the controller submodule as a new capability owner - capOwners.Set(capabilitytypes.NewOwner(types.SubModuleName, name)) - - capabilityKeeper.SetOwners(ctx, index, capOwners) + owners.Set(capabilitytypes.NewOwner(types.SubModuleName, name)) + capabilityKeeper.SetOwners(ctx, index, owners) + capabilityKeeper.InitializeCapability(ctx, index, owners) } return nil diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go index 76577fb55d9..584d1d11184 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go @@ -4,6 +4,9 @@ import ( capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/keeper" + "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types" + host "github.com/cosmos/ibc-go/v5/modules/core/24-host" + ibcmock "github.com/cosmos/ibc-go/v5/testing/mock" ) func (suite *KeeperTestSuite) TestMigrateChannelCapability() { @@ -15,15 +18,33 @@ func (suite *KeeperTestSuite) TestMigrateChannelCapability() { err := SetupICAPath(path, TestOwnerAddress) suite.Require().NoError(err) + capName := host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + // assert the capability is owned by the auth module pre migration + cap, found := suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName) + suite.Require().NotNil(cap) + suite.Require().True(found) + + cap, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName) + suite.Require().Nil(cap) + suite.Require().False(found) + err = keeper.MigrateChannelCapability( suite.chainA.GetContext(), suite.chainA.Codec, suite.chainA.GetSimApp().GetMemKey(capabilitytypes.MemStoreKey), - *suite.chainA.GetSimApp().CapabilityKeeper, - "mockicacontroller", + suite.chainA.GetSimApp().CapabilityKeeper, + ibcmock.ModuleName+types.SubModuleName, ) suite.Require().NoError(err) - // TODO: follow up assertions + // assert the capability is now owned by the controller submodule + cap, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName) + suite.Require().NotNil(cap) + suite.Require().True(found) + + cap, found = suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName) + suite.Require().Nil(cap) + suite.Require().False(found) } From a79d10e93aa9b60924ecae6b6f983835257a2a04 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 26 Aug 2022 16:56:17 +0200 Subject: [PATCH 06/18] updating and moving migrations code --- .../controller/keeper/migrations_test.go | 50 ------- .../{keeper => migrations/v5}/migrations.go | 35 +++-- .../migrations/v5/migrations_test.go | 125 ++++++++++++++++++ 3 files changed, 142 insertions(+), 68 deletions(-) delete mode 100644 modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go rename modules/apps/27-interchain-accounts/controller/{keeper => migrations/v5}/migrations.go (62%) create mode 100644 modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go deleted file mode 100644 index 584d1d11184..00000000000 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go +++ /dev/null @@ -1,50 +0,0 @@ -package keeper_test - -import ( - capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" - - "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/keeper" - "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types" - host "github.com/cosmos/ibc-go/v5/modules/core/24-host" - ibcmock "github.com/cosmos/ibc-go/v5/testing/mock" -) - -func (suite *KeeperTestSuite) TestMigrateChannelCapability() { - suite.SetupTest() - - path := NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) - - err := SetupICAPath(path, TestOwnerAddress) - suite.Require().NoError(err) - - capName := host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - - // assert the capability is owned by the auth module pre migration - cap, found := suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName) - suite.Require().NotNil(cap) - suite.Require().True(found) - - cap, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName) - suite.Require().Nil(cap) - suite.Require().False(found) - - err = keeper.MigrateChannelCapability( - suite.chainA.GetContext(), - suite.chainA.Codec, - suite.chainA.GetSimApp().GetMemKey(capabilitytypes.MemStoreKey), - suite.chainA.GetSimApp().CapabilityKeeper, - ibcmock.ModuleName+types.SubModuleName, - ) - - suite.Require().NoError(err) - - // assert the capability is now owned by the controller submodule - cap, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName) - suite.Require().NotNil(cap) - suite.Require().True(found) - - cap, found = suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName) - suite.Require().Nil(cap) - suite.Require().False(found) -} diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go similarity index 62% rename from modules/apps/27-interchain-accounts/controller/keeper/migrations.go rename to modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go index 61f5da3a0eb..921b107d95e 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go @@ -1,12 +1,12 @@ -package keeper +package v5 import ( "fmt" - "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/store/prefix" storetypes "github.com/cosmos/cosmos-sdk/store/types" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" @@ -15,41 +15,40 @@ import ( host "github.com/cosmos/ibc-go/v5/modules/core/24-host" ) -// MigrateChannelCapability performs a search on a prefix store using the provided store key and module name. +// MigrateICS27ChannelCapability performs a search on a prefix store using the provided store key and module name. // It retrieves the associated channel capability index and reassigns ownership to the ICS27 controller submodule. -func MigrateChannelCapability( +func MigrateICS27ChannelCapability( ctx sdk.Context, - cdc codec.BinaryCodec, memStoreKey storetypes.StoreKey, capabilityKeeper *capabilitykeeper.Keeper, module string, ) error { + // construct a prefix store using the x/capability reverse lookup key: {module}/rev/{name} -> index keyPrefix := capabilitytypes.RevCapabilityKey(module, fmt.Sprintf("%s/%s/%s", host.KeyChannelCapabilityPrefix, host.KeyPortPrefix, icatypes.PortPrefix)) prefixStore := prefix.NewStore(ctx.KVStore(memStoreKey), keyPrefix) - iterator := sdk.KVStorePrefixIterator(prefixStore, nil) + iterator := sdk.KVStorePrefixIterator(prefixStore, nil) defer iterator.Close() - for ; iterator.Valid(); iterator.Next() { - key := string(iterator.Key()) // search prefix is omitted - - // reconstruct the capability name using the prefix and iterator key - name := fmt.Sprintf("%s/%s/%s%s", host.KeyChannelCapabilityPrefix, host.KeyPortPrefix, icatypes.PortPrefix, key) - owner := capabilitytypes.NewOwner(module, name) - - ctx.Logger().Info("migrating ibc channel capability", "owner", owner.String()) + for ; iterator.Valid(); iterator.Next() { + // unmarshal the index value and retrieve the set of owners index := sdk.BigEndianToUint64(iterator.Value()) owners, found := capabilityKeeper.GetOwners(ctx, index) if !found { - panic(fmt.Sprintf("no owners for capability at index: %d", index)) + return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityOwnersNotFound, "no owners found for capability at index: %d", index) } + // reconstruct the capability name using the prefixes and iterator key + name := fmt.Sprintf("%s/%s/%s%s", host.KeyChannelCapabilityPrefix, host.KeyPortPrefix, icatypes.PortPrefix, string(iterator.Key())) + prevOwner := capabilitytypes.NewOwner(module, name) + newOwner := capabilitytypes.NewOwner(types.SubModuleName, name) + // remove the existing module owner - owners.Remove(owner) + owners.Remove(prevOwner) prefixStore.Delete(iterator.Key()) - // add the controller submodule as a new capability owner - owners.Set(capabilitytypes.NewOwner(types.SubModuleName, name)) + // add the controller submodule to the set of owners and initialise the capability + owners.Set(newOwner) capabilityKeeper.SetOwners(ctx, index, owners) capabilityKeeper.InitializeCapability(ctx, index, owners) } diff --git a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go new file mode 100644 index 00000000000..4aaad118c3e --- /dev/null +++ b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go @@ -0,0 +1,125 @@ +package v5_test + +import ( + "testing" + + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + "github.com/stretchr/testify/suite" + + v5 "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/migrations/v5" + "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" + 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" + ibcmock "github.com/cosmos/ibc-go/v5/testing/mock" +) + +type MigrationsTestSuite struct { + suite.Suite + + chainA *ibctesting.TestChain + chainB *ibctesting.TestChain + + coordinator *ibctesting.Coordinator + path *ibctesting.Path +} + +func (suite *MigrationsTestSuite) SetupTest() { + suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2) + + suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1)) + suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2)) + + suite.path = ibctesting.NewPath(suite.chainA, suite.chainB) + suite.path.EndpointA.ChannelConfig.PortID = icatypes.PortID + suite.path.EndpointB.ChannelConfig.PortID = icatypes.PortID + suite.path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED + suite.path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED + suite.path.EndpointA.ChannelConfig.Version = icatypes.NewDefaultMetadataString(ibctesting.FirstConnectionID, ibctesting.FirstConnectionID) + suite.path.EndpointB.ChannelConfig.Version = icatypes.NewDefaultMetadataString(ibctesting.FirstConnectionID, ibctesting.FirstConnectionID) +} + +func (suite *MigrationsTestSuite) SetupPath() error { + if err := suite.RegisterInterchainAccount(suite.path.EndpointA, ibctesting.TestAccAddress); err != nil { + return err + } + + if err := suite.path.EndpointB.ChanOpenTry(); err != nil { + return err + } + + if err := suite.path.EndpointA.ChanOpenAck(); err != nil { + return err + } + + if err := suite.path.EndpointB.ChanOpenConfirm(); err != nil { + return err + } + + return nil +} + +func (suite *MigrationsTestSuite) RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) error { + portID, err := icatypes.NewControllerPortID(owner) + if err != nil { + return err + } + + channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext()) + + if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version); err != nil { + return err + } + + // commit state changes for proof verification + endpoint.Chain.NextBlock() + + // update port/channel ids + endpoint.ChannelID = channeltypes.FormatChannelIdentifier(channelSequence) + endpoint.ChannelConfig.PortID = portID + + return nil +} + +func TestKeeperTestSuite(t *testing.T) { + suite.Run(t, new(MigrationsTestSuite)) +} + +func (suite *MigrationsTestSuite) TestMigrateChannelCapability() { + suite.SetupTest() + + suite.coordinator.SetupConnections(suite.path) + + err := suite.SetupPath() + suite.Require().NoError(err) + + capName := host.ChannelCapabilityPath(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) + + // assert the capability is owned by the mock module + cap, found := suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName) + suite.Require().NotNil(cap) + suite.Require().True(found) + + cap, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName) + suite.Require().Nil(cap) + suite.Require().False(found) + + err = v5.MigrateICS27ChannelCapability( + suite.chainA.GetContext(), + suite.chainA.GetSimApp().GetMemKey(capabilitytypes.MemStoreKey), + suite.chainA.GetSimApp().CapabilityKeeper, + ibcmock.ModuleName+types.SubModuleName, + ) + + suite.Require().NoError(err) + + // assert the capability is now owned by the ICS27 controller submodule + cap, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName) + suite.Require().NotNil(cap) + suite.Require().True(found) + + cap, found = suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName) + suite.Require().Nil(cap) + suite.Require().False(found) +} From c2da8b2367a32a1a167850fa4f5db0a93243c67c Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 26 Aug 2022 17:16:17 +0200 Subject: [PATCH 07/18] adding additional checks to tests --- .../controller/migrations/v5/migrations_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go index 4aaad118c3e..881ad84adb5 100644 --- a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go @@ -101,10 +101,16 @@ func (suite *MigrationsTestSuite) TestMigrateChannelCapability() { suite.Require().NotNil(cap) suite.Require().True(found) + isAuthenticated := suite.chainA.GetSimApp().ScopedICAMockKeeper.AuthenticateCapability(suite.chainA.GetContext(), cap, capName) + suite.Require().True(isAuthenticated) + cap, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName) suite.Require().Nil(cap) suite.Require().False(found) + isAuthenticated = suite.chainA.GetSimApp().ScopedICAControllerKeeper.AuthenticateCapability(suite.chainA.GetContext(), cap, capName) + suite.Require().False(isAuthenticated) + err = v5.MigrateICS27ChannelCapability( suite.chainA.GetContext(), suite.chainA.GetSimApp().GetMemKey(capabilitytypes.MemStoreKey), @@ -119,7 +125,13 @@ func (suite *MigrationsTestSuite) TestMigrateChannelCapability() { suite.Require().NotNil(cap) suite.Require().True(found) + isAuthenticated = suite.chainA.GetSimApp().ScopedICAControllerKeeper.AuthenticateCapability(suite.chainA.GetContext(), cap, capName) + suite.Require().True(isAuthenticated) + cap, found = suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName) suite.Require().Nil(cap) suite.Require().False(found) + + isAuthenticated = suite.chainA.GetSimApp().ScopedICAControllerKeeper.AuthenticateCapability(suite.chainA.GetContext(), cap, capName) + suite.Require().False(isAuthenticated) } From 6ea20140a191a7042a0ef3276c5b6a955581fc0a Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 26 Aug 2022 17:19:26 +0200 Subject: [PATCH 08/18] cleaning up tests --- .../controller/migrations/v5/migrations_test.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go index 881ad84adb5..626866c0c0e 100644 --- a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go @@ -88,7 +88,6 @@ func TestKeeperTestSuite(t *testing.T) { func (suite *MigrationsTestSuite) TestMigrateChannelCapability() { suite.SetupTest() - suite.coordinator.SetupConnections(suite.path) err := suite.SetupPath() @@ -108,9 +107,6 @@ func (suite *MigrationsTestSuite) TestMigrateChannelCapability() { suite.Require().Nil(cap) suite.Require().False(found) - isAuthenticated = suite.chainA.GetSimApp().ScopedICAControllerKeeper.AuthenticateCapability(suite.chainA.GetContext(), cap, capName) - suite.Require().False(isAuthenticated) - err = v5.MigrateICS27ChannelCapability( suite.chainA.GetContext(), suite.chainA.GetSimApp().GetMemKey(capabilitytypes.MemStoreKey), @@ -131,7 +127,4 @@ func (suite *MigrationsTestSuite) TestMigrateChannelCapability() { cap, found = suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName) suite.Require().Nil(cap) suite.Require().False(found) - - isAuthenticated = suite.chainA.GetSimApp().ScopedICAControllerKeeper.AuthenticateCapability(suite.chainA.GetContext(), cap, capName) - suite.Require().False(isAuthenticated) } From c981bcc8657f44ff588ceea74c0d398cec1bdd56 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 26 Aug 2022 17:20:36 +0200 Subject: [PATCH 09/18] cleaning up tests --- .../controller/migrations/v5/migrations_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go index 626866c0c0e..cfb03c57736 100644 --- a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go @@ -86,7 +86,7 @@ func TestKeeperTestSuite(t *testing.T) { suite.Run(t, new(MigrationsTestSuite)) } -func (suite *MigrationsTestSuite) TestMigrateChannelCapability() { +func (suite *MigrationsTestSuite) TestMigrateICS27ChannelCapability() { suite.SetupTest() suite.coordinator.SetupConnections(suite.path) From 88124f49236de021fb1e5486a229cb25782aa396 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 26 Aug 2022 17:32:10 +0200 Subject: [PATCH 10/18] updating inline doc comments, checking err return --- .../controller/migrations/v5/migrations.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go index 921b107d95e..ebac7b00d21 100644 --- a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go @@ -31,7 +31,7 @@ func MigrateICS27ChannelCapability( defer iterator.Close() for ; iterator.Valid(); iterator.Next() { - // unmarshal the index value and retrieve the set of owners + // unmarshal the capability index value and retrieve the set of owners index := sdk.BigEndianToUint64(iterator.Value()) owners, found := capabilityKeeper.GetOwners(ctx, index) if !found { @@ -48,7 +48,10 @@ func MigrateICS27ChannelCapability( prefixStore.Delete(iterator.Key()) // add the controller submodule to the set of owners and initialise the capability - owners.Set(newOwner) + if err := owners.Set(newOwner); err != nil { + return err + } + capabilityKeeper.SetOwners(ctx, index, owners) capabilityKeeper.InitializeCapability(ctx, index, owners) } From 443b548630d2220fc7958d2eded5075afba5b2c0 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 29 Aug 2022 12:53:30 +0200 Subject: [PATCH 11/18] using InitMemStore in favour of InitializeCapability, adjusting tests --- .../controller/migrations/v5/migrations.go | 7 +++++-- .../controller/migrations/v5/migrations_test.go | 5 +++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go index ebac7b00d21..50b8d7e13d7 100644 --- a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go @@ -47,13 +47,16 @@ func MigrateICS27ChannelCapability( owners.Remove(prevOwner) prefixStore.Delete(iterator.Key()) - // add the controller submodule to the set of owners and initialise the capability + // add the controller submodule to the set of owners if err := owners.Set(newOwner); err != nil { return err } + // set the new owners for the appropriate capability index capabilityKeeper.SetOwners(ctx, index, owners) - capabilityKeeper.InitializeCapability(ctx, index, owners) + + // initialise the x/capability memstore + capabilityKeeper.InitMemStore(ctx) } return nil diff --git a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go index cfb03c57736..ad3850fbb9b 100644 --- a/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go @@ -107,6 +107,11 @@ func (suite *MigrationsTestSuite) TestMigrateICS27ChannelCapability() { suite.Require().Nil(cap) suite.Require().False(found) + // manually delete the `KeyMemInitialised` from the x/capability memstore + // this allows capabilityKeeper.InitMemStore(ctx) to re-initialise capabilities + memStore := suite.chainA.GetContext().KVStore(suite.chainA.GetSimApp().GetMemKey(capabilitytypes.MemStoreKey)) + memStore.Delete(capabilitytypes.KeyMemInitialized) + err = v5.MigrateICS27ChannelCapability( suite.chainA.GetContext(), suite.chainA.GetSimApp().GetMemKey(capabilitytypes.MemStoreKey), From 52291590ad1fa37d5e0834ab2c06b4b50c99e0d8 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 29 Aug 2022 15:41:46 +0200 Subject: [PATCH 12/18] adding assertion of channel capabilities migration --- .../controller/keeper/migrations.go | 44 +++++++++++++++++++ .../controller/keeper/migrations_test.go | 38 ++++++++++++++++ modules/apps/27-interchain-accounts/module.go | 7 ++- .../types/expected_keepers.go | 1 + 4 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 modules/apps/27-interchain-accounts/controller/keeper/migrations.go create mode 100644 modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go new file mode 100644 index 00000000000..0d779b7184b --- /dev/null +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -0,0 +1,44 @@ +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/apps/27-interchain-accounts/controller/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 { + for _, channel := range m.keeper.GetAllActiveChannels(ctx) { + name := host.ChannelCapabilityPath(channel.PortId, channel.ChannelId) + owners, found := m.keeper.scopedKeeper.GetOwners(ctx, name) + if !found { + return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityOwnersNotFound, "failed to find capability owners for: %s", name) + } + + ibcOwner := capabilitytypes.NewOwner(host.ModuleName, name) + if index, found := owners.Get(ibcOwner); !found && index != 0 { + return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", host.ModuleName) + } + + controllerOwner := capabilitytypes.NewOwner(types.SubModuleName, name) + if index, found := owners.Get(controllerOwner); !found && index != 1 { + return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", types.SubModuleName) + } + } + + return nil +} diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go new file mode 100644 index 00000000000..2b819fbfdb6 --- /dev/null +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go @@ -0,0 +1,38 @@ +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, + }, + } + + 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.MigrateCapabilitiesConfirm(suite.chainA.GetContext()) + suite.Require().NoError(err) + }) + } +} diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index 18c52693313..759b7564b4d 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -147,6 +147,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. @@ -187,7 +192,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) { diff --git a/modules/apps/27-interchain-accounts/types/expected_keepers.go b/modules/apps/27-interchain-accounts/types/expected_keepers.go index 787969d9f0e..d1a51a3c657 100644 --- a/modules/apps/27-interchain-accounts/types/expected_keepers.go +++ b/modules/apps/27-interchain-accounts/types/expected_keepers.go @@ -43,4 +43,5 @@ type ScopedKeeper interface { AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool LookupModules(ctx sdk.Context, name string) ([]string, *capabilitytypes.Capability, error) ClaimCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) error + GetOwners(ctx sdk.Context, name string) (*capabilitytypes.CapabilityOwners, bool) } From bf88e6d9c9874c1495dfc8a5cc57643808404172 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 1 Sep 2022 10:50:08 +0200 Subject: [PATCH 13/18] adapting migration code to use get/authenticate capability --- .../controller/keeper/migrations.go | 15 +++++---------- .../controller/keeper/migrations_test.go | 2 +- .../controller/keeper/msg_server_test.go | 3 +-- .../types/expected_keepers.go | 1 - 4 files changed, 7 insertions(+), 14 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go index 0d779b7184b..c689b32e561 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -5,7 +5,7 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" - "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types" + "github.com/cosmos/ibc-go/v5/modules/core/23-commitment/types" host "github.com/cosmos/ibc-go/v5/modules/core/24-host" ) @@ -24,18 +24,13 @@ func NewMigrator(keeper *Keeper) Migrator { func (m Migrator) AssertChannelCapabilityMigrations(ctx sdk.Context) error { for _, channel := range m.keeper.GetAllActiveChannels(ctx) { name := host.ChannelCapabilityPath(channel.PortId, channel.ChannelId) - owners, found := m.keeper.scopedKeeper.GetOwners(ctx, name) + cap, found := m.keeper.scopedKeeper.GetCapability(ctx, name) if !found { - return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityOwnersNotFound, "failed to find capability owners for: %s", name) + return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotFound, "failed to find capability: %s", name) } - ibcOwner := capabilitytypes.NewOwner(host.ModuleName, name) - if index, found := owners.Get(ibcOwner); !found && index != 0 { - return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", host.ModuleName) - } - - controllerOwner := capabilitytypes.NewOwner(types.SubModuleName, name) - if index, found := owners.Get(controllerOwner); !found && index != 1 { + isAuthenticated := m.keeper.scopedKeeper.AuthenticateCapability(ctx, cap, name) + if !isAuthenticated { return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", types.SubModuleName) } } diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go index 2b819fbfdb6..8a0cc13f58a 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go @@ -31,7 +31,7 @@ func (suite *KeeperTestSuite) TestAssertChannelCapabilityMigrations() { tc.malleate() migrator := keeper.NewMigrator(&suite.chainA.GetSimApp().ICAControllerKeeper) - err = migrator.MigrateCapabilitiesConfirm(suite.chainA.GetContext()) + err = migrator.AssertChannelCapabilityMigrations(suite.chainA.GetContext()) suite.Require().NoError(err) }) } diff --git a/modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go b/modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go index 38aa673015c..143512150f7 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go @@ -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" @@ -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) diff --git a/modules/apps/27-interchain-accounts/types/expected_keepers.go b/modules/apps/27-interchain-accounts/types/expected_keepers.go index d1a51a3c657..787969d9f0e 100644 --- a/modules/apps/27-interchain-accounts/types/expected_keepers.go +++ b/modules/apps/27-interchain-accounts/types/expected_keepers.go @@ -43,5 +43,4 @@ type ScopedKeeper interface { AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool LookupModules(ctx sdk.Context, name string) ([]string, *capabilitytypes.Capability, error) ClaimCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) error - GetOwners(ctx sdk.Context, name string) (*capabilitytypes.CapabilityOwners, bool) } From 8949fa983ea355145bfd441982248843d47ee574 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 1 Sep 2022 10:54:08 +0200 Subject: [PATCH 14/18] adding changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94df3525f2f..0a89bcac0b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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. The ICS27 module consensus version has been bumped from 1 to 2. ### Features From 0bd27e977ca27bb8c58d8a5f3403fca0021c9367 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 1 Sep 2022 11:07:08 +0200 Subject: [PATCH 15/18] updating tests --- .../controller/keeper/migrations_test.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go index 8a0cc13f58a..549d319e931 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go @@ -16,6 +16,18 @@ func (suite *KeeperTestSuite) TestAssertChannelCapabilityMigrations() { 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 { @@ -32,7 +44,12 @@ func (suite *KeeperTestSuite) TestAssertChannelCapabilityMigrations() { migrator := keeper.NewMigrator(&suite.chainA.GetSimApp().ICAControllerKeeper) err = migrator.AssertChannelCapabilityMigrations(suite.chainA.GetContext()) - suite.Require().NoError(err) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } }) } } From dda03986bdb8adec28720a9b838c1747fc373e16 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 1 Sep 2022 17:06:07 +0200 Subject: [PATCH 16/18] set middleware enabled for existing channels --- .../27-interchain-accounts/controller/keeper/migrations.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go index c689b32e561..ffe86e9f6e7 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -22,8 +22,8 @@ func NewMigrator(keeper *Keeper) Migrator { // 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 { - for _, channel := range m.keeper.GetAllActiveChannels(ctx) { - name := host.ChannelCapabilityPath(channel.PortId, channel.ChannelId) + 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) @@ -33,6 +33,8 @@ func (m Migrator) AssertChannelCapabilityMigrations(ctx sdk.Context) error { if !isAuthenticated { return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", types.SubModuleName) } + + m.keeper.SetMiddlewareEnabled(ctx, ch.PortId, ch.ChannelId) } return nil From 49df7dd3967843be2a125feb77d02da85c37c59c Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 6 Sep 2022 18:01:59 +0200 Subject: [PATCH 17/18] assert middleware enabled in tests --- .../controller/keeper/migrations_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go index 549d319e931..be20f12cfae 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go @@ -47,6 +47,14 @@ func (suite *KeeperTestSuite) TestAssertChannelCapabilityMigrations() { if tc.expPass { suite.Require().NoError(err) + + 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) } From 4fc9aaecd248490c5db18228d205b79f72c0bc2a Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 6 Sep 2022 18:09:50 +0200 Subject: [PATCH 18/18] updating changelog with middleware enabled flag --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a89bcac0b6..4fd048355c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,7 +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. The ICS27 module consensus version has been bumped from 1 to 2. +* (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