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.

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: fix 08-wasm light client documentation #5439

Merged
merged 6 commits into from
Dec 19, 2023

Conversation

Reecepbcups
Copy link
Member

@Reecepbcups Reecepbcups commented Dec 19, 2023

Description

Fixes some differences from documentation & v7.3 chain integration (ref: CosmosContracts/juno#921). It looks like documentation is only in main & not the v7.3 08-wasm branch? lmk if there is a better place to target this PR.

  • If using x/wasm, do we register both x/wasm NewWasmSnapshotter and the ibc WasmSnapshoter?
  • Same question for InitializePinnedCodes ^^
  • Using wasmtypes import namespace is kind of a pain for x/wasm users, can wasmlctypes or ibcwasmtypes be used instead?

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)


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.

  • Targeted PR against the correct branch (see CONTRIBUTING.md).
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

memoryCacheSize,
dataDir,
availableCapabilities,
contractMemoryLimit, // default of 32
Copy link
Member Author

Choose a reason for hiding this comment

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

wasmvm does not expose this in any way, just a nice to have to let others know the default

@@ -276,7 +276,7 @@ You may leave any of the fields in the `QueryPlugins` object as `nil` if you do
Then, we pass the `QueryPlugins` object to the `WithQueryPlugins` option:

```go
querierOption := ibcwasmtypes.WithQueryPlugins(queryPlugins)
querierOption := ibcwasmkeeper.WithQueryPlugins(&queryPlugins)
Copy link
Member Author

Choose a reason for hiding this comment

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

looks like this reference is required for both v7 & v8

dataDir,
availableCapabilities,
contractMemoryLimit, // default of 32
wasmConfig.ContractDebugMode,
Copy link
Member Author

@Reecepbcups Reecepbcups Dec 19, 2023

Choose a reason for hiding this comment

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

since this is in the If x/wasm is present section, it can be used here

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess maybe not using the defaults specified in wasmConfig would be better? Having these as was done previously should force folks to ideally look it up and specify something sensible for their case instead of relying on defaults we specified for the case where we create the VM (i.e not x/wasm present). wdyt?

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Thank you @Reecepbcups! LGTM!

If using x/wasm, do we register both x/wasm NewWasmSnapshotter and the ibc WasmSnapshoter?

Yes, x/wasm loads contract info from snapshots into CodeInfos and stores the bytecode in the vm. The 08-wasm module doesn't use CodeInfo rather stores a collection of checksums directly for an allow list.

Same question for InitializePinnedCodes ^^

Yes, both would be required afaik.

Using wasmtypes import namespace is kind of a pain for x/wasm users, can wasmlctypes or ibcwasmtypes be used instead?

Agree! I'd recommend going for ibcwasmtypes personally!

Comment on lines 349 to 358
if upgradeInfo.Name == UpgradeName && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) {
storeUpgrades := storetypes.StoreUpgrades{
Added: []string{
ibcwasmtypes.ModuleName,
},
}


// configure store loader that checks if version == upgradeHeight and applies store upgrades
app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades))
Copy link
Member

Choose a reason for hiding this comment

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

Thank you!! 👍🏻

@damiannolan damiannolan added the priority PRs that need prompt reviews label Dec 19, 2023
@DimitrisJim
Copy link
Contributor

It looks like documentation is only in main & not the v7.3 08-wasm branch?

ah, yea! 😅 Had #5263 to track this and it seems #5445 should address that.

@@ -116,7 +116,7 @@ func NewSimApp(
ctx := app.BaseApp.NewUncachedContext(true, cmtproto.Header{})

// Initialize pinned codes in wasmvm as they are not persisted there
if err := wasmkeeper.InitializePinnedCodes(ctx); err != nil {
if err := wasmkeeper.InitializePinnedCodes(ctx); err != nil { // appCodec is required in v7
Copy link
Contributor

Choose a reason for hiding this comment

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

I will update this when creating the versioned docs for v7.3.x. Thanks for noticing!

@@ -237,8 +237,8 @@ wasmConfig := wasmtypes.WasmConfig{
}
app.WasmClientKeeper = wasmkeeper.NewKeeperWithConfig(
appCodec,
runtime.NewKVStoreService(keys[wasmtypes.StoreKey]),
app.IBCKeeper.ClientKeeper,
runtime.NewKVStoreService(keys[wasmtypes.StoreKey]), // v7 use app.keys[wasmtypes.StoreKey]
Copy link
Contributor

Choose a reason for hiding this comment

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

I will also fix this when creating the versioned docs.

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Thank you, @Reecepbcups.

@crodriguezvega crodriguezvega merged commit 4c7621c into cosmos:main Dec 19, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority PRs that need prompt reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants