Skip to content

Implement CLI/REST server support for new messages#131

Merged
alpe merged 7 commits into0.9from
contract_admin_rest
Jun 5, 2020
Merged

Implement CLI/REST server support for new messages#131
alpe merged 7 commits into0.9from
contract_admin_rest

Conversation

@alpe
Copy link
Copy Markdown
Contributor

@alpe alpe commented Jun 4, 2020

🚧 After #124

CLI/REST Support:

  • Set/clear admin on contract
  • Migrate contract to new code id

Reviewer note

  • No tests implemented, yet 🚨
  • Rest routes are currently discussed 🗣️

--

  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alpe alpe changed the title Implement REST server support for new messages Implement CLI/REST server support for new messages Jun 4, 2020
@alpe alpe changed the base branch from master to 0.9 June 4, 2020 15:03
@CosmWasm CosmWasm deleted a comment from codecov-commenter Jun 4, 2020
@alpe
Copy link
Copy Markdown
Contributor Author

alpe commented Jun 4, 2020

I did some manual tests with a local instance:

wasmcli keys add fred
wasmcli tx send $(wasmcli keys show validator -a) $(wasmcli keys show fred -a) 5000000ucosm --chain-id=testing
# store first version
wasmcli tx wasm store contract.wasm --from validator --gas 600000  -y --chain-id=testing
wasmcli query wasm list-code

# create instance with admin: fred
wasmcli tx wasm instantiate 1 '{"verifier":"cosmos1rsj2m3qas3jy50s8psns95qka09lhwhx2wvdq8","beneficiary":"cosmos1rsj2m3qas3jy50s8psns95qka09lhwhx2wvdq8"}'  --from validator --amount=50000ucosm  --label "escrow 1" -y --chain-id=testing --admin=$(wasmcli keys show fred -a)

# migrate with user validator => fails
wasmcli tx wasm migrate cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5 2 "{}" --from validator --chain-id=testing


# migrate with user fred => ok
wasmcli tx wasm migrate cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5 2 "{}" --from fred --chain-id=testing


# show status
wasmcli query wasm contract cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5

{
  "code_id": 2,
  "creator": "cosmos1r5kgu6y7fqfh4dksw4c28hv7ns9ydnusf33vgs",
  "admin": "cosmos1rsj2m3qas3jy50s8psns95qka09lhwhx2wvdq8",
  "label": "escrow 1",
  "init_msg": {
    "verifier": "cosmos1rsj2m3qas3jy50s8psns95qka09lhwhx2wvdq8",
    "beneficiary": "cosmos1rsj2m3qas3jy50s8psns95qka09lhwhx2wvdq8"
  },
  "address": "cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5"
}

@alpe alpe marked this pull request as ready for review June 4, 2020 15:06
@alpe alpe requested a review from ethanfrey as a code owner June 4, 2020 15:06
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 4, 2020

Codecov Report

Merging #131 into 0.9 will decrease coverage by 0.07%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              0.9     #131      +/-   ##
==========================================
- Coverage   62.13%   62.06%   -0.08%     
==========================================
  Files          20       20              
  Lines        1764     1766       +2     
==========================================
  Hits         1096     1096              
- Misses        576      578       +2     
  Partials       92       92              
Impacted Files Coverage Δ
x/wasm/internal/keeper/querier.go 36.13% <0.00%> (-0.62%) ⬇️
x/wasm/internal/types/types.go 0.00% <0.00%> (ø)

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 9a16d58...233b508. Read the comment docs.

Copy link
Copy Markdown
Contributor

@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.

Looks good. And there are unfortunately no tests for CLI/REST (most anywhere).
I just ask you to test them out manually before merging (honor system).

The PR looks good for adding TX support. However, it doesn't close the linked issues. For that it must add query support - adding the administrator field to the contract info - this is essential info to know if I want to trust a contract, as important as the code behind it.

Comment thread x/wasm/client/cli/tx.go
cmd := &cobra.Command{
Use: "set-contract-admin [contract_addr_bech32] [optional_new_admin_addr_bech32]",
Short: "Set new admin for a contract. Can be empty to prevent further migrations",
Args: cobra.RangeArgs(1, 2),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i didn't know this one. I like it.

i kind of think setting it to nil should require a bit more intention than a missing argument. but cli ux is definitely not high on my list of concerns (or anyone's I think).

Maybe a flag for this? Fine to leave as is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍 A flag is a good idea. Will add it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added --no-admin

@ethanfrey
Copy link
Copy Markdown
Contributor

If you add the field to query and manually test, feel free to merge without another review

@ethanfrey
Copy link
Copy Markdown
Contributor

Sorry @alpe, I missed that the field was there. I expected it needed a code change and didn't come automatically.

Good to merge as is

@alpe alpe force-pushed the contract_admin_rest branch from 3821c0f to 233b508 Compare June 5, 2020 12:10
@alpe
Copy link
Copy Markdown
Contributor Author

alpe commented Jun 5, 2020

Manual CLI tests for set new contract admin

wasmcli keys add greg
wasmcli tx send $(wasmcli keys show validator -a) $(wasmcli keys show greg -a) 5000000ucosm --chain-id=testing

# non admin -> fail
wasmcli tx wasm set-contract-admin cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5  $(wasmcli keys show greg -a) --from validator --chain-id=testing

# as admin -> success
wasmcli tx wasm set-contract-admin cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5  $(wasmcli keys show greg -a) --from fred --chain-id=testing
# confirm
wasmcli query wasm contract cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5

# fred not admin -> fail
wasmcli tx wasm set-contract-admin cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5  $(wasmcli keys show greg -a) --from fred --chain-id=testing

# clear admin
wasmcli tx wasm set-contract-admin cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5  --no-admin --from greg --chain-id=testing -y -b=block

@alpe alpe merged commit ebac9aa into 0.9 Jun 5, 2020
@alpe alpe deleted the contract_admin_rest branch June 5, 2020 13:08
alpe added a commit that referenced this pull request Jun 9, 2020
* Cleanup ContractInfo type

* Add admin to contract instanciation

* Add cli commands for new TX

* Add rest support for new TX

* Update changelog

* Make optional admin flag for better UX

* Add flag to not accidentally clear admin on update
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.

Add CLI client support for new messages Add REST server support for new messages

3 participants