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

ADR 037: Governance Split Votes #7733

Merged
merged 15 commits into from
Nov 16, 2020
Merged

Conversation

sunnya97
Copy link
Member

@sunnya97 sunnya97 commented Oct 29, 2020

Description

This ADR introduces the concept of governance split votes. Please see the ADR for details.

Coauthored with @antstalepresh


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@sunnya97 sunnya97 added C:x/gov T: ADR An issue or PR relating to an architectural decision record labels Oct 29, 2020
@sunnya97 sunnya97 assigned sunnya97 and unassigned sunnya97 Oct 29, 2020
@sunnya97
Copy link
Member Author

Is there any preference to whether the vote struct is done as currently proposed in the ADR:

Vote {
    ProposalId  int64
    Voter           sdk.Address
    options       []Option
    rates           []sdk.Dec
}

vs creating a new SubVote struct that contains Option+Rate?

Vote {
    ProposalId  int64
    Voter           sdk.Address
    subvotes    []SubVotes
}

SubVote {
    Option  option
    Rate     sdk.Dec
}

docs/architecture/adr-033-gov-split-vote.md Outdated Show resolved Hide resolved
docs/architecture/adr-033-gov-split-vote.md Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor

Concept ACK!

@alexanderbez
Copy link
Contributor

I prefer the sub-vote option @sunnya97. I don't see any merit to splitting up the types, since they're logically so connected.

@zmanian
Copy link
Member

zmanian commented Oct 30, 2020

This is a very important feature to me. I am trying to reach out to custodians to see if they would embrace it.

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

This does break backwards compatibility and I see that as a negative but it is not listed.

Please try to find a way to do this that doesn't break existing APIs, esp MsgVote. Introduce a new msg instead. I see this a rather niche use case and shouldn't be exposed to users by default.

aaronc
aaronc previously requested changes Oct 30, 2020
docs/architecture/adr-033-gov-split-vote.md Outdated Show resolved Hide resolved
docs/architecture/adr-033-gov-split-vote.md Outdated Show resolved Hide resolved
docs/architecture/adr-033-gov-split-vote.md Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

I do agree that we should avoid breaking existing message types and APIs, but I don't see this as niche at all. I think this will be very useful, even for the average user. ACK

@sunnya97
Copy link
Member Author

@aaronc If we want to keep the types the same for easier wallet compatibility, could we just create a new Msg type, but not a separate underlying struct?

If SimpleVote and WeightedVote were two different structs in state, we'd have to create a new vote interface that both of those satisfy, and this will likely be a larger change.

Instead, the on-chain Vote struct could include weighted votes, and a simple MsgVote (backwards compatible) received by the chain is just converted to a single Option WeightedVote that's stored in state.

So we'd have:

type MsgVote struct {
  ProposalID int64
  Voter      sdk.Address
  Option   Option
}

type MsgWeightedVote struct {
  ProposalID int64
  Voter      sdk.Address
  Options    []WeightedVoteOption
}

type Vote struct {
  ProposalID int64
  Voter      sdk.Address
  Options    []WeightedVoteOption
}

@aaronc
Copy link
Member

aaronc commented Oct 30, 2020

@sunnya97 that could work 👍

We could also do this for Vote and it wouldn't even be breaking at all:

type Vote struct {
  ProposalID int64
  Voter      sdk.Address
  Option     VoteOption // if set to VOTE_OPTION_UNSPECIFIED, then Options is set instead
  Options    []WeightedVoteOption
}

Btw, I'm not completely opposed to breaking changes in .proto files we've marked as beta, but I would prefer to reserve those changes for cases where we find architectural problems as opposed to want to add new features. This seems more or less like a new, additive feature.

@zmanian
Copy link
Member

zmanian commented Oct 30, 2020

We really want to encourage large custodians to build a proxy voting infrastructure to go with their staking operations.

This is a complement to efforts like staking derivatives to ensure the viability of proof of stake as large custodians become more and more important.

@sunnya97 sunnya97 requested a review from aaronc November 2, 2020 16:57
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

This looks sensible to me.

docs/architecture/adr-033-gov-split-vote.md Outdated Show resolved Hide resolved
@sunnya97 sunnya97 changed the title ADR: Governance Split Votes ADR 037: Governance Split Votes Nov 11, 2020
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Very interesting add on for custodians.

@sunnya97 sunnya97 merged commit 6261df8 into cosmos:master Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/gov T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants