Skip to content

Commit

Permalink
cleanup: cleanup: governance
Browse files Browse the repository at this point in the history
  • Loading branch information
buffalojoec committed Nov 8, 2023
1 parent faf317d commit b0f135a
Show file tree
Hide file tree
Showing 16 changed files with 218 additions and 181 deletions.
10 changes: 5 additions & 5 deletions governance/addin-api/src/max_voter_weight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ use {
#[derive(Clone, Debug, PartialEq, Eq, BorshDeserialize, BorshSerialize, BorshSchema)]
pub struct MaxVoterWeightRecord {
/// VoterWeightRecord discriminator
/// sha256("account:MaxVoterWeightRecord")[..8] Note: The discriminator
/// size must match the addin implementing program discriminator size to
/// ensure it's stored in the private space of the account data and it's
/// unique
/// sha256("account:MaxVoterWeightRecord")[..8]
/// Note: The discriminator size must match the addin implementing program
/// discriminator size to ensure it's stored in the private space of the
/// account data and it's unique
pub account_discriminator: [u8; 8],

/// The Realm the MaxVoterWeightRecord belongs to
Expand All @@ -35,7 +35,7 @@ pub struct MaxVoterWeightRecord {
/// The slot when the max voting weight expires
/// It should be set to None if the weight never expires
/// If the max vote weight decays with time, for example for time locked
/// based weights, then the expiry must be set As a pattern Revise
/// based weights, then the expiry must be set. As a pattern Revise
/// instruction to update the max weight should be invoked before governance
/// instruction within the same transaction and the expiry set to the
/// current slot to provide up to date weight
Expand Down
8 changes: 4 additions & 4 deletions governance/addin-api/src/voter_weight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,24 +59,24 @@ pub struct VoterWeightRecord {
/// The slot when the voting weight expires
/// It should be set to None if the weight never expires
/// If the voter weight decays with time, for example for time locked based
/// weights, then the expiry must be set As a common pattern Revise
/// weights, then the expiry must be set. As a common pattern Revise
/// instruction to update the weight should be invoked before governance
/// instruction within the same transaction and the expiry set to the
/// current slot to provide up to date weight
pub voter_weight_expiry: Option<Slot>,

/// The governance action the voter's weight pertains to
/// It allows to provided voter's weight specific to the particular action
/// the weight is evaluated for When the action is provided then the
/// the weight is evaluated for. When the action is provided then the
/// governance program asserts the executing action is the same as specified
/// by the addin
pub weight_action: Option<VoterWeightAction>,

/// The target the voter's weight action pertains to
/// It allows to provided voter's weight specific to the target the weight
/// is evaluated for For example when addin supplies weight to vote on a
/// is evaluated for. For example when addin supplies weight to vote on a
/// particular proposal then it must specify the proposal as the action
/// target When the target is provided then the governance program
/// target. When the target is provided then the governance program
/// asserts the target is the same as specified by the addin
pub weight_action_target: Option<Pubkey>,

Expand Down
5 changes: 3 additions & 2 deletions governance/program/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ use {
};

/// Errors that may be returned by the Governance program
// Start Governance custom errors from 500 to avoid conflicts with programs
// invoked via CPI
#[derive(Clone, Debug, Eq, Error, FromPrimitive, PartialEq)]
pub enum GovernanceError {
/// Invalid instruction passed to program
#[error("Invalid instruction passed to program")]
InvalidInstruction = 500, /* Start Governance custom errors from 500 to avoid conflicts
* with programs invoked via CPI */
InvalidInstruction = 500,

/// Realm with the given name and governing mints already exists
#[error("Realm with the given name and governing mints already exists")]
Expand Down
225 changes: 122 additions & 103 deletions governance/program/src/instruction.rs

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ pub fn process_create_mint_governance(
// If the mint has freeze_authority then transfer it as well
let mint_data = Mint::unpack(&governed_mint_info.data.borrow())?;
// Note: The code assumes mint_authority==freeze_authority
// If this is not the case then the caller should set freeze_authority
// If this is not the case then the caller should set freeze_authority
// accordingly before making the transfer
if mint_data.freeze_authority.is_some() {
set_spl_token_account_authority(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ pub fn process_create_token_governance(
// If the token account has close_authority then transfer it as well
let token_account_data = Account::unpack(&governed_token_info.data.borrow())?;
// Note: The code assumes owner==close_authority
// If this is not the case then the caller should set close_authority
// If this is not the case then the caller should set close_authority
// accordingly before making the transfer
if token_account_data.close_authority.is_some() {
set_spl_token_account_authority(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub fn process_execute_transaction(program_id: &Pubkey, accounts: &[AccountInfo]
.map(Instruction::from);

// In the current implementation accounts for all instructions are passed to
// each instruction invocation This is an overhead but shouldn't be a
// each instruction invocation. This is an overhead but shouldn't be a
// showstopper because if we can invoke the parent instruction with that many
// accounts then we should also be able to invoke all the nested ones
// TODO: Optimize the invocation to split the provided accounts for each
Expand Down
53 changes: 29 additions & 24 deletions governance/program/src/state/proposal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,10 @@ pub struct ProposalOption {
pub enum VoteType {
/// Single choice vote with mutually exclusive choices
/// In the SingeChoice mode there can ever be a single winner
/// If multiple options score the same highest vote then the Proposal is not
/// resolved and considered as Failed Note: Yes/No vote is a single
/// choice (Yes) vote with the deny option (No)
/// If multiple options score the same highest vote then the Proposal is
/// not resolved and considered as Failed.
/// Note: Yes/No vote is a single choice (Yes) vote with the deny
/// option (No)
SingleChoice,

/// Multiple options can be selected with up to max_voter_options per voter
Expand Down Expand Up @@ -122,9 +123,10 @@ pub enum MultiChoiceType {
/// approved option
FullWeight,

/// Multiple options can be approved with weight allocated proportionally to
/// the percentage of the total weight The full weight has to be voted
/// among the approved options, i.e., 100% of the weight has to be allocated
/// Multiple options can be approved with weight allocated proportionally
/// to the percentage of the total weight.
/// The full weight has to be voted among the approved options, i.e.,
/// 100% of the weight has to be allocated
Weighted,
}

Expand Down Expand Up @@ -215,21 +217,23 @@ pub struct ProposalV2 {
pub execution_flags: InstructionExecutionFlags,

/// The max vote weight for the Governing Token mint at the time Proposal
/// was decided It's used to show correct vote results for historical
/// proposals in cases when the mint supply or max weight source changed
/// after vote was completed.
/// was decided.
/// It's used to show correct vote results for historical proposals in
/// cases when the mint supply or max weight source changed after vote was
/// completed.
pub max_vote_weight: Option<u64>,

/// Max voting time for the proposal if different from parent Governance
/// (only higher value possible) Note: This field is not used in the
/// current version
/// (only higher value possible).
/// Note: This field is not used in the current version
pub max_voting_time: Option<u32>,

/// The vote threshold at the time Proposal was decided
/// It's used to show correct vote results for historical proposals in cases
/// when the threshold was changed for governance config after vote was
/// completed. TODO: Use this field to override the threshold from
/// parent Governance (only higher value possible)
/// completed.
/// TODO: Use this field to override the threshold from parent Governance
/// (only higher value possible)
pub vote_threshold: Option<VoteThreshold>,

/// Reserved space for future versions
Expand Down Expand Up @@ -682,8 +686,8 @@ impl ProposalV2 {
}

/// Checks if vote can be tipped and automatically transitioned to
/// Succeeded, Defeated or Vetoed state If yes then Some(ProposalState)
/// is returned and None otherwise
/// Succeeded, Defeated or Vetoed state.
/// If yes then Some(ProposalState) is returned and None otherwise
pub fn try_get_tipped_vote_state(
&mut self,
max_voter_weight: u64,
Expand All @@ -705,21 +709,22 @@ impl ProposalV2 {
}

/// Checks if Electorate vote can be tipped and automatically transitioned
/// to Succeeded or Defeated state If yes then Some(ProposalState) is
/// returned and None otherwise
/// to Succeeded or Defeated state.
/// If yes then Some(ProposalState) is returned and None otherwise
fn try_get_tipped_electorate_vote_state(
&mut self,
max_voter_weight: u64,
vote_tipping: &VoteTipping,
min_vote_threshold_weight: u64,
) -> Option<ProposalState> {
// Vote tipping is currently supported for SingleChoice votes with single Yes
// and No (rejection) options only Note: Tipping for multiple options
// (single choice and multiple choices) should be possible but it requires a
// great deal of considerations and I decided to fight it another
// day
// Vote tipping is currently supported for SingleChoice votes with
// single Yes and No (rejection) options only.
// Note: Tipping for multiple options (single choice and multiple
// choices) should be possible but it requires a great deal of
// considerations and I decided to fight it another day
if self.vote_type != VoteType::SingleChoice
// Tipping should not be allowed for opinion only proposals (surveys without rejection) to allow everybody's voice to be heard
// Tipping should not be allowed for opinion only proposals (surveys
// without rejection) to allow everybody's voice to be heard
|| self.deny_vote_weight.is_none()
|| self.options.len() != 1
{
Expand Down Expand Up @@ -935,7 +940,7 @@ impl ProposalV2 {
max_winning_options: _,
} => {
// Calculate the total percentage for all choices for weighted
// choice vote The total must add up
// choice vote. The total must add up
// to exactly 100%
total_choice_weight_percentage = total_choice_weight_percentage
.checked_add(choice.weight_percentage)
Expand Down
7 changes: 4 additions & 3 deletions governance/program/src/state/realm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,10 @@ pub enum SetRealmAuthorityAction {

/// Sets realm authority and checks the new new authority is one of the
/// realm's governances
// Note: This is not a security feature because governance creation is only gated with
// min_community_weight_to_create_governance The check is done to prevent scenarios
// where the authority could be accidentally set to a wrong or none existing account
// Note: This is not a security feature because governance creation is only
// gated with min_community_weight_to_create_governance.
// The check is done to prevent scenarios where the authority could be
// accidentally set to a wrong or none existing account.
SetChecked,

/// Removes realm authority
Expand Down
15 changes: 9 additions & 6 deletions governance/program/src/state/realm_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,20 @@ use {
#[derive(Clone, Debug, PartialEq, Eq, BorshDeserialize, BorshSerialize, BorshSchema)]
pub enum GoverningTokenType {
/// Liquid token is a token which is fully liquid and the token owner
/// retains full authority over it Deposit - Yes
/// retains full authority over it.
/// Deposit - Yes
/// Withdraw - Yes
/// Revoke - No, Realm authority cannot revoke liquid tokens
Liquid,

/// Membership token is a token controlled by Realm authority
/// Deposit - Yes, membership tokens can be deposited to gain governance
/// power The membership tokens are conventionally minted into
/// the holding account to keep them out of members possession
/// power.
/// The membership tokens are conventionally minted into the holding
/// account to keep them out of members possession.
/// Withdraw - No, after membership tokens are deposited they are no longer
/// transferable and can't be withdrawn Revoke - Yes, Realm authority
/// can Revoke (burn) membership tokens
/// transferable and can't be withdrawn.
/// Revoke - Yes, Realm authority can Revoke (burn) membership tokens.
Membership,

/// Dormant token is a token which is only a placeholder and its deposits
Expand All @@ -55,7 +57,8 @@ pub enum GoverningTokenType {
/// Deposit - No, dormant tokens can't be deposited into the Realm
/// Withdraw - Yes, tokens can still be withdrawn from Realm to support
/// scenario where the config is changed while some tokens are still
/// deposited Revoke - No, Realm authority cannot revoke dormant tokens
/// deposited.
/// Revoke - No, Realm authority cannot revoke dormant tokens
Dormant,
}

Expand Down
5 changes: 3 additions & 2 deletions governance/program/src/state/vote_record.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ use {

/// Voter choice for a proposal option
/// In the current version only 1) Single choice, 2) Multiple choices proposals
/// and 3) Weighted voting are supported In the future versions we can add
/// support for 1) Quadratic voting and 2) Ranked choice voting
/// and 3) Weighted voting are supported.
/// In the future versions we can add support for 1) Quadratic voting and
/// 2) Ranked choice voting
#[derive(Clone, Debug, PartialEq, Eq, BorshDeserialize, BorshSerialize, BorshSchema)]
pub struct VoteChoice {
/// The rank given to the choice by voter
Expand Down
9 changes: 7 additions & 2 deletions governance/program/src/tools/spl_token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,13 @@ pub fn assert_is_valid_spl_token_account(account_info: &AccountInfo) -> Result<(
return Err(GovernanceError::SplTokenInvalidTokenAccountData.into());
}

// TokeAccount layout: mint(32), owner(32), amount(8), delegate(36), state(1),
// ...
// TokenAccount layout:
// mint(32)
// owner(32)
// amount(8)
// delegate(36)
// state(1)
// ...
let data = account_info.try_borrow_data()?;
let state = array_ref![data, 108, 1];

Expand Down
24 changes: 12 additions & 12 deletions governance/program/tests/process_refund_proposal_deposit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,18 +306,18 @@ async fn test_refund_proposal_deposit_with_invalid_proposal_deposit_account_erro
.unwrap();

// Act
let err =
governance_test
.refund_proposal_deposit_using_instruction(
&proposal_cookie,
|i| {
i.accounts[1].pubkey = proposal_cookie.address; // Try to drain the Proposal account
},
None,
)
.await
.err()
.unwrap();
let err = governance_test
.refund_proposal_deposit_using_instruction(
&proposal_cookie,
|i| {
// Try to drain the Proposal account
i.accounts[1].pubkey = proposal_cookie.address;
},
None,
)
.await
.err()
.unwrap();

// Assert

Expand Down
17 changes: 10 additions & 7 deletions governance/program/tests/program_test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,13 @@ impl GovernanceProgramTest {
use_voter_weight_addin: bool,
use_max_voter_weight_addin: bool,
) -> Self {
// We only ensure the addin mock program is built but it doesn't detect changes
// If the addin is changed then it needs to be manually rebuilt
// Note: The crate of the mock is built when spl-governance is built but we also
// need spl_governance_addin_mock.so And we can't use build.rs
// script because cargo build-sbf hangs when executed from the script
// We only ensure the addin mock program is built but it doesn't detect
// changes.
// If the addin is changed then it needs to be manually rebuilt.
// Note: The crate of the mock is built when spl-governance is built
// but we also need spl_governance_addin_mock.so.
// And we can't use build.rs script because cargo build-sbf hangs when
// executed from the script.
ensure_addin_mock_is_built();

Self::start_impl(use_voter_weight_addin, use_max_voter_weight_addin).await
Expand Down Expand Up @@ -2782,8 +2784,9 @@ impl GovernanceProgramTest {
.iter()
.map(|a| AccountMeta {
pubkey: a.pubkey,
is_signer: false, /* Remove signer since the Governance account PDA will be
* signing the instruction for us */
// Remove signer since the Governance account PDA will be
// signing the instruction for us
is_signer: false,
is_writable: a.is_writable,
})
.collect();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,8 @@ async fn test_tip_vote_with_max_voter_weight_addin_and_max_below_total_cast_vote
.await;

assert_eq!(proposal_account.state, ProposalState::Succeeded);
assert_eq!(proposal_account.max_vote_weight, Some(100)); // Adjusted max
// based on cast
// votes
// Adjusted max based on cast votes
assert_eq!(proposal_account.max_vote_weight, Some(100));
}

#[tokio::test]
Expand Down Expand Up @@ -360,9 +359,8 @@ async fn test_finalize_vote_with_max_voter_weight_addin_and_max_below_total_cast
.await;

assert_eq!(proposal_account.state, ProposalState::Succeeded);
assert_eq!(proposal_account.max_vote_weight, Some(100)); // Adjusted max
// based on cast
// votes
// Adjusted max based on cast votes
assert_eq!(proposal_account.max_vote_weight, Some(100));
}

#[tokio::test]
Expand Down
5 changes: 3 additions & 2 deletions governance/tools/src/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,9 @@ pub fn create_and_serialize_account_with_owner_signed<'a, T: BorshSerialize + Ac
let total_lamports = rent_exempt_lamports.checked_add(extra_lamports).unwrap();

// If the account has some lamports already it can't be created using
// create_account instruction Anybody can send lamports to a PDA and by
// doing so create the account and perform DoS attack by blocking create_account
// create_account instruction.
// Anybody can send lamports to a PDA and by doing so create the account
// and perform DoS attack by blocking create_account
if account_info.lamports() > 0 {
let top_up_lamports = total_lamports.saturating_sub(account_info.lamports());

Expand Down

0 comments on commit b0f135a

Please sign in to comment.