docs: ADR 069 - x/gov modularity, multiple choice and optimistic proposal
#18498
Conversation
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe Cosmos SDK's Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- docs/architecture/README.md (1 hunks)
- docs/architecture/adr-069-gov-improvements.md (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/architecture/README.md
Additional comments: 1
docs/architecture/adr-069-gov-improvements.md (1)
- 1-200: The ADR 069 draft provides a comprehensive overview of the proposed enhancements to the
x/govmodule in the Cosmos SDK. The introduction of multiple choice and optimistic proposals, along with the newSPAMvote option, are significant changes that aim to improve the governance process within the Cosmos ecosystem. The ADR also outlines the necessary adjustments to the proposal tallying method and governance parameters to accommodate these new features.Here are some specific points to consider:
The addition of
ProposalTypeto thev1.Proposalstruct (lines 42-52) is a good approach to extend the functionality without introducing breaking changes. However, it's important to ensure that all existing code that interacts withv1.Proposalis compatible with this change.The concept of optimistic proposals (lines 61-75) is interesting, but it's crucial to ensure that the governance parameters for these proposals are well-documented and understood by the community to prevent any misuse or confusion.
The limitation of four voting options for multiple choice proposals (lines 79-85) seems reasonable to keep the voting process manageable. However, the introduction of the
SPAMvote option (lines 108-116) could have significant implications for governance, as it introduces a new dynamic to the voting process. It's important to clearly define what constitutesSPAMand ensure that the community is aligned on its usage.The changes to the voting options and the tallying method (lines 120-150) will require careful implementation to maintain backward compatibility and ensure that the new system is robust and secure.
The ADR acknowledges the impact on client applications (lines 155-157) and the need for backward compatibility (lines 159-165). It's important to provide clear migration paths and support for client developers to adapt to these changes.
The ADR mentions further discussions and potential future improvements to the
x/govmodule (lines 180-185). It would be beneficial to track these discussions and ensure that any subsequent changes are consistent with the goals outlined in this ADR.Overall, the ADR seems to be a well-thought-out proposal that could significantly enhance the governance capabilities of the Cosmos SDK. It's important to ensure that the community is involved in the discussion and that there is clear documentation and support for implementing these changes.
julienrbrt
requested review from
kocubinski,
alexanderbez,
facundomedica,
tac0turtle,
cool-develope,
atheeshp,
likhita-809 and
testinginprod
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- docs/architecture/adr-069-gov-improvements.md (1 hunks)
Additional comments: 1
docs/architecture/adr-069-gov-improvements.md (1)
- 1-200: The ADR is well-structured and provides a comprehensive overview of the proposed enhancements to the
x/govmodule. The introduction of multiple choice and optimistic proposals, along with the newSPAMvote option, are significant changes that aim to improve the flexibility and responsiveness of the governance process.
- The addition of
optimistic_authorized_addressesandoptimistic_rejected_thresholdparameters (lines 69-74) should be carefully reviewed to ensure they align with the intended permission and security model.- The limitation of four options for multiple choice proposals (line 81) seems reasonable to prevent decision fatigue and maintain clarity, but it's important to ensure that this limit is well-justified and documented.
- The introduction of a
SPAMvote (lines 108-116) is an interesting approach to filter out non-serious proposals, but the mechanism for penalizing incorrectSPAMvotes (line 116) should be clear and fair to avoid deterring participation.- The renaming of vote options to accommodate multiple choice proposals (lines 120-129) and the attempt to make these changes non-breaking (line 132) are important for maintaining backward compatibility.
- The custom tallying function interface (lines 144-150) adds modularity, but it's crucial to ensure that the implementation details are secure and do not introduce any vulnerabilities.
- The backward compatibility section (lines 159-165) acknowledges the impact on legacy systems, which is important for a smooth transition.
- The further discussions section (lines 180-187) indicates ongoing development and potential future changes, which stakeholders should be aware of.
Overall, the ADR seems to address the community's needs and requests effectively while also considering the implications of the changes. It's important to ensure that the proposed features are implemented securely and that they integrate well with the existing system.
|
@coderabbitai pause |
julienrbrt
force-pushed
the
julien/adr
branch
from
659f95b
to
2cacb0c
Compare
| // optimistic_authorized_addreses is an optional governance parameter that limits the authorized accounts than can submit optimisitc proposals | ||
| repeated string optimistic_authorized_addreses = 17 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | ||
|
|
||
| // Optimistic rejected threshold defines at which percentage of NO votes, the optimistic proposal should fail and be converted to a normal proposal. |
There was a problem hiding this comment.
Are optimistic props subject to the same VP? So if enough NO votes come in at the last second, it then gets converted to a normal prop and the entire VP starts again?
There was a problem hiding this comment.
There will be just one option, which will be no. So nothing can come second. You won't be able to vote yes or abstain.
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
|
|
||
| A multiple choice proposal is a proposal where the voting options can be defined by the proposer. | ||
|
|
||
| The number of voting option will be limited to a maximum of 4. A new vote option `SPAM` will be added and distinguished of those voting options. |
There was a problem hiding this comment.
Why limit to 4? I agree with limiting the number of options, but why not 6 or 8?
There was a problem hiding this comment.
Oh, I think I got it, this is because you want to keep the existing tally function which only handles 4 options.
There was a problem hiding this comment.
That would indeed simplify everything if we keep it 4, but we can maybe consider increasing it, if it is really wanted.
There was a problem hiding this comment.
Not sure if it's really wanted, but for sure this is a smart way to bring the feature with minimal changes 👍
There was a problem hiding this comment.
I think it really depends how this feature would be practically used.
Have you guys thought of some example proposals that would be suited to use the multiple choices and how they may fit within the 4 options limits? Maybe worth providing these examples in the ADR (just a bulleted list)?
Off the top of my head, I could see e.g. voting on electing group of peoples for some task. And the options might be different sets of candidates, in which case limiting it to 4 kinda makes sense since I assume we also want to not overwhelm the voters.
I also like the fact that we are reusing the existing VoteOption a lot. No point in adding complexity if not needed.
I am missing an aspect, how do you handle the case when the proposer wants to have less than 4 options, say 3?
There was a problem hiding this comment.
This feature will be used for signaling proposals (given that a multiple choice won't execute any message).
This leaves to many possibilities: choosing the canonical bridge for a chain, elections as you said, or surveys (hopefully not too much 😅),..
Voting on multiple choice proposal will make an extra query to get the options. That way, we will be able to refuse non set options.
There was a problem hiding this comment.
This feature will be used for signaling proposals (given that a multiple choice won't execute any message).
Yeah, I appreciated a lot that this is just for text proposals.
This leaves to many possibilities: choosing the canonical bridge for a chain, elections as you said, or surveys (hopefully not too much 😅),..
Right, and if the community is civil enough they should be able to boil down the final choices to go on-chain to a max of 4 using forum discussions.
He knew it
Jokes aside, I think it's better to implement the feature reusing as much of the existing as possible for now as you are planning. We don't need to overcomplicate things unless there's an explicit need, and until this feature is out and tested in the field we can't really know.
Voting on multiple choice proposal will make an extra query to get the options. That way, we will be able to refuse non set options.
This makes sense, thanks.
There was a problem hiding this comment.
This is great and very much needed. Thanks @julienrbrt and @tac0turtle for the work here!
I suggest restructuring the Proposals section in the same order as the proposal types, i.e first sub-section for standard text proposal, second for multiple choice, etc.
I also added a bunch of comments for things that didn't have context (eg. mentioning the SPAM vote option without introducing the concept or context). I suggest adding more context for first-time readers of this ADR.
Happy to also provide eng resources in this initiative once it's approved
| > An expedited proposal is by design a normal proposal with a quicker voting period and higher threshold. When an expedited proposal fails, it gets converted to a normal proposal. | ||
|
|
||
| An expedited optimistic proposal and an expedited multiple choice proposal do not make sense based on the definition above and is a proposal type instead of a proposal characteristic. |
There was a problem hiding this comment.
curious why these don't have their corresponding sections
There was a problem hiding this comment.
This one already exists, this is for context. I can add a section if you think it is clearer.
|
|
||
| Note, that expedited becomes a proposal type itself instead of a boolean on the `v1.Proposal` struct. | ||
|
|
||
| > An expedited proposal is by design a normal proposal with a quicker voting period and higher threshold. When an expedited proposal fails, it gets converted to a normal proposal. |
There was a problem hiding this comment.
can you define an expedited proposal with the same threshold?
There was a problem hiding this comment.
it gets converted to a normal proposal.
Does this mean that it continues to be open for voting until the end of the voting period?
There was a problem hiding this comment.
Currently, if an expedited proposal fails it becomes a standard proposal:
Line 202 in fc52ae5
We are proposing to do the same for rejected optimistic proposals
|
|
||
| However, chains may want to change the tallying function (weighted vote per voting power) of `x/gov` for a different algorithm (using a quadratic function on the voter stake, for instance). | ||
|
|
||
| The custom tallying function can be passed to the `x/gov` keeper with the following interface: |
There was a problem hiding this comment.
this is extremely useful
| The custom tallying function can be passed to the `x/gov` keeper with the following interface: | ||
|
|
||
| ```go | ||
| type Tally interface{ |
There was a problem hiding this comment.
I need to think a bit more of what exactly we need but at a high level you should be able to process a custom result with the voting power that has voted so far
There was a problem hiding this comment.
at a high level you should be able to process a custom result with the voting power that has voted so far
could you elaborate here? I dont understand what you mean
| A proposal is marked as `SPAM` when the total of weighted votes for all options is lower than the amount of weighted vote on `SPAM` | ||
| (`spam` > `option_one + option_two + option_three + option_four` = proposal marked as spam). |
There was a problem hiding this comment.
Is tallying done at the end of the VP (EndBlock)?
There was a problem hiding this comment.
Yes. Tallying stays in end blocker in this ADR. Future improvements will attempt to make it lazy.
As it's an implementation detail I don't think we need to cover it here.
However we do need methods in the tally interface for that.
|
|
||
| #### Multiple Choice Proposal | ||
|
|
||
| A multiple choice proposal is a proposal where the voting options can be defined by the proposer. |
There was a problem hiding this comment.
I am not really a fan of the fact that the options can only be set by the proposer. It relies on the proposer being fair in setting them. But definitely doing anything different would add a lot of complexity and it's not guaranteed to have any benefit.
A voter that is not agreeing with any option - and want none of them to pass - but does not want to abuse the SPAM vote can only choose to not vote, and try to keep the quorum lower than the threshold.
Maybe this is enough, but maybe not?
There was a problem hiding this comment.
When we first thought about it I thought the same. We thought of adding a blank vote option, but eventually decided to let that as well for the proposer to specify.
Usually, chains have an off chain process for before submitting a proposal. The voting option discovery should be done there and if someone decides to skip that process, it makes sense to consider the proposal as spam.
There was a problem hiding this comment.
Yeah, the voting option discovery should be done on the forum, completely agree with that, and ideally it's what makes the most sense.
I am just a little skeptical that it'll work like this all the times, and it might be very controversial when it doesn't.
| // to be decided | ||
|
|
||
| // Calculate calculates the tally result | ||
| Calculate(proposal v1.Proposal, govKeeper GovKeeper, stakingKeeper StakingKeeper) govv1.TallyResult |
There was a problem hiding this comment.
I think Tally could be simplified to Tally(ctx context.Context, govKeeper GovKeeper, prop v1.Proposal) (govv1.TallyResult, error)
I assume Tally could be a module's keeper implementing this interface and already holding within it other keepers besides gov. Passing GovKeeper in instead of having it in the tally implementer avoids circular dependencies between the tally interface and gov itself.
There was a problem hiding this comment.
The GovKeeper does not have the other keepers it uses public.
Which I think is good, we will be able to remove staking from the gov keeper as well altogether because it is only used for tallying.
|
|
||
| #### Optimistic Proposal | ||
|
|
||
| An optimistic proposal is a proposal that passes unless a threshold a NO votes is reached. |
There was a problem hiding this comment.
What is the intended use case?
There was a problem hiding this comment.
A chain can decide how they want to use it (given the possibility to restrict proposers).
However, we can think of it as a way to alleviate governance burden for recurring proposals or any boring proposals where no governance discussion needs to be involved. In the case of Osmosis, that would be periodic incentive updates for example. I have no example for the Hub however 😅
Description
ref: #14403, #17781
RENDERED
Follow the ADR implementation along: https://github.com/orgs/cosmos/projects/26/views/36
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.mdmake lintandmake testReviewers 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 changeSummary by CodeRabbit
x/govmodule, including the introduction of multiple choice and optimistic proposals, new voting options, and updated tallying methods.