Implement improvements to new address generation #1014

alpe · 2022-09-20T13:48:04Z

Resolves #1000
With this PR the sequence based (classic) address generation is added again. This PR got rather big as a lot of files are touched. But tests and code were restored from the original version.

A first round of feedback from @webmaster128 was applied on an early version, before this branch was rebased. More is very welcome!

Modifications to the CLI for the predictable address type instantiate2 was introduced

x/wasm/keeper/keeper.go

codecov · 2022-09-20T16:28:27Z

Codecov Report

Merging #1014 (18a6a6f) into main (54fec05) will decrease coverage by 0.63%.
The diff coverage is 40.96%.

❗ Current head 18a6a6f differs from pull request most recent head 7f1407d. Consider uploading reports for the commit 7f1407d to get more accurate results

@@            Coverage Diff             @@
##             main    #1014      +/-   ##
==========================================
- Coverage   60.33%   59.70%   -0.64%     
==========================================
  Files          52       52              
  Lines        6437     6587     +150     
==========================================
+ Hits         3884     3933      +49     
- Misses       2267     2367     +100     
- Partials      286      287       +1

Impacted Files	Coverage Δ
x/wasm/client/cli/query.go	`0.00% <0.00%> (ø)`
x/wasm/handler.go	`66.66% <0.00%> (-3.34%)`	⬇️
x/wasm/keeper/msg_server.go	`31.34% <0.00%> (-5.83%)`	⬇️
x/wasm/module.go	`49.45% <ø> (+0.51%)`	⬆️
x/wasm/types/keys.go	`51.11% <ø> (ø)`
x/wasm/client/cli/tx.go	`32.50% <5.63%> (-10.13%)`	⬇️
x/wasm/types/tx.go	`43.91% <42.85%> (-0.19%)`	⬇️
x/wasm/keeper/addresses.go	`77.14% <77.14%> (ø)`
x/wasm/client/cli/genesis_msg.go	`71.42% <88.23%> (+2.45%)`	⬆️
x/wasm/keeper/contract_keeper.go	`95.00% <100.00%> (+2.14%)`	⬆️
... and 5 more

proto/cosmwasm/wasm/v1/tx.proto

x/wasm/client/cli/genesis_msg.go

x/wasm/keeper/addresses.go

x/wasm/keeper/contract_keeper.go

x/wasm/types/validation.go

…method Please enter the commit message for your changes. Lines starting

ethanfrey · 2022-09-21T14:53:51Z

Modifications to the CLI for the predictable address type (instantiate2):

--salt [hex,optional]
--fix-msg [bool,optional]

I am confused of --salt optional.
It is required for instantiate2.
If I understand correctly, if not provided, it uses old algorithm. If provided, it uses new algorithm?

Just we need to document that better than "optional" (I will review cli help text)

webmaster128 · 2022-09-21T15:06:47Z

I am confused of --salt optional.

This is because in the current implementation, no instantiate2 CLI command was created. But I think we should have instantiate and instantiate2 separately.

ethanfrey

Looks good and fine to merge as is.

Comments are either stylistic (on test code) or documentation to add (or just praising the code).

contrib/local/02-contracts.sh

proto/cosmwasm/wasm/v1/tx.proto

x/wasm/client/cli/tx.go

x/wasm/keeper/addresses.go

x/wasm/keeper/contract_keeper_test.go

x/wasm/keeper/keeper.go

x/wasm/keeper/msg_server.go

x/wasm/types/validation.go

x/wasm/keeper/addresses.go

webmaster128 · 2022-09-21T18:44:14Z

I updated the tests at https://gist.github.com/webmaster128/e4d401d414bd0e7e6f70482f11877fbe (cosmos/cosmjs#1253) to the version on this PR. msg: null means fix_msg: false. A solution to the msg length problem is not yet implemented.

webmaster128

Very nice!

The two open issues for me are

I strongly believe we should have two different CLI commands. instantiate and instantiate2 are two different operations, which is reflected in two messages and will lead to two high level client functions instantiate and instantiate2 in CosmJS. Using two has a number of advantages. First, existing instantiate users do not need to bother about the new arguments which just confuse them and might lead to misusage. Second, by making all the new important fields required for the instantiate2 case, we get much stronger input validation. Third, the code becomes easier to audit since less code branches exists per command.
The len(msg) can easily exceed 255. We can either make this one length field a big endian uin32 or uint64. Or (my preference) we use the same length implementation for all fields and move away to the address specific prefixer function. In our use case we do not need to save space (in contrast to the storage use case for which the address prefixer was written for), so we can just use uint64 for all of them.

x/wasm/keeper/addresses.go

x/wasm/keeper/keeper_test.go

x/wasm/keeper/addresses.go

ethanfrey

Looks good. One minor comment on the migration, but good to merge

x/wasm/module.go

webmaster128

Very nice! Just a few smaller things but overall good to go as fas as I can see.

x/wasm/client/cli/tx.go

* wasmd_v0.29.1: (94 commits) fix test. fix test. update cosmos-sdk to v45.9, add dragonberry. Changelog updates (CosmWasm#1024) Validate incoming messages Add dependencies for protobuf and remove third_party forlder (CosmWasm#1030) Bump bufbuild/buf-setup-action from 1.7.0 to 1.8.0 (CosmWasm#1006) Add check version wasm (CosmWasm#1029) Make SenderPrivKey field public Bump actions/checkout from 3.0.2 to 3.1.0 Revert module version to 1 as there is no migration Doc ante handler Fix: typos Changelog for v0.29.0-rc1 (CosmWasm#1018) Implement improvements to new address generation (CosmWasm#1014) Prune vesting accounts balances (CosmWasm#1003) Bump github.com/cosmos/ibc-go/v3 from 3.2.0 to 3.3.0 Fix path to proofs in protos Upgrade .a files to 1.1.1 Bump wasmvm to 1.1.1 ... # Conflicts: # .circleci/config.yml # .github/workflows/proto-buf-publisher.yml # CHANGELOG.md # Dockerfile # Makefile # README.md # app/app.go # app/app_test.go # app/export.go # app/sim_test.go # app/test_helpers.go # go.mod # go.sum # scripts/protocgen.sh # third_party/proto/cosmos/base/query/v1beta1/pagination.proto # third_party/proto/cosmos/base/v1beta1/coin.proto # third_party/proto/tendermint/blockchain/types.pb.go # third_party/proto/tendermint/consensus/types.pb.go # third_party/proto/tendermint/consensus/wal.pb.go # third_party/proto/tendermint/p2p/conn.pb.go # third_party/proto/tendermint/privval/types.pb.go # third_party/proto/tendermint/state/types.pb.go # third_party/proto/tendermint/types/types.pb.go # third_party/proto/tendermint/types/validator.pb.go # x/wasm/alias.go # x/wasm/client/cli/genesis_msg.go # x/wasm/client/cli/genesis_msg_test.go # x/wasm/client/cli/query.go # x/wasm/client/cli/tx.go # x/wasm/client/rest/gov.go # x/wasm/handler.go # x/wasm/ibctesting/chain.go # x/wasm/ibctesting/wasm.go # x/wasm/ioutils/ioutil_test.go # x/wasm/ioutils/utils_test.go # x/wasm/keeper/authz_policy.go # x/wasm/keeper/bench_test.go # x/wasm/keeper/contract_keeper.go # x/wasm/keeper/gas_register.go # x/wasm/keeper/gas_register_test.go # x/wasm/keeper/genesis_test.go # x/wasm/keeper/ibc_test.go # x/wasm/keeper/keeper.go # x/wasm/keeper/keeper_test.go # x/wasm/keeper/legacy_querier_test.go # x/wasm/keeper/msg_server.go # x/wasm/keeper/options.go # x/wasm/keeper/options_test.go # x/wasm/keeper/proposal_handler.go # x/wasm/keeper/proposal_integration_test.go # x/wasm/keeper/querier.go # x/wasm/keeper/querier_test.go # x/wasm/keeper/recurse_test.go # x/wasm/keeper/reflect_test.go # x/wasm/keeper/relay_test.go # x/wasm/keeper/staking_test.go # x/wasm/keeper/submsg_test.go # x/wasm/keeper/test_common.go # x/wasm/keeper/testdata/reflect.go # x/wasm/keeper/testdata/reflect.wasm # x/wasm/keeper/wasmtesting/coin_transferrer.go # x/wasm/keeper/wasmtesting/gas_register.go # x/wasm/module.go # x/wasm/module_test.go # x/wasm/simulation/genesis.go # x/wasm/simulation/operations.go # x/wasm/simulation/params.go # x/wasm/types/codec.go # x/wasm/types/events.go # x/wasm/types/exported_keepers.go # x/wasm/types/genesis.pb.go # x/wasm/types/iavl_range_test.go # x/wasm/types/ibc.pb.go # x/wasm/types/params.go # x/wasm/types/proposal.pb.go # x/wasm/types/query.pb.go # x/wasm/types/query.pb.gw.go # x/wasm/types/tx.pb.go # x/wasm/types/types.pb.go

* Revert default instance address generation to classic sequence based method Please enter the commit message for your changes. Lines starting * Start re-adding classic address generator * Extract address generation to file; minor updates * Review comments * Set max salt size * Support predictable address on instantiation * Switch attribute order for backwards compatiblity * Fix salt param check in CLI * Enable tests * Add more tests * Minor fix * Remove migration * Better cli description * Fix init message length prefix * Add sanity checks to address generation and minor updates * Reduce max length in tests for CI * CLI and address generation updates * Add test vectors * Minor updates * Fix cli long doc

alpe commented Sep 20, 2022

View reviewed changes

x/wasm/keeper/keeper.go Outdated Show resolved Hide resolved

webmaster128 reviewed Sep 20, 2022

View reviewed changes

webmaster128 mentioned this pull request Sep 20, 2022

Improvements to new address generation #1000

Closed

Base automatically changed from 942_gas to main September 21, 2022 13:14

alpe added 9 commits September 21, 2022 15:17

Revert default instance address generation to classic sequence based …

d1113be

…method Please enter the commit message for your changes. Lines starting

Start re-adding classic address generator

b86febc

Extract address generation to file; minor updates

c283a42

Review comments

a2bafea

Set max salt size

5bb97cc

Support predictable address on instantiation

d09ca07

Switch attribute order for backwards compatiblity

17f9f01

Fix salt param check in CLI

a639c15

Enable tests

1ddfc37

alpe force-pushed the 1000_instantiate2 branch from 6a9282d to 1ddfc37 Compare September 21, 2022 13:19

webmaster128 mentioned this pull request Sep 21, 2022

Add support for MsgInstantiateContract2 CosmWasm/cosmwasm#1436

Closed

Add more tests

391a47f

alpe marked this pull request as ready for review September 21, 2022 13:39

Minor fix

718b31d

alpe requested a review from ethanfrey September 21, 2022 13:41

ethanfrey approved these changes Sep 21, 2022

View reviewed changes

Remove migration

7bc3ae5

webmaster128 mentioned this pull request Sep 21, 2022

Implement determinstic contract addresses cosmos/cosmjs#1253

Merged

webmaster128 reviewed Sep 21, 2022

View reviewed changes

x/wasm/keeper/addresses.go Outdated Show resolved Hide resolved

Better cli description

51dfd2a

webmaster128 reviewed Sep 21, 2022

View reviewed changes

x/wasm/keeper/addresses.go Outdated Show resolved Hide resolved

x/wasm/keeper/keeper_test.go Outdated Show resolved Hide resolved

Fix init message length prefix

a9a715c

alpe added 2 commits September 21, 2022 22:11

Add sanity checks to address generation and minor updates

b9a15d3

Reduce max length in tests for CI

3b6ea34

webmaster128 reviewed Sep 22, 2022

View reviewed changes

x/wasm/keeper/addresses.go Outdated Show resolved Hide resolved

x/wasm/keeper/addresses.go Show resolved Hide resolved

x/wasm/keeper/addresses.go Show resolved Hide resolved

alpe added 3 commits September 22, 2022 12:04

CLI and address generation updates

da83bdb

Add test vectors

884e4fc

Minor updates

18a6a6f

alpe requested a review from webmaster128 September 22, 2022 12:33

ethanfrey approved these changes Sep 22, 2022

View reviewed changes

x/wasm/module.go Show resolved Hide resolved

webmaster128 reviewed Sep 22, 2022

View reviewed changes

x/wasm/client/cli/tx.go Outdated Show resolved Hide resolved

x/wasm/client/cli/tx.go Outdated Show resolved Hide resolved

x/wasm/client/cli/tx.go Outdated Show resolved Hide resolved

x/wasm/client/cli/tx.go Outdated Show resolved Hide resolved

Fix cli long doc

7f1407d

alpe merged commit 9c5ebbb into main Sep 22, 2022

alpe deleted the 1000_instantiate2 branch September 22, 2022 16:22

faddat mentioned this pull request Oct 2, 2022

sdk 46 neutron-org/wasmd#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement improvements to new address generation #1014

Implement improvements to new address generation #1014

alpe commented Sep 20, 2022 •

edited

Loading

codecov bot commented Sep 20, 2022 •

edited

Loading

ethanfrey commented Sep 21, 2022

webmaster128 commented Sep 21, 2022

ethanfrey left a comment

webmaster128 commented Sep 21, 2022

webmaster128 left a comment

ethanfrey left a comment

webmaster128 left a comment

Implement improvements to new address generation #1014

Implement improvements to new address generation #1014

Conversation

alpe commented Sep 20, 2022 • edited Loading

codecov bot commented Sep 20, 2022 • edited Loading

Codecov Report

ethanfrey commented Sep 21, 2022

webmaster128 commented Sep 21, 2022

ethanfrey left a comment

Choose a reason for hiding this comment

webmaster128 commented Sep 21, 2022

webmaster128 left a comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

webmaster128 left a comment

Choose a reason for hiding this comment

alpe commented Sep 20, 2022 •

edited

Loading

codecov bot commented Sep 20, 2022 •

edited

Loading