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

refactor: add burnable params to governance #15151

Merged
merged 20 commits into from
Feb 28, 2023
Merged

Conversation

tac0turtle
Copy link
Member

@tac0turtle tac0turtle commented Feb 24, 2023

Description

Closes: #11057

add configurable params for burning coins in governance


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

keeper.RefundAndDeleteDeposits(ctx, proposal.Id) // refund deposit if proposal got removed without getting 100% of the proposal

if !keeper.GetParams(ctx).BurnProposalDeposit {
keeper.RefundAndDeleteDeposits(ctx, proposal.Id) // refund deposit if proposal got removed without getting 100% of the proposal

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
if !keeper.GetParams(ctx).BurnProposalDeposit {
keeper.RefundAndDeleteDeposits(ctx, proposal.Id) // refund deposit if proposal got removed without getting 100% of the proposal
} else {
keeper.DeleteAndBurnDeposits(ctx, proposal.Id) // burn the deposit if proposal got removed without getting 100% of the proposal

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
@tac0turtle tac0turtle marked this pull request as ready for review February 24, 2023 17:23
@tac0turtle tac0turtle requested a review from a team as a code owner February 24, 2023 17:23
Copy link
Contributor

@atheeshp atheeshp left a comment

Choose a reason for hiding this comment

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

mostly lgtm, but a few doubts.

proto/cosmos/gov/v1/gov.proto Show resolved Hide resolved
@@ -27,6 +27,9 @@ var (
DefaultMinInitialDepositRatio = sdk.ZeroDec()
DefaultProposalCancelRatio = sdk.MustNewDecFromStr("0.5")
DefaultProposalCancelDestAddress = ""
DefaultBurnProposal = false // set to false to replicate behavior of when this change was made (0.47)
DefaultBurnVoteQuorom = false // set to false to replicate behavior of when this change was made (0.47)
DefaultBurnVoteVeto = true // set to true to replicate behavior of when this change was made (0.47)
Copy link
Contributor

Choose a reason for hiding this comment

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

There won't be any difference between No and NoWithVeto if this param set to false?

Copy link
Member

Choose a reason for hiding this comment

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

Agreeing with this, maybe instead of not burning, the condition should send it to the community pool?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the existing behavior. Don't want to chains and allow chains to decide.

Copy link
Member

@julienrbrt julienrbrt Feb 24, 2023

Choose a reason for hiding this comment

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

Yea, I meant instead of disabling the burn, possibly change that setting to allow sending the deposit to the community pool.
Disabling the burn, as Atheesh mentioned effectively transform a no with veto in a no with a lower fail threshold.

Although I agree this will make the settings a bit more confusing if the last one does something different. So nvm 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

we can add another param to decide if its burn or send to community pool? but its up to the chain what they would want to set here. Would rather leave it to chains then we decide for them

Copy link
Contributor

@atheeshp atheeshp Feb 27, 2023

Choose a reason for hiding this comment

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

Okay, but I'm personally thinking this param is not needed, community in any chain may want to punish the proposal depositors if it is not appropriate. Sorry if I'm missing context.

// burn deposits if a proposal does not meet qourum
bool burn_vote_quorum = 13;
// burn deposits if the proposal does not get enough deposits
bool burn_proposal_deposit = 14;
Copy link
Contributor

@atheeshp atheeshp Feb 24, 2023

Choose a reason for hiding this comment

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

This param name might mislead the dev, as they might get it as burning the proposal deposits in any case. wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated the comment. do you have a suggestion for the name?

Copy link
Member Author

Choose a reason for hiding this comment

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

appended prevote, do you think that is better?

Copy link
Contributor

@atheeshp atheeshp Feb 27, 2023

Choose a reason for hiding this comment

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

do you think that is better?

much better than the previous :)

@@ -56,7 +59,7 @@ func NewVotingParams(votingPeriod *time.Duration) VotingParams {
// NewParams creates a new Params instance with given values.
func NewParams(
minDeposit, expeditedminDeposit sdk.Coins, maxDepositPeriod, votingPeriod, expeditedVotingPeriod time.Duration,
quorum, threshold, expeditedThreshold, vetoThreshold, minInitialDepositRatio, proposalCancelRatio, proposalCancelDest string,
quorum, threshold, expeditedThreshold, vetoThreshold, minInitialDepositRatio, proposalCancelRatio, proposalCancelDest string, burnProposalDeposit, burnVoteQuorum, burnVoteVeto bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Changelog entry for this? And one more entry for introduction of new params?

@julienrbrt julienrbrt added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Feb 24, 2023
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm, but agreeing with the comments of @atheeshp above.
Documentation should be updated as well.

x/gov/simulation/genesis.go Outdated Show resolved Hide resolved
proto/cosmos/gov/v1/gov.proto Outdated Show resolved Hide resolved
proto/cosmos/gov/v1/gov.proto Outdated Show resolved Hide resolved
x/gov/abci.go Show resolved Hide resolved
Copy link
Contributor

@atheeshp atheeshp left a comment

Choose a reason for hiding this comment

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

lgtm, pending specs.

CHANGELOG.md Outdated Show resolved Hide resolved
x/gov/types/v1/params.go Outdated Show resolved Hide resolved
Copy link
Contributor

@likhita-809 likhita-809 left a comment

Choose a reason for hiding this comment

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

need to address @atheeshp 's comments, otherwise lgtm

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

e2e tests and integration tests still need to be updated, other than that looks good!
I will play with simapp tomorrow and give a tACK when I've tested it locally.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

tACK. (one e2e needs to be fixed still, however)

@tac0turtle tac0turtle enabled auto-merge (squash) February 28, 2023 18:09
@julienrbrt julienrbrt added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 28, 2023
@tac0turtle tac0turtle merged commit 44495e7 into main Feb 28, 2023
@tac0turtle tac0turtle deleted the marko/configurable-govBurn branch February 28, 2023 20:44
mergify bot pushed a commit that referenced this pull request Feb 28, 2023
(cherry picked from commit 44495e7)

# Conflicts:
#	CHANGELOG.md
#	api/cosmos/gov/v1/gov.pulsar.go
#	api/cosmos/tx/v1beta1/service.pulsar.go
#	proto/cosmos/gov/v1/gov.proto
#	tests/e2e/gov/query.go
#	types/tx/service.pb.go
#	x/gov/README.md
#	x/gov/abci.go
#	x/gov/migrations/v4/json.go
#	x/gov/migrations/v4/json_test.go
#	x/gov/migrations/v4/store.go
#	x/gov/migrations/v5/store.go
#	x/gov/simulation/genesis.go
#	x/gov/types/v1/gov.pb.go
#	x/gov/types/v1/params.go
julienrbrt added a commit that referenced this pull request Feb 28, 2023
@julienrbrt julienrbrt mentioned this pull request Feb 28, 2023
19 tasks
julienrbrt added a commit that referenced this pull request Mar 1, 2023
julienrbrt added a commit that referenced this pull request Mar 1, 2023
Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release C:Simulations C:x/gov
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the gov proposal options configurable
4 participants