From 4d3623d2cc0c345870fac58b9ce17da5e119c4bf Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 18 Apr 2024 01:18:02 +0300 Subject: [PATCH] chore: address some additional nits. (#6176) Make GetQueryPlugins and SetQueryPlugins as moved on the keeper private. Rename var to match semantics. Remove unecessary client state look-up in migrate contract. --- .../08-wasm/keeper/export_test.go | 10 ++++++++ .../light-clients/08-wasm/keeper/keeper.go | 25 ++++++++----------- .../light-clients/08-wasm/keeper/keeper_vm.go | 2 +- .../08-wasm/keeper/msg_server_test.go | 2 +- .../light-clients/08-wasm/keeper/options.go | 4 +-- .../08-wasm/light_client_module.go | 12 ++++----- 6 files changed, 30 insertions(+), 25 deletions(-) diff --git a/modules/light-clients/08-wasm/keeper/export_test.go b/modules/light-clients/08-wasm/keeper/export_test.go index a1e992b7f8c..82f23ab307c 100644 --- a/modules/light-clients/08-wasm/keeper/export_test.go +++ b/modules/light-clients/08-wasm/keeper/export_test.go @@ -6,3 +6,13 @@ import sdk "github.com/cosmos/cosmos-sdk/types" func (k Keeper) MigrateContractCode(ctx sdk.Context, clientID string, newChecksum, migrateMsg []byte) error { return k.migrateContractCode(ctx, clientID, newChecksum, migrateMsg) } + +// GetQueryPlugins is a wrapper around k.getQueryPlugins to allow the method to be directly called in tests. +func (k Keeper) GetQueryPlugins() QueryPlugins { + return k.getQueryPlugins() +} + +// SetQueryPlugins is a wrapper around k.setQueryPlugins to allow the method to be directly called in tests. +func (k *Keeper) SetQueryPlugins(plugins QueryPlugins) { + k.setQueryPlugins(plugins) +} diff --git a/modules/light-clients/08-wasm/keeper/keeper.go b/modules/light-clients/08-wasm/keeper/keeper.go index 0556189bbc7..98071622644 100644 --- a/modules/light-clients/08-wasm/keeper/keeper.go +++ b/modules/light-clients/08-wasm/keeper/keeper.go @@ -68,18 +68,18 @@ func (k Keeper) GetChecksums() collections.KeySet[[]byte] { return k.checksums } -// GetQueryPlugins returns the set query plugins. -func (k Keeper) GetQueryPlugins() QueryPlugins { +// getQueryPlugins returns the set query plugins. +func (k Keeper) getQueryPlugins() QueryPlugins { return k.queryPlugins } // SetQueryPlugins sets the plugins. -func (k *Keeper) SetQueryPlugins(plugins QueryPlugins) { +func (k *Keeper) setQueryPlugins(plugins QueryPlugins) { k.queryPlugins = plugins } func (k Keeper) newQueryHandler(ctx sdk.Context, callerID string) *queryHandler { - return newQueryHandler(ctx, k.GetQueryPlugins(), callerID) + return newQueryHandler(ctx, k.getQueryPlugins(), callerID) } // storeWasmCode stores the contract to the VM, pins the checksum in the VM's in memory cache and stores the checksum @@ -142,32 +142,27 @@ func (k Keeper) storeWasmCode(ctx sdk.Context, code []byte, storeFn func(code wa // migrateContractCode migrates the contract for a given light client to one denoted by the given new checksum. The checksum we // are migrating to must first be stored using storeWasmCode and must not match the checksum currently stored for this light client. func (k Keeper) migrateContractCode(ctx sdk.Context, clientID string, newChecksum, migrateMsg []byte) error { - wasmClientState, err := k.GetWasmClientState(ctx, clientID) - if err != nil { - return errorsmod.Wrap(err, "failed to retrieve wasm client state") - } - oldChecksum := wasmClientState.Checksum - clientStore := k.clientKeeper.ClientStore(ctx, clientID) - clientState, found := types.GetClientState(clientStore, k.cdc) + wasmClientState, found := types.GetClientState(clientStore, k.cdc) if !found { return errorsmod.Wrap(clienttypes.ErrClientNotFound, clientID) } + oldChecksum := wasmClientState.Checksum if !k.HasChecksum(ctx, newChecksum) { return types.ErrWasmChecksumNotFound } - if bytes.Equal(clientState.Checksum, newChecksum) { - return errorsmod.Wrapf(types.ErrWasmCodeExists, "new checksum (%s) is the same as current checksum (%s)", hex.EncodeToString(newChecksum), hex.EncodeToString(clientState.Checksum)) + if bytes.Equal(wasmClientState.Checksum, newChecksum) { + return errorsmod.Wrapf(types.ErrWasmCodeExists, "new checksum (%s) is the same as current checksum (%s)", hex.EncodeToString(newChecksum), hex.EncodeToString(wasmClientState.Checksum)) } // update the checksum, this needs to be done before the contract migration // so that wasmMigrate can call the right code. Note that this is not // persisted to the client store. - clientState.Checksum = newChecksum + wasmClientState.Checksum = newChecksum - err = k.WasmMigrate(ctx, clientStore, clientState, clientID, migrateMsg) + err := k.WasmMigrate(ctx, clientStore, wasmClientState, clientID, migrateMsg) if err != nil { return err } diff --git a/modules/light-clients/08-wasm/keeper/keeper_vm.go b/modules/light-clients/08-wasm/keeper/keeper_vm.go index 623733ab246..69991d65eaa 100644 --- a/modules/light-clients/08-wasm/keeper/keeper_vm.go +++ b/modules/light-clients/08-wasm/keeper/keeper_vm.go @@ -68,7 +68,7 @@ func NewKeeperWithVM( // set query plugins to ensure there is a non-nil query plugin // regardless of what options the user provides - keeper.SetQueryPlugins(NewDefaultQueryPlugins(queryRouter)) + keeper.setQueryPlugins(NewDefaultQueryPlugins(queryRouter)) for _, opt := range opts { opt.apply(keeper) diff --git a/modules/light-clients/08-wasm/keeper/msg_server_test.go b/modules/light-clients/08-wasm/keeper/msg_server_test.go index 72a113680fc..460ff992559 100644 --- a/modules/light-clients/08-wasm/keeper/msg_server_test.go +++ b/modules/light-clients/08-wasm/keeper/msg_server_test.go @@ -218,7 +218,7 @@ func (suite *KeeperTestSuite) TestMsgMigrateContract() { func() { msg = types.NewMsgMigrateContract(govAcc, ibctesting.InvalidID, newChecksum, []byte("{}")) }, - clienttypes.ErrClientTypeNotFound, + clienttypes.ErrClientNotFound, }, { "failure: vm returns error", diff --git a/modules/light-clients/08-wasm/keeper/options.go b/modules/light-clients/08-wasm/keeper/options.go index 320f37be351..e05acaa3332 100644 --- a/modules/light-clients/08-wasm/keeper/options.go +++ b/modules/light-clients/08-wasm/keeper/options.go @@ -15,9 +15,9 @@ func (f optsFn) apply(keeper *Keeper) { // Missing fields will be filled with default queriers. func WithQueryPlugins(plugins *QueryPlugins) Option { return optsFn(func(k *Keeper) { - currentPlugins := k.GetQueryPlugins() + currentPlugins := k.getQueryPlugins() newPlugins := currentPlugins.Merge(plugins) - k.SetQueryPlugins(newPlugins) + k.setQueryPlugins(newPlugins) }) } diff --git a/modules/light-clients/08-wasm/light_client_module.go b/modules/light-clients/08-wasm/light_client_module.go index d702ef425d9..ee5e081724e 100644 --- a/modules/light-clients/08-wasm/light_client_module.go +++ b/modules/light-clients/08-wasm/light_client_module.go @@ -431,8 +431,8 @@ func (l LightClientModule) RecoverClient(ctx sdk.Context, clientID, substituteCl cdc := l.keeper.Codec() - clientStore := l.storeProvider.ClientStore(ctx, clientID) - clientState, found := types.GetClientState(clientStore, cdc) + subjectClientStore := l.storeProvider.ClientStore(ctx, clientID) + subjectClientState, found := types.GetClientState(subjectClientStore, cdc) if !found { return errorsmod.Wrap(clienttypes.ErrClientNotFound, clientID) } @@ -445,17 +445,17 @@ func (l LightClientModule) RecoverClient(ctx sdk.Context, clientID, substituteCl // check that checksums of subject client state and substitute client state match // changing the checksum is only allowed through the migrate contract RPC endpoint - if !bytes.Equal(clientState.Checksum, substituteClientState.Checksum) { - return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "expected checksums to be equal: expected %s, got %s", hex.EncodeToString(clientState.Checksum), hex.EncodeToString(substituteClientState.Checksum)) + if !bytes.Equal(subjectClientState.Checksum, substituteClientState.Checksum) { + return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "expected checksums to be equal: expected %s, got %s", hex.EncodeToString(subjectClientState.Checksum), hex.EncodeToString(substituteClientState.Checksum)) } - store := types.NewMigrateClientWrappedStore(clientStore, substituteClientStore) + store := types.NewMigrateClientWrappedStore(subjectClientStore, substituteClientStore) payload := types.SudoMsg{ MigrateClientStore: &types.MigrateClientStoreMsg{}, } - _, err = l.keeper.WasmSudo(ctx, clientID, store, clientState, payload) + _, err = l.keeper.WasmSudo(ctx, clientID, store, subjectClientState, payload) return err }