Enable wasm mint bindings #1445

maurolacy · 2022-05-07T15:14:15Z

Closes / Related to: #1029

What is the purpose of the change

Re-instate the CoswmWasm message handlers for Mint and FullDenom. Related to osmosis-labs/bindings#25 on the Rust side.

Brief change log

Uncomment the Mint-related code.
Implemented Mint using TokenFactory.
Implemented FullDenom in terms of TokenFactory.

Testing and Verifying

Uncommented and adapted already existing tests.

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes. For the CosmWasm side)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (will do)
How is the feature or change documented? (not documented)

codecov-commenter · 2022-05-07T19:04:57Z

Codecov Report

Merging #1445 (44f26ed) into main (f56fbe5) will decrease coverage by 0.42%.
The diff coverage is 16.39%.

@@            Coverage Diff             @@
##             main    #1445      +/-   ##
==========================================
- Coverage   19.82%   19.40%   -0.43%     
==========================================
  Files         202      229      +27     
  Lines       27685    31452    +3767     
==========================================
+ Hits         5489     6102     +613     
- Misses      21175    24237    +3062     
- Partials     1021     1113      +92

Impacted Files	Coverage Δ
app/wasm/bindings/query.go	`0.00% <ø> (ø)`
x/epochs/client/cli/query.go	`0.00% <ø> (ø)`
x/gamm/client/cli/query.go	`36.74% <0.00%> (-0.36%)`	⬇️
x/gamm/pool-models/stableswap/msgs.go	`0.00% <0.00%> (ø)`
x/gamm/pool-models/stableswap/pool.go	`0.00% <0.00%> (ø)`
x/gamm/pool-models/stableswap/stableswap_pool.go	`0.00% <0.00%> (ø)`
.../gamm/pool-models/stableswap/stableswap_pool.pb.go	`0.58% <0.00%> (-0.09%)`	⬇️
x/gamm/types/pool.go	`0.00% <ø> (ø)`
x/incentives/client/cli/query.go	`0.00% <ø> (ø)`
x/incentives/keeper/distribute.go	`61.00% <0.00%> (ø)`
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5227c05...44f26ed. Read the comment docs.

ethanfrey

Nice one.

One test I would like to see (minting the same subdenom twice). Otherwise good to merge.

One open question for the Osmosis core devs before this is tagged. But please don't block merging this, we can enhance it in a follow up.

app/wasm/message_plugin.go

ethanfrey · 2022-05-09T19:07:59Z

app/wasm/message_plugin.go

+
+	msgServer := tokenfactorykeeper.NewMsgServerImpl(*f)
+
+	// Check if denom already exists


Okay, this is the critical question.

Assuming we never set any metadata that is not present in the MintTokens command, this works great and makes a simple API.

However, if they plan to add more metadata there, we will want a separate command to run once to register the metadata (and another to query it?).

I would merge this as is (we can test stuff with it), but have a discussion with @ValarDragon and @sunnya97 before tagging an Osmosis release.

If they want to add more required metadata, they should add it in Go and we should expose it in Rust now. Otherwise, it will most likely be a breaking contract API change, which we want very much to avoid.

app/wasm/message_plugin.go

app/wasm/test/custom_msg_test.go

app/wasm/test/messages_test.go

p0mvn

LGTM once CI is passing

app/wasm/message_plugin.go

app/wasm/testdata/version.txt

p0mvn · 2022-05-09T19:36:16Z

app/wasm/test/custom_msg_test.go

@@ -2,6 +2,7 @@ package wasm



Can we make this a wasm_test package as well as all of the other test files:

custom_msg_test.go

custom_query_test.go

messages_test.go

queries_test.go

Let's not block merge on this but creating an issue for that would be great!

What is the reasoning here?

Basically, the default is usually the same package, and there are valid reasons to make it a separate wasm_test package, but it is good to discuss which one to prefer

mconcat

LGTM!

ValarDragon · 2022-05-16T05:26:06Z

I don't have permission to update the branch, but once its updated lets go ahead and merge this in

nicolaslara · 2022-05-24T10:00:50Z

@maurolacy What process did you use here to update app/wasm/testdata/osmo_reflect.wasm. I tried updating it to run the tests in my branch (#1484) by compiling it in (https://github.com/osmosis-labs/osmosis-bindings/pull/4) and copying the resulting .wasm file but keep getting:

=== RUN   TestLockTokensMsg
    custom_query_test.go:312:
        	Error Trace:	custom_query_test.go:312
        	            				custom_query_test.go:37
        	            				custom_msg_test.go:607
        	Error:      	Received unexpected error:

        	            	github.com/cosmos/cosmos-sdk/x/gov/keeper.Keeper.SubmitProposal
        	            		/Users/nicolas/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/gov/keeper/proposal.go:24
        	            	github.com/osmosis-labs/osmosis/v7/app/wasm/test.storeReflectCode
        	            		/Users/nicolas/devel/osmosis/app/wasm/test/custom_query_test.go:311
        	            	github.com/osmosis-labs/osmosis/v7/app/wasm/test.SetupCustomApp
        	            		/Users/nicolas/devel/osmosis/app/wasm/test/custom_query_test.go:37
        	            	github.com/osmosis-labs/osmosis/v7/app/wasm/test.TestLockTokensMsg
        	            		/Users/nicolas/devel/osmosis/app/wasm/test/custom_msg_test.go:607
        	            	code bytes cannot be longer than 819200 bytes: exceeds limit: invalid request: invalid proposal content
        	Test:       	TestLockTokensMsg
--- FAIL: TestLockTokensMsg (0.06s)
FAIL
FAIL	github.com/osmosis-labs/osmosis/v7/app/wasm/test	0.669s
FAIL

Is there something I'm missing there?

maurolacy · 2022-05-24T11:33:03Z

~~I just updated it manually. That's why the version.txt file has a commit id (from the osmosis-bindings repository).~~ No, sorry, this comment is outdated.

If you use the committed version of the wasm file, you should be good to go for tests.

nicolaslara · 2022-05-24T12:42:54Z

@maurolacy thanks for the quick reply! ...Using the committed version I get the a "variant missing" error. Which makes sense, since that wasm file doesn't have information about the new type I've added on osmosis-bindings

=== RUN   TestLockTokensMsg
    custom_msg_test.go:638:
        	Error Trace:	custom_msg_test.go:638
        	Error:      	Received unexpected error:

        	            	github.com/CosmWasm/wasmd/x/wasm/keeper.Keeper.execute
        	            		/Users/nicolas/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/wasm/keeper/keeper.go:354
        	            	github.com/CosmWasm/wasmd/x/wasm/keeper.PermissionedKeeper.Execute
        	            		/Users/nicolas/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/wasm/keeper/contract_keeper.go:51
        	            	github.com/osmosis-labs/osmosis/v7/app/wasm/test.executeCustom
        	            		/Users/nicolas/devel/osmosis/app/wasm/test/custom_msg_test.go:543
        	            	github.com/osmosis-labs/osmosis/v7/app/wasm/test.TestLockTokensMsg
        	            		/Users/nicolas/devel/osmosis/app/wasm/test/custom_msg_test.go:637
        	            	Error parsing into type osmo_reflect::msg::ExecuteMsg: unknown variant `lock_tokens`, expected `mint_tokens` or `swap`: execute wasm contract failed
        	Test:       	TestLockTokensMsg
--- FAIL: TestLockTokensMsg (0.36s)
FAIL
FAIL	github.com/osmosis-labs/osmosis/v7/app/wasm/test	0.952s
FAIL

nicolaslara · 2022-05-24T12:56:05Z

~~ahh, I think I know what the problem is. I hadn't finished the mock in multitest.rs. I'll fix that and report back.~~

That wasn't it. Clearly the internal tests of the rust package shouldn't affect the go test.

maurolacy requested a review from a team May 7, 2022 15:14

maurolacy force-pushed the enable-mint-bindings branch from 015ec4f to 2734e3c Compare May 7, 2022 18:58

maurolacy force-pushed the enable-mint-bindings branch from 2734e3c to 289b1dd Compare May 8, 2022 07:19

ethanfrey approved these changes May 9, 2022

View reviewed changes

p0mvn approved these changes May 9, 2022

View reviewed changes

maurolacy force-pushed the enable-mint-bindings branch from 6d8c061 to bff2f7e Compare May 10, 2022 09:14

mconcat approved these changes May 11, 2022

View reviewed changes

ValarDragon added the A:automerge label May 16, 2022

maurolacy force-pushed the enable-mint-bindings branch from 8e46fe3 to 1d54f4b Compare May 16, 2022 05:45

Mauro Lacy added 14 commits May 16, 2022 07:48

Uncomment mint / full-denom related code and tests

85a00ec

Update osmo_reflect to v0.5.1

2ddac4c

Update osmo_reflect to commit tag

a8fa499

Add token factory keeper to wasm messenger

a3c9974

Adapt GetFullDenom query and Mint message to token factory

0124697

Fix syntax / typos

2d4ecef

Adapt full denom / mint tests

9e3f1c9

Use MsgServer CreateDenom and Mint

1bf0f0f

Add fees for mint test accounts

5bd61cb

Fix invalid sub-denom test description

6cd8628

Update osmo_reflect to v0.6.0

fcb540e

Add mint same denom twice check

46bb412

Add mint another denom check

5219712

Fix lint error

a64b13f

maurolacy force-pushed the enable-mint-bindings branch from 1d54f4b to a64b13f Compare May 16, 2022 05:50

mergify bot merged commit 23ff223 into osmosis-labs:main May 16, 2022

maurolacy deleted the enable-mint-bindings branch May 16, 2022 06:00

nicolaslara mentioned this pull request May 25, 2022

Lock tokens via cosmwasm #1484

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable wasm mint bindings #1445

Enable wasm mint bindings #1445

maurolacy commented May 7, 2022

codecov-commenter commented May 7, 2022 •

edited

Loading

ethanfrey left a comment

ethanfrey May 9, 2022

ethanfrey May 9, 2022

p0mvn left a comment

p0mvn May 9, 2022

ethanfrey May 10, 2022

mconcat left a comment

ValarDragon commented May 16, 2022

nicolaslara commented May 24, 2022

maurolacy commented May 24, 2022 •

edited

Loading

nicolaslara commented May 24, 2022

nicolaslara commented May 24, 2022 •

edited

Loading


		msgServer := tokenfactorykeeper.NewMsgServerImpl(*f)

		// Check if denom already exists

Enable wasm mint bindings #1445

Enable wasm mint bindings #1445

Conversation

maurolacy commented May 7, 2022

What is the purpose of the change

Brief change log

Testing and Verifying

Documentation and Release Note

codecov-commenter commented May 7, 2022 • edited Loading

Codecov Report

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey May 9, 2022

Choose a reason for hiding this comment

ethanfrey May 9, 2022

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

p0mvn May 9, 2022

Choose a reason for hiding this comment

ethanfrey May 10, 2022

Choose a reason for hiding this comment

mconcat left a comment

Choose a reason for hiding this comment

ValarDragon commented May 16, 2022

nicolaslara commented May 24, 2022

maurolacy commented May 24, 2022 • edited Loading

nicolaslara commented May 24, 2022

nicolaslara commented May 24, 2022 • edited Loading

codecov-commenter commented May 7, 2022 •

edited

Loading

maurolacy commented May 24, 2022 •

edited

Loading

nicolaslara commented May 24, 2022 •

edited

Loading