feat(x/gov): add governors #73
Conversation
Only sketching out how the tally would work for now. Lots of stuff is left undefined but the strategy is the one outlined in https://gist.github.com/giunatale/95e9b43f6e265ba32b29e2769f7b8a37?permalink_comment_id=5067400#gistcomment-5067400 but with the restriction of being able to delegate a percentage of its bonded tokens to at most one governor (https://gist.github.com/giunatale/95e9b43f6e265ba32b29e2769f7b8a37?permalink_comment_id=5187246#gistcomment-5187246) It assumes that every time someone redelegates, undelegates or adds to its delegations a hook is called to check if there is a governor delegated to in x/gov, and the governor's total shares updated accordingly. The governor's total shares are a collection of share amounts for different validators.
can only delegate all or nothing to governors now
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
| - The system tracks each governor’s address, description, status (e.g., active), | ||
| and last status change time (to limit frequent status toggles). | ||
|
|
||
| 2. Delegation and Undelegation: |
There was a problem hiding this comment.
(When I am making this comment I haven't read the implementation yet).
I suppose the governor inherits the votes of whoever delegate to them at proposal execution and not voting right?
Given a proposal passing expects super majority it makes sense I think it is okay to have no timeout or maximum redelegation amount (like you have in normal x/gov due to the x/staking redelegation limitations).
There was a problem hiding this comment.
Yes, at proposal tally.
Also, the limitations to x/gov for the canonical module are indeed inherited from the dependency with x/staking that we don't have here. So indeed there is no need to have these limitations in place. The representative voting system can be more "liquid" as a result.
| `MsgUpdateGovernorStatus`. Governors that set themselves to inactive are allowed | ||
| to delegate their governance voting power to another governor. A status change | ||
| can however only occur after a waiting period (`GovernorStatusChangePeriod`). | ||
| - Attempts to rapidly change status are disallowed; changes can only occur |
There was a problem hiding this comment.
Can a governor having voted to a current proposal, with users delegated to it change its status during the voting period? If so, should it really?
There was a problem hiding this comment.
So you are proposing to prevent a status change if there is at least a proposal active (i.e. in voting period?)
Anyway, it would be the same as validators, that can unbond regardless of the status of proposals. We can further discuss this, but on a first level, what is your concern?
There was a problem hiding this comment.
My concern is that a governor can abruptly become inactive during a vote, or right before the voting period ends, meaning their delegators that didn't vote because their delegators had voted and no more represented.
It could even change the proposal outcome in some very close scenario.
I agree that a waiting period is required, but instead of having it "in between", i'd instead queue that change for the waiting period duration.
So for any change, force the waiting period before the change takes place, not depending if you have just changed your status or if you waited a long time.
Right now, if you changed a long time ago, the change would be instant.
There was a problem hiding this comment.
@julienrbrt Similarly, a governor can change this vote, which might also change the outcome of the proposal.
Given that, I'm not sure we should try to delay or prevent a change in his status.
Same for validators, a validator can be rejected from the set or being tombstoned, which immediately affects the votes (for other chains).
There was a problem hiding this comment.
yeah, @tbruyelle brings up a great point. I think it's ok to leave the system as is for this reason. Moreover, even if we wanted to change it (something which I actually worked on), we would have the issue of having to also prevent staking undelegations which bring the bonded amount below the min stake requirement, otherwise that would also be a way to bypass the queued change. And in practice the hooks do not allow to distinguish between an undelegation and a redelegation, which make doing this very challenging.
For the reason above and the observation from Thomas, I think it's better if we keep this as is
There was a problem hiding this comment.
There was a problem hiding this comment.
maybe a good exercice to use collections here?
There was a problem hiding this comment.
Aside from the exercise what would be the benefit of using collections here exactly? Don't get me wrong this is a genuine question I don't know the answer to and would like to understand
There was a problem hiding this comment.
less boilerplate basically, you define the collections in the keeper and you do not need this whole boilerplate of getters and setters.
There was a problem hiding this comment.
I love collections, it is great piece of work. It has just the required abstraction to make dev life easier w/o losing control. That said, to use it for governor we need to wait for SDK50 merge (or to use the port I made for onchain multisig, but that's not reasonnsable since sdk50 upgrade is close ^^)
There was a problem hiding this comment.
The x/gov module in main is not in sync with the v0.50 one, it is not using collections . We need to update it so that using collections for delegations and governors is aligned with the rest. Otherwise tbh it would look a pretty ugly frankenstein.
https://github.com/atomone-hub/atomone/blob/main/x/gov/keeper/keeper.go#L20 vs https://github.com/atomone-hub/cosmos-sdk/blob/release/v0.50.x/x/gov/keeper/keeper.go#L21 makes it evident as the keeper does not contain the collections, and they aren't used anywhere contrary to the v0.50 version.
@julienrbrt do you mind working on it?
cc @tbruyelle
There was a problem hiding this comment.
For the time being, I won't be using collections. Once #217 is addressed, I will update this PR accordingly
There was a problem hiding this comment.
I commented #217 (comment)
|
im realizing now that will have to come too to my gigantic PR 😅 |
I will deal with it, once the gov module update is done I will adapt this PR to port the governors. |
3 participants
iterating over the work started in #16, this PR adds governors to the x/gov module.
More details can be also read in #16