feat(x/gov): dynamic quorum #135
Conversation
tbruyelle
force-pushed
the
giunatale/gov/dynamic-quorum
branch
from
ce6987a to
5a79faf
Compare
proto/atomone/gov/v1/gov.proto
Outdated
| string max_quorum = 26 [(cosmos_proto.scalar) = "cosmos.Dec"]; | ||
|
|
||
| // Minimum achievable quorum | ||
| string min_quorum = 27 [(cosmos_proto.scalar) = "cosmos.Dec"]; | ||
|
|
||
| // Maximum achievable quorum for constitution amendment proposals | ||
| string max_constitution_amendment_quorum = 28 [(cosmos_proto.scalar) = "cosmos.Dec"]; | ||
|
|
||
| // Minimum achievable quorum for constitution amendment proposals | ||
| string min_constitution_amendment_quorum = 29 [(cosmos_proto.scalar) = "cosmos.Dec"]; | ||
|
|
||
| // Maximum achievable quorum for law proposals | ||
| string max_law_quorum = 30 [(cosmos_proto.scalar) = "cosmos.Dec"]; | ||
|
|
||
| // Minimum achievable quorum for law proposals | ||
| string min_law_quorum = 31 [(cosmos_proto.scalar) = "cosmos.Dec"]; |
There was a problem hiding this comment.
wouldn't it be better to have a struct with both caps inside for each quorum? (like for dynamic deposit) instead of separate min/max params?
There was a problem hiding this comment.
Yes, what about embedding the threshold as well ?
There was a problem hiding this comment.
hmm that will break the API if we embed the threshold
There was a problem hiding this comment.
ok but I still don't see the two params in a single struct though :P
There was a problem hiding this comment.
Yes I didn't forget but given the ongoing audit I think it's preferrable to merge this now and address this after the audit.
There was a problem hiding this comment.
I created an issue to track the remaining work #145
|
Moreover, we need a starting value in genesis for the 3 quorums, not just the EMAs (which for a fresh chains are 0). Unless we use the EMAs as starting values, but that's kinda confusing imho. Maybe it could be ok to rely on the EMAs as starting values (since otherwise we would need a separate genesis value and that gets messy probably) but we should make it explicit also in the comments that the EMA should be non-zero for a fresh chain and error in case it's 0 |
Yes, this is part of the TODOs in the PR body. |
Good point. On the other hand, if you start a chain with 0 EMA, you just end up with the minQuorum value for the quorum, that's not catastrophic. But I agree this needs to be well documented. Adding this in the TODO list. |
|
@giunatale I think we're good with that PR, let's address the e2e tests and the README update later. |
|
should be good now, I am fine if the last two points of #145 aren't yet addressed since they don't modify the core logic at all (tests and the readme should not matter in that sense). Thanks for taking care of adding the ranges |
Instead of having a fixed quorum from the parameters, the quorum is adjusted between 2 bounds, given the proposal vote's participation.
TODO:
params.LawQuorumandparams.ConstitutionAmendmentQuorumto new dynamic modeltypes.NewParamsto remove deprecated paramsParamsquery that returns the dynamic quorum inTallyParamsbut the deprecated quorum value in theparamsstruct itself (see also in x/gov/client/cli/query.go)instead of adding a new Quorum query, consider adding a quorum field to the proposal response (but that could break the compatibility)