This repository was archived by the owner on Mar 11, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joncinque
reviewed
Feb 21, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great for the most part! Just one part to address, and a few nits
governance/program/src/processor/process_set_realm_config_item.rs
Outdated
Show resolved
Hide resolved
governance/program/src/processor/process_set_token_owner_record_lock.rs
Outdated
Show resolved
Hide resolved
governance/program/tests/process_remove_token_owner_record_lock.rs
Outdated
Show resolved
Hide resolved
governance/program/tests/process_set_token_owner_record_lock.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jon C <[email protected]>
…/solana-program-library into governance-tor-locks
…already_exists_error
joncinque
previously approved these changes
Feb 23, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Pull request has been modified.
Improve test_add_token_owner_record_lock_authority_with_authority_already_exist readability
…without signature
joncinque
previously approved these changes
Feb 23, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one little nit, but it can be addressed in a follow-up. Looks good!
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Make it possible to control governance token withdrawals by external authority. The main use case is to support chained governance plugins where a middle chain plugin can decide whether tokens held by another plugin can be withdrawn. This requirement exists for the
Clans
plugin which allows members to join clans to aggregate their voting power and vote as a single entity. When members join a clan then they can't withdraw their tokens which would be held by another plugin.There are potentially over use cases where more advanced token locking scenarios could be implemented. For example a DAO could issue a grant or distribute rewards in exchange for a longer token lockup or a DAO could decide proposal creators should have their tokens locked for a longer time then the duration of the proposal.
The time based locking mechanism can also replace the current
unrelinquished_votes_count
base mechanism to lock voters tokens during vote on active proposals. Once this is implemented it won't be necessary to manually relinquish votes for each proposal to allow withdrawals, which is currently a source of pain for users.Implementation
Instructions
SetTokenOwnerRecordLock
andRelinquishTokenOwnerRecordLocks
where added to allow setting and removing permanent and time based locks forTokenOwnerRecords
The authorities which are allowed to control the locks are stored in
GoverningTokenConfig
separately for each governing token. In order to facilitate setup of the authorities a new instruction was addedSetRealmConfigItem
.The new instruction is an improvement over
SetRealmConfig
and allows to set one config item at a time. It makes it easier to reason about the changes made by the instruction because there is no need for configuration diff and the change is still visible in a proposal after executing it while the diff based change is lost. It also makes it safer by preventing accidental changes to the configuration by using invalid diff logic. AllRealm
andGovernance
config changes should eventually be supported by the single item logic.The implementation is backward compatible and the relevant accounts
RealmConfig
andTokenOwnerRecord
are translated and/or extended if necessary to a new size to accommodate the additional data.This change makes the payer account mandatory for
SetRealmConfig
. It was treated in the api as mandatory before but the code was fetching it conditionally which is no longer the case.