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.

Sign up for GitHub

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

refactor: simplify automatic migration code by using client keeper functions #2864

Merged
merged 7 commits into from
Dec 7, 2022
Merged

refactor: simplify automatic migration code by using client keeper functions #2864

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8044923
refactor: simplify automatic migration code by using client keeper fu…
colin-axner Dec 1, 2022
029df42
chore: add code comment
colin-axner Dec 1, 2022
68412fe
chore: remove unnecessary code
colin-axner Dec 5, 2022
f16f92a
Merge branch 'main' into colin/2845-simplify-automatic-migrations
colin-axner Dec 5, 2022
bf89747
Merge branch 'main' of github.com:cosmos/ibc-go into colin/2845-simpl…
colin-axner Dec 7, 2022
56d83ec
fix: update tests from merging main
colin-axner Dec 7, 2022
b59b294
chore: reduce expected keeper interface
colin-axner Dec 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
2 changes: 1 addition & 1 deletion modules/core/02-client/keeper/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,5 @@ func (m Migrator) Migrate1to2(ctx sdk.Context) error {
// - removes the localhost client
// - asserts that existing tendermint clients are properly registered on the chain codec
func (m Migrator) Migrate2to3(ctx sdk.Context) error {
return v7.MigrateStore(ctx, m.keeper.storeKey, m.keeper.cdc)
return v7.MigrateStore(ctx, m.keeper.storeKey, m.keeper.cdc, m.keeper)
}
15 changes: 15 additions & 0 deletions modules/core/02-client/migrations/v7/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package v7

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

"github.com/cosmos/ibc-go/v6/modules/core/exported"
)

// ClientKeeper expected IBC client keeper
type ClientKeeper interface {
GetClientState(ctx sdk.Context, clientID string) (exported.ClientState, bool)
SetClientState(ctx sdk.Context, clientID string, clientState exported.ClientState)
IterateClientStates(ctx sdk.Context, prefix []byte, cb func(string, exported.ClientState) bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a usage of this function, can we remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: I see we use it in #2862!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice call! It can be removed since this is a different expected keepers definition

ClientStore(ctx sdk.Context, clientID string) sdk.KVStore
}
41 changes: 16 additions & 25 deletions modules/core/02-client/migrations/v7/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (

"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"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"
Expand All @@ -29,18 +28,18 @@ const Localhost string = "09-localhost"
// - Pruning all solo machine consensus states
// - Removing the localhost client
// - Asserting existing tendermint clients are properly registered on the chain codec
func MigrateStore(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.BinaryCodec) error {
func MigrateStore(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.BinaryCodec, clientKeeper ClientKeeper) error {
store := ctx.KVStore(storeKey)

if err := handleSolomachineMigration(ctx, store, cdc); err != nil {
if err := handleSolomachineMigration(ctx, store, cdc, clientKeeper); err != nil {
return err
}

if err := handleTendermintMigration(ctx, store, cdc); err != nil {
if err := handleTendermintMigration(ctx, store, cdc, clientKeeper); err != nil {
return err
}

if err := handleLocalhostMigration(ctx, store, cdc); err != nil {
if err := handleLocalhostMigration(ctx, store, cdc, clientKeeper); err != nil {
return err
}

Expand All @@ -49,15 +48,14 @@ func MigrateStore(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.Binar

// handleSolomachineMigration iterates over the solo machine clients and migrates client state from
// protobuf definition v2 to v3. All consensus states stored outside of the client state are pruned.
func handleSolomachineMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.BinaryCodec) error {
func handleSolomachineMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.BinaryCodec, clientKeeper ClientKeeper) error {
clients, err := collectClients(ctx, store, exported.Solomachine)
if err != nil {
return err
}

for _, clientID := range clients {
clientPrefix := []byte(fmt.Sprintf("%s/%s/", host.KeyClientStorePrefix, clientID))
clientStore := prefix.NewStore(store, clientPrefix)
clientStore := clientKeeper.ClientStore(ctx, clientID)

bz := clientStore.Get(host.ClientStateKey())
if bz == nil {
Expand All @@ -82,7 +80,7 @@ func handleSolomachineMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.Bi
}

// update solomachine in store
clientStore.Set(host.ClientStateKey(), bz)
clientKeeper.SetClientState(ctx, clientID, &updatedClientState)
Copy link
Contributor

Choose a reason for hiding this comment

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

If updatedClientState is used here, is it still necessary to do bz, err := clienttypes.MarshalClientState(cdc, &updatedClientState) above?

Copy link
Member

Choose a reason for hiding this comment

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

I think @crodriguezvega is right here, we can probably remove bz, err := ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, nice catch!


removeAllClientConsensusStates(clientStore)
}
Expand All @@ -92,7 +90,7 @@ func handleSolomachineMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.Bi

// handlerTendermintMigration asserts that the tendermint client in state can be decoded properly.
// This ensures the upgrading chain properly registered the tendermint client types on the chain codec.
func handleTendermintMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.BinaryCodec) error {
func handleTendermintMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.BinaryCodec, clientKeeper ClientKeeper) error {
clients, err := collectClients(ctx, store, exported.Tendermint)
if err != nil {
return err
Expand All @@ -104,20 +102,14 @@ func handleTendermintMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.Bin

clientID := clients[0]

clientPrefix := []byte(fmt.Sprintf("%s/%s/", host.KeyClientStorePrefix, clientID))
clientStore := prefix.NewStore(store, clientPrefix)

bz := clientStore.Get(host.ClientStateKey())
if bz == nil {
return clienttypes.ErrClientNotFound
}

var clientState exported.ClientState
if err := cdc.UnmarshalInterface(bz, &clientState); err != nil {
return sdkerrors.Wrap(err, "failed to unmarshal client state bytes into tendermint client state")
// unregistered tendermint client types will panic when unmarshaling the client state
// in GetClientState
clientState, ok := clientKeeper.GetClientState(ctx, clientID)
if !ok {
return sdkerrors.Wrapf(clienttypes.ErrClientNotFound, "clientID %s", clientID)
}

_, ok := clientState.(*ibctm.ClientState)
_, ok = clientState.(*ibctm.ClientState)
if !ok {
return sdkerrors.Wrap(clienttypes.ErrInvalidClient, "client state is not tendermint even though client id contains 07-tendermint")
}
Expand All @@ -126,15 +118,14 @@ func handleTendermintMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.Bin
}

// handleLocalhostMigration removes all client and consensus states associated with the localhost client type.
func handleLocalhostMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.BinaryCodec) error {
func handleLocalhostMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.BinaryCodec, clientKeeper ClientKeeper) error {
clients, err := collectClients(ctx, store, Localhost)
if err != nil {
return err
}

for _, clientID := range clients {
clientPrefix := []byte(fmt.Sprintf("%s/%s/", host.KeyClientStorePrefix, clientID))
clientStore := prefix.NewStore(store, clientPrefix)
clientStore := clientKeeper.ClientStore(ctx, clientID)

// delete the client state
clientStore.Delete(host.ClientStateKey())
Expand Down
23 changes: 1 addition & 22 deletions modules/core/02-client/migrations/v7/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,11 @@ import (
"strconv"
"testing"

"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/stretchr/testify/suite"

"github.com/cosmos/ibc-go/v6/modules/core/02-client/migrations/v7"
"github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
solomachine "github.com/cosmos/ibc-go/v6/modules/light-clients/06-solomachine"
ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint"
ibctesting "github.com/cosmos/ibc-go/v6/testing"
)

Expand Down Expand Up @@ -40,23 +36,6 @@ func TestIBCTestSuite(t *testing.T) {
suite.Run(t, new(MigrationsV7TestSuite))
}

// test that MigrateStore returns an error if the codec used doesn't register tendermint types
func (suite *MigrationsV7TestSuite) TestMigrateStoreTendermint() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

GetClientState uses the client keeper codec which has tendermint client types registered so this test is no longer useful

path := ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.SetupClients(path)

registry := codectypes.NewInterfaceRegistry()
cdc := codec.NewProtoCodec(registry)

solomachine.RegisterInterfaces(registry)
err := v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().GetKey(host.StoreKey), cdc)
suite.Require().Error(err)

ibctm.RegisterInterfaces(registry)
err = v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().GetKey(host.StoreKey), cdc)
suite.Require().NoError(err)
}

// create multiple solo machine clients, tendermint and localhost clients
// ensure that solo machine clients are migrated and their consensus states are removed
// ensure the localhost is deleted entirely.
Expand All @@ -79,7 +58,7 @@ func (suite *MigrationsV7TestSuite) TestMigrateStore() {
suite.createSolomachineClients(solomachines)
suite.createLocalhostClients()

err := v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().GetKey(host.StoreKey), suite.chainA.App.AppCodec())
err := v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().GetKey(host.StoreKey), suite.chainA.App.AppCodec(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper)
suite.Require().NoError(err)

suite.assertSolomachineClients(solomachines)
Expand Down