Fix removing account logic at pallet-evm-system

frame/evm-balances/src/tests.rs

@@ -8,6 +8,16 @@ use sp_std::str::FromStr;
 
 use crate::{mock::*, *};
 
+fn assert_total_issuance_invariant() {
+	let iterated_total_issuance: u64 = <pallet_evm_system::Account<Test>>::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,8 @@ fn basic_setup_works() {
 
 		// Check the total balance value.
 		assert_eq!(EvmBalances::total_issuance(), 2 * INIT_BALANCE);
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -72,6 +84,8 @@ fn currency_deactivate_reactivate_works() {
 		EvmBalances::reactivate(40);
 		// Assert state changes.
 		assert_eq!(<InactiveIssuance<Test>>::get(), 60);
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -88,6 +102,8 @@ fn currency_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();
 	});
 }
 
@@ -104,6 +120,8 @@ fn currency_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();
 	});
 }
 
@@ -140,6 +158,8 @@ fn currency_transfer_works() {
 			to: bob(),
 			amount: transfered_amount,
 		}));
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -166,6 +186,8 @@ fn currency_slash_works() {
 			who: alice(),
 			amount: slashed_amount,
 		}));
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -181,10 +203,8 @@ fn currency_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!(
@@ -195,6 +215,8 @@ fn currency_deposit_into_existing_works() {
 			who: alice(),
 			amount: deposited_amount,
 		}));
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -222,6 +244,8 @@ fn currency_deposit_creating_works() {
 		System::assert_has_event(RuntimeEvent::EvmSystem(
 			pallet_evm_system::Event::NewAccount { account: charlie },
 		));
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -237,12 +261,14 @@ fn currency_withdraw_works() {
 		System::set_block_number(1);
 
 		// Invoke the function under test.
-		assert_ok!(EvmBalances::withdraw(
+		let imbalance = EvmBalances::withdraw(
 			&alice(),
 			1000,
 			WithdrawReasons::FEE,
-			ExistenceRequirement::KeepAlive
-		));
+			ExistenceRequirement::KeepAlive,
+		)
+		.unwrap();
+		drop(imbalance);
 
 		// Assert state changes.
 		assert_eq!(
@@ -253,6 +279,8 @@ fn currency_withdraw_works() {
 			who: alice(),
 			amount: withdrawed_amount,
 		}));
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -271,6 +299,8 @@ fn currency_make_free_balance_be_works() {
 
 		// Assert state changes.
 		assert_eq!(EvmBalances::total_balance(&charlie), made_free_balance);
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -297,6 +327,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();
 	});
 }
 
@@ -317,6 +349,34 @@ fn evm_balances_transferring_too_high_value_should_not_panic() {
 	});
 }
 
+#[test]
+fn evm_system_removing_account_non_zero_balance() {
+	new_test_ext().execute_with_ext(|_| {
+		// Prepare test preconditions.
+		let contract = H160::from_str("1000000000000000000000000000000000000003").unwrap();
+		EVM::create_account(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(&contract);
+
+		// Assert state changes.
+		assert_eq!(EvmBalances::free_balance(&contract), 1000);
+		assert!(EvmSystem::account_exists(&contract));
+
+		assert_total_issuance_invariant();
+	});
+}
+
 #[test]
 fn evm_fee_deduction() {
 	new_test_ext().execute_with_ext(|_| {
@@ -338,6 +398,8 @@ fn evm_fee_deduction() {
 		// Refund fees as 5 units
 		<<Test as pallet_evm::Config>::OnChargeTransaction as pallet_evm::OnChargeEVMTransaction<Test>>::correct_and_deposit_fee(&charlie, U256::from(5), U256::from(5), imbalance);
 		assert_eq!(EvmBalances::free_balance(&charlie), 95);
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -374,6 +436,8 @@ fn evm_issuance_after_tip() {
 		let after_tip = <Test as pallet_evm::Config>::Currency::total_issuance();
 
 		assert_eq!(after_tip, (before_tip - (base_fee * 21_000)));
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -410,6 +474,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()).0.balance;
 		assert_eq!(after_call, before_call - total_cost);
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -451,6 +517,8 @@ fn evm_refunds_and_priority_should_work() {
 		let after_call = EVM::account_basic(&alice()).0.balance;
 		// The tip is deducted but never refunded to the caller.
 		assert_eq!(after_call, before_call - total_cost);
+
+		assert_total_issuance_invariant();
 	});
 }
 
@@ -482,6 +550,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();
 	});
 }
 
@@ -512,5 +582,7 @@ fn evm_call_should_succeed_with_priority_equal_to_max_fee() {
 			<Test as pallet_evm::Config>::config(),
 		);
 		assert!(result.is_ok());
+
+		assert_total_issuance_invariant();
 	});
 }

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<T: Config> Pallet<T> {
 	/// Check the account existence.
 	pub fn account_exists(who: &<T as Config>::AccountId) -> bool {
@@ -144,25 +164,29 @@ impl<T: Config> Pallet<T> {
 	}
 
 	/// Create an account.
-	pub fn create_account(who: &<T as Config>::AccountId) -> DispatchResult {
+	pub fn create_account(who: &<T as Config>::AccountId) -> AccountCreationOutcome {
 		if Self::account_exists(who) {
-			return Err(Error::<T>::AccountAlreadyExist.into());
+			return AccountCreationOutcome::AlreadyExists;
 		}
 
 		Account::<T>::insert(who.clone(), AccountInfo::<_, _>::default());
 		Self::on_created_account(who.clone());
-		Ok(())
+		AccountCreationOutcome::Created
 	}
 
 	/// Remove an account.
-	pub fn remove_account(who: &<T as Config>::AccountId) -> DispatchResult {
+	pub fn remove_account(who: &<T as Config>::AccountId) -> AccountRemovalOutcome {
 		if !Self::account_exists(who) {
-			return Err(Error::<T>::AccountNotExist.into());
+			return AccountRemovalOutcome::DidNotExist;
+		}
+
+		if Account::<T>::get(who).data != <T as Config>::AccountData::default() {
+			return AccountRemovalOutcome::Retained;
 		}
 
 		Account::<T>::remove(who);
 		Self::on_killed_account(who.clone());
-		Ok(())
+		AccountRemovalOutcome::Reaped
 	}
 }
 

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;
 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),
+			AccountCreationStatus::Created
+		);
 
 		// Assert state changes.
 		assert!(EvmSystem::account_exists(&account_id));
@@ -52,9 +55,9 @@ fn create_account_fails() {
 		<Account<Test>>::insert(account_id.clone(), AccountInfo::<_, _>::default());
 
 		// Invoke the function under test.
-		assert_noop!(
+		assert_eq!(
 			EvmSystem::create_account(&account_id),
-			Error::<Test>::AccountAlreadyExist
+			AccountCreationStatus::Existed
 		);
 	});
 }
@@ -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),
+			AccountRemovalStatus::Reaped
+		);
 
 		// Assert state changes.
 		assert!(!EvmSystem::account_exists(&account_id));
@@ -100,9 +106,9 @@ fn remove_account_fails() {
 		let account_id = H160::from_str("1000000000000000000000000000000000000001").unwrap();
 
 		// Invoke the function under test.
-		assert_noop!(
+		assert_eq!(
 			EvmSystem::remove_account(&account_id),
-			Error::<Test>::AccountNotExist
+			AccountRemovalStatus::NotExists
 		);
 	});
 }

rust-toolchain.toml

@@ -0,0 +1,5 @@
+[toolchain]
+channel = "nightly-2023-06-08"
+components = ["rustfmt", "clippy"]
+targets = ["wasm32-unknown-unknown"]
+profile = "minimal"