feat: optimize multi signer feature and update existing tests (#155)

* feat: optimise multi-signer feature implementation

* feat: optimise multi-signer implementation

src/lib.rs

@@ -28,7 +28,7 @@ pub mod pallet {
 
     use codec::{FullCodec, FullEncode};
     use frame_support::{
-        dispatch::{DispatchResultWithPostInfo, GetDispatchInfo, PostDispatchInfo},
+        dispatch::DispatchResultWithPostInfo,
         pallet_prelude::*,
         traits::{Currency, Get, LockableCurrency, ReservableCurrency},
     };
@@ -44,7 +44,6 @@ pub mod pallet {
         types::ScriptTransaction,
     };
     use sp_core::crypto::AccountId32;
-    use sp_runtime::traits::Dispatchable;
     use sp_std::{vec, vec::Vec};
 
     use super::*;
@@ -69,7 +68,7 @@ pub mod pallet {
     /// Storage for multi-signature/signer requests.
     #[pallet::storage]
     pub type MultisigStorage<T: Config> =
-        StorageMap<_, Blake2_128Concat, CallHash, Multisig<T::MaxScriptSigners>>;
+        StorageMap<_, Blake2_128Concat, CallHash, Multisig<T::AccountId, T::MaxScriptSigners>>;
 
     /// MoveVM pallet configuration trait
     #[pallet::config]
@@ -87,12 +86,6 @@ pub mod pallet {
         #[pallet::constant]
         type MaxScriptSigners: Get<u32>;
 
-        /// The overarching call type.
-        type RuntimeCall: Parameter
-            + Dispatchable<RuntimeOrigin = Self::RuntimeOrigin, PostInfo = PostDispatchInfo>
-            + GetDispatchInfo
-            + From<frame_system::Call<Self>>;
-
         /// Because this pallet emits events, it depends on the runtime's definition of an event.
         type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
 
@@ -203,6 +196,7 @@ pub mod pallet {
             // Make sure the scripts are not maliciously trying to use forged signatures.
             let signer_count =
                 verify_script_integrity_and_check_signers(&bytecode).map_err(Error::<T>::from)?;
+            let accounts = Self::extract_account_ids_from_args(&args, signer_count)?;
 
             let (mut signature_handler, call_hash) = if signer_count > 1 {
                 // Generate the call hash to identify this multi-sign call request.
@@ -211,16 +205,13 @@ pub mod pallet {
                 let call_hash: CallHash = hasher.finalize().into();
 
                 let multisig = MultisigStorage::<T>::get(call_hash).unwrap_or(
-                    Multisig::<T::MaxScriptSigners>::new(&args, signer_count)
+                    Multisig::<T::AccountId, T::MaxScriptSigners>::new(accounts)
                         .map_err(Into::<Error<T>>::into)?,
                 );
 
                 (ScriptSignatureHandler::<T>::from(multisig), call_hash)
             } else {
-                (
-                    ScriptSignatureHandler::<T>::new(&args, signer_count)?,
-                    [0u8; 32],
-                )
+                (ScriptSignatureHandler::<T>::new(accounts)?, [0u8; 32])
             };
             if signer_count > 0 {
                 signature_handler.sign_script(&who, cheque_limit.into())?;
@@ -458,6 +449,25 @@ pub mod pallet {
             vm.get_resource(&address, tag)
                 .map_err(|e| format!("error in get_resource: {e:?}").into())
         }
+
+        fn extract_account_ids_from_args(
+            script_args: &[&[u8]],
+            signer_count: usize,
+        ) -> Result<Vec<T::AccountId>, Error<T>> {
+            if signer_count > script_args.len() {
+                return Err(Error::<T>::ScriptSignatureFailure);
+            }
+
+            let mut accounts = Vec::<T::AccountId>::new();
+            for signer in &script_args[..signer_count] {
+                let account_address =
+                    bcs::from_bytes(signer).map_err(|_| Error::<T>::UnableToDeserializeAccount)?;
+                let account = Self::to_native_account(&account_address)?;
+                accounts.push(account);
+            }
+
+            Ok(accounts)
+        }
     }
 
     #[pallet::error]
@@ -483,6 +493,8 @@ pub mod pallet {
         StdlibAddressNotAllowed,
         /// Error about signing multi-signature execution request twice.
         UserHasAlreadySigned,
+        /// Script contains more signers than allowed maximum number of signers.
+        MaxSignersExceeded,
 
         // Errors that can be received from MoveVM
         /// Unknown validation status

src/mock.rs

@@ -77,14 +77,13 @@ impl pallet_balances::Config for Test {
 
 parameter_types! {
     pub const MaxRequestLifetime: u32 = 5;
-    pub const MaxScriptSigners: u32 = 3;
+    pub const MaxScriptSigners: u32 = 8;
 }
 
 impl pallet_move::Config for Test {
     type Currency = Balances;
     type MaxRequestLifetime = MaxRequestLifetime;
     type MaxScriptSigners = MaxScriptSigners;
-    type RuntimeCall = RuntimeCall;
     type RuntimeEvent = RuntimeEvent;
     type WeightInfo = ();
 }
@@ -149,23 +148,30 @@ pub const BOB_ADDR: &str = "5FHneW46xGXgs5mUiveU4sbTyGBzmstUspZC92UhjJM694ty";
 pub const ALICE_ADDR: &str = "5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY";
 pub const DAVE_ADDR: &str = "5DAAnrj7VHTznn2AWBemMuyBwZWs6FNFjdyVXUeYum3PTXFy";
 
-pub fn addr32_from_ss58(ss58addr: &str) -> Result<AccountId32, Error<Test>> {
+pub(crate) fn addr32_from_ss58(ss58addr: &str) -> Result<AccountId32, Error<Test>> {
     let (pk, _) = Public::from_ss58check_with_version(ss58addr)
         .map_err(|_| Error::<Test>::InvalidAccountSize)?;
     let account: AccountId32 = pk.into();
     Ok(account)
 }
 
-pub fn addr32_to_move(addr32: &AccountId32) -> Result<AccountAddress, Error<Test>> {
+pub(crate) fn addr32_to_move(addr32: &AccountId32) -> Result<AccountAddress, Error<Test>> {
     MoveModule::to_move_address(addr32)
 }
 
-pub fn addrs_from_ss58(ss58: &str) -> Result<(AccountId32, AccountAddress), Error<Test>> {
+pub(crate) fn addrs_from_ss58(ss58: &str) -> Result<(AccountId32, AccountAddress), Error<Test>> {
     let addr_32 = addr32_from_ss58(ss58)?;
     let addr_mv = addr32_to_move(&addr_32)?;
     Ok((addr_32, addr_mv))
 }
 
+pub(crate) fn int_to_addr32(n: u128) -> AccountId32 {
+    let mut addr = [0u8; 32];
+    let bytes: [u8; 16] = n.to_be_bytes();
+    bytes.iter().enumerate().for_each(|(i, b)| addr[i] = *b);
+    AccountId32::from(addr)
+}
+
 pub mod assets {
     const MOVE_PROJECTS: &str = "src/tests/assets/move-projects";
 

src/signer.rs

@@ -1,14 +1,14 @@
 use core::marker::PhantomData;
 
 use codec::{Decode, Encode, MaxEncodedLen};
-use frame_support::{pallet_prelude::RuntimeDebug, traits::Get, BoundedBTreeMap};
+use frame_support::{pallet_prelude::RuntimeDebug, traits::Get, BoundedBTreeMap, Parameter};
 use frame_system::Config as SysConfig;
-use move_core_types::account_address::AccountAddress;
 use scale_info::TypeInfo;
+use sp_runtime::traits::MaybeSerializeDeserialize;
 
 use crate::{
     balance::{BalanceAdapter, BalanceOf},
-    Config, Error, Pallet,
+    Config, Error,
 };
 
 /// This definition stores the hash value of a script transaction.
@@ -26,6 +26,7 @@ pub enum Signature {
 
 #[derive(Clone, Eq, PartialEq)]
 pub enum MultisigError {
+    MaxSignersExceeded,
     ScriptSignatureFailure,
     UnableToDeserializeAccount,
     UserHasAlreadySigned,
@@ -34,6 +35,7 @@ pub enum MultisigError {
 impl<T> From<MultisigError> for Error<T> {
     fn from(err: MultisigError) -> Self {
         match err {
+            MultisigError::MaxSignersExceeded => Error::<T>::MaxSignersExceeded,
             MultisigError::ScriptSignatureFailure => Error::<T>::ScriptSignatureFailure,
             MultisigError::UnableToDeserializeAccount => Error::<T>::UnableToDeserializeAccount,
             MultisigError::UserHasAlreadySigned => Error::<T>::UserHasAlreadySigned,
@@ -50,52 +52,64 @@ pub struct SignerData {
 }
 
 #[derive(Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug, TypeInfo, MaxEncodedLen)]
-#[scale_info(skip_type_params(S))]
-pub struct Multisig<S>(BoundedBTreeMap<AccountAddress, SignerData, S>)
+#[scale_info(skip_type_params(Size))]
+pub struct Multisig<AccountId, Size>(BoundedBTreeMap<AccountId, SignerData, Size>)
 where
-    S: Get<u32>;
+    AccountId: Parameter + Ord + MaybeSerializeDeserialize,
+    Size: Get<u32>;
 
-impl<S> Multisig<S>
+impl<AccountId, Size> Multisig<AccountId, Size>
 where
-    S: Get<u32>,
+    AccountId: Parameter + Ord + MaybeSerializeDeserialize,
+    Size: Get<u32>,
 {
     /// Creates a new [`Multisig`] with all blank signatures for the provided script.
-    pub fn new(script_args: &[&[u8]], signer_count: usize) -> Result<Self, MultisigError> {
-        if signer_count > script_args.len() || signer_count > (S::get() as usize) {
-            return Err(MultisigError::ScriptSignatureFailure);
+    pub fn new(signers: Vec<AccountId>) -> Result<Self, MultisigError> {
+        if signers.len() > (Size::get() as usize) {
+            return Err(MultisigError::MaxSignersExceeded);
         }
 
-        let mut multisig_info = Multisig::<S>::default();
-        for signer in &script_args[..signer_count] {
-            let account_address =
-                bcs::from_bytes(signer).map_err(|_| MultisigError::UnableToDeserializeAccount)?;
+        let mut multisig_info = Multisig::<AccountId, Size>::default();
+        for account in signers.iter() {
             multisig_info
-                .try_insert(account_address, SignerData::default())
+                .try_insert(account.clone(), SignerData::default())
                 .map_err(|_| MultisigError::UnableToDeserializeAccount)?;
         }
 
         Ok(multisig_info)
     }
 }
 
-impl<S: Get<u32>> core::ops::Deref for Multisig<S> {
-    type Target = BoundedBTreeMap<AccountAddress, SignerData, S>;
+impl<AccountId, Size> core::ops::Deref for Multisig<AccountId, Size>
+where
+    AccountId: Parameter + Ord + MaybeSerializeDeserialize,
+    Size: Get<u32>,
+{
+    type Target = BoundedBTreeMap<AccountId, SignerData, Size>;
 
     fn deref(&self) -> &Self::Target {
         &self.0
     }
 }
 
-impl<S: Get<u32>> core::ops::DerefMut for Multisig<S> {
+impl<AccountId, Size> core::ops::DerefMut for Multisig<AccountId, Size>
+where
+    AccountId: Parameter + Ord + MaybeSerializeDeserialize,
+    Size: Get<u32>,
+{
     fn deref_mut(&mut self) -> &mut Self::Target {
         &mut self.0
     }
 }
 
 // Because in substrate_move::AccountAddress Default impl is missing.
-impl<S: Get<u32>> Default for Multisig<S> {
+impl<AccountId, Size> Default for Multisig<AccountId, Size>
+where
+    AccountId: Parameter + Ord + MaybeSerializeDeserialize,
+    Size: Get<u32>,
+{
     fn default() -> Self {
-        Multisig(BoundedBTreeMap::<AccountAddress, SignerData, S>::new())
+        Multisig(BoundedBTreeMap::<AccountId, SignerData, Size>::new())
     }
 }
 
@@ -104,23 +118,25 @@ impl<S: Get<u32>> Default for Multisig<S> {
 pub(crate) struct ScriptSignatureHandler<T>
 where
     T: Config + SysConfig,
+    T::AccountId: Parameter + Ord + MaybeSerializeDeserialize,
     BalanceOf<T>: From<u128> + Into<u128>,
 {
     _pd_config: PhantomData<T>,
     /// All required multisig_info.
-    multisig_info: Multisig<T::MaxScriptSigners>,
+    multisig_info: Multisig<T::AccountId, T::MaxScriptSigners>,
 }
 
 impl<T> ScriptSignatureHandler<T>
 where
     T: Config + SysConfig,
+    T::AccountId: Parameter + Ord + MaybeSerializeDeserialize,
     BalanceOf<T>: From<u128> + Into<u128>,
 {
     /// Creates a new [`ScriptSignatureHandler`] with all blank signatures for the provided script.
-    pub(crate) fn new(script_args: &[&[u8]], signer_count: usize) -> Result<Self, Error<T>> {
+    pub(crate) fn new(accounts: Vec<T::AccountId>) -> Result<Self, Error<T>> {
         Ok(Self {
             _pd_config: PhantomData,
-            multisig_info: Multisig::<T::MaxScriptSigners>::new(script_args, signer_count)
+            multisig_info: Multisig::<T::AccountId, T::MaxScriptSigners>::new(accounts)
                 .map_err(Into::<Error<T>>::into)?,
         })
     }
@@ -137,18 +153,16 @@ where
         account: &T::AccountId,
         cheque_limit: u128,
     ) -> Result<(), Error<T>> {
-        let account_address = Pallet::<T>::to_move_address(account)?;
-
-        if let Some(ms_data) = self.multisig_info.get_mut(&account_address) {
+        if let Some(ms_data) = self.multisig_info.get_mut(account) {
             if matches!(ms_data.signature, Signature::Approved) {
-                Err(Error::UserHasAlreadySigned)
+                Err(Error::<T>::UserHasAlreadySigned)
             } else {
                 ms_data.signature = Signature::Approved;
                 ms_data.cheque_limit = cheque_limit;
                 Ok(())
             }
         } else {
-            Err(Error::ScriptSignatureFailure)
+            Err(Error::<T>::ScriptSignatureFailure)
         }
     }
 
@@ -167,38 +181,33 @@ where
         }
 
         let mut balances = BalanceAdapter::<T>::new();
-        for (address, ms_data) in self.multisig_info.iter() {
-            let account = Pallet::<T>::to_native_account(address)?;
+        for (account, ms_data) in self.multisig_info.iter() {
             balances
-                .write_cheque(&account, &BalanceOf::<T>::from(ms_data.cheque_limit))
+                .write_cheque(account, &BalanceOf::<T>::from(ms_data.cheque_limit))
                 .map_err(|_| Error::<T>::InsufficientBalance)?;
         }
 
         Ok(balances)
     }
 
-    /// Consumes `ScriptSignatureHandler` and returns innner `Multisig`.
-    pub(crate) fn into_inner(self) -> Multisig<T::MaxScriptSigners> {
+    /// Consumes [`ScriptSignatureHandler`] and returns innner `Multisig`.
+    pub(crate) fn into_inner(self) -> Multisig<T::AccountId, T::MaxScriptSigners> {
         self.multisig_info
     }
 
-    /// Consumes `ScriptSignatureHandler` and returns accounts of all signers as vector.
+    /// Consumes [`ScriptSignatureHandler`] and returns accounts of all signers as vector.
     pub(crate) fn into_signer_accounts(self) -> Result<Vec<T::AccountId>, Error<T>> {
-        let mut accounts = Vec::<T::AccountId>::new();
-        for key in self.multisig_info.keys() {
-            let acc = Pallet::<T>::to_native_account(key)?;
-            accounts.push(acc);
-        }
+        let accounts: Vec<T::AccountId> = self.multisig_info.keys().cloned().collect();
         Ok(accounts)
     }
 }
 
-impl<T> From<Multisig<T::MaxScriptSigners>> for ScriptSignatureHandler<T>
+impl<T> From<Multisig<T::AccountId, T::MaxScriptSigners>> for ScriptSignatureHandler<T>
 where
     T: Config + SysConfig,
     BalanceOf<T>: From<u128> + Into<u128>,
 {
-    fn from(multisig_info: Multisig<T::MaxScriptSigners>) -> Self {
+    fn from(multisig_info: Multisig<T::AccountId, T::MaxScriptSigners>) -> Self {
         Self {
             _pd_config: PhantomData,
             multisig_info,