Governance: FlagInstructionError instruction (#2099)

* feat: implement FlagInstructionError

* fix: ensure instruction can still be executed after being flagged with error

* chore: review cleanup

* chore: bump version

* chore: code review updates

Co-authored-by: Jon Cinque <[email protected]>

Co-authored-by: Jon Cinque <[email protected]>

Author: SebastianBor

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "spl-governance"
-version = "1.0.1"
+version = "1.0.3"
 description = "Solana Program Library Governance Program"
 authors = ["Solana Maintainers <[email protected]>"]
 repository = "https://github.com/solana-labs/solana-program-library"

governance/program/Cargo.toml

@@ -275,6 +275,10 @@ pub enum GovernanceError {
     /// Governance PDA must sign
     #[error("Governance PDA must sign")]
     GovernancePdaMustSign,
+
+    /// Instruction already flagged with error
+    #[error("Instruction already flagged with error")]
+    InstructionAlreadyFlaggedWithError,
 }
 
 impl PrintProgramError for GovernanceError {

governance/program/src/error.rs

@@ -347,6 +347,18 @@ pub enum GovernanceInstruction {
         /// New governance config
         config: GovernanceConfig,
     },
+
+    /// Flags an instruction and its parent Proposal with error status
+    /// It can be used by Proposal owner in case the instruction is permanently broken and can't be executed
+    /// Note: This instruction is a workaround because currently it's not possible to catch errors from CPI calls
+    ///       and the Governance program has no way to know when instruction failed and flag it automatically
+    ///
+    ///   0. `[writable]` Proposal account
+    ///   1. `[]` TokenOwnerRecord account for Proposal owner
+    ///   2. `[signer]` Governance Authority (Token Owner or Governance Delegate)    
+    ///   3. `[writable]` ProposalInstruction account to flag
+    ///   4. `[]` Clock sysvar
+    FlagInstructionError,
 }
 
 /// Creates CreateRealm instruction
@@ -1031,3 +1043,29 @@ pub fn set_governance_config(
         data: instruction.try_to_vec().unwrap(),
     }
 }
+
+/// Creates FlagInstructionError instruction
+pub fn flag_instruction_error(
+    program_id: &Pubkey,
+    // Accounts
+    proposal: &Pubkey,
+    token_owner_record: &Pubkey,
+    governance_authority: &Pubkey,
+    proposal_instruction: &Pubkey,
+) -> Instruction {
+    let accounts = vec![
+        AccountMeta::new(*proposal, false),
+        AccountMeta::new_readonly(*token_owner_record, false),
+        AccountMeta::new_readonly(*governance_authority, true),
+        AccountMeta::new(*proposal_instruction, false),
+        AccountMeta::new_readonly(sysvar::clock::id(), false),
+    ];
+
+    let instruction = GovernanceInstruction::FlagInstructionError {};
+
+    Instruction {
+        program_id: *program_id,
+        accounts,
+        data: instruction.try_to_vec().unwrap(),
+    }
+}

governance/program/src/instruction.rs

@@ -12,6 +12,7 @@ mod process_create_token_governance;
 mod process_deposit_governing_tokens;
 mod process_execute_instruction;
 mod process_finalize_vote;
+mod process_flag_instruction_error;
 mod process_insert_instruction;
 mod process_relinquish_vote;
 mod process_remove_instruction;
@@ -36,6 +37,7 @@ use process_create_token_governance::*;
 use process_deposit_governing_tokens::*;
 use process_execute_instruction::*;
 use process_finalize_vote::*;
+use process_flag_instruction_error::*;
 use process_insert_instruction::*;
 use process_relinquish_vote::*;
 use process_remove_instruction::*;
@@ -160,5 +162,9 @@ pub fn process_instruction(
         GovernanceInstruction::SetGovernanceConfig { config } => {
             process_set_governance_config(program_id, accounts, config)
         }
+
+        GovernanceInstruction::FlagInstructionError {} => {
+            process_flag_instruction_error(program_id, accounts)
+        }
     }
 }

governance/program/src/processor/mod.rs

@@ -70,7 +70,10 @@ pub fn process_execute_instruction(program_id: &Pubkey, accounts: &[AccountInfo]
         .checked_add(1)
         .unwrap();
 
-    if proposal_data.state == ProposalState::Executing
+    // Checking for Executing and ExecutingWithErrors states because instruction can still be executed after being flagged with error
+    // The check for instructions_executed_count ensures Proposal can't be transitioned to Completed state from ExecutingWithErrors
+    if (proposal_data.state == ProposalState::Executing
+        || proposal_data.state == ProposalState::ExecutingWithErrors)
         && proposal_data.instructions_executed_count == proposal_data.instructions_count
     {
         proposal_data.closed_at = Some(clock.unix_timestamp);

governance/program/src/processor/process_execute_instruction.rs

@@ -0,0 +1,67 @@
+//! Program state processor
+
+use borsh::BorshSerialize;
+use solana_program::{
+    account_info::{next_account_info, AccountInfo},
+    clock::Clock,
+    entrypoint::ProgramResult,
+    pubkey::Pubkey,
+    sysvar::Sysvar,
+};
+
+use crate::state::{
+    enums::{InstructionExecutionStatus, ProposalState},
+    proposal::get_proposal_data,
+    proposal_instruction::get_proposal_instruction_data_for_proposal,
+    token_owner_record::get_token_owner_record_data_for_proposal_owner,
+};
+
+/// Processes FlagInstructionError instruction
+pub fn process_flag_instruction_error(
+    program_id: &Pubkey,
+    accounts: &[AccountInfo],
+) -> ProgramResult {
+    let account_info_iter = &mut accounts.iter();
+
+    let proposal_info = next_account_info(account_info_iter)?; // 0
+    let token_owner_record_info = next_account_info(account_info_iter)?; // 1
+    let governance_authority_info = next_account_info(account_info_iter)?; // 2
+
+    let proposal_instruction_info = next_account_info(account_info_iter)?; // 3
+
+    let clock_info = next_account_info(account_info_iter)?; // 4
+    let clock = Clock::from_account_info(clock_info)?;
+
+    let mut proposal_data = get_proposal_data(program_id, proposal_info)?;
+
+    let mut proposal_instruction_data = get_proposal_instruction_data_for_proposal(
+        program_id,
+        proposal_instruction_info,
+        proposal_info.key,
+    )?;
+
+    proposal_data
+        .assert_can_flag_instruction_error(&proposal_instruction_data, clock.unix_timestamp)?;
+
+    let token_owner_record_data = get_token_owner_record_data_for_proposal_owner(
+        program_id,
+        token_owner_record_info,
+        &proposal_data.token_owner_record,
+    )?;
+
+    token_owner_record_data.assert_token_owner_or_delegate_is_signer(governance_authority_info)?;
+
+    // If this is the first instruction to be executed then set executing_at timestamp
+    // It indicates when we started executing instructions for the Proposal and the fact we only flag it as error is irrelevant here
+    if proposal_data.state == ProposalState::Succeeded {
+        proposal_data.executing_at = Some(clock.unix_timestamp);
+    }
+
+    proposal_data.state = ProposalState::ExecutingWithErrors;
+    proposal_data.serialize(&mut *proposal_info.data.borrow_mut())?;
+
+    proposal_instruction_data.execution_status = InstructionExecutionStatus::Error;
+    proposal_instruction_data.serialize(&mut *proposal_instruction_info.data.borrow_mut())?;
+
+    Ok(())
+}

governance/program/src/processor/process_flag_instruction_error.rs

@@ -74,7 +74,8 @@ pub enum ProposalState {
     /// Voting ended with success
     Succeeded,
 
-    /// Voting completed and now instructions are being execute. Proposal enter this state when first instruction is executed and leaves when the last instruction is executed
+    /// Voting on Proposal succeeded and now instructions are being executed
+    /// Proposal enter this state when first instruction is executed and leaves when the last instruction is executed
     Executing,
 
     /// Completed
@@ -85,6 +86,10 @@ pub enum ProposalState {
 
     /// Defeated
     Defeated,
+
+    /// Same as Executing but indicates some instructions failed to execute
+    /// Proposal can't be transitioned from ExecutingWithErrors to Completed state
+    ExecutingWithErrors,
 }
 
 impl Default for ProposalState {
@@ -133,8 +138,6 @@ pub enum InstructionExecutionStatus {
     Success,
 
     /// Instruction execution failed
-    /// Note: Error status is not supported yet because when CPI call fails it always terminates parent instruction
-    /// We either have to make it possible to change that behavior or add an instruction to manually set the status
     Error,
 }
 

governance/program/src/state/enums.rs

@@ -10,8 +10,8 @@ use crate::{
     error::GovernanceError,
     state::{
         enums::{
-            GovernanceAccountType, InstructionExecutionFlags, ProposalState,
-            VoteThresholdPercentage,
+            GovernanceAccountType, InstructionExecutionFlags, InstructionExecutionStatus,
+            ProposalState, VoteThresholdPercentage,
         },
         governance::GovernanceConfig,
         proposal_instruction::ProposalInstruction,
@@ -119,6 +119,7 @@ impl Proposal {
         match self.state {
             ProposalState::Draft | ProposalState::SigningOff => Ok(()),
             ProposalState::Executing
+            | ProposalState::ExecutingWithErrors
             | ProposalState::Completed
             | ProposalState::Cancelled
             | ProposalState::Voting
@@ -279,6 +280,7 @@ impl Proposal {
         match self.state {
             ProposalState::Draft | ProposalState::SigningOff | ProposalState::Voting => Ok(()),
             ProposalState::Executing
+            | ProposalState::ExecutingWithErrors
             | ProposalState::Completed
             | ProposalState::Cancelled
             | ProposalState::Succeeded
@@ -301,7 +303,9 @@ impl Proposal {
         current_unix_timestamp: UnixTimestamp,
     ) -> Result<(), ProgramError> {
         match self.state {
-            ProposalState::Succeeded | ProposalState::Executing => {}
+            ProposalState::Succeeded
+            | ProposalState::Executing
+            | ProposalState::ExecutingWithErrors => {}
             ProposalState::Draft
             | ProposalState::SigningOff
             | ProposalState::Completed
@@ -328,6 +332,22 @@ impl Proposal {
 
         Ok(())
     }
+
+    /// Checks if the instruction can be flagged with error for the Proposal in the given state
+    pub fn assert_can_flag_instruction_error(
+        &self,
+        proposal_instruction_data: &ProposalInstruction,
+        current_unix_timestamp: UnixTimestamp,
+    ) -> Result<(), ProgramError> {
+        // Instruction can be flagged for error only when it's eligible for execution
+        self.assert_can_execute_instruction(proposal_instruction_data, current_unix_timestamp)?;
+
+        if proposal_instruction_data.execution_status == InstructionExecutionStatus::Error {
+            return Err(GovernanceError::InstructionAlreadyFlaggedWithError.into());
+        }
+
+        Ok(())
+    }
 }
 
 /// Converts threshold in percentages to actual vote count
@@ -511,6 +531,7 @@ mod test {
             Just(ProposalState::Voting),
             Just(ProposalState::Succeeded),
             Just(ProposalState::Executing),
+            Just(ProposalState::ExecutingWithErrors),
             Just(ProposalState::Completed),
             Just(ProposalState::Cancelled),
             Just(ProposalState::Defeated),
@@ -551,6 +572,7 @@ mod test {
             Just(ProposalState::Voting),
             Just(ProposalState::Succeeded),
             Just(ProposalState::Executing),
+            Just(ProposalState::ExecutingWithErrors),
             Just(ProposalState::Completed),
             Just(ProposalState::Cancelled),
             Just(ProposalState::Defeated),
@@ -596,6 +618,7 @@ mod test {
         prop_oneof![
             Just(ProposalState::Succeeded),
             Just(ProposalState::Executing),
+            Just(ProposalState::ExecutingWithErrors),
             Just(ProposalState::Completed),
             Just(ProposalState::Cancelled),
             Just(ProposalState::Defeated),

governance/program/src/state/proposal.rs

@@ -101,7 +101,6 @@ pub struct ProposalInstruction {
     pub executed_at: Option<UnixTimestamp>,
 
     /// Instruction execution status
-    /// Note: The field is not used in the current version
     pub execution_status: InstructionExecutionStatus,
 }