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

Add libwasm-version query command #846

Merged
merged 2 commits into from
May 9, 2022

Conversation

jhernandezb
Copy link
Contributor

New wasmvm introduces LibwasmvmVersion api method that can help to get the version of the library that is being resolved specially for dynamic linked binaries.

I'm adding it as a query because it will be automatically wired to apps using wasmd but can be just a helper command and requires manual wiring.

@jhernandezb jhernandezb requested a review from alpe as a code owner May 6, 2022 16:00
@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #846 (0180a51) into main (96a9b5a) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #846      +/-   ##
==========================================
- Coverage   60.01%   59.89%   -0.12%     
==========================================
  Files          52       52              
  Lines        5972     5987      +15     
==========================================
+ Hits         3584     3586       +2     
- Misses       2120     2134      +14     
+ Partials      268      267       -1     
Impacted Files Coverage Δ
x/wasm/client/cli/query.go 0.00% <0.00%> (ø)
x/wasm/keeper/keeper.go 88.09% <0.00%> (+0.35%) ⬆️

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Nice

// GetCmdVMVersion gets current libwasmvm version.
func GetCmdVMVersion() *cobra.Command {
cmd := &cobra.Command{
Use: "vm-version",
Copy link
Member

Choose a reason for hiding this comment

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

The term "VM" is a bit overused and wasmvm is the Go project that contains code for both sides of the FFI interface. To avoid confusion, I'd go with "libwasmvm-version". A better abbreviation would be "lib-version". If we need something short, let's use that as alias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed and added the alias as well.

@@ -17,6 +17,7 @@ import (
flag "github.com/spf13/pflag"

"github.com/CosmWasm/wasmd/x/wasm/types"
wasmvmapi "github.com/CosmWasm/wasmvm/api"
Copy link
Member

Choose a reason for hiding this comment

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

Question to the Go pros: is the export via the /api folder good that way? Or should I make this available in the root project github.com/CosmWasm/wasmvm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like all the imports in wasmd are from the root project.
maybe just set an alias at root level var LibwasmvmVersion = api.LibwasmvmVersion

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO root level would be the first place to look for this kind of functionality

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! Will move to the root. The ./api package will become internal soon (CosmWasm/wasmvm#333).

@webmaster128
Copy link
Member

For some more context: this specific command is temporary and can be removed once the Cosmos SDK version command is extensible (see cosmos/cosmos-sdk#11690).

@jhernandezb jhernandezb changed the title Add vm-version query command Add libwasm-version query command May 6, 2022
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Great, thanks.

Let's wait for Alex's review next week.

@webmaster128 webmaster128 added this to the v0.27.0 milestone May 7, 2022
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks for the PR 💐

@@ -17,6 +17,7 @@ import (
flag "github.com/spf13/pflag"

"github.com/CosmWasm/wasmd/x/wasm/types"
wasmvmapi "github.com/CosmWasm/wasmvm/api"
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO root level would be the first place to look for this kind of functionality

@@ -35,10 +36,31 @@ func GetQueryCmd() *cobra.Command {
GetCmdGetContractHistory(),
GetCmdGetContractState(),
GetCmdListPinnedCode(),
GetCmdLibVersion(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I was expecting this at the command root level, too but I can see that it is easier to maintain this way 👍

@alpe alpe merged commit 25c8ac6 into CosmWasm:main May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants