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

Governance: voters weights add-in #2450

Merged
merged 22 commits into from
Oct 13, 2021
Merged

Governance: voters weights add-in #2450

merged 22 commits into from
Oct 13, 2021

Conversation

SebastianBor
Copy link
Contributor

@SebastianBor SebastianBor commented Sep 24, 2021

Summary

The purpose of this change is to support voter weights provided by an external program (addin). This would allow to move away from a simple token deposit based weights and make it possible to provide custom strategies without the need to modify the governance program (open/close principle).
The following voter weight addins are planned (not part of this PR):

  • Time locked token deposits
  • Time locked token deposits with multiple locks (ex. 1mm locked for 4 years and 1k locked for 1year)
  • Token freezing instead of deposits
  • Multiple tokens deposits
  • NFTs deposits
  • Reputation score

Solution

  • VoterWeight addin interface was defined using VoterWeightRecord account. Governance program reads the account to get voter's weight instead of using the internal deposit pool.
  • Each addin program manages their own VoterWeightRecord accounts
  • The Governance program stores the chosen addin program id in RealmConfig extension account and flags it in realm config use_community_voter_weight_addin
  • Sample voter-weight-addin program is provided as a template for creating custom addins

Time sensitive voter weights

VoterWeightRecord has expiry flag for time sensitive voter weights. When the up to date voter weight needs to be calculated at the time of voting the client (UI) would call Revise instruction for the addin with all the needed accounts within the same transaction as the governance instruction and the addin would set the expiry to the current slot. Then the governance instruction would validate the expiry to ensure it uses up to date weight.
This way any custom logic can be supported and the addins might use any arbitrary set of accounts and current time to calculate the up to date voter weight.

Alternative solution I was considering was to make a CPI call from the governance program to the addin program to get voter's weight, but that would need a custom set of accounts to be provided in all but trivial scenarios, and I decided the Revise instruction with vote_weight_expiry was a simpler solution and would cover any arbitrary scenario.

feat: add use_voter_weight_add_in flag to realm config

chore: use spl-governance 1.1.1 version

chore: make clippy happy

chore: add test to deserialise v1 CreateRealm instruction from v2

feat: add voter-weight-addin skeleton project

chore: build voter-weight-addin before governance

fix: temp workaround to make spl_governance_voter_weight_addin available in CI

chore: add tests with voter-weight-addin

feat: implement deposit instruction for voter weight addin

feat: add voter_weight_expiry

fix: set voter_weight_expiry

chore: restore positive execute tests

chore: restore ignored tests

wip: pass voter weight accounts to create_account_governance2

wip: read voter weight account

chore: make clippy happy

wip: add realm and validation to voter_weight deposit

fix: update addin

chore: make clippy happy

chore: fix voter_weight_record names

feat: use voter weight provided by addin when governance created

chore: update addin

chore: remove governance stake pool program

feat: remove time offset from revise

chore: fix build

feat: create RealmAddins account when realm with addin is created

chore: make clippy happy

feat: set voter weight addin using SetRealmConfig instruction

chore: make clippy happy

chore: update comments

chore: reorder SetrealmConfig accounts

chore: infer use_community_voter_weight_addin

chore: infer use_community_voter_weight_addin

chore: update voter weight addin comments

feat: use voter weight addin id from RealmAddins account
@SebastianBor SebastianBor marked this pull request as ready for review October 1, 2021 15:07
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

It took me awhile to understand it all, but I think the approach is sound. My main question is about the provided skeleton program, and whether it'll actually move tokens. A lot of the logic in DepositGoverningTokens relies on governance info, so you'll have to be careful duplicating those parts. There's also the problem with topups / relinquishes, and making sure that it's reflected in vote records.

I wonder if there's some nice refactoring that can be done for the deposit / withdraw logic, so that the governance program reuses bits of the basic addin.

governance/program/tests/process_flag_instruction_error.rs Outdated Show resolved Hide resolved
governance/program/tests/process_flag_instruction_error.rs Outdated Show resolved Hide resolved
governance/program/src/instruction.rs Outdated Show resolved Hide resolved
governance/program/src/state/realm_config.rs Show resolved Hide resolved
governance/program/src/state/realm_config.rs Show resolved Hide resolved
governance/program/src/state/realm_config.rs Outdated Show resolved Hide resolved
governance/program/src/state/realm_config.rs Show resolved Hide resolved
@SebastianBor
Copy link
Contributor Author

It took me awhile to understand it all, but I think the approach is sound. My main question is about the provided skeleton program, and whether it'll actually move tokens. A lot of the logic in DepositGoverningTokens relies on governance info, so you'll have to be careful duplicating those parts. There's also the problem with topups / relinquishes, and making sure that it's reflected in vote records.

I wonder if there's some nice refactoring that can be done for the deposit / withdraw logic, so that the governance program reuses bits of the basic addin.

Your observations are correct, the provided add-in skeleton is not a full working example yet. For this PR I implemented only the bare minimum to make it possible to use it in governance tests. I'm planning to do the full end to end implementation in the next PR.

And yes, it's a good idea to make the common code to create add-ins reusable. Since the add-in has dependency on the governance program (and knowledge about it) I think it could just reference the spl-governance crate, but we could also create a crate with just the reusable utilities. I'm not sure which one would be better yet.

Note, when the add-in is used it doesn't eliminate need for TokenOwnerRecord. It's still used to track realm members, their active proposals, active votes and allows to use governance delegates. And it's still the governance program responsibility to manage all of that. This is why CreateTokenOwnerRecord instruction was created as part of this PR to make it possible to create the account without depositing any tokens.

@SebastianBor SebastianBor merged commit c99f419 into solana-labs:master Oct 13, 2021
@joncinque
Copy link
Contributor

Sorry for the late re-review -- most of the points are cosmetic, and the overall concept behind this is sound. Do add the build.rs file to remove that *.so in the repo please!

@SebastianBor
Copy link
Contributor Author

Sorry for the late re-review -- most of the points are cosmetic, and the overall concept behind this is sound. Do add the build.rs file to remove that *.so in the repo please!

Thank you, I've addressed the issues in a follow up PR #2512

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants