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 wasmd module #769

Merged
merged 17 commits into from
Feb 3, 2022
Merged

Add wasmd module #769

merged 17 commits into from
Feb 3, 2022

Conversation

ethanfrey
Copy link
Contributor

@ethanfrey ethanfrey commented Jan 19, 2022

This is to add a v1 of wasmd integration.

  • Builds, app compiles, and doesn't break tests
  • Properly read in more system config info from commands (more args to NewApp)
  • Update default genesis to use permissioned wasmd
  • Add minimal integration test - cannot upload, can create gov proposal to upload code
  • Add tx and query commands to cli

In another PR we look to add the x/upgrade logic to set the global permissions on upgrade not just genesis

Copy link
Contributor Author

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Good point

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2022

Codecov Report

Merging #769 (c062467) into main (e97b00a) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #769      +/-   ##
==========================================
- Coverage   20.00%   20.00%   -0.01%     
==========================================
  Files         189      189              
  Lines       24546    24549       +3     
==========================================
  Hits         4910     4910              
- Misses      18780    18783       +3     
  Partials      856      856              
Impacted Files Coverage Δ
x/incentives/keeper/gauge.go 57.69% <0.00%> (-1.37%) ⬇️

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 e97b00a...c062467. Read the comment docs.

@ethanfrey ethanfrey changed the title WIP: Add wasmd module Add wasmd module Feb 1, 2022
@ethanfrey ethanfrey marked this pull request as ready for review February 1, 2022 20:09
@ethanfrey
Copy link
Contributor Author

I just want to write a few full stack tests on the integration, but the "production" code is done and can be reviewed

@ethanfrey ethanfrey requested a review from ValarDragon February 1, 2022 20:26
app/app.go Outdated Show resolved Hide resolved
Comment on lines +29 to +35
// here we override wasm config to make it permissioned by default
wasmGen := wasm.GenesisState{
Params: wasmtypes.Params{
CodeUploadAccess: wasmtypes.AllowNobody,
InstantiateDefaultPermission: wasmtypes.AccessTypeEverybody,
MaxWasmCodeSize: DefaultMaxWasmCodeSize,
},
Copy link
Member

Choose a reason for hiding this comment

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

Oh cool! So when we do the upgrade to cosmwasm, do we need to override these in InitGenesis as well?

Heres what we had to do for x/txfees in our v5 upgrade: https://github.com/osmosis-labs/osmosis/blob/main/app/upgrades/v5/upgrades.go#L60-L66

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we will need a migration step, that will set the params explicitly.
I wanted to do that on another PR. I could copy your migration setup, but would be happier to add a few lines to an existing one (if you could set up the framework for v6 upgrade)

app/keepers.go Outdated Show resolved Hide resolved
app/keepers.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Comment on lines +30 to +32
// we need to make this deterministic (same every test run), as content might affect gas costs
func keyPubAddr() (crypto.PrivKey, crypto.PubKey, sdk.AccAddress) {
key := ed25519.GenPrivKey()
Copy link
Member

Choose a reason for hiding this comment

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

So do we need to switch to GenPrivKeyFromSecret? (https://github.com/tendermint/tendermint/blob/master/crypto/ed25519/ed25519.go#L144 )

Also a bit surprised that this is an ed25519 address, versus a secp one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, copied some code I found elsewhere (in wasmd).

The original one in wasmd had a deterministic seed (generated from a counter). I tried making this random, but can use this.

Can you link me to where you generate random addresses in your codebase? I can just import that instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or just update this code in a separate PR yourself. As long as it returns valid addresses, it is fine

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Feel free to merge, we can adjust the minor point around the pointer receiver for wasm keeper in a second PR if you'd like.

@ethanfrey
Copy link
Contributor Author

Happy to make the adjustments here.

My only question is how to improve the "random key/address" generation, or align it better to the osmosis standard.

@ethanfrey
Copy link
Contributor Author

ethanfrey commented Feb 3, 2022

Made the fixes and it passes CI!
Can you merge this, as I am not authorised to merge it?

If you start a migration PR (it would be v6 or v7?) let me know and I will add the wasm-specific migration so we can test that.

@ValarDragon ValarDragon merged commit efe2853 into osmosis-labs:main Feb 3, 2022
@tac0turtle
Copy link
Contributor

bye bye my hard drives 😉

@ValarDragon
Copy link
Member

Awesome thanks! I'll get the initial migration handlers up in a PR!

@ValarDragon
Copy link
Member

@marbar3778 haha, hopefully cosmwasm state sync will save us in the future

@@ -842,6 +890,7 @@ func initParamsKeeper(appCodec codec.BinaryCodec, legacyAmino *codec.LegacyAmino
paramsKeeper.Subspace(poolincentivestypes.ModuleName)
paramsKeeper.Subspace(superfluidtypes.ModuleName)
paramsKeeper.Subspace(gammtypes.ModuleName)
paramsKeeper.Subspace(wasm.ModuleName)
Copy link

Choose a reason for hiding this comment

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

@ethanfrey
The pinned contracts are not loaded within the if loadLatest block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good catch. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants