Don't run checkLibwasmVersion automatically on start #1338
Conversation
webmaster128
changed the title
|
This makes sense to me, letting apps having more control on how to perform the check is better IMO. |
webmaster128
force-pushed
the
0.31.0-unchecked
branch
from
4d20e9d to
27d2e3c
Compare
webmaster128
changed the title
alpe
added
backport/v0.3x
|
This is hard to test via unit test. |
x/wasm/module.go
Outdated
| // An alternative method to obtain the libwasmvm version loaded at runtime is executing | ||
| // `wasmd query wasm libwasmvm-version`. | ||
| func CheckLibwasmVersion(cmd *cobra.Command, args []string) error { | ||
| skip, err := cmd.Flags().GetBool(flagWasmSkipWasmVMVersionCheck) |
There was a problem hiding this comment.
Wouldn't it be nicer to move this switch to the caller code? Then CheckLibwasmVersion always performs a check (when called)
There was a problem hiding this comment.
TBH, I went for the quickest solution. But with the method public, you have a point. The skip condition should be outside and the expected version passed.
x/wasm/module.go
Outdated
| // | ||
| // An alternative method to obtain the libwasmvm version loaded at runtime is executing | ||
| // `wasmd query wasm libwasmvm-version`. | ||
| func CheckLibwasmVersion(cmd *cobra.Command, args []string) error { |
There was a problem hiding this comment.
Should we change this back to private or leave it available to folks building their own AddModuleInitFlags?
|
I had tested the fix manually with wasmvm main and got |
* Don't run checkLibwasmVersion automatically on start * Remove unused preRunFn, chainPreRuns * Add documentation to CheckLibwasmVersion * Add skip wasmvm version flag * Linter false positive * Review feedback * Pass expected version --------- Co-authored-by: Alex Peters <alpe@users.noreply.github.com> (cherry picked from commit 0929168) # Conflicts: # x/wasm/module.go
|
Thank you for finishing this! |
Don't run checkLibwasmVersion automatically on start (backport #1338)
While this check makes sense in the happy path, there are many occasions when the version of the Go project wasmvm and the library diverge. This can be any patch to either of them. Especially Go's dummy versioning makes it easy for the versions to be different.
Now when a chain integrates wasmd like here it suddenly becomes very difficult to skip this check. At least I don't see a way to do it without forking wasmd.
The idea of this PR is to keep
CheckLibwasmVersionavailable but don't force-run it. This way callers can use the function but easily disable it then they need to.Ping @jhernandezb who kindof invented this check.