Governance: Review feedback (#139)

SebastianBor · web-flow · commit 3e4e2d6d01ce · 2022-12-07T11:15:08.000Z
* chore: Update active_proposal_count comment

* chore: Update account indices in process_create_proposal

* fix: Disallow u8::MAX for deposit_exempt_proposal_count

* chore: Use u64 without option for extra_lamports

* fix: Use explicit version 1 for TokenOwnerRecord migration

* chore: Update math for RealmConfigAccount::get_max_size

* fix: Flag ProposalCoolOffTimeNotSupported as legacy instead of reusing it
diff --git a/governance/program/src/error.rs b/governance/program/src/error.rs
@@ -255,9 +255,9 @@ pub enum GovernanceError {
     #[error("Given VoteWeightSource is not supported")]
     VoteWeightSourceNotSupported, // 559
 
-    /// GoverningTokenMint not allowed to vote
-    #[error("GoverningTokenMint not allowed to vote")]
-    GoverningTokenMintNotAllowedToVote, // 560
+    /// Legacy1
+    #[error("Legacy1")]
+    Legacy1, // 560
 
     /// Governance PDA must sign
     #[error("Governance PDA must sign")]
@@ -450,6 +450,14 @@ pub enum GovernanceError {
     /// Invalid ProposalDeposit account address
     #[error("Invalid ProposalDeposit account address")]
     InvalidProposalDepositAccountAddress, // 608
+
+    /// Invalid deposit_exempt_proposal_count
+    #[error("Invalid deposit_exempt_proposal_count")]
+    InvalidDepositExemptProposalCount, // 609
+
+    /// GoverningTokenMint not allowed to vote
+    #[error("GoverningTokenMint not allowed to vote")]
+    GoverningTokenMintNotAllowedToVote, // 610
 }
 
 impl PrintProgramError for GovernanceError {
diff --git a/governance/program/src/processor/process_add_signatory.rs b/governance/program/src/processor/process_add_signatory.rs
@@ -62,7 +62,7 @@ pub fn process_add_signatory(
         program_id,
         system_info,
         &rent,
-        None,
+        0,
     )?;
 
     proposal_data.signatories_count = proposal_data.signatories_count.checked_add(1).unwrap();
diff --git a/governance/program/src/processor/process_cast_vote.rs b/governance/program/src/processor/process_cast_vote.rs
@@ -214,7 +214,7 @@ pub fn process_cast_vote(
         program_id,
         system_info,
         &rent,
-        None,
+        0,
     )?;
 
     Ok(())
diff --git a/governance/program/src/processor/process_create_governance.rs b/governance/program/src/processor/process_create_governance.rs
@@ -72,7 +72,7 @@ pub fn process_create_governance(
         program_id,
         system_info,
         &rent,
-        None,
+        0,
     )?;
 
     Ok(())
diff --git a/governance/program/src/processor/process_create_mint_governance.rs b/governance/program/src/processor/process_create_mint_governance.rs
@@ -82,7 +82,7 @@ pub fn process_create_mint_governance(
         program_id,
         system_info,
         &rent,
-        None,
+        0,
     )?;
 
     if transfer_mint_authorities {
diff --git a/governance/program/src/processor/process_create_native_treasury.rs b/governance/program/src/processor/process_create_native_treasury.rs
@@ -42,7 +42,7 @@ pub fn process_create_native_treasury(
         &system_program::id(), // System program as the PDA owner
         system_info,
         &rent,
-        None,
+        0,
     )?;
 
     Ok(())
diff --git a/governance/program/src/processor/process_create_program_governance.rs b/governance/program/src/processor/process_create_program_governance.rs
@@ -84,7 +84,7 @@ pub fn process_create_program_governance(
         program_id,
         system_info,
         &rent,
-        None,
+        0,
     )?;
 
     if transfer_upgrade_authority {
diff --git a/governance/program/src/processor/process_create_proposal.rs b/governance/program/src/processor/process_create_proposal.rs
@@ -85,12 +85,12 @@ pub fn process_create_proposal(
     proposal_owner_record_data
         .assert_token_owner_or_delegate_is_signer(governance_authority_info)?;
 
-    let realm_config_info = next_account_info(account_info_iter)?; // 10
+    let realm_config_info = next_account_info(account_info_iter)?; // 8
     let realm_config_data =
         get_realm_config_data_for_realm(program_id, realm_config_info, realm_info.key)?;
 
     let voter_weight = proposal_owner_record_data.resolve_voter_weight(
-        account_info_iter, // voter_weight_record  *11
+        account_info_iter, // voter_weight_record  *9
         &realm_data,
         &realm_config_data,
         VoterWeightAction::CreateProposal,
@@ -177,7 +177,7 @@ pub fn process_create_proposal(
         program_id,
         system_info,
         &rent,
-        None,
+        0,
     )?;
 
     governance_data.active_proposal_count = governance_data
@@ -188,7 +188,7 @@ pub fn process_create_proposal(
     // Take Proposal deposit if needed
     let proposal_deposit_amount = governance_data.get_proposal_deposit_amount();
     if proposal_deposit_amount > 0 {
-        let proposal_deposit_info = next_account_info(account_info_iter)?; // *8
+        let proposal_deposit_info = next_account_info(account_info_iter)?; // *10
         let proposal_deposit_data = ProposalDeposit {};
 
         create_and_serialize_account_signed::<ProposalDeposit>(
@@ -199,7 +199,7 @@ pub fn process_create_proposal(
             program_id,
             system_info,
             &rent,
-            Some(proposal_deposit_amount),
+            proposal_deposit_amount,
         )?;
     }
 
diff --git a/governance/program/src/processor/process_create_realm.rs b/governance/program/src/processor/process_create_realm.rs
@@ -118,7 +118,7 @@ pub fn process_create_realm(
         program_id,
         system_info,
         rent,
-        None,
+        0,
     )?;
 
     // Create and serialize Realm
@@ -151,7 +151,7 @@ pub fn process_create_realm(
         program_id,
         system_info,
         rent,
-        None,
+        0,
     )?;
 
     Ok(())
diff --git a/governance/program/src/processor/process_create_token_governance.rs b/governance/program/src/processor/process_create_token_governance.rs
@@ -82,7 +82,7 @@ pub fn process_create_token_governance(
         program_id,
         system_info,
         &rent,
-        None,
+        0,
     )?;
 
     if transfer_account_authorities {
diff --git a/governance/program/src/processor/process_create_token_owner_record.rs b/governance/program/src/processor/process_create_token_owner_record.rs
@@ -15,7 +15,8 @@ use crate::{
         enums::GovernanceAccountType,
         realm::get_realm_data,
         token_owner_record::{
-            get_token_owner_record_address_seeds, TokenOwnerRecordV2, TOKEN_OWNER_RECORD_VERSION,
+            get_token_owner_record_address_seeds, TokenOwnerRecordV2,
+            TOKEN_OWNER_RECORD_LAYOUT_VERSION,
         },
     },
 };
@@ -51,7 +52,7 @@ pub fn process_create_token_owner_record(
         governance_delegate: None,
         unrelinquished_votes_count: 0,
         outstanding_proposal_count: 0,
-        version: TOKEN_OWNER_RECORD_VERSION,
+        version: TOKEN_OWNER_RECORD_LAYOUT_VERSION,
         reserved: [0; 6],
         reserved_v2: [0; 128],
     };
@@ -68,6 +69,6 @@ pub fn process_create_token_owner_record(
         program_id,
         system_info,
         &rent,
-        None,
+        0,
     )
 }
diff --git a/governance/program/src/processor/process_deposit_governing_tokens.rs b/governance/program/src/processor/process_deposit_governing_tokens.rs
@@ -17,7 +17,7 @@ use crate::{
         realm_config::get_realm_config_data_for_realm,
         token_owner_record::{
             get_token_owner_record_address_seeds, get_token_owner_record_data_for_seeds,
-            TokenOwnerRecordV2, TOKEN_OWNER_RECORD_VERSION,
+            TokenOwnerRecordV2, TOKEN_OWNER_RECORD_LAYOUT_VERSION,
         },
     },
     tools::spl_token::{
@@ -105,7 +105,7 @@ pub fn process_deposit_governing_tokens(
             governance_delegate: None,
             unrelinquished_votes_count: 0,
             outstanding_proposal_count: 0,
-            version: TOKEN_OWNER_RECORD_VERSION,
+            version: TOKEN_OWNER_RECORD_LAYOUT_VERSION,
             reserved: [0; 6],
             reserved_v2: [0; 128],
         };
@@ -118,7 +118,7 @@ pub fn process_deposit_governing_tokens(
             program_id,
             system_info,
             &rent,
-            None,
+            0,
         )?;
     } else {
         let mut token_owner_record_data = get_token_owner_record_data_for_seeds(
diff --git a/governance/program/src/processor/process_insert_transaction.rs b/governance/program/src/processor/process_insert_transaction.rs
@@ -109,7 +109,7 @@ pub fn process_insert_transaction(
         program_id,
         system_info,
         rent,
-        None,
+        0,
     )?;
 
     Ok(())
diff --git a/governance/program/src/processor/process_set_realm_config.rs b/governance/program/src/processor/process_set_realm_config.rs
@@ -109,7 +109,7 @@ pub fn process_set_realm_config(
             program_id,
             system_info,
             &rent,
-            None,
+            0,
         )?;
     } else {
         realm_config_data.serialize(&mut *realm_config_info.data.borrow_mut())?;
diff --git a/governance/program/src/processor/process_update_program_metadata.rs b/governance/program/src/processor/process_update_program_metadata.rs
@@ -52,7 +52,7 @@ pub fn process_update_program_metadata(
             program_id,
             system_info,
             &rent,
-            None,
+            0,
         )?;
     } else {
         let mut program_metadata_data =
diff --git a/governance/program/src/state/governance.rs b/governance/program/src/state/governance.rs
@@ -110,7 +110,7 @@ pub struct GovernanceV2 {
     ///
     /// Note: The counter was introduced in program V3 and didn't exist in program V1 & V2
     /// If the program is upgraded from program V1 or V2 while there are any outstanding active proposals
-    /// the counter won't be accurate until all the preexisting proposal are transitioned to an inactive final state
+    /// the counter won't be accurate until all proposals are transitioned to an inactive final state and the counter reset
     pub active_proposal_count: u64,
 }
 
@@ -584,6 +584,11 @@ pub fn assert_is_valid_governance_config(
         return Err(GovernanceError::AtLeastOneVoteThresholdRequired.into());
     }
 
+    // Make u8::MAX invalid value in case we would like to use the magic number as Disabled value in the future
+    if governance_config.deposit_exempt_proposal_count == u8::MAX {
+        return Err(GovernanceError::InvalidDepositExemptProposalCount.into());
+    }
+
     Ok(())
 }
 
@@ -925,4 +930,22 @@ mod test {
         // Assert
         assert_eq!(deposit_amount, SECURITY_DEPOSIT_BASE_LAMPORTS * 10);
     }
+
+    #[test]
+    fn test_assert_config_invalid_with_max_deposit_exempt_proposal_count() {
+        // Arrange
+        let mut governance_config = create_test_governance_config();
+        governance_config.deposit_exempt_proposal_count = u8::MAX;
+
+        // Act
+        let err = assert_is_valid_governance_config(&governance_config)
+            .err()
+            .unwrap();
+
+        // Assert
+        assert_eq!(
+            err,
+            GovernanceError::InvalidDepositExemptProposalCount.into()
+        );
+    }
 }
diff --git a/governance/program/src/state/realm_config.rs b/governance/program/src/state/realm_config.rs
@@ -96,7 +96,7 @@ pub struct RealmConfigAccount {
 
 impl AccountMaxSize for RealmConfigAccount {
     fn get_max_size(&self) -> Option<usize> {
-        Some(1 + 32 + 33 * 4 + 128)
+        Some(1 + 32 + 75 * 2 + 110)
     }
 }
 
diff --git a/governance/program/src/state/token_owner_record.rs b/governance/program/src/state/token_owner_record.rs
@@ -64,12 +64,12 @@ pub struct TokenOwnerRecordV2 {
     /// The new account type and versioning scheme can be introduced once we migrate UI to use indexer to fetch all the accounts
     /// Once the new versioning scheme is introduced this field can be migrated and removed
     ///
-    /// The other issues which need to be adresses before we can cleanup the account versioning code:
+    /// The other issues which need to be addressed before we can cleanup the account versioning code:
     /// 1) Remove the specific governance accounts (ProgramGovernance, TokenGovernance, MintGovernance)
     ///    The only reason they exist is the UI which can't handle the generic use case for those assets
     /// 2) For account layout breaking changes all plugins would have to be upgraded
     /// 3) For account layout changes the Holaplex indexer would have to be upgraded
-    /// 4) We should migrate the UI to use the indexer for fetching data and stop using getProgramAccoutns
+    /// 4) We should migrate the UI to use the indexer for fetching data and stop using getProgramAccounts
     /// 5) The UI would have to be upgraded to support account migration to the latest version
     /// 6) The client sdk is already messy because of the different program/account versions and it should be cleaned up before we add even more versions.
     pub version: u8,
@@ -86,11 +86,12 @@ pub struct TokenOwnerRecordV2 {
     pub reserved_v2: [u8; 128],
 }
 
-/// The current version of TokenOwnerRecord
-/// Note: It's the version of the account layout and not the version of the program
-/// program V1,V2 -> account version 0
-/// program V3 -> account version 1
-pub const TOKEN_OWNER_RECORD_VERSION: u8 = 1;
+/// The current version of TokenOwnerRecord account layout
+/// Note: It's the version of the account layout and not the version of the program or the account type
+///
+/// program V1,V2 -> account layout version 0
+/// program V3 -> account layout version 1
+pub const TOKEN_OWNER_RECORD_LAYOUT_VERSION: u8 = 1;
 
 impl AccountMaxSize for TokenOwnerRecordV2 {
     fn get_max_size(&self) -> Option<usize> {
@@ -345,9 +346,9 @@ pub fn get_token_owner_record_data(
         get_account_data::<TokenOwnerRecordV2>(program_id, token_owner_record_info)?
     };
 
-    // If the deserialized account is of the old account version then migrate the data to the latest one
-    if token_owner_record_data.version < TOKEN_OWNER_RECORD_VERSION {
-        token_owner_record_data.version = TOKEN_OWNER_RECORD_VERSION;
+    // If the deserialized account uses the old account layout indicated by the version value then migrate the data to version 1
+    if token_owner_record_data.version < 1 {
+        token_owner_record_data.version = 1;
 
         // In previous versions unrelinquished_votes_count was u32 followed by total_votes_count:u32
         // In program V3 unrelinquished_votes_count was changed to u64 by extending it into the space previously used by total_votes_count:u32
@@ -555,7 +556,7 @@ mod test {
         assert_eq!(token_owner_record_program_v3.unrelinquished_votes_count, 10);
         assert_eq!(
             token_owner_record_program_v3.version,
-            TOKEN_OWNER_RECORD_VERSION
+            TOKEN_OWNER_RECORD_LAYOUT_VERSION
         );
     }
 }
diff --git a/governance/program/tests/process_create_token_owner_record.rs b/governance/program/tests/process_create_token_owner_record.rs
@@ -5,7 +5,7 @@ use solana_program_test::*;
 mod program_test;
 
 use program_test::*;
-use spl_governance::state::token_owner_record::TOKEN_OWNER_RECORD_VERSION;
+use spl_governance::state::token_owner_record::TOKEN_OWNER_RECORD_LAYOUT_VERSION;
 
 #[tokio::test]
 async fn test_create_token_owner_record() {
@@ -26,7 +26,7 @@ async fn test_create_token_owner_record() {
     assert_eq!(0, token_owner_record_account.governing_token_deposit_amount);
 
     assert_eq!(
-        TOKEN_OWNER_RECORD_VERSION,
+        TOKEN_OWNER_RECORD_LAYOUT_VERSION,
         token_owner_record_account.version
     );
     assert_eq!(0, token_owner_record_account.unrelinquished_votes_count);
diff --git a/governance/program/tests/process_deposit_governing_tokens.rs b/governance/program/tests/process_deposit_governing_tokens.rs
@@ -10,7 +10,9 @@ use solana_sdk::signature::{Keypair, Signer};
 use spl_governance::{
     error::GovernanceError,
     instruction::deposit_governing_tokens,
-    state::{realm_config::GoverningTokenType, token_owner_record::TOKEN_OWNER_RECORD_VERSION},
+    state::{
+        realm_config::GoverningTokenType, token_owner_record::TOKEN_OWNER_RECORD_LAYOUT_VERSION,
+    },
 };
 
 use crate::program_test::args::*;
@@ -35,7 +37,10 @@ async fn test_deposit_initial_community_tokens() {
 
     assert_eq!(token_owner_record_cookie.account, token_owner_record);
 
-    assert_eq!(TOKEN_OWNER_RECORD_VERSION, token_owner_record.version);
+    assert_eq!(
+        TOKEN_OWNER_RECORD_LAYOUT_VERSION,
+        token_owner_record.version
+    );
     assert_eq!(0, token_owner_record.unrelinquished_votes_count);
 
     let source_account = governance_test
diff --git a/governance/program/tests/program_test/mod.rs b/governance/program/tests/program_test/mod.rs
diff --git a/governance/tools/src/account.rs b/governance/tools/src/account.rs
