diff --git a/.github/workflows/common.yml b/.github/workflows/common.yml index 2247813..36e9010 100644 --- a/.github/workflows/common.yml +++ b/.github/workflows/common.yml @@ -3,7 +3,7 @@ name: Common jobs (fmt, clippy, test) on: pull_request: paths: - - src/** + - pallet/** - Cargo.toml - rpc/** - .github/workflows/common.yml diff --git a/pallet/src/lib.rs b/pallet/src/lib.rs index 986f542..622a90a 100644 --- a/pallet/src/lib.rs +++ b/pallet/src/lib.rs @@ -34,14 +34,16 @@ pub mod pallet { extern crate alloc; #[cfg(not(feature = "std"))] use alloc::format; - use blake2::{Blake2s256, Digest}; + use blake2::{digest::consts::U8, Blake2b, Blake2s256, Digest}; use core::convert::AsRef; use codec::{FullCodec, FullEncode}; use frame_support::{ dispatch::DispatchResultWithPostInfo, pallet_prelude::*, - traits::{Currency, Get, LockableCurrency, ReservableCurrency}, + traits::{ + tokens::currency::LockIdentifier, Currency, Get, LockableCurrency, ReservableCurrency, + }, }; use frame_system::pallet_prelude::*; pub use move_core_types::language_storage::TypeTag; @@ -247,7 +249,6 @@ pub mod pallet { gas_limit: u64, cheque_limit: BalanceOf, ) -> DispatchResultWithPostInfo { - // TODO(neutrinoks): Add currency locking in multi-signature executions. // A signer for the extrinsic and a signer for the Move script. let who = ensure_signed(origin)?; @@ -286,7 +287,8 @@ pub mod pallet { ) }; if signer_count > 0 { - signature_handler.sign_script(&who, cheque_limit.into())?; + let lock_id = Self::multi_signer_lock_id(&who, &transaction_bc[..], &cheque_limit); + signature_handler.sign_script(&who, cheque_limit.into(), lock_id)?; } // If the script is signed correctly, we can execute it in MoveVM and update the @@ -561,6 +563,18 @@ pub mod pallet { hasher.update(transaction_bc); hasher.finalize().into() } + + pub fn multi_signer_lock_id( + who: &T::AccountId, + transaction_bc: &[u8], + cheque_limit: &BalanceOf, + ) -> LockIdentifier { + let mut hasher = Blake2b::::new(); + hasher.update(transaction_bc); + hasher.update(&who.encode()[..]); + hasher.update(&cheque_limit.encode()[..]); + hasher.finalize().into() + } } #[pallet::error] diff --git a/pallet/src/mock.rs b/pallet/src/mock.rs index 6d3966e..bcb51ce 100644 --- a/pallet/src/mock.rs +++ b/pallet/src/mock.rs @@ -1,4 +1,6 @@ use frame_support::{ + dispatch::DispatchErrorWithPostInfo, + pallet_prelude::{DispatchError, DispatchResultWithPostInfo}, parameter_types, traits::{ConstU128, ConstU16, ConstU32, ConstU64, OnFinalize, OnIdle, OnInitialize}, }; @@ -181,6 +183,22 @@ pub(crate) fn last_event() -> RuntimeEvent { System::events().pop().expect("Event expected").event } +pub(crate) fn verify_module_error_with_msg( + res: DispatchResultWithPostInfo, + e_msg: &str, +) -> Result { + if let Err(DispatchErrorWithPostInfo { + error: DispatchError::Module(moderr), + .. + }) = res + { + if let Some(msg) = moderr.message { + return Ok(msg == e_msg); + } + } + Err(format!("{res:?} does not match '{e_msg}'")) +} + pub mod assets { const MOVE_PROJECTS: &str = "src/tests/assets/move-projects"; diff --git a/pallet/src/signer.rs b/pallet/src/signer.rs index 456afb2..121964a 100644 --- a/pallet/src/signer.rs +++ b/pallet/src/signer.rs @@ -1,7 +1,17 @@ use core::marker::PhantomData; use codec::{Decode, Encode, MaxEncodedLen}; -use frame_support::{pallet_prelude::RuntimeDebug, traits::Get, BoundedBTreeMap, Parameter}; +use frame_support::{ + pallet_prelude::RuntimeDebug, + traits::{ + tokens::{ + currency::{LockIdentifier, LockableCurrency}, + WithdrawReasons, + }, + Get, + }, + BoundedBTreeMap, Parameter, +}; use frame_system::{pallet_prelude::BlockNumberFor, Config as SysConfig}; use scale_info::TypeInfo; use sp_runtime::traits::MaybeSerializeDeserialize; @@ -54,6 +64,8 @@ pub struct SignerData { pub signature: Signature, /// Individual cheque-limit. pub cheque_limit: u128, + /// Lock ID for locked currency. + pub lock_id: LockIdentifier, } /// Storage struct definition for a multi-signer request. @@ -183,6 +195,7 @@ where &mut self, account: &T::AccountId, cheque_limit: u128, + lock_id: LockIdentifier, ) -> Result<(), Error> { if let Some(ms_data) = self.multisig_info.get_mut(account) { if matches!(ms_data.signature, Signature::Approved) { @@ -190,6 +203,9 @@ where } else { ms_data.signature = Signature::Approved; ms_data.cheque_limit = cheque_limit; + ms_data.lock_id = lock_id; + let amount = BalanceOf::::from(cheque_limit); + T::Currency::set_lock(lock_id, account, amount, WithdrawReasons::all()); Ok(()) } } else { @@ -209,6 +225,7 @@ where pub(crate) fn write_cheques(&self) -> Result, Error> { let mut balances = BalanceAdapter::::new(); for (account, ms_data) in self.multisig_info.iter() { + T::Currency::remove_lock(ms_data.lock_id, account); balances .write_cheque(account, &BalanceOf::::from(ms_data.cheque_limit)) .map_err(|_| Error::::InsufficientBalance)?; diff --git a/pallet/src/tests/signer.rs b/pallet/src/tests/signer.rs index 02ce490..a89ce6f 100644 --- a/pallet/src/tests/signer.rs +++ b/pallet/src/tests/signer.rs @@ -1,6 +1,10 @@ use crate::{mock::*, no_type_args, script_transaction, Error, Event, MultisigStorage}; -use frame_support::{assert_err, assert_ok, pallet_prelude::DispatchResultWithPostInfo}; +use frame_support::{ + assert_err, assert_ok, + pallet_prelude::DispatchResultWithPostInfo, + traits::{tokens::WithdrawReasons, Currency}, +}; use move_core_types::{language_storage::TypeTag, u256::U256}; use move_vm_backend::types::MAX_GAS_AMOUNT; use rand::{distributions::Standard, prelude::Distribution, rngs::ThreadRng, Rng}; @@ -296,6 +300,7 @@ fn script_with_vector_containing_signer_fails() { #[test] fn multiple_signers_in_multisig_script_works() { const BALANCE: Balance = 80_000_000_000_000; + const CHANGE: Balance = 20_000_000_000_000; let (bob_addr_32, bob_addr_mv) = addrs_from_ss58(BOB_ADDR).unwrap(); let (alice_addr_32, alice_addr_mv) = addrs_from_ss58(ALICE_ADDR).unwrap(); let (dave_addr_32, dave_addr_mv) = addrs_from_ss58(DAVE_ADDR).unwrap(); @@ -353,7 +358,29 @@ fn multiple_signers_in_multisig_script_works() { &2u8 ); let call_hash = MoveModule::transaction_bc_call_hash(&transaction_bc[..]); + + // Verify that no lock has been set so far and that the Multisig request entry cannot + // be found in storage. assert!(MultisigStorage::::try_get(call_hash).is_err()); + assert_ok!(Balances::ensure_can_withdraw( + &alice_addr_32, + BALANCE, + WithdrawReasons::TRANSFER, + 0 + )); + assert_ok!(Balances::ensure_can_withdraw( + &dave_addr_32, + BALANCE, + WithdrawReasons::TRANSFER, + 0 + )); + assert_ok!(Balances::ensure_can_withdraw( + &eve_addr_32, + BALANCE, + WithdrawReasons::TRANSFER, + 0 + )); + assert_ok!(MoveModule::execute( RuntimeOrigin::signed(alice_addr_32.clone()), transaction_bc.clone(), @@ -367,6 +394,25 @@ fn multiple_signers_in_multisig_script_works() { }) ); assert!(MultisigStorage::::try_get(call_hash).is_ok()); + assert!(Balances::ensure_can_withdraw( + &alice_addr_32, + BALANCE, + WithdrawReasons::TRANSFER, + 0 + ) + .is_err()); + assert_ok!(Balances::ensure_can_withdraw( + &dave_addr_32, + BALANCE, + WithdrawReasons::TRANSFER, + 0 + )); + assert_ok!(Balances::ensure_can_withdraw( + &eve_addr_32, + BALANCE, + WithdrawReasons::TRANSFER, + 0 + )); assert_ok!(MoveModule::execute( RuntimeOrigin::signed(dave_addr_32.clone()), @@ -380,6 +426,27 @@ fn multiple_signers_in_multisig_script_works() { who: dave_addr_32.clone() }) ); + // Now this candidate should also not be able to transfer the locked tokens. + assert!(Balances::ensure_can_withdraw( + &alice_addr_32, + BALANCE, + WithdrawReasons::TRANSFER, + 0 + ) + .is_err()); + assert!(Balances::ensure_can_withdraw( + &dave_addr_32, + BALANCE, + WithdrawReasons::TRANSFER, + 0 + ) + .is_err()); + assert_ok!(Balances::ensure_can_withdraw( + &eve_addr_32, + BALANCE, + WithdrawReasons::TRANSFER, + 0 + )); assert_ok!(MoveModule::execute( RuntimeOrigin::signed(eve_addr_32.clone()), @@ -397,6 +464,24 @@ fn multiple_signers_in_multisig_script_works() { ] }) ); + assert_ok!(Balances::ensure_can_withdraw( + &alice_addr_32, + CHANGE, + WithdrawReasons::TRANSFER, + 0 + )); + assert_ok!(Balances::ensure_can_withdraw( + &dave_addr_32, + CHANGE, + WithdrawReasons::TRANSFER, + 0 + )); + assert_ok!(Balances::ensure_can_withdraw( + &eve_addr_32, + CHANGE, + WithdrawReasons::TRANSFER, + 0 + )); // Now after all signers have signed the multsig script, we can expect that the script // will be executed and the multisg storage will remove the pending mutlisig script // data, since the script has been executed. @@ -448,7 +533,8 @@ fn verify_old_multi_signer_requests_getting_removed() { no_type_args!(), &alice_addr_mv, &dave_addr_mv, - &eve_addr_mv + &eve_addr_mv, + &2u8 ); assert_ok!(MoveModule::execute( RuntimeOrigin::signed(alice_addr_32.clone()), @@ -497,3 +583,71 @@ fn verify_old_multi_signer_requests_getting_removed() { ); }) } + +#[test] +fn cheque_limit_in_multi_signer_execution_works() { + const BALANCE: Balance = 80_000_000_000_000; + let (bob_addr_32, bob_addr_mv) = addrs_from_ss58(BOB_ADDR).unwrap(); + let (alice_addr_32, alice_addr_mv) = addrs_from_ss58(ALICE_ADDR).unwrap(); + let (dave_addr_32, dave_addr_mv) = addrs_from_ss58(DAVE_ADDR).unwrap(); + let (eve_addr_32, eve_addr_mv) = addrs_from_ss58(EVE_ADDR).unwrap(); + + ExtBuilder::default() + .with_balances(vec![ + (bob_addr_32.clone(), BALANCE), + (alice_addr_32.clone(), BALANCE), + (dave_addr_32.clone(), BALANCE), + (eve_addr_32.clone(), BALANCE), + ]) + .build() + .execute_with(|| { + // Roll to first block in case of block based event checkings and processes. + roll_to(1); + + // Initialisation & Setup by developer Bob. + let module = assets::read_module_from_project("multiple-signers", "Dorm"); + assert_ok!(MoveModule::publish_module( + RuntimeOrigin::signed(bob_addr_32.clone()), + module, + MAX_GAS_AMOUNT + )); + let script = assets::read_script_from_project("multiple-signers", "init_module"); + let transaction_bc = script_transaction!(script, no_type_args!(), &bob_addr_mv); + assert_ok!(MoveModule::execute( + RuntimeOrigin::signed(bob_addr_32.clone()), + transaction_bc.clone(), + MAX_GAS_AMOUNT, + BALANCE, + )); + + // Now only 2 of 3 planned signers will sign the script execution. + let script = assets::read_script_from_project("multiple-signers", "rent_apartment"); + let transaction_bc = script_transaction!( + script, + no_type_args!(), + &alice_addr_mv, + &dave_addr_mv, + &eve_addr_mv, + &2u8 + ); + assert_ok!(MoveModule::execute( + RuntimeOrigin::signed(dave_addr_32.clone()), + transaction_bc.clone(), + MAX_GAS_AMOUNT, + BALANCE, + )); + assert_ok!(MoveModule::execute( + RuntimeOrigin::signed(eve_addr_32.clone()), + transaction_bc.clone(), + MAX_GAS_AMOUNT, + BALANCE, + )); + let res = MoveModule::execute( + RuntimeOrigin::signed(alice_addr_32.clone()), + transaction_bc.clone(), + MAX_GAS_AMOUNT, + BALANCE / 2, + ); + assert!(verify_module_error_with_msg(res, "Aborted").unwrap()); + }) +} diff --git a/pallet/src/tests/update_stdlib.rs b/pallet/src/tests/update_stdlib.rs index 2ad71d3..68af93d 100644 --- a/pallet/src/tests/update_stdlib.rs +++ b/pallet/src/tests/update_stdlib.rs @@ -3,9 +3,7 @@ use crate::mock::*; use crate::{no_type_args, script_transaction}; -use frame_support::{ - assert_err, assert_ok, dispatch::DispatchErrorWithPostInfo, pallet_prelude::*, -}; +use frame_support::{assert_err, assert_ok, pallet_prelude::*}; use move_stdlib::move_stdlib_bundle; use move_vm_backend::types::MAX_GAS_AMOUNT; @@ -17,22 +15,6 @@ fn mock_substrate_stdlib() -> Vec { assets::read_bundle_from_project("testing-substrate-stdlib", "testing-substrate-stdlib") } -fn verify_module_error_with_msg( - res: DispatchResultWithPostInfo, - e_msg: &str, -) -> Result { - if let Err(DispatchErrorWithPostInfo { - error: DispatchError::Module(moderr), - .. - }) = res - { - if let Some(msg) = moderr.message { - return Ok(msg == e_msg); - } - } - Err(format!("{res:?} does not match '{e_msg}'")) -} - #[test] fn regular_user_update_fail() { let bob_addr_native = addr32_from_ss58(BOB_ADDR).unwrap();