From e4f5a8e641c51d1702cbf8d60eebbb507458481b Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov <39522748+dmitrylavrenov@users.noreply.github.com> Date: Fri, 19 Apr 2024 15:16:15 +0300 Subject: [PATCH] Fix removing account logic at `pallet-evm-system` (#111) (#116) (#119) * Fix removing account logic at `pallet-evm-system` (#111) (#116) * Add assert_total_issuance_invariant helper * Add evm_system_removing_account_non_zero_balance test * Fix removing account logic * Undo redundant changes * Redesign fix usage * Undo try_mutate_exists_account_removed changes * Rename Exists account removal status into Remains * Improve docs * Apply renaming at tests * Use assert_storage_noop * Remove rusttoolchain * Use u64 accounts at evm_system_removing_account_non_zero_balance test --- frame/evm-balances/src/tests/currency.rs | 52 ++++++++++++++++++------ frame/evm-balances/src/tests/fungible.rs | 34 +++++++++++++++- frame/evm-balances/src/tests/mod.rs | 52 ++++++++++++++++++++++++ frame/evm-system/src/lib.rs | 38 +++++++++++++---- frame/evm-system/src/tests.rs | 24 +++++++---- 5 files changed, 171 insertions(+), 29 deletions(-) diff --git a/frame/evm-balances/src/tests/currency.rs b/frame/evm-balances/src/tests/currency.rs index 635343b451..2f1165543e 100644 --- a/frame/evm-balances/src/tests/currency.rs +++ b/frame/evm-balances/src/tests/currency.rs @@ -3,7 +3,7 @@ use frame_support::{assert_noop, assert_ok, traits::Currency}; use sp_runtime::TokenError; -use crate::{mock::*, *}; +use crate::{mock::*, tests::assert_total_issuance_invariant, *}; #[test] fn total_issuance_works() { @@ -66,6 +66,8 @@ fn deactivate_reactivate_works() { EvmBalances::reactivate(40); // Assert state changes. assert_eq!(>::get(), 60); + + assert_total_issuance_invariant(); }); } @@ -90,6 +92,8 @@ fn burn_works() { assert_eq!(EvmBalances::total_issuance(), 2 * INIT_BALANCE - 100); drop(imbalance); assert_eq!(EvmBalances::total_issuance(), 2 * INIT_BALANCE); + + assert_total_issuance_invariant(); }); } @@ -106,6 +110,8 @@ fn issue_works() { assert_eq!(EvmBalances::total_issuance(), 2 * INIT_BALANCE + 100); drop(imbalance); assert_eq!(EvmBalances::total_issuance(), 2 * INIT_BALANCE); + + assert_total_issuance_invariant(); }); } @@ -142,6 +148,8 @@ fn transfer_works() { to: bob(), amount: transfered_amount, })); + + assert_total_issuance_invariant(); }); } @@ -182,6 +190,8 @@ fn transfer_works_full_balance() { System::assert_has_event(RuntimeEvent::EvmSystem( pallet_evm_system::Event::KilledAccount { account: alice() }, )); + + assert_total_issuance_invariant(); }); } @@ -256,6 +266,8 @@ fn slash_works() { who: alice(), amount: slashed_amount, })); + + assert_total_issuance_invariant(); }); } @@ -286,6 +298,8 @@ fn slash_works_full_balance() { System::assert_has_event(RuntimeEvent::EvmSystem( pallet_evm_system::Event::KilledAccount { account: alice() }, )); + + assert_total_issuance_invariant(); }); } @@ -301,10 +315,8 @@ fn deposit_into_existing_works() { System::set_block_number(1); // Invoke the function under test. - assert_ok!(EvmBalances::deposit_into_existing( - &alice(), - deposited_amount - )); + let imbalance = EvmBalances::deposit_into_existing(&alice(), deposited_amount).unwrap(); + drop(imbalance); // Assert state changes. assert_eq!( @@ -315,6 +327,8 @@ fn deposit_into_existing_works() { who: alice(), amount: deposited_amount, })); + + assert_total_issuance_invariant(); }); } @@ -376,6 +390,8 @@ fn deposit_creating_works() { System::assert_has_event(RuntimeEvent::EvmSystem( pallet_evm_system::Event::NewAccount { account: charlie }, )); + + assert_total_issuance_invariant(); }); } @@ -391,12 +407,14 @@ fn withdraw_works() { System::set_block_number(1); // Invoke the function under test. - assert_ok!(EvmBalances::withdraw( + let imbalance = EvmBalances::withdraw( &alice(), withdrawed_amount, WithdrawReasons::FEE, - ExistenceRequirement::KeepAlive - )); + ExistenceRequirement::KeepAlive, + ) + .unwrap(); + drop(imbalance); // Assert state changes. assert_eq!( @@ -407,6 +425,8 @@ fn withdraw_works() { who: alice(), amount: withdrawed_amount, })); + + assert_total_issuance_invariant(); }); } @@ -422,12 +442,14 @@ fn withdraw_works_full_balance() { System::set_block_number(1); // Invoke the function under test. - assert_ok!(EvmBalances::withdraw( + let imbalance = EvmBalances::withdraw( &alice(), withdrawed_amount, - WithdrawReasons::TRANSFER, - ExistenceRequirement::AllowDeath - )); + WithdrawReasons::FEE, + ExistenceRequirement::AllowDeath, + ) + .unwrap(); + drop(imbalance); // Assert state changes. assert_eq!( @@ -442,6 +464,8 @@ fn withdraw_works_full_balance() { System::assert_has_event(RuntimeEvent::EvmSystem( pallet_evm_system::Event::KilledAccount { account: alice() }, )); + + assert_total_issuance_invariant(); }); } @@ -502,6 +526,8 @@ fn make_free_balance_be_works() { // Assert state changes. assert_eq!(EvmBalances::total_balance(&charlie), made_free_balance); + + assert_total_issuance_invariant(); }); } @@ -528,6 +554,8 @@ fn evm_system_account_should_be_reaped() { System::assert_has_event(RuntimeEvent::EvmSystem( pallet_evm_system::Event::KilledAccount { account: bob() }, )); + + assert_total_issuance_invariant(); }); } diff --git a/frame/evm-balances/src/tests/fungible.rs b/frame/evm-balances/src/tests/fungible.rs index 3b221c5e3d..df7a700c14 100644 --- a/frame/evm-balances/src/tests/fungible.rs +++ b/frame/evm-balances/src/tests/fungible.rs @@ -9,7 +9,7 @@ use frame_support::{ }; use sp_runtime::TokenError; -use crate::{mock::*, *}; +use crate::{mock::*, tests::assert_total_issuance_invariant, *}; #[test] fn total_issuance_works() { @@ -353,6 +353,8 @@ fn deactivate_reactivate_works() { EvmBalances::reactivate(40); // Assert state changes. assert_eq!(>::get(), 60); + + assert_total_issuance_invariant(); }); } @@ -384,6 +386,8 @@ fn mint_into_works() { who: alice(), amount: minted_balance, })); + + assert_total_issuance_invariant(); }); } @@ -436,6 +440,8 @@ fn burn_from_works() { who: alice(), amount: burned_balance, })); + + assert_total_issuance_invariant(); }); } @@ -470,6 +476,8 @@ fn burn_from_works_best_effort() { System::assert_has_event(RuntimeEvent::EvmSystem( pallet_evm_system::Event::KilledAccount { account: alice() }, )); + + assert_total_issuance_invariant(); }); } @@ -504,6 +512,8 @@ fn burn_from_works_exact_full_balance() { System::assert_has_event(RuntimeEvent::EvmSystem( pallet_evm_system::Event::KilledAccount { account: alice() }, )); + + assert_total_issuance_invariant(); }); } @@ -560,6 +570,8 @@ fn shelve_works() { who: alice(), amount: shelved_balance, })); + + assert_total_issuance_invariant(); }); } @@ -589,6 +601,8 @@ fn shelve_works_exact_full_balance() { System::assert_has_event(RuntimeEvent::EvmSystem( pallet_evm_system::Event::KilledAccount { account: alice() }, )); + + assert_total_issuance_invariant(); }); } @@ -640,6 +654,8 @@ fn restore_works() { who: alice(), amount: restored_balance, })); + + assert_total_issuance_invariant(); }); } @@ -692,6 +708,8 @@ fn transfer_works() { to: bob(), amount: transfered_amount, })); + + assert_total_issuance_invariant(); }); } @@ -732,6 +750,8 @@ fn transfer_works_full_balance() { System::assert_has_event(RuntimeEvent::EvmSystem( pallet_evm_system::Event::KilledAccount { account: alice() }, )); + + assert_total_issuance_invariant(); }); } @@ -815,6 +835,8 @@ fn rescind_works() { System::assert_has_event(RuntimeEvent::EvmBalances(Event::Rescinded { amount: rescinded_balance, })); + + assert_total_issuance_invariant(); }); } @@ -842,6 +864,8 @@ fn issue_works() { System::assert_has_event(RuntimeEvent::EvmBalances(Event::Issued { amount: issued_balance, })); + + assert_total_issuance_invariant(); }); } @@ -872,6 +896,8 @@ fn deposit_flow_works() { let _ = EvmBalances::settle(&bob(), debt, Preservation::Expendable); assert_eq!(EvmBalances::total_issuance(), 2 * INIT_BALANCE); + + assert_total_issuance_invariant(); }); } @@ -904,6 +930,8 @@ fn deposit_works_new_account() { System::assert_has_event(RuntimeEvent::EvmSystem( pallet_evm_system::Event::NewAccount { account: charlie }, )); + + assert_total_issuance_invariant(); }); } @@ -940,6 +968,8 @@ fn withdraw_works() { assert_eq!(EvmBalances::total_issuance(), 2 * INIT_BALANCE); let _ = EvmBalances::resolve(&bob(), credit); assert_eq!(EvmBalances::total_issuance(), 2 * INIT_BALANCE); + + assert_total_issuance_invariant(); }); } @@ -980,5 +1010,7 @@ fn withdraw_works_full_balance() { assert_eq!(EvmBalances::total_issuance(), 2 * INIT_BALANCE); let _ = EvmBalances::resolve(&bob(), credit); assert_eq!(EvmBalances::total_issuance(), 2 * INIT_BALANCE); + + assert_total_issuance_invariant(); }); } diff --git a/frame/evm-balances/src/tests/mod.rs b/frame/evm-balances/src/tests/mod.rs index 0c6a8cc418..1b03a031c2 100644 --- a/frame/evm-balances/src/tests/mod.rs +++ b/frame/evm-balances/src/tests/mod.rs @@ -11,6 +11,16 @@ mod currency; mod fungible; mod fungible_conformance; +fn assert_total_issuance_invariant() { + let iterated_total_issuance: u64 = >::iter_values() + .map(|account_data| account_data.data.total()) + .sum(); + + let total_issuance = EvmBalances::total_issuance(); + + assert_eq!(iterated_total_issuance, total_issuance); +} + #[test] fn basic_setup_works() { new_test_ext().execute_with_ext(|_| { @@ -23,6 +33,36 @@ fn basic_setup_works() { ::account(&bob()), account_data::AccountData { free: INIT_BALANCE } ); + + assert_total_issuance_invariant(); + }); +} + +#[test] +fn evm_system_removing_account_non_zero_balance() { + new_test_ext().execute_with_ext(|_| { + // Prepare test preconditions. + let contract = 3; + EVM::create_account(H160::from_low_u64_be(contract), vec![1, 2, 3]); + + // Transfer some balance to contract address. + assert_ok!(EvmBalances::transfer( + &alice(), + &contract, + 1000, + ExistenceRequirement::KeepAlive + )); + + assert_eq!(EvmBalances::free_balance(&contract), 1000); + + // Invoke the function under test. + EVM::remove_account(&H160::from_low_u64_be(contract)); + + // Assert state changes. + assert_eq!(EvmBalances::free_balance(&contract), 1000); + assert!(EvmSystem::account_exists(&contract)); + + assert_total_issuance_invariant(); }); } @@ -47,6 +87,8 @@ fn evm_fee_deduction() { // Refund fees as 5 units <::OnChargeTransaction as pallet_evm::OnChargeEVMTransaction>::correct_and_deposit_fee(&H160::from_low_u64_be(charlie), U256::from(5), U256::from(5), imbalance); assert_eq!(EvmBalances::free_balance(&charlie), 95); + + assert_total_issuance_invariant(); }); } @@ -83,6 +125,8 @@ fn evm_issuance_after_tip() { let after_tip = ::Currency::total_issuance(); assert_eq!(after_tip, (before_tip - (base_fee * 21_000))); + + assert_total_issuance_invariant(); }); } @@ -119,6 +163,8 @@ fn evm_refunds_should_work() { let total_cost = (U256::from(21_000) * base_fee) + U256::from(1); let after_call = EVM::account_basic(&alice_h160()).0.balance; assert_eq!(after_call, before_call - total_cost); + + assert_total_issuance_invariant(); }); } @@ -160,6 +206,8 @@ fn evm_refunds_and_priority_should_work() { let after_call = EVM::account_basic(&alice_h160()).0.balance; // The tip is deducted but never refunded to the caller. assert_eq!(after_call, before_call - total_cost); + + assert_total_issuance_invariant(); }); } @@ -191,6 +239,8 @@ fn evm_call_should_fail_with_priority_greater_than_max_fee() { assert!(result.is_err()); // Some used weight is returned as part of the error. assert_eq!(result.unwrap_err().weight, Weight::from_parts(7, 0)); + + assert_total_issuance_invariant(); }); } @@ -221,5 +271,7 @@ fn evm_call_should_succeed_with_priority_equal_to_max_fee() { ::config(), ); assert!(result.is_ok()); + + assert_total_issuance_invariant(); }); } diff --git a/frame/evm-system/src/lib.rs b/frame/evm-system/src/lib.rs index fe25d3aed3..fe2c422ca4 100644 --- a/frame/evm-system/src/lib.rs +++ b/frame/evm-system/src/lib.rs @@ -8,7 +8,7 @@ use frame_support::traits::StoredMap; use scale_codec::{Decode, Encode, FullCodec, MaxEncodedLen}; use scale_info::TypeInfo; -use sp_runtime::{traits::One, DispatchError, DispatchResult, RuntimeDebug}; +use sp_runtime::{traits::One, DispatchError, RuntimeDebug}; #[cfg(test)] mod mock; @@ -115,6 +115,26 @@ pub mod pallet { } } +/// The outcome of the account creation operation. +#[derive(Eq, PartialEq, RuntimeDebug)] +pub enum AccountCreationOutcome { + /// Account was created. + Created, + /// Account already exists in the system, so action was taken. + AlreadyExists, +} + +/// The outcome of the account removal operation. +#[derive(Eq, PartialEq, RuntimeDebug)] +pub enum AccountRemovalOutcome { + /// Account was destroyed and no longer exists. + Reaped, + /// Account was non-empty, and it was retained and still exists in the system. + Retained, + /// Account did not exist in the first place, so no action was taken. + DidNotExist, +} + impl Pallet { /// Check the account existence. pub fn account_exists(who: &::AccountId) -> bool { @@ -144,25 +164,29 @@ impl Pallet { } /// Create an account. - pub fn create_account(who: &::AccountId) -> DispatchResult { + pub fn create_account(who: &::AccountId) -> AccountCreationOutcome { if Self::account_exists(who) { - return Err(Error::::AccountAlreadyExist.into()); + return AccountCreationOutcome::AlreadyExists; } Account::::insert(who.clone(), AccountInfo::<_, _>::default()); Self::on_created_account(who.clone()); - Ok(()) + AccountCreationOutcome::Created } /// Remove an account. - pub fn remove_account(who: &::AccountId) -> DispatchResult { + pub fn remove_account(who: &::AccountId) -> AccountRemovalOutcome { if !Self::account_exists(who) { - return Err(Error::::AccountNotExist.into()); + return AccountRemovalOutcome::DidNotExist; + } + + if Account::::get(who).data != ::AccountData::default() { + return AccountRemovalOutcome::Retained; } Account::::remove(who); Self::on_killed_account(who.clone()); - Ok(()) + AccountRemovalOutcome::Reaped } } diff --git a/frame/evm-system/src/tests.rs b/frame/evm-system/src/tests.rs index b175d5e6c0..a7e747661a 100644 --- a/frame/evm-system/src/tests.rs +++ b/frame/evm-system/src/tests.rs @@ -2,7 +2,7 @@ use sp_std::str::FromStr; -use frame_support::{assert_noop, assert_ok}; +use frame_support::{assert_noop, assert_storage_noop}; use mockall::predicate; use sp_core::H160; @@ -30,7 +30,10 @@ fn create_account_works() { .return_const(()); // Invoke the function under test. - assert_ok!(EvmSystem::create_account(&account_id)); + assert_eq!( + EvmSystem::create_account(&account_id), + AccountCreationOutcome::Created + ); // Assert state changes. assert!(EvmSystem::account_exists(&account_id)); @@ -52,10 +55,10 @@ fn create_account_fails() { >::insert(account_id.clone(), AccountInfo::<_, _>::default()); // Invoke the function under test. - assert_noop!( + assert_storage_noop!(assert_eq!( EvmSystem::create_account(&account_id), - Error::::AccountAlreadyExist - ); + AccountCreationOutcome::AlreadyExists + )); }); } @@ -79,7 +82,10 @@ fn remove_account_works() { .return_const(()); // Invoke the function under test. - assert_ok!(EvmSystem::remove_account(&account_id)); + assert_eq!( + EvmSystem::remove_account(&account_id), + AccountRemovalOutcome::Reaped + ); // Assert state changes. assert!(!EvmSystem::account_exists(&account_id)); @@ -100,10 +106,10 @@ fn remove_account_fails() { let account_id = H160::from_str("1000000000000000000000000000000000000001").unwrap(); // Invoke the function under test. - assert_noop!( + assert_storage_noop!(assert_eq!( EvmSystem::remove_account(&account_id), - Error::::AccountNotExist - ); + AccountRemovalOutcome::DidNotExist + )); }); }