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

Fix SudoContractProposal and ExecuteContractProposal #808

Merged
merged 3 commits into from
Apr 22, 2022
Merged

Fix SudoContractProposal and ExecuteContractProposal #808

merged 3 commits into from
Apr 22, 2022

Conversation

the-frey
Copy link
Contributor

@the-frey the-frey commented Apr 17, 2022

Follow up fix for #730

Add missing SudoContractProposal and ExecuteContractProposal where it was missinng in types/codec and basic validation unit test.

Could be some more to this that I've missed.

@the-frey the-frey requested a review from alpe as a code owner April 17, 2022 22:01
@the-frey
Copy link
Contributor Author

the-frey commented Apr 17, 2022

Will also be running some manual tests tomorrow to check if this resolves the issues we've seen with gov/sudo.

EDIT: 🚀

@codecov
Copy link

codecov bot commented Apr 17, 2022

Codecov Report

Merging #808 (190b8cc) into main (57ead1a) will increase coverage by 0.54%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #808      +/-   ##
==========================================
+ Coverage   58.76%   59.31%   +0.54%     
==========================================
  Files          50       50              
  Lines        5862     5896      +34     
==========================================
+ Hits         3445     3497      +52     
+ Misses       2164     2145      -19     
- Partials      253      254       +1     
Impacted Files Coverage Δ
x/wasm/types/codec.go 100.00% <100.00%> (ø)
x/wasm/types/test_fixtures.go 95.61% <100.00%> (+0.66%) ⬆️
x/wasm/types/proposal.go 69.05% <0.00%> (+5.86%) ⬆️

@webmaster128 webmaster128 requested a review from maurolacy April 18, 2022 08:00
@the-frey
Copy link
Contributor Author

Update: we have run this on a testnet with a sudo contract and successfully moved funds via a gov prop. Our auto-CI hook that was failing with 0.25.0 also passes when we substitute in this branch, so we're pretty confident it's the fix.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

LGTM. Perhaps @alpe wants to take a look.

@webmaster128 webmaster128 added this to the v0.27.0 milestone Apr 21, 2022
Copy link
Contributor

@pinosu pinosu left a comment

Choose a reason for hiding this comment

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

lgtm. 👍
I added just some minor comments.
Also it would be nice if you could update the description mentioning that also ExecuteContractProposal was added.

@the-frey
Copy link
Contributor Author

@pinosu have updated the description and some additional test cases as you suggested.

@webmaster128 webmaster128 changed the title Add missing SudoContractProposal Fix SudoContractProposal and ExecuteContractProposal Apr 22, 2022
@webmaster128 webmaster128 merged commit d5ef3ba into CosmWasm:main Apr 22, 2022
@ethanfrey
Copy link
Member

Good find. I have a mental block with amino. Can't believe we still need to register all this stuff.

@the-frey the-frey deleted the sudo_proposals branch April 25, 2022 10:16
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.

5 participants