-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Conversation
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
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
7e179a6
to
e6e4f25
Compare
There was a problem hiding this 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.
@@ -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) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😬
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
proto/cosmos/gov/v1/gov.proto
Outdated
// 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
3ef753f
to
fb014d8
Compare
4511dc4
to
ea2ecfd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, pending specs.
There was a problem hiding this 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
dfb65ce
to
faf6661
Compare
There was a problem hiding this 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.
There was a problem hiding this 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)
(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
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change