From ef72e567385f543f6407a0fcf47b4769145929d7 Mon Sep 17 00:00:00 2001 From: Sebastian Bor Date: Wed, 20 Oct 2021 20:48:02 +0100 Subject: [PATCH] Governance: Use max_voting_time to determine proposal final state (#2529) * feat: do not allow withdrawing votes before proposal is finalised * feat: do not allow proposal cancellation before proposal is finalised * chore: update comments and add test Co-authored-by: Jon Cinque --- governance/program/src/instruction.rs | 3 + .../src/processor/process_cancel_proposal.rs | 12 ++- .../src/processor/process_relinquish_vote.rs | 12 ++- governance/program/src/state/proposal.rs | 60 ++++++++---- .../program/tests/process_cancel_proposal.rs | 94 +++++++++++++++++++ .../program/tests/process_relinquish_vote.rs | 78 +++++++++++++++ governance/program/tests/program_test/mod.rs | 2 + 7 files changed, 237 insertions(+), 24 deletions(-) diff --git a/governance/program/src/instruction.rs b/governance/program/src/instruction.rs index 69461b2f981..8acbf6e05f7 100644 --- a/governance/program/src/instruction.rs +++ b/governance/program/src/instruction.rs @@ -253,6 +253,7 @@ pub enum GovernanceInstruction { /// 1. `[writable]` TokenOwnerRecord account of the Proposal owner /// 2. `[signer]` Governance Authority (Token Owner or Governance Delegate) /// 3. `[]` Clock sysvar + /// 4. `[]` Governance account CancelProposal, /// Signs off Proposal indicating the Signatory approves the Proposal @@ -1064,12 +1065,14 @@ pub fn cancel_proposal( proposal: &Pubkey, proposal_owner_record: &Pubkey, governance_authority: &Pubkey, + governance: &Pubkey, ) -> Instruction { let accounts = vec![ AccountMeta::new(*proposal, false), AccountMeta::new(*proposal_owner_record, false), AccountMeta::new_readonly(*governance_authority, true), AccountMeta::new_readonly(sysvar::clock::id(), false), + AccountMeta::new_readonly(*governance, false), ]; let instruction = GovernanceInstruction::CancelProposal {}; diff --git a/governance/program/src/processor/process_cancel_proposal.rs b/governance/program/src/processor/process_cancel_proposal.rs index 779265137d3..ae4879e0150 100644 --- a/governance/program/src/processor/process_cancel_proposal.rs +++ b/governance/program/src/processor/process_cancel_proposal.rs @@ -10,7 +10,8 @@ use solana_program::{ }; use crate::state::{ - enums::ProposalState, proposal::get_proposal_data, + enums::ProposalState, governance::get_governance_data, + proposal::get_proposal_data_for_governance, token_owner_record::get_token_owner_record_data_for_proposal_owner, }; @@ -25,8 +26,13 @@ pub fn process_cancel_proposal(program_id: &Pubkey, accounts: &[AccountInfo]) -> let clock_info = next_account_info(account_info_iter)?; // 3 let clock = Clock::from_account_info(clock_info)?; - let mut proposal_data = get_proposal_data(program_id, proposal_info)?; - proposal_data.assert_can_cancel()?; + let governance_info = next_account_info(account_info_iter)?; // 4 + + let governance_data = get_governance_data(program_id, governance_info)?; + + let mut proposal_data = + get_proposal_data_for_governance(program_id, proposal_info, governance_info.key)?; + proposal_data.assert_can_cancel(&governance_data.config, clock.unix_timestamp)?; let mut proposal_owner_record_data = get_token_owner_record_data_for_proposal_owner( program_id, diff --git a/governance/program/src/processor/process_relinquish_vote.rs b/governance/program/src/processor/process_relinquish_vote.rs index ab81363e209..2d387f48dac 100644 --- a/governance/program/src/processor/process_relinquish_vote.rs +++ b/governance/program/src/processor/process_relinquish_vote.rs @@ -2,8 +2,10 @@ use solana_program::{ account_info::{next_account_info, AccountInfo}, + clock::Clock, entrypoint::ProgramResult, pubkey::Pubkey, + sysvar::Sysvar, }; use spl_governance_tools::account::dispose_account; @@ -52,8 +54,14 @@ pub fn process_relinquish_vote(program_id: &Pubkey, accounts: &[AccountInfo]) -> )?; vote_record_data.assert_can_relinquish_vote()?; - // If the Proposal is still being voted on then the token owner vote won't count towards the outcome - if proposal_data.state == ProposalState::Voting { + let clock = Clock::get()?; + + // If the Proposal is still being voted on then the token owner vote will be withdrawn and it won't count towards the vote outcome + // Note: If there is no tipping point the proposal can be still in Voting state but already past the configured max_voting_time + // It means it awaits manual finalization (FinalizeVote) and it should no longer be possible to withdraw the vote and we only release the tokens + if proposal_data.state == ProposalState::Voting + && !proposal_data.has_vote_time_ended(&governance_data.config, clock.unix_timestamp) + { let governance_authority_info = next_account_info(account_info_iter)?; // 5 let beneficiary_info = next_account_info(account_info_iter)?; // 6 diff --git a/governance/program/src/state/proposal.rs b/governance/program/src/state/proposal.rs index 48a20b271cf..1fb2526f097 100644 --- a/governance/program/src/state/proposal.rs +++ b/governance/program/src/state/proposal.rs @@ -168,19 +168,27 @@ impl Proposal { .map_err(|_| GovernanceError::InvalidStateCannotVote)?; // Check if we are still within the configured max_voting_time period - if self - .voting_at - .unwrap() - .checked_add(config.max_voting_time as i64) - .unwrap() - < current_unix_timestamp - { + if self.has_vote_time_ended(config, current_unix_timestamp) { return Err(GovernanceError::ProposalVotingTimeExpired.into()); } Ok(()) } + /// Checks whether the voting time has ended for the proposal + pub fn has_vote_time_ended( + &self, + config: &GovernanceConfig, + current_unix_timestamp: UnixTimestamp, + ) -> bool { + // Check if we passed vote_end_time determined by the configured max_voting_time period + self.voting_at + .unwrap() + .checked_add(config.max_voting_time as i64) + .unwrap() + < current_unix_timestamp + } + /// Checks if Proposal can be finalized pub fn assert_can_finalize_vote( &self, @@ -190,14 +198,8 @@ impl Proposal { self.assert_is_voting_state() .map_err(|_| GovernanceError::InvalidStateCannotFinalize)?; - // Check if we passed the configured max_voting_time period yet - if self - .voting_at - .unwrap() - .checked_add(config.max_voting_time as i64) - .unwrap() - >= current_unix_timestamp - { + // We can only finalize the vote after the configured max_voting_time has expired and vote time ended + if !self.has_vote_time_ended(config, current_unix_timestamp) { return Err(GovernanceError::CannotFinalizeVotingInProgress.into()); } @@ -344,9 +346,21 @@ impl Proposal { } /// Checks if Proposal can be canceled in the given state - pub fn assert_can_cancel(&self) -> Result<(), ProgramError> { + pub fn assert_can_cancel( + &self, + config: &GovernanceConfig, + current_unix_timestamp: UnixTimestamp, + ) -> Result<(), ProgramError> { match self.state { - ProposalState::Draft | ProposalState::SigningOff | ProposalState::Voting => Ok(()), + ProposalState::Draft | ProposalState::SigningOff => Ok(()), + ProposalState::Voting => { + // Note: If there is no tipping point the proposal can be still in Voting state but already past the configured max_voting_time + // In that case we treat the proposal as finalized and it's no longer allowed to be canceled + if self.has_vote_time_ended(config, current_unix_timestamp) { + return Err(GovernanceError::ProposalVotingTimeExpired.into()); + } + Ok(()) + } ProposalState::Executing | ProposalState::ExecutingWithErrors | ProposalState::Completed @@ -700,9 +714,15 @@ mod test { #[test] fn test_assert_can_cancel(state in cancellable_states()) { + // Arrange let mut proposal = create_test_proposal(); + let governance_config = create_test_governance_config(); + + // Act proposal.state = state; - proposal.assert_can_cancel().unwrap(); + + // Assert + proposal.assert_can_cancel(&governance_config,1).unwrap(); } @@ -726,8 +746,10 @@ mod test { let mut proposal = create_test_proposal(); proposal.state = state; + let governance_config = create_test_governance_config(); + // Act - let err = proposal.assert_can_cancel().err().unwrap(); + let err = proposal.assert_can_cancel(&governance_config,1).err().unwrap(); // Assert assert_eq!(err, GovernanceError::InvalidStateCannotCancelProposal.into()); diff --git a/governance/program/tests/process_cancel_proposal.rs b/governance/program/tests/process_cancel_proposal.rs index d28fadda8a8..2029609d6b1 100644 --- a/governance/program/tests/process_cancel_proposal.rs +++ b/governance/program/tests/process_cancel_proposal.rs @@ -152,3 +152,97 @@ async fn test_cancel_proposal_with_owner_or_delegate_must_sign_error() { GovernanceError::GoverningTokenOwnerOrDelegateMustSign.into() ); } + +#[tokio::test] +async fn test_cancel_proposal_with_vote_time_expired_error() { + // Arrange + let mut governance_test = GovernanceProgramTest::start_new().await; + + let realm_cookie = governance_test.with_realm().await; + let governed_account_cookie = governance_test.with_governed_account().await; + + let token_owner_record_cookie = governance_test + .with_community_token_deposit(&realm_cookie) + .await + .unwrap(); + + let mut account_governance_cookie = governance_test + .with_account_governance( + &realm_cookie, + &governed_account_cookie, + &token_owner_record_cookie, + ) + .await + .unwrap(); + + let clock = governance_test.bench.get_clock().await; + + let proposal_cookie = governance_test + .with_signed_off_proposal(&token_owner_record_cookie, &mut account_governance_cookie) + .await + .unwrap(); + + // Advance timestamp past max_voting_time + governance_test + .advance_clock_past_timestamp( + account_governance_cookie.account.config.max_voting_time as i64 + clock.unix_timestamp, + ) + .await; + + // Act + + let err = governance_test + .cancel_proposal(&proposal_cookie, &token_owner_record_cookie) + .await + .err() + .unwrap(); + + // Assert + + assert_eq!(err, GovernanceError::ProposalVotingTimeExpired.into()); +} + +#[tokio::test] +async fn test_cancel_proposal_in_voting_state() { + // Arrange + let mut governance_test = GovernanceProgramTest::start_new().await; + + let realm_cookie = governance_test.with_realm().await; + let governed_account_cookie = governance_test.with_governed_account().await; + + let token_owner_record_cookie = governance_test + .with_community_token_deposit(&realm_cookie) + .await + .unwrap(); + + let mut account_governance_cookie = governance_test + .with_account_governance( + &realm_cookie, + &governed_account_cookie, + &token_owner_record_cookie, + ) + .await + .unwrap(); + + let proposal_cookie = governance_test + .with_signed_off_proposal(&token_owner_record_cookie, &mut account_governance_cookie) + .await + .unwrap(); + + governance_test.advance_clock().await; + + // Act + + governance_test + .cancel_proposal(&proposal_cookie, &token_owner_record_cookie) + .await + .unwrap(); + + // Assert + + let proposal_account = governance_test + .get_proposal_account(&proposal_cookie.address) + .await; + + assert_eq!(ProposalState::Cancelled, proposal_account.state); +} diff --git a/governance/program/tests/process_relinquish_vote.rs b/governance/program/tests/process_relinquish_vote.rs index b9fe6555ba5..6d5d9c92158 100644 --- a/governance/program/tests/process_relinquish_vote.rs +++ b/governance/program/tests/process_relinquish_vote.rs @@ -439,3 +439,81 @@ async fn test_relinquish_vote_with_already_relinquished_error() { assert_eq!(err, GovernanceError::VoteAlreadyRelinquished.into()); } + +#[tokio::test] +async fn test_relinquish_proposal_in_voting_state_after_vote_time_ended() { + // Arrange + let mut governance_test = GovernanceProgramTest::start_new().await; + + let realm_cookie = governance_test.with_realm().await; + let governed_account_cookie = governance_test.with_governed_account().await; + + // Deposit 100 tokens + let token_owner_record_cookie = governance_test + .with_community_token_deposit(&realm_cookie) + .await + .unwrap(); + + // Add 200 tokens (total 300) to prevent the vote being tipped + governance_test + .mint_community_tokens(&realm_cookie, 200) + .await; + + let mut account_governance_cookie = governance_test + .with_account_governance( + &realm_cookie, + &governed_account_cookie, + &token_owner_record_cookie, + ) + .await + .unwrap(); + + let proposal_cookie = governance_test + .with_signed_off_proposal(&token_owner_record_cookie, &mut account_governance_cookie) + .await + .unwrap(); + + let clock = governance_test.bench.get_clock().await; + + let mut vote_record_cookie = governance_test + .with_cast_vote(&proposal_cookie, &token_owner_record_cookie, Vote::Yes) + .await + .unwrap(); + + // Advance timestamp past max_voting_time + governance_test + .advance_clock_past_timestamp( + account_governance_cookie.account.config.max_voting_time as i64 + clock.unix_timestamp, + ) + .await; + + // Act + governance_test + .relinquish_vote(&proposal_cookie, &token_owner_record_cookie) + .await + .unwrap(); + + // Assert + + let proposal_account = governance_test + .get_proposal_account(&proposal_cookie.address) + .await; + + // Proposal should be still in voting state but the vote count should not change + assert_eq!(100, proposal_account.yes_votes_count); + assert_eq!(ProposalState::Voting, proposal_account.state); + + let token_owner_record = governance_test + .get_token_owner_record_account(&token_owner_record_cookie.address) + .await; + + assert_eq!(0, token_owner_record.unrelinquished_votes_count); + assert_eq!(1, token_owner_record.total_votes_count); + + let vote_record_account = governance_test + .get_vote_record_account(&vote_record_cookie.address) + .await; + + vote_record_cookie.account.is_relinquished = true; + assert_eq!(vote_record_cookie.account, vote_record_account); +} diff --git a/governance/program/tests/program_test/mod.rs b/governance/program/tests/program_test/mod.rs index 88296b23680..7af779f9e57 100644 --- a/governance/program/tests/program_test/mod.rs +++ b/governance/program/tests/program_test/mod.rs @@ -354,6 +354,7 @@ impl GovernanceProgramTest { } } + // Creates TokenOwner which owns 100 community tokens and deposits them into the given Realm #[allow(dead_code)] pub async fn with_community_token_deposit( &mut self, @@ -1668,6 +1669,7 @@ impl GovernanceProgramTest { &proposal_cookie.address, &token_owner_record_cookie.address, &token_owner_record_cookie.token_owner.pubkey(), + &proposal_cookie.account.governance, ); self.bench