feat(multisig_add_member,multisig_remove_member): implement ixs and refactor code to use invariants

Author: vovacodes

Anchor.toml

@@ -3,7 +3,7 @@ seeds = false
 skip-lint = false
 
 [programs.localnet]
-multisig = "7YYnaRgQeHYd2FKGKkwASM2ZNZHTo1GvcicsyKKFvcoh"
+multisig = "SQDS4ep65T869zMMBKyuUq6aD6EgTu8psMjkvj52pCf"
 
 [registry]
 url = "https://api.apr.dev"

programs/multisig/Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "multisig"
 version = "0.1.0"
-description = "Created with Anchor"
+description = "Squads Multisig Program V4"
 edition = "2021"
 
 [lib]

programs/multisig/src/errors.rs

@@ -4,14 +4,10 @@ use anchor_lang::prelude::*;
 pub enum MultisigError {
     #[msg("Found multiple members with the same pubkey")]
     DuplicateMember,
-    #[msg("Member is already in multisig")]
-    MemberAlreadyExists,
     #[msg("Members array is empty")]
     EmptyMembers,
     #[msg("Too many members, can be up to 65535")]
     TooManyMembers,
-    #[msg("Maximum number of members already reached")]
-    MaxMembersReached,
     #[msg("Invalid threshold, must be between 1 and number of members with Vote permission")]
     InvalidThreshold,
     #[msg("Attempted to perform an unauthorized action")]
@@ -36,4 +32,10 @@ pub enum MultisigError {
     InvalidAccount,
     #[msg("transaction_execute reentrancy is forbidden")]
     ExecuteReentrancy,
+    #[msg("Cannot remove last member")]
+    RemoveLastMember,
+    #[msg("Members don't include any voters")]
+    NoVoters,
+    #[msg("config_authority must be set to non-default key")]
+    InvalidStaleTransactionIndex,
 }

programs/multisig/src/instructions/multisig_config.rs

@@ -8,7 +8,21 @@ use crate::state::*;
 #[derive(AnchorSerialize, AnchorDeserialize)]
 pub struct MultisigAddMemberArgs {
     new_member: Member,
-    /// Memo isn't used for anything, but is included in `CreatedEvent` that can later be parsed and indexed.
+    /// Memo isn't used for anything, but is included in `AddMemberEvent` that can later be parsed and indexed.
+    pub memo: Option<String>,
+}
+
+#[derive(AnchorSerialize, AnchorDeserialize)]
+pub struct MultisigRemoveMemberArgs {
+    old_member: Member,
+    /// Memo isn't used for anything, but is included in `RemoveMemberEvent` that can later be parsed and indexed.
+    pub memo: Option<String>,
+}
+
+#[derive(AnchorSerialize, AnchorDeserialize)]
+pub struct MultisigChangeThresholdArgs {
+    new_threshold: u16,
+    /// Memo isn't used for anything, but is included in `ChangeThreshold` that can later be parsed and indexed.
     pub memo: Option<String>,
 }
 
@@ -18,11 +32,13 @@ pub struct MultisigConfig<'info> {
         mut,
         seeds = [SEED_PREFIX, multisig.create_key.as_ref(), SEED_MULTISIG],
         bump = multisig.bump,
-        has_one = config_authority @ MultisigError::Unauthorized,
     )]
     multisig: Account<'info, Multisig>,
 
-    #[account(mut)]
+    #[account(
+        mut,
+        constraint = config_authority.key() == multisig.config_authority @ MultisigError::Unauthorized,
+    )]
     pub config_authority: Signer<'info>,
 
     /// We might need it in case reallocation is needed.
@@ -32,28 +48,19 @@ pub struct MultisigConfig<'info> {
 impl MultisigConfig<'_> {
     /// Add a member/key to the multisig and reallocate space if necessary.
     pub fn multisig_add_member(ctx: Context<Self>, args: MultisigAddMemberArgs) -> Result<()> {
-        let multisig = &mut ctx.accounts.multisig;
-
         let MultisigAddMemberArgs { new_member, memo } = args;
 
-        require!(
-            multisig.is_member(new_member.key).is_none(),
-            MultisigError::MemberAlreadyExists
-        );
+        let multisig = &mut ctx.accounts.multisig;
+        let multisig_key = multisig.to_account_info().key();
 
         let current_members_length = multisig.members.len();
-        // If max is already reached, we can't have more members.
-        require!(
-            current_members_length < usize::from(u16::MAX),
-            MultisigError::MaxMembersReached
-        );
-
         let current_account_size = multisig.to_account_info().data.borrow().len();
+
         // Check if we need to reallocate space.
         let reallocated = if current_account_size < Multisig::size(current_members_length + 1) {
             // We need to allocate more space. To avoid doing this operation too often, we increment it by 10 members.
             let new_size = current_account_size + (10 * Member::size());
-            // Reallocate more space
+            // Reallocate more space.
             AccountInfo::realloc(&multisig.to_account_info(), new_size, false)?;
 
             // If more lamports are needed, transfer them to the account
@@ -78,9 +85,10 @@ impl MultisigConfig<'_> {
             false
         };
 
-        multisig.add_member_if_not_exists(new_member);
+        multisig.add_member(new_member);
+
+        multisig.invariant()?;
 
-        let multisig_key = multisig.to_account_info().key();
         multisig.config_updated(
             multisig_key,
             ConfigUpdateType::AddMember { reallocated },
@@ -89,4 +97,58 @@ impl MultisigConfig<'_> {
 
         Ok(())
     }
+
+    /// Remove a member/key from the multisig.
+    pub fn multisig_remove_member(
+        ctx: Context<Self>,
+        args: MultisigRemoveMemberArgs,
+    ) -> Result<()> {
+        let multisig = &mut ctx.accounts.multisig;
+        let multisig_key = multisig.to_account_info().key();
+
+        require!(multisig.members.len() > 1, MultisigError::RemoveLastMember);
+
+        let old_member_index = match multisig.is_member(args.old_member.key) {
+            Some(old_member_index) => old_member_index,
+            None => return err!(MultisigError::NotAMember),
+        };
+
+        multisig.members.remove(old_member_index);
+
+        // Update the threshold if necessary.
+        if usize::from(multisig.threshold) > multisig.members.len() {
+            multisig.threshold = multisig
+                .members
+                .len()
+                .try_into()
+                .expect("didn't expect more that `u16::MAX` members");
+        }
+
+        multisig.invariant()?;
+
+        multisig.config_updated(multisig_key, ConfigUpdateType::RemoveMember, args.memo);
+
+        Ok(())
+    }
+
+    pub fn multisig_change_threshold(
+        ctx: Context<Self>,
+        args: MultisigChangeThresholdArgs,
+    ) -> Result<()> {
+        let MultisigChangeThresholdArgs {
+            new_threshold,
+            memo,
+        } = args;
+
+        let multisig = &mut ctx.accounts.multisig;
+        let multisig_key = multisig.to_account_info().key();
+
+        multisig.threshold = new_threshold;
+
+        multisig.invariant()?;
+
+        multisig.config_updated(multisig_key, ConfigUpdateType::ChangeThreshold, memo);
+
+        Ok(())
+    }
 }

programs/multisig/src/instructions/multisig_create.rs

@@ -74,8 +74,10 @@ impl MultisigCreate<'_> {
         multisig.create_key = args.create_key;
         multisig.bump = *ctx.bumps.get("multisig").unwrap();
 
+        multisig.invariant()?;
+
         emit!(MultisigCreated {
-            multisig: ctx.accounts.multisig.to_account_info().key(),
+            multisig: multisig.to_account_info().key(),
             memo: args.memo,
         });
 

programs/multisig/src/instructions/transaction_create.rs

@@ -105,6 +105,8 @@ impl TransactionCreate<'_> {
         // Updated last transaction index in the multisig account.
         multisig.transaction_index = transaction_index;
 
+        multisig.invariant()?;
+
         emit!(TransactionCreated {
             multisig: multisig_key,
             transaction: transaction.key(),

programs/multisig/src/instructions/transaction_execute.rs

@@ -11,7 +11,6 @@ use crate::utils::*;
 #[derive(Accounts)]
 pub struct TransactionExecute<'info> {
     #[account(
-        mut,
         seeds = [SEED_PREFIX, multisig.create_key.as_ref(), SEED_MULTISIG],
         bump = multisig.bump,
     )]

programs/multisig/src/lib.rs

@@ -1,4 +1,5 @@
 #![allow(clippy::result_large_err)]
+#![deny(unused_must_use)]
 
 use anchor_lang::prelude::*;
 
@@ -12,7 +13,7 @@ pub mod instructions;
 pub mod state;
 mod utils;
 
-declare_id!("7YYnaRgQeHYd2FKGKkwASM2ZNZHTo1GvcicsyKKFvcoh");
+declare_id!("SQDS4ep65T869zMMBKyuUq6aD6EgTu8psMjkvj52pCf");
 
 #[program]
 pub mod multisig {
@@ -31,6 +32,14 @@ pub mod multisig {
         MultisigConfig::multisig_add_member(ctx, args)
     }
 
+    /// Remove a member/key from the multisig.
+    pub fn multisig_remove_member(
+        ctx: Context<MultisigConfig>,
+        args: MultisigRemoveMemberArgs,
+    ) -> Result<()> {
+        MultisigConfig::multisig_remove_member(ctx, args)
+    }
+
     /// Create a new multisig transaction.
     pub fn transaction_create(
         ctx: Context<TransactionCreate>,

programs/multisig/src/state.rs

@@ -35,8 +35,6 @@ pub struct Multisig {
     /// Last stale transaction index. All transactions up until this index are stale.
     /// This index is updated when multisig config (members/threshold) changes.
     pub stale_transaction_index: u64,
-    /// Reserved for future use.
-    pub _reserved: u8,
     /// Key that is used to seed the multisig PDA.
     /// Used solely as bytes for the seed, doesn't have any other meaning or function.
     pub create_key: Pubkey,
@@ -54,11 +52,51 @@ impl Multisig {
         1  + // authority_index
         8  + // transaction_index
         8  + // stale_transaction_index 
-        1  + // allow_external_execute
         32 + // create_key
         1 // bump
     }
 
+    // Makes sure the multisig state is valid.
+    // This must be called at the end of every instruction that modifies a Multisig account.
+    pub fn invariant(&self) -> Result<()> {
+        let Self {
+            threshold,
+            members,
+            transaction_index,
+            stale_transaction_index,
+            ..
+        } = self;
+        // Max number of members is u16::MAX.
+        require!(
+            members.len() <= usize::from(u16::MAX),
+            MultisigError::TooManyMembers
+        );
+
+        // There must be no duplicate members.
+        let has_duplicates = members.windows(2).any(|win| win[0].key == win[1].key);
+        require!(!has_duplicates, MultisigError::DuplicateMember);
+
+        // There must be at least one member with Vote permissions.
+        let num_voters: u16 = Self::num_voters(members)
+            .try_into()
+            .expect("didn't expect more that `u16::MAX` members");
+        require!(num_voters > 0, MultisigError::NoVoters);
+
+        // Threshold must be greater than 0.
+        require!(*threshold > 0, MultisigError::InvalidThreshold);
+
+        // Threshold must not exceed the number of voters.
+        require!(*threshold <= num_voters, MultisigError::InvalidThreshold);
+
+        // `state.stale_transaction_index` must be less than or equal to `state.transaction_index`.
+        require!(
+            stale_transaction_index <= transaction_index,
+            MultisigError::InvalidStaleTransactionIndex
+        );
+
+        Ok(())
+    }
+
     /// Captures the fact that the multisig config has changed in the multisig state
     /// and emits a `ConfigUpdatedEvent`.
     pub fn config_updated(
@@ -99,10 +137,14 @@ impl Multisig {
             .count()
     }
 
-    pub fn add_member_if_not_exists(&mut self, new_member: Member) {
-        if self.is_member(new_member.key).is_none() {
-            self.members.push(new_member);
-            self.members.sort_by_key(|m| m.key);
+    pub fn add_member(&mut self, new_member: Member) {
+        self.members.push(new_member);
+        self.members.sort_by_key(|m| m.key);
+    }
+
+    pub fn remove_member(&mut self, member_pubkey: Pubkey) {
+        if let Some(index) = self.is_member(member_pubkey) {
+            self.members.remove(index);
         }
     }
 }

sdk/multisig/.solitarc.js

@@ -9,7 +9,7 @@ const binaryInstallDir = path.join(__dirname, ".crates");
 module.exports = {
   idlGenerator: "anchor",
   programName: "multisig",
-  programId: "7YYnaRgQeHYd2FKGKkwASM2ZNZHTo1GvcicsyKKFvcoh",
+  programId: "SQDS4ep65T869zMMBKyuUq6aD6EgTu8psMjkvj52pCf",
   idlDir,
   sdkDir,
   binaryInstallDir,