Release v1 code review (#40)

SebastianBor · web-flow · commit f0957efda24c · 2022-03-24T22:07:38.000Z
* chore: validate realm address against registrar

* chore: validate token owner address against VoterWeightRecord

* chore: validate token owner address against VoterWeightRecord

* chore: Update CastNftVote comment

* chore: Update CastNftVote comments

* chore: use owner and executable constraints

* chore: remove unnecessary mut account flag

* chore: add governance account comments

* chore: add NftVoteRecord seeds validation comment

* chore: remove redundant spl-token dependency

* chore: add missing  log_version()

* chore: update IDL
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -4,6 +4,7 @@
     "arrayref",
     "itertools",
     "Keypair",
+    "lamports",
     "Metaplex",
     "Metas",
     "Pubkey",
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
     "name": "@solana/governance-program-library",
-    "version": "0.14.0",
+    "version": "0.15.0",
     "description": "Client for Governance Program Library which is a set of extensions for Solana's spl-governance program.",
     "author": "Solana Maintainers <maintainers@solana.foundation>",
     "license": "MIT",
diff --git a/programs/nft-voter/Cargo.toml b/programs/nft-voter/Cargo.toml
@@ -29,5 +29,4 @@ spl-token = { version = "3.3", features = [ "no-entrypoint" ] }
 [dev-dependencies]
 borsh = "0.9.1"
 solana-sdk = "1.9.5"
-solana-program-test = "1.9.5"
-spl-token = { version = "^3.3.0", features = ["no-entrypoint"] }
+solana-program-test = "1.9.5"
diff --git a/programs/nft-voter/src/error.rs b/programs/nft-voter/src/error.rs
@@ -5,8 +5,8 @@ pub enum NftVoterError {
     #[msg("Invalid Realm Authority")]
     InvalidRealmAuthority,
 
-    #[msg("Invalid Registrar Realm")]
-    InvalidRegistrarRealm,
+    #[msg("Invalid Realm for Registrar")]
+    InvalidRealmForRegistrar,
 
     #[msg("Invalid Collection Size")]
     InvalidCollectionSize,
@@ -26,8 +26,8 @@ pub enum NftVoterError {
     #[msg("Invalid VoterWeightRecord Mint")]
     InvalidVoterWeightRecordMint,
 
-    #[msg("Invalid VoterWeightRecord Owner")]
-    InvalidVoterWeightRecordOwner,
+    #[msg("Invalid TokenOwner for VoterWeightRecord")]
+    InvalidTokenOwnerForVoterWeightRecord,
 
     #[msg("Collection must be verified")]
     CollectionMustBeVerified,
diff --git a/programs/nft-voter/src/instructions/cast_nft_vote.rs b/programs/nft-voter/src/instructions/cast_nft_vote.rs
@@ -7,7 +7,11 @@ use crate::error::NftVoterError;
 
 /// Casts NFT vote. The NFTs used for voting are tracked using NftVoteRecord accounts
 /// This instruction updates VoterWeightRecord which is valid for the current Slot and the target Proposal only
-/// and hance the instruction has to be executed inside the same transaction as spl-gov CastVote
+/// and hance the instruction has to be executed inside the same transaction as spl-gov.CastVote
+/// 
+/// CastNftVote instruction and NftVoteRecord are not directional. They don't record vote choice (ex Yes/No)
+/// VoteChoice is recorded by spl-gov in VoteRecord and this CastNftVote only tracks voting NFTs
+/// 
 #[derive(Accounts)]
 #[instruction(proposal: Pubkey)]
 pub struct CastNftVote<'info> {
@@ -21,14 +25,13 @@ pub struct CastNftVote<'info> {
 
         constraint = voter_weight_record.governing_token_mint == registrar.governing_token_mint
         @ NftVoterError::InvalidVoterWeightRecordMint,
-
-        constraint = voter_weight_record.governing_token_owner == governing_token_owner.key()
-        @ NftVoterError::InvalidVoterWeightRecordOwner,
     )]
     pub voter_weight_record: Account<'info, VoterWeightRecord>,
 
     /// The token owner who casts the vote
-    #[account(mut)]
+    #[account(
+        address = voter_weight_record.governing_token_owner @ NftVoterError::InvalidTokenOwnerForVoterWeightRecord
+    )]
     pub governing_token_owner: Signer<'info>,
     
     /// The account which pays for the transaction 
@@ -64,6 +67,8 @@ pub fn cast_nft_vote<'a,'b,'c,'info>(ctx: Context<'a,'b,'c,'info,CastNftVote<'in
         voter_weight = voter_weight.checked_add(nft_vote_weight as u64).unwrap();
 
         // Create NFT vote record to ensure the same NFT hasn't been already used for voting
+        // Note: The correct PDA of the NftVoteRecord is validated in create_and_serialize_account_signed
+        // It ensures the NftVoteRecord is for ('nft-vote-record',proposal,nft_mint) seeds
         require!(
             nft_vote_record_info.data_is_empty(),
             NftVoterError::NftAlreadyVoted
diff --git a/programs/nft-voter/src/instructions/configure_collection.rs b/programs/nft-voter/src/instructions/configure_collection.rs
@@ -18,12 +18,13 @@ use crate::{error::NftVoterError};
 #[derive(Accounts)]
 pub struct ConfigureCollection<'info> {
     /// Registrar for which we configure this Collection
-    #[account(mut,
-        constraint = registrar.realm == realm.key() 
-        @ NftVoterError::InvalidRegistrarRealm
-    )]
+    #[account(mut)]
     pub registrar: Account<'info, Registrar>,
 
+    #[account(
+       address = registrar.realm @ NftVoterError::InvalidRealmForRegistrar,
+       owner = registrar.governance_program_id
+    )]
     /// CHECK: Owned by spl-governance instance specified in registrar.governance_program_id
     pub realm: UncheckedAccount<'info>,
 
diff --git a/programs/nft-voter/src/instructions/create_max_voter_weight_record.rs b/programs/nft-voter/src/instructions/create_max_voter_weight_record.rs
@@ -20,8 +20,10 @@ pub struct CreateMaxVoterWeightRecord<'info> {
 
     /// The program id of the spl-governance program the realm belongs to
     /// CHECK: Can be any instance of spl-governance and it's not known at the compilation time
+    #[account(executable)]
     pub governance_program_id: UncheckedAccount<'info>,
 
+    #[account(owner = governance_program_id.key())]
     /// CHECK: Owned by spl-governance instance specified in governance_program_id
     pub realm: UncheckedAccount<'info>,
 
diff --git a/programs/nft-voter/src/instructions/create_registrar.rs b/programs/nft-voter/src/instructions/create_registrar.rs
@@ -23,6 +23,7 @@ pub struct CreateRegistrar<'info> {
 
     /// The program id of the spl-governance program the realm belongs to
     /// CHECK: Can be any instance of spl-governance and it's not known at the compilation time
+    #[account(executable)]
     pub governance_program_id: UncheckedAccount<'info>,
 
     /// An spl-governance Realm
@@ -32,6 +33,7 @@ pub struct CreateRegistrar<'info> {
     /// - governing_token_mint must be the community or council mint
     /// - realm_authority is realm.authority
     /// CHECK: Owned by spl-governance instance specified in governance_program_id
+    #[account(owner = governance_program_id.key())]
     pub realm: UncheckedAccount<'info>,
 
     /// Either the realm community mint or the council mint.
diff --git a/programs/nft-voter/src/instructions/create_voter_weight_record.rs b/programs/nft-voter/src/instructions/create_voter_weight_record.rs
@@ -22,9 +22,11 @@ pub struct CreateVoterWeightRecord<'info> {
 
     /// The program id of the spl-governance program the realm belongs to
     /// CHECK: Can be any instance of spl-governance and it's not known at the compilation time
+    #[account(executable)]
     pub governance_program_id: UncheckedAccount<'info>,
 
     /// CHECK: Owned by spl-governance instance specified in governance_program_id
+    #[account(owner = governance_program_id.key())]
     pub realm: UncheckedAccount<'info>,
 
     /// Either the realm community mint or the council mint.
diff --git a/programs/nft-voter/src/instructions/relinquish_nft_vote.rs b/programs/nft-voter/src/instructions/relinquish_nft_vote.rs
@@ -20,23 +20,28 @@ pub struct RelinquishNftVote<'info> {
 
         constraint = voter_weight_record.governing_token_mint == registrar.governing_token_mint
         @ NftVoterError::InvalidVoterWeightRecordMint,
-
-        constraint = voter_weight_record.governing_token_owner == governing_token_owner.key()
-        @ NftVoterError::InvalidVoterWeightRecordOwner,
     )]
     pub voter_weight_record: Account<'info, VoterWeightRecord>,
 
     /// CHECK: Owned by spl-governance instance specified in registrar.governance_program_id
+    /// Governance account the Proposal is for
+    #[account(owner = registrar.governance_program_id)]
     pub governance: UncheckedAccount<'info>,
 
     /// CHECK: Owned by spl-governance instance specified in registrar.governance_program_id
+    #[account(owner = registrar.governance_program_id)]
     pub proposal: UncheckedAccount<'info>,
 
     /// The token owner who cast the original vote
-    #[account(mut)]
+    #[account(
+        address = voter_weight_record.governing_token_owner  @ NftVoterError::InvalidTokenOwnerForVoterWeightRecord
+    )]
     pub governing_token_owner: Signer<'info>,
 
     /// CHECK: Owned by spl-governance instance specified in registrar.governance_program_id
+    /// The account is used to validate that it doesn't exist and if it doesn't then Anchor owner check throws error
+    /// The check is disabled here and performed inside the instruction 
+    /// #[account(owner = registrar.governance_program_id)] 
     pub vote_record: UncheckedAccount<'info>,
 
     /// CHECK: The beneficiary who receives lamports from the disposed NftVoterRecord accounts can be any account
diff --git a/programs/nft-voter/src/lib.rs b/programs/nft-voter/src/lib.rs
@@ -42,6 +42,7 @@ pub mod nft_voter {
         instructions::update_voter_weight_record(ctx, voter_weight_action)
     }
     pub fn relinquish_nft_vote(ctx: Context<RelinquishNftVote>) -> Result<()> {
+        log_version();
         instructions::relinquish_nft_vote(ctx)
     }
     pub fn configure_collection(
diff --git a/programs/nft-voter/tests/cast_nft_vote.rs b/programs/nft-voter/tests/cast_nft_vote.rs
@@ -342,7 +342,7 @@ async fn test_cast_nft_vote_invalid_voter_error() -> Result<(), TransportError>
         .unwrap();
 
     // Assert
-    assert_nft_voter_err(err, NftVoterError::InvalidVoterWeightRecordOwner);
+    assert_nft_voter_err(err, NftVoterError::InvalidTokenOwnerForVoterWeightRecord);
 
     Ok(())
 }
diff --git a/programs/nft-voter/tests/configure_collection.rs b/programs/nft-voter/tests/configure_collection.rs
@@ -1,4 +1,3 @@
-use anchor_lang::prelude::Pubkey;
 use gpl_nft_voter::error::NftVoterError;
 use program_test::{
     nft_voter_test::NftVoterTest,
@@ -238,14 +237,17 @@ async fn test_configure_collection_with_invalid_realm_error() -> Result<(), Tran
         .with_max_voter_weight_record(&registrar_cookie)
         .await?;
 
+    // Try to use a different Realm
+    let realm_cookie2 = nft_voter_test.governance.with_realm().await?;
+
     // Act
     let err = nft_voter_test
         .with_collection_using_ix(
             &registrar_cookie,
             &nft_collection_cookie,
             &max_voter_weight_record_cookie,
             None,
-            |i| i.accounts[1].pubkey = Pubkey::new_unique(), // realm
+            |i| i.accounts[1].pubkey = realm_cookie2.address, // realm
             None,
         )
         .await
@@ -254,7 +256,7 @@ async fn test_configure_collection_with_invalid_realm_error() -> Result<(), Tran
 
     // Assert
 
-    assert_nft_voter_err(err, NftVoterError::InvalidRegistrarRealm);
+    assert_nft_voter_err(err, NftVoterError::InvalidRealmForRegistrar);
 
     Ok(())
 }
diff --git a/programs/nft-voter/tests/create_registrar.rs b/programs/nft-voter/tests/create_registrar.rs
@@ -8,11 +8,7 @@ use solana_program::instruction::InstructionError;
 use solana_program_test::*;
 use solana_sdk::signature::Keypair;
 
-use program_test::tools::{
-    assert_anchor_err, assert_gov_tools_err, assert_ix_err, assert_nft_voter_err,
-};
-
-use spl_governance_tools::error::GovernanceToolsError;
+use program_test::tools::{assert_anchor_err, assert_ix_err, assert_nft_voter_err};
 
 #[tokio::test]
 async fn test_create_registrar() -> Result<(), BanksClientError> {
@@ -89,18 +85,21 @@ async fn test_create_registrar_with_invalid_spl_gov_program_id_error(
     let mut realm_cookie = nft_voter_test.governance.with_realm().await?;
     realm_cookie.realm_authority = Keypair::new();
 
+    // Try to use a different program id
+    let governance_program_id = nft_voter_test.program_id;
+
     // Act
     let err = nft_voter_test
         .with_registrar_using_ix(
             &realm_cookie,
-            |i| i.accounts[1].pubkey = Pubkey::new_unique(), //governance_program_id
+            |i| i.accounts[1].pubkey = governance_program_id, //governance_program_id
             None,
         )
         .await
         .err()
         .unwrap();
 
-    assert_gov_tools_err(err, GovernanceToolsError::InvalidAccountOwner);
+    assert_anchor_err(err, anchor_lang::error::ErrorCode::ConstraintOwner);
 
     Ok(())
 }
diff --git a/programs/nft-voter/tests/program_test/nft_voter_test.rs b/programs/nft-voter/tests/program_test/nft_voter_test.rs
@@ -67,6 +67,7 @@ pub struct NftVoteRecordCookie {
 }
 
 pub struct NftVoterTest {
+    pub program_id: Pubkey,
     pub bench: Arc<ProgramTestBench>,
     pub governance: GovernanceTest,
     pub token_metadata: TokenMetadataTest,
@@ -96,6 +97,7 @@ impl NftVoterTest {
         let token_metadata_bench = TokenMetadataTest::new(bench_rc.clone());
 
         Self {
+            program_id,
             bench: bench_rc,
             governance: governance_bench,
             token_metadata: token_metadata_bench,
diff --git a/programs/nft-voter/tests/relinquish_nft_vote.rs b/programs/nft-voter/tests/relinquish_nft_vote.rs
@@ -275,3 +275,85 @@ async fn test_relinquish_nft_vote_for_proposal_in_voting_state_and_vote_record_e
 
     Ok(())
 }
+
+#[tokio::test]
+async fn test_relinquish_nft_vote_with_invalid_voter_error() -> Result<(), TransportError> {
+    // Arrange
+    let mut nft_voter_test = NftVoterTest::start_new().await;
+
+    let realm_cookie = nft_voter_test.governance.with_realm().await?;
+
+    let registrar_cookie = nft_voter_test.with_registrar(&realm_cookie).await?;
+
+    let nft_collection_cookie = nft_voter_test.token_metadata.with_nft_collection().await?;
+
+    let max_voter_weight_record_cookie = nft_voter_test
+        .with_max_voter_weight_record(&registrar_cookie)
+        .await?;
+
+    nft_voter_test
+        .with_collection(
+            &registrar_cookie,
+            &nft_collection_cookie,
+            &max_voter_weight_record_cookie,
+            Some(ConfigureCollectionArgs { weight: 1, size: 1 }), // Set Size == 1 to complete voting with just one vote
+        )
+        .await?;
+
+    let voter_cookie = nft_voter_test.bench.with_wallet().await;
+
+    let voter_token_owner_record_cookie = nft_voter_test
+        .governance
+        .with_token_owner_record(&realm_cookie, &voter_cookie)
+        .await?;
+
+    let voter_weight_record_cookie = nft_voter_test
+        .with_voter_weight_record(&registrar_cookie, &voter_cookie)
+        .await?;
+
+    let proposal_cookie = nft_voter_test
+        .governance
+        .with_proposal(&realm_cookie)
+        .await?;
+
+    let nft_cookie1 = nft_voter_test
+        .token_metadata
+        .with_nft_v2(&nft_collection_cookie, &voter_cookie, None)
+        .await?;
+
+    let nft_vote_record_cookies = nft_voter_test
+        .cast_nft_vote(
+            &registrar_cookie,
+            &voter_weight_record_cookie,
+            &max_voter_weight_record_cookie,
+            &proposal_cookie,
+            &voter_cookie,
+            &voter_token_owner_record_cookie,
+            &[&nft_cookie1],
+        )
+        .await?;
+
+    // Try to use a different voter
+    let voter_cookie2 = nft_voter_test.bench.with_wallet().await;
+
+    // Act
+
+    let err = nft_voter_test
+        .relinquish_nft_vote(
+            &registrar_cookie,
+            &voter_weight_record_cookie,
+            &proposal_cookie,
+            &voter_cookie2,
+            &voter_token_owner_record_cookie,
+            &nft_vote_record_cookies,
+        )
+        .await
+        .err()
+        .unwrap();
+
+    // Assert
+
+    assert_nft_voter_err(err, NftVoterError::InvalidTokenOwnerForVoterWeightRecord);
+
+    Ok(())
+}
diff --git a/src/nft_voter.ts b/src/nft_voter.ts

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@solana/governance-program-library",`
`3`		`- "version": "0.14.0",`
	`3`	`+ "version": "0.15.0",`
`4`	`4`	`"description": "Client for Governance Program Library which is a set of extensions for Solana's spl-governance program.",`
`5`	`5`	`"author": "Solana Maintainers <[email protected]>",`
`6`	`6`	`"license": "MIT",`