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 governance proposal handler #37

Merged
merged 9 commits into from
May 28, 2021
Merged

Add governance proposal handler #37

merged 9 commits into from
May 28, 2021

Conversation

alpe
Copy link
Contributor

@alpe alpe commented May 4, 2021

Resolves #6

After #41

@alpe alpe marked this pull request as draft May 4, 2021 15:03
@alpe alpe changed the title Gov exec 6 Add governance proposal handler May 5, 2021
Copy link
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. Gave some feedback to help finish it off.

Shall I make a tgrade-contract that registers for governance voting and will allow some admin to make governance proposals so you can do a full-stack test? (Something like the reflect contract, but a bit more powerful... you can use the reflect tests from wasmd as a template here)

proto/confio/twasm/v1beta1/proposal.proto Outdated Show resolved Hide resolved
x/twasm/contract/incoming_msgs.go Show resolved Hide resolved
x/twasm/contract/incoming_msgs.go Show resolved Hide resolved
x/twasm/keeper/handler_plugin.go Outdated Show resolved Hide resolved
x/twasm/keeper/handler_plugin.go Show resolved Hide resolved
x/twasm/keeper/handler_plugin.go Show resolved Hide resolved
x/twasm/keeper/handler_plugin.go Show resolved Hide resolved
x/twasm/keeper/handler_plugin.go Show resolved Hide resolved
x/twasm/keeper/handler_plugin_test.go Show resolved Hide resolved
x/twasm/keeper/proposal_handler.go Outdated Show resolved Hide resolved
@alpe alpe changed the base branch from main to wasmd_upgrade May 18, 2021 11:00
@alpe alpe marked this pull request as ready for review May 18, 2021 11:01
Base automatically changed from wasmd_upgrade to main May 18, 2021 14:58
Copy link
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.

Good stuff. 98% satisfied and happy to see all the marshaling logic well covered with tests.

Just pointed out that 2 of the example JSON that is tested is not the format we return from the contract (it uses {} rather than "e30=").

If you update that (and optionally some of the other comments), I am happy to approve an merge.

testing/gov_test.go Show resolved Hide resolved
testing/gov_test.go Show resolved Hide resolved
x/twasm/contract/incoming_msgs.go Show resolved Hide resolved
x/twasm/contract/incoming_msgs.go Outdated Show resolved Hide resolved
x/twasm/contract/incoming_msgs.go Outdated Show resolved Hide resolved
x/twasm/contract/incoming_msgs_test.go Show resolved Hide resolved
x/twasm/contract/schema/tgrade_msg.json Show resolved Hide resolved
x/twasm/contract/schema/tgrade_msg.json Show resolved Hide resolved
x/twasm/keeper/handler_plugin.go Show resolved Hide resolved
x/twasm/keeper/handler_plugin_test.go Show resolved Hide resolved
@alpe
Copy link
Contributor Author

alpe commented May 21, 2021

I have addressed the issues. The last commit contains a wasmd upgrade as dependency that points to a branch.

Copy link
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.

Let's just get the wasmd PR merged and a release cut before merging this one (to keep deps clean on main)

@alpe alpe merged commit b519f53 into main May 28, 2021
@alpe alpe deleted the gov_exec_6 branch May 28, 2021 07:16
@ethanfrey
Copy link
Contributor

Nice!

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 governance proposal callback hooks
2 participants