-
Notifications
You must be signed in to change notification settings - Fork 580
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
Remove usage of globals from 08-wasm #6103
Conversation
…ethods. - 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.
- 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.
- 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.
- Remove globals for state (Checksums) - Move access functions from types to keeper, move tests. - Update calls using global to instead go through keeper.
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 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.
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the 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 (
|
…in export_test.go for types/.
- Remove generic types and do unmarshalling in light client methods. - Move all functions onto keeper and adjust call sites.
…e to light client tests.
- Consistently use wasmClientKeeper as local var name. - Reorg keeper fields slightly. - Rm 'vm.go' which was already empty.
@@ -305,6 +307,7 @@ func (suite *TypesTestSuite) TestWasmMigrate() { | |||
tc := tc | |||
suite.Run(tc.name, func() { | |||
suite.SetupWasmWithMockVM() | |||
_ = storeWasmCode(suite, wasmtesting.Code) |
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.
many of the type tests were written to assume a checksum exists in state, keeper tests don't. This should require some refactoring at some point.
Numerator: 15, | ||
Denominator: 100, | ||
} | ||
VMGasRegister = NewDefaultWasmGasRegister() |
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.
unsure of where to place this.
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.
yeah, I remember we spoke before with the confio guys about potentially separating out things like this to a module which both x/wam and 08-wasm could depend on - as I think we have copied what they use. We should make sure they are still in sync for next release and maybe bring up this point again about a shared module to not introduce as import cycles
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.
I think carlos synced em recentlyf or wasmvm v2. We should definitely ping them for sharing it.
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.
Yes, I did sync this when upgraded to wasmvm v2.
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.
Amazing work, @DimitrisJim. This is really an exhaustive and nice refactoring.
I agree that we should move a bunch of types, variables and constants in types
that are only used in keeper
to an internal package. Happy to write an issue for this a tackle it a bit later (but before v9 release).
We should also add an issue to update documentation and migration docs (I guess for v0.3.x of 08-wasm with ibc-go v9.0) with these changes and any subsequent changes in future PRs. We can also tackle this later on.
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.
Overall LGTM! Excellent work on the refactor :)
I left some comments and questions we should address before merge
} | ||
|
||
// validatePostExecutionClientState validates that the contract has not many any invalid modifications | ||
// to the client state during execution. It ensures that | ||
// - the client state is still present | ||
// - the client state can be unmarshaled successfully. | ||
// - the client state is of type *ClientState | ||
func validatePostExecutionClientState(clientStore storetypes.KVStore, cdc codec.BinaryCodec) (*ClientState, error) { | ||
func validatePostExecutionClientState(clientStore storetypes.KVStore, cdc codec.BinaryCodec) (*types.ClientState, error) { |
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.
would be nice to get rid of this restriction sometime soon. this is what prevents contracts from storing their own client state types and ultimately what prevents using tendermint go/wasm interchangeably.
if a contract wants to store its own client state type and not the wasm wrapper it MUST be encoded as a protobuf Any - the wasm wrapper struct is still required for routing the Initialise
call to the lightclient module tho
Numerator: 15, | ||
Denominator: 100, | ||
} | ||
VMGasRegister = NewDefaultWasmGasRegister() |
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.
yeah, I remember we spoke before with the confio guys about potentially separating out things like this to a module which both x/wam and 08-wasm could depend on - as I think we have copied what they use. We should make sure they are still in sync for next release and maybe bring up this point again about a shared module to not introduce as import cycles
@@ -123,8 +122,3 @@ type QueryRouter interface { | |||
// if not found | |||
Route(path string) baseapp.GRPCQueryHandler | |||
} | |||
|
|||
type QueryPluginsI interface { |
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.
This interface isn't needed anymore? what was it being used for previously?
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.
The main problem previously was the Set/Get
for query plugins being globally accessible through types
. Currently these are moved (and exported) on Keeper which in cosmos-sdk context would require someone explicitly hold a ref to keeper to modify, hence, not as unexpected (was my thinking).
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.
oh, think I could make them private though! I'll do a follow up here too.
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.
in general my plan was do refactor -> merge, move as much as possible into internal/make symbols private.
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.
Awesome work with this, thanks for taking on this monster LGTM
Happy to have some of the other comments addressed in a follow up 🖖
Quality Gate passed for 'ibc-go'Issues Measures |
* 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.
* tests: move tests from client state to light client module for 08-wasm. * Remove usage of globals from 08-wasm (#6103) * 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. * chore: address remaining TODOs. * nit: don't hold ref to schema after building it. * Update modules/light-clients/08-wasm/types/gas_register.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * lint: shut up --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Description
Removes the globals in ibcwasm and restructures module to move more functionality over to the keeper. This was previously present in
types/
due to limitations imposed by having all calls flow through the client state.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/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.