-
Notifications
You must be signed in to change notification settings - Fork 597
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: ics27 channel capability migrations #2134
Changes from 14 commits
a6575eb
94091cf
3c95389
2bb7c3f
d546d20
a79d10e
c2da8b2
6ea2014
c981bcc
88124f4
cf3bfaf
443b548
bdec1bc
e11be4c
b5ad7aa
8571ecc
bf8457b
5840e16
96b733b
738135d
a1e34b9
8fba15f
dc258a2
1cee4fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
package v5 | ||
|
||
import ( | ||
"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/controller/types" | ||
) | ||
|
||
// 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 MigrateICS27ChannelCapability( | ||
ctx sdk.Context, | ||
cdc codec.BinaryCodec, | ||
storeKey storetypes.StoreKey, | ||
capabilityKeeper *capabilitykeeper.Keeper, | ||
module string, | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) error { | ||
// construct a prefix store using the x/capability index prefix: index->capability owners | ||
prefixStore := prefix.NewStore(ctx.KVStore(storeKey), capabilitytypes.KeyPrefixIndexCapability) | ||
iterator := sdk.KVStorePrefixIterator(prefixStore, nil) | ||
defer iterator.Close() | ||
|
||
for ; iterator.Valid(); iterator.Next() { | ||
// unmarshal the capability index value and set of owners | ||
index := capabilitytypes.IndexFromKey(iterator.Key()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the significance of this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Is this a fair description @AdityaSripal? |
||
|
||
var owners capabilitytypes.CapabilityOwners | ||
cdc.MustUnmarshal(iterator.Value(), &owners) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the function already has an error as a method signature, should be return an error instead? (or do we want the panic to be handled elsewhere?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error would be very unexpected. It would imply a bug in the code, so I think a panic makes sense There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I can update this to return the err. I would expect the panic to be caught by the sdk, but it's probably cleaner to return the error. I just grabbed this from some code in |
||
|
||
for _, owner := range owners.GetOwners() { | ||
if owner.Module == module { | ||
// remove the existing module owners | ||
owners.Remove(owner) | ||
|
||
// reassign the owner module to icacontroller | ||
owner.Module = types.SubModuleName | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
chatton marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// add the controller submodule to the set of owners | ||
if err := owners.Set(owner); err != nil { | ||
return err | ||
} | ||
|
||
// set the new owners for the current capability index | ||
capabilityKeeper.SetOwners(ctx, index, owners) | ||
} | ||
} | ||
} | ||
|
||
// initialise the x/capability memstore | ||
capabilityKeeper.InitMemStore(ctx) | ||
|
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,147 @@ | ||
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) TestMigrateICS27ChannelCapability() { | ||
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) | ||
|
||
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) | ||
|
||
suite.ResetMemStore() // empty the x/capability in-memory store | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to add this to the tests to mock/simulate a new chain binary running the migration handler against persisted state only. I think with this I feel better about the assertions done below after |
||
|
||
err = v5.MigrateICS27ChannelCapability( | ||
suite.chainA.GetContext(), | ||
suite.chainA.Codec, | ||
suite.chainA.GetSimApp().GetKey(capabilitytypes.StoreKey), | ||
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) | ||
|
||
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) | ||
} | ||
|
||
// ResetMemstore removes all existing fwd and rev capability kv pairs and deletes `KeyMemInitialised` from the x/capability memstore. | ||
// This effectively mocks a new chain binary being started. Migration code is run against persisted state only and allows the memstore to be reinitialised. | ||
func (suite *MigrationsTestSuite) ResetMemStore() { | ||
memStore := suite.chainA.GetContext().KVStore(suite.chainA.GetSimApp().GetMemKey(capabilitytypes.MemStoreKey)) | ||
memStore.Delete(capabilitytypes.KeyMemInitialized) | ||
|
||
iterator := memStore.Iterator(nil, nil) | ||
defer iterator.Close() | ||
|
||
for ; iterator.Valid(); iterator.Next() { | ||
memStore.Delete(iterator.Key()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth linking to an issue describing why the migration is necessary 🤔