Governance: Use max_voting_time to determine proposal final state (so…

…lana-labs#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 <[email protected]>

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 {};

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,

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
 

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());

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);
+}

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);
+}

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