-
Notifications
You must be signed in to change notification settings - Fork 620
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
Pin code to the wasm VM cache #4093
Pin code to the wasm VM cache #4093
Conversation
@@ -98,6 +98,11 @@ func (k Keeper) storeWasmCode(ctx sdk.Context, code []byte) ([]byte, error) { | |||
return nil, errorsmod.Wrap(err, "failed to store contract") | |||
} | |||
|
|||
// pin the code in the vm | |||
if err := k.wasmVM.Pin(codeHash); err != nil { | |||
return nil, errorsmod.Wrap(types.ErrPinContractFailed, err.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.
Sorry, I know I wrote this in the issue, but now that I look at it, I think it's better if we do (because it's more consistent with error wrapping standards in ibc-go):
return nil, errorsmod.Wrap(types.ErrPinContractFailed, err.Error()) | |
return nil, errorsmod.Wrap(err, "pinning contract failed") |
And the we don't need to ErrPinContractFailed
.
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.
Resolved in e18cd55
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 is to pin the contract to the vm LRU cache, correct? nit from me, but maybe we could adjust the in-line code comment and error msg to reflect that?
maybe something like: errorsmod.Wrap(err, "failed to pin contract to vm cache")
could be good to check if the err
returned from wasmvm.Pin()
includes the codeHash
or if we should include that also as a Wrapf()
arg
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.
https://github.com/CosmWasm/wasmvm/blob/main/internal/api/lib.go#L105-L114
I reckon best to wrapf the code hash
Co-authored-by: Reece Williams <31943163+Reecepbcups@users.noreply.github.com>
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.
Thank you, @Reecepbcups :)
Description
Pins the code after upload for the 08-wasm client. If it fails to pin, returns a new type
ErrPinContractFailed
error with the message "pinning contract failed"closes: #4087
Commit Message / Changelog Entry
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.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.