Refactor 08-wasm with improvements after 02-client refactor #6088
Conversation
WalkthroughWalkthroughThe changes in the Changes
Assessment against linked issues
Possibly related issues
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
* Move functionality from client state methods to light client module methods. - Yank all statements and delete functions as needed. - Make any required functions from types/ public. - Add the vm in the 08-wasm keeper. - Update documentation to include documentation of deleted functions. * Remove global VM, introspection to retrieve client identifier. - Refactor signatures to explicitly pass vm, client identifier for time being. - Refactor tests to conform with new signatures. - Replace occurences of `ibcwasm.GetVM` in other keeper functions. * Move migrateContract from clientState to keeper method. - Matches moves for other functions. - Move is required to address future circular dependency for state management (keeper would import types and types would import keeper) - Rm migrate_contract(test).go files. * Move state management to keeper. - Remove globals for state (Checksums) - Move access functions from types to keeper, move tests. - Update calls using global to instead go through keeper. * Make InitializePinnedCodes a method on the keeper. * Remove pending todo in test for migrate contract. This previously was testing a function that didn't set the client state after calling the contract (invoking through the keeper always sets a client state after contract invocation). Hence, even if we don't set explicitly in the migrateFn callback, we _still_ get a client state in the end. * Move vm entrypoint functions in keeper/ package. * feat: move vm entry points to keeper. * chore: simplify signatures. * test: add amended tests for contact keeper. * refactor: move querier to keeper package. * lint: you'll never fix me. * refactor: remove queryRouter global variable - Move into the func for creating new plugins, pass it to acceptStargateQuery directly. This is probably what wasmd did (see they don't accept a query router now?). Since we don't use the get elsewhere, this was the smallest diff for a globa. - Preemptively ran make lint-fix, gotcha linting crybaby. * refactor: remove global for query plugins. * nit: rename to queryPlugins. * refactor: remove QueryPluginsI interface. * refactor: make queryHandler a private type. * Make migrateContractCode a private function, clean up uneeded export in export_test.go for types/. * refactor: Move vm entrypoints to keeper. - Remove generic types and do unmarshalling in light client methods. - Move all functions onto keeper and adjust call sites. * chore: address some testing todos. Move testing unmarshal failure case to light client tests. * chore: additional tiny nits. - Consistently use wasmClientKeeper as local var name. - Reorg keeper fields slightly. - Rm 'vm.go' which was already empty. * chore: restructure definitions to make diff more readable. * d'oh: rm vm arg from instantiate. * nit: Space out keeper fields. Remove TODO for method name.
DimitrisJim
force-pushed
the
08-wasm-refactor
branch
from
fc05cb9
to
0b27b84
Compare
|
Will clean up some of the pending TODOs and then open for review. Diff is already large enough so don't want to add to it atm with additional diffs for the follow ups in #6103 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return clientState.UpdateState(ctx, cdc, clientStore, clientMsg) | ||
| clientMessage, ok := clientMsg.(*types.ClientMessage) | ||
| if !ok { | ||
| panic(fmt.Errorf("expected type %T, got %T", &types.ClientMessage{}, clientMsg)) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
|
|
||
| res, err := l.keeper.WasmSudo(ctx, clientID, clientStore, clientState, payload) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
|
|
||
| var result types.UpdateStateResult | ||
| if err := json.Unmarshal(res, &result); err != nil { | ||
| panic(errorsmod.Wrap(types.ErrWasmInvalidResponseData, err.Error())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
DimitrisJim
requested review from
colin-axner,
AdityaSripal,
damiannolan,
charleenfei,
chatton,
srdtrk and
crodriguezvega
as code owners
| @@ -45,7 +45,6 @@ type queryHandler struct { | |||
| } | |||
|
|
|||
| // newQueryHandler returns a default querier that can be used in the contract. | |||
| // TODO(jim): Make private and use export_test? | |||
| @@ -328,13 +328,14 @@ func (suite *KeeperTestSuite) TestStargateQuery() { | |||
| // NOTE: we register query callbacks against: types.TimestampAtHeightMsg{} | |||
| // in practise, this can against any client state msg, however registering against types.StatusMsg{} introduces recursive loops | |||
| // due to test case: "success: verify membership query" | |||
| // TODO(Jim): Sanity check that it unmarshals correctly? | |||
There was a problem hiding this comment.
just checked the typical res is nil/not nil.
| if !found { | ||
| return errorsmod.Wrap(clienttypes.ErrClientNotFound, substituteClientID) | ||
| } | ||
|
|
||
| substituteClientState := substituteClient |
There was a problem hiding this comment.
dunno why tf I had done this 😅
| @@ -854,7 +854,9 @@ func (suite *WasmTestSuite) TestVerifyUpgradeAndUpdateState() { | |||
| }, | |||
| clienttypes.ErrClientNotFound, | |||
| }, | |||
| /* TODO(jim): Get back to these. | |||
| /* NOTE(jim): This can't fail on unmarshalling, it appears. Any consensus type | |||
There was a problem hiding this comment.
does it make sense to keep this in to note for covering this edge case?
There was a problem hiding this comment.
is there something we should do about this commented out code for a follow up?
There was a problem hiding this comment.
unsure tbh. On the one hand I think we should be aware of this quirk, on the other hand, can't seem to best find a place to add it.
| @@ -1462,24 +1463,20 @@ func (suite *WasmTestSuite) TestRecoverClient() { | |||
| }, | |||
| clienttypes.ErrClientNotFound, | |||
| }, | |||
| /* TODO(jim): We don't pass a client state directly now so we need to modify how this is tested. | |||
There was a problem hiding this comment.
first test was removed, was already checked. Second test was amended to cover the case where the substitute has a diff checksum
| storeService: storeService, | ||
| clientKeeper: clientKeeper, | ||
| authority: authority, | ||
| } | ||
|
|
||
| _, err := sb.Build() |
There was a problem hiding this comment.
I wonder if we'd be better placed just keeping the schema around on the Keeper?
Avoid having to add it sometime when/if it becomes necessary.
I don't feel too strongly about it tho, happy to defer to your judgement on this.
There was a problem hiding this comment.
I'll leave it off the keeper for time being, have a slight pref for not adding it if it isn't doing something (and makes someone wonder why its there in the future)
| testCases := []struct { | ||
| name string | ||
| malleate func() | ||
| malleate func(k *keeper.Keeper) |
There was a problem hiding this comment.
might be nice to change these tests to use GetSimApp(suite.chainA).WasmClientKeeper directly inside malleate instead of passing the arg. If only for consistencies sake with the rest of the codebase.
There was a problem hiding this comment.
yup, part of one of the follow ups to refactor this!
| @@ -132,17 +148,17 @@ func (suite *TypesTestSuite) TestStargateQuery() { | |||
|
|
|||
| testCases := []struct { | |||
| name string | |||
| malleate func() | |||
| malleate func(k *keeper.Keeper) | |||
| @@ -854,7 +854,9 @@ func (suite *WasmTestSuite) TestVerifyUpgradeAndUpdateState() { | |||
| }, | |||
| clienttypes.ErrClientNotFound, | |||
| }, | |||
| /* TODO(jim): Get back to these. | |||
| /* NOTE(jim): This can't fail on unmarshalling, it appears. Any consensus type | |||
There was a problem hiding this comment.
is there something we should do about this commented out code for a follow up?
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (1)
modules/light-clients/08-wasm/types/gas_register.go (1)
Line range hint
3-11: Reorder imports according to the specified custom order to comply with the project's coding standards.import ( + wasmvmtypes "github.com/CosmWasm/wasmvm/v2/types" + errorsmod "cosmossdk.io/errors" + sdkmath "cosmossdk.io/math" + storetypes "cosmossdk.io/store/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - wasmvmtypes "github.com/CosmWasm/wasmvm/v2/types" - errorsmod "cosmossdk.io/errors" - sdkmath "cosmossdk.io/math" - storetypes "cosmossdk.io/store/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" )
| Denominator: 100, | ||
| } | ||
|
|
||
| VMGasRegister = NewDefaultWasmGasRegister() |
There was a problem hiding this comment.
Consider adding a blank line before the declaration of VMGasRegister for better readability.
+
VMGasRegister = NewDefaultWasmGasRegister()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| VMGasRegister = NewDefaultWasmGasRegister() | |
| VMGasRegister = NewDefaultWasmGasRegister() |
|
Linter complaining about import ordering I think 👀 edit: maybe it was my code suggestion |
|
Description
closes: #5833
closes: #5834
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
Refactor
keeper.gofor improved performance and code organization.keeper_vm.gofor better reliability and maintainability.gas_register.goto optimize gas-related functionalities.New Features
keeper.go.keeper_vm.goto enhance functionality.VMGasRegisteringas_register.gofor improved gas handling.Bug Fixes
keeper.gofor better error handling and code consistency.keeper_vm.gofor enhanced stability.gas_register.gofor improved performance.Documentation
keeper.go,keeper_vm.go, andgas_register.gofor better clarity and understanding.