Fix try_mutate_exists implementation and add tests to check it (#70)

* Fix try_mutate_exists logic * Add tests * Fix AccountData type at mock * Remove redundant mock expectations * Add comments for new tests * More explicitly handle (none,false) case * Rename some_data back to maybe_account_data * Add data changes for try_mutate_exists_fails_without_changes test * Add try_mutate_exists_account_not_created test * Add assert_noop to check state chages * Return success for try_mutate_exists_account_not_created test
humanode-network · Oct 12, 2023 · a233869 · a233869
1 parent 8a542e3
commit a233869
Show file tree

Hide file tree

Showing 3 changed files with 195 additions and 8 deletions.
diff --git a/frame/evm-system/src/lib.rs b/frame/evm-system/src/lib.rs
@@ -181,16 +181,32 @@ impl<T: Config> StoredMap<<T as Config>::AccountId, <T as Config>::AccountData>
 		k: &<T as Config>::AccountId,
 		f: impl FnOnce(&mut Option<<T as Config>::AccountData>) -> Result<R, E>,
 	) -> Result<R, E> {
-		let mut maybe_account_data = if Self::account_exists(k) {
-			Some(Account::<T>::get(k).data)
+		let (mut maybe_account_data, was_providing) = if Self::account_exists(k) {
+			(Some(Account::<T>::get(k).data), true)
 		} else {
-			None
+			(None, false)
 		};
-		let r = f(&mut maybe_account_data)?;
-		if let Some(account_data) = maybe_account_data {
-			Account::<T>::mutate(k, |a| a.data = account_data);
+
+		let result = f(&mut maybe_account_data)?;
+
+		match (maybe_account_data, was_providing) {
+			(Some(data), false) => {
+				Account::<T>::mutate(k, |a| a.data = data);
+				Self::on_created_account(k.clone());
+			}
+			(Some(data), true) => {
+				Account::<T>::mutate(k, |a| a.data = data);
+			}
+			(None, true) => {
+				Account::<T>::remove(k);
+				Self::on_killed_account(k.clone());
+			}
+			(None, false) => {
+				// Do nothing.
+			}
 		}
-		Ok(r)
+
+		Ok(result)
 	}
 }
 

diff --git a/frame/evm-system/src/mock.rs b/frame/evm-system/src/mock.rs
@@ -84,7 +84,7 @@ impl pallet_evm_system::Config for Test {
 	type RuntimeEvent = RuntimeEvent;
 	type AccountId = H160;
 	type Nonce = u64;
-	type AccountData = ();
+	type AccountData = u64;
 	type OnNewAccount = MockDummyOnNewAccount;
 	type OnKilledAccount = MockDummyOnKilledAccount;
 }

diff --git a/frame/evm-system/src/tests.rs b/frame/evm-system/src/tests.rs
@@ -118,3 +118,174 @@ fn inc_account_nonce_works() {
 		assert_eq!(EvmSystem::account_nonce(&account_id), nonce_before + 1);
 	});
 }
+
+/// This test verifies that try_mutate_exists works as expected in case data wasn't providing
+/// and returned data is `Some`. As a result, a new account has been created.
+#[test]
+fn try_mutate_exists_account_created() {
+	new_test_ext().execute_with(|| {
+		// Prepare test data.
+		let account_id = H160::from_str("1000000000000000000000000000000000000001").unwrap();
+
+		// Check test preconditions.
+		assert!(!EvmSystem::account_exists(&account_id));
+
+		// Set mock expectations.
+		let on_new_account_ctx = MockDummyOnNewAccount::on_new_account_context();
+		on_new_account_ctx
+			.expect()
+			.once()
+			.with(predicate::eq(account_id))
+			.return_const(());
+
+		// Set block number to enable events.
+		System::set_block_number(1);
+
+		// Invoke the function under test.
+		EvmSystem::try_mutate_exists(&account_id, |maybe_data| -> Result<(), DispatchError> {
+			*maybe_data = Some(1);
+			Ok(())
+		})
+		.unwrap();
+
+		// Assert state changes.
+		assert!(EvmSystem::account_exists(&account_id));
+		assert_eq!(EvmSystem::get(&account_id), 1);
+		System::assert_has_event(RuntimeEvent::EvmSystem(Event::NewAccount {
+			account: account_id,
+		}));
+
+		// Assert mock invocations.
+		on_new_account_ctx.checkpoint();
+	});
+}
+
+/// This test verifies that try_mutate_exists works as expected in case data was providing
+/// and returned data is `Some`. As a result, the account has been updated.
+#[test]
+fn try_mutate_exists_account_updated() {
+	new_test_ext().execute_with(|| {
+		// Prepare test data.
+		let account_id = H160::from_str("1000000000000000000000000000000000000001").unwrap();
+		let nonce = 1;
+		let data = 1;
+		<Account<Test>>::insert(account_id.clone(), AccountInfo { nonce, data });
+
+		// Check test preconditions.
+		assert!(EvmSystem::account_exists(&account_id));
+
+		// Set block number to enable events.
+		System::set_block_number(1);
+
+		// Invoke the function under test.
+		EvmSystem::try_mutate_exists(&account_id, |maybe_data| -> Result<(), DispatchError> {
+			if let Some(ref mut data) = maybe_data {
+				*data += 1;
+			}
+			Ok(())
+		})
+		.unwrap();
+
+		// Assert state changes.
+		assert!(EvmSystem::account_exists(&account_id));
+		assert_eq!(EvmSystem::get(&account_id), data + 1);
+	});
+}
+
+/// This test verifies that try_mutate_exists works as expected in case data was providing
+/// and returned data is `None`. As a result, the account has been removed.
+#[test]
+fn try_mutate_exists_account_removed() {
+	new_test_ext().execute_with(|| {
+		// Prepare test data.
+		let account_id = H160::from_str("1000000000000000000000000000000000000001").unwrap();
+		let nonce = 1;
+		let data = 1;
+		<Account<Test>>::insert(account_id.clone(), AccountInfo { nonce, data });
+
+		// Check test preconditions.
+		assert!(EvmSystem::account_exists(&account_id));
+
+		// Set mock expectations.
+		let on_killed_account_ctx = MockDummyOnKilledAccount::on_killed_account_context();
+		on_killed_account_ctx
+			.expect()
+			.once()
+			.with(predicate::eq(account_id))
+			.return_const(());
+
+		// Set block number to enable events.
+		System::set_block_number(1);
+
+		// Invoke the function under test.
+		EvmSystem::try_mutate_exists(&account_id, |maybe_data| -> Result<(), DispatchError> {
+			*maybe_data = None;
+			Ok(())
+		})
+		.unwrap();
+
+		// Assert state changes.
+		assert!(!EvmSystem::account_exists(&account_id));
+		System::assert_has_event(RuntimeEvent::EvmSystem(Event::KilledAccount {
+			account: account_id,
+		}));
+
+		// Assert mock invocations.
+		on_killed_account_ctx.checkpoint();
+	});
+}
+
+/// This test verifies that try_mutate_exists works as expected in case data wasn't providing
+/// and returned data is `None`. As a result, the account hasn't been created.
+#[test]
+fn try_mutate_exists_account_not_created() {
+	new_test_ext().execute_with(|| {
+		// Prepare test data.
+		let account_id = H160::from_str("1000000000000000000000000000000000000001").unwrap();
+
+		// Check test preconditions.
+		assert!(!EvmSystem::account_exists(&account_id));
+
+		// Set block number to enable events.
+		System::set_block_number(1);
+
+		// Invoke the function under test.
+		<Account<Test>>::try_mutate_exists(account_id, |maybe_data| -> Result<(), ()> {
+			*maybe_data = None;
+			Ok(())
+		})
+		.unwrap();
+
+		// Assert state changes.
+		assert!(!EvmSystem::account_exists(&account_id));
+	});
+}
+
+/// This test verifies that try_mutate_exists works as expected in case getting error
+/// during data mutation.
+#[test]
+fn try_mutate_exists_fails_without_changes() {
+	new_test_ext().execute_with(|| {
+		// Prepare test data.
+		let account_id = H160::from_str("1000000000000000000000000000000000000001").unwrap();
+		let nonce = 1;
+		let data = 1;
+		<Account<Test>>::insert(account_id.clone(), AccountInfo { nonce, data });
+
+		// Check test preconditions.
+		assert!(EvmSystem::account_exists(&account_id));
+
+		// Invoke the function under test.
+		assert_noop!(
+			<Account<Test>>::try_mutate_exists(account_id, |maybe_data| -> Result<(), ()> {
+				*maybe_data = None;
+				Err(())
+			}),
+			()
+		);
+
+		// Assert state changes.
+		assert!(EvmSystem::account_exists(&account_id));
+		assert_eq!(EvmSystem::get(&account_id), data);
+	});
+}