From ae3e43fd7d713588ead644368f7e7a898dc74d19 Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Thu, 15 Jun 2023 12:38:34 +0300 Subject: [PATCH 01/11] Fix try_mutate_exists logic --- frame/evm-system/src/lib.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/frame/evm-system/src/lib.rs b/frame/evm-system/src/lib.rs index 577bb3cc52..e1f1587932 100644 --- a/frame/evm-system/src/lib.rs +++ b/frame/evm-system/src/lib.rs @@ -182,16 +182,30 @@ impl StoredMap<::AccountId, ::AccountData> k: &::AccountId, f: impl FnOnce(&mut Option<::AccountData>) -> Result, ) -> Result { - let mut maybe_account_data = if Self::account_exists(k) { - Some(Account::::get(k).data) + let (mut some_data, was_providing) = if Self::account_exists(k) { + (Some(Account::::get(k).data), true) } else { - None + (None, false) }; - let r = f(&mut maybe_account_data)?; - if let Some(account_data) = maybe_account_data { - Account::::mutate(k, |a| a.data = account_data); + + let result = f(&mut some_data)?; + + match (some_data, was_providing) { + (Some(data), false) => { + Account::::mutate(k, |a| a.data = data); + Self::on_created_account(k.clone()); + } + (Some(data), true) => { + Account::::mutate(k, |a| a.data = data); + } + (None, true) => { + Account::::remove(k); + Self::on_killed_account(k.clone()); + } + _ => {} } - Ok(r) + + Ok(result) } } From 61fd1860fd726e28fc9500fa29709cbe67772d8d Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Thu, 15 Jun 2023 16:10:20 +0300 Subject: [PATCH 02/11] Add tests --- frame/evm-system/src/tests.rs | 158 ++++++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) diff --git a/frame/evm-system/src/tests.rs b/frame/evm-system/src/tests.rs index 978ddad7a4..5bc3a994b6 100644 --- a/frame/evm-system/src/tests.rs +++ b/frame/evm-system/src/tests.rs @@ -124,3 +124,161 @@ fn inc_account_nonce_works() { assert_eq!(EvmSystem::account_nonce(&account_id), nonce_before + 1); }); } + +#[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(()); + let on_killed_account_ctx = MockDummyOnKilledAccount::on_killed_account_context(); + on_killed_account_ctx.expect().never(); + + // 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(); + on_killed_account_ctx.checkpoint(); + }); +} + +#[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; + >::insert(account_id.clone(), AccountInfo { nonce, data }); + + // 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().never(); + let on_killed_account_ctx = MockDummyOnKilledAccount::on_killed_account_context(); + on_killed_account_ctx.expect().never(); + + // 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); + + // Assert mock invocations. + on_new_account_ctx.checkpoint(); + on_killed_account_ctx.checkpoint(); + }); +} + +#[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; + >::insert(account_id.clone(), AccountInfo { nonce, data }); + + // 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().never(); + 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_new_account_ctx.checkpoint(); + on_killed_account_ctx.checkpoint(); + }); +} + +#[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; + >::insert(account_id.clone(), AccountInfo { nonce, data }); + + // 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().never(); + let on_killed_account_ctx = MockDummyOnKilledAccount::on_killed_account_context(); + on_killed_account_ctx.expect().never(); + + // Invoke the function under test. + >::try_mutate_exists(account_id, |_maybe_data| -> Result<(), ()> { Err(()) }) + .unwrap_err(); + + // Assert state changes. + assert!(EvmSystem::account_exists(&account_id)); + assert_eq!(EvmSystem::get(&account_id), data); + + // Assert mock invocations. + on_new_account_ctx.checkpoint(); + on_killed_account_ctx.checkpoint(); + }); +} From bb8266bed03d5d576ddf3ba526b514625724e99c Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Thu, 15 Jun 2023 16:47:22 +0300 Subject: [PATCH 03/11] Fix AccountData type at mock --- frame/evm-system/src/mock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/evm-system/src/mock.rs b/frame/evm-system/src/mock.rs index 255402ad9e..7688a55ef8 100644 --- a/frame/evm-system/src/mock.rs +++ b/frame/evm-system/src/mock.rs @@ -92,7 +92,7 @@ impl pallet_evm_system::Config for Test { type RuntimeEvent = RuntimeEvent; type AccountId = H160; type Index = u64; - type AccountData = (); + type AccountData = u64; type OnNewAccount = MockDummyOnNewAccount; type OnKilledAccount = MockDummyOnKilledAccount; } From 7cc0c4a0c43922e5273b178d529fc9b5db7c25a2 Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Thu, 15 Jun 2023 16:50:06 +0300 Subject: [PATCH 04/11] Remove redundant mock expectations --- frame/evm-system/src/tests.rs | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/frame/evm-system/src/tests.rs b/frame/evm-system/src/tests.rs index 5bc3a994b6..ac74d76397 100644 --- a/frame/evm-system/src/tests.rs +++ b/frame/evm-system/src/tests.rs @@ -141,8 +141,6 @@ fn try_mutate_exists_account_created() { .once() .with(predicate::eq(account_id)) .return_const(()); - let on_killed_account_ctx = MockDummyOnKilledAccount::on_killed_account_context(); - on_killed_account_ctx.expect().never(); // Set block number to enable events. System::set_block_number(1); @@ -163,7 +161,6 @@ fn try_mutate_exists_account_created() { // Assert mock invocations. on_new_account_ctx.checkpoint(); - on_killed_account_ctx.checkpoint(); }); } @@ -179,12 +176,6 @@ fn try_mutate_exists_account_updated() { // 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().never(); - let on_killed_account_ctx = MockDummyOnKilledAccount::on_killed_account_context(); - on_killed_account_ctx.expect().never(); - // Set block number to enable events. System::set_block_number(1); @@ -200,10 +191,6 @@ fn try_mutate_exists_account_updated() { // Assert state changes. assert!(EvmSystem::account_exists(&account_id)); assert_eq!(EvmSystem::get(&account_id), data + 1); - - // Assert mock invocations. - on_new_account_ctx.checkpoint(); - on_killed_account_ctx.checkpoint(); }); } @@ -220,8 +207,6 @@ fn try_mutate_exists_account_removed() { assert!(EvmSystem::account_exists(&account_id)); // Set mock expectations. - let on_new_account_ctx = MockDummyOnNewAccount::on_new_account_context(); - on_new_account_ctx.expect().never(); let on_killed_account_ctx = MockDummyOnKilledAccount::on_killed_account_context(); on_killed_account_ctx .expect() @@ -246,7 +231,6 @@ fn try_mutate_exists_account_removed() { })); // Assert mock invocations. - on_new_account_ctx.checkpoint(); on_killed_account_ctx.checkpoint(); }); } @@ -263,12 +247,6 @@ fn try_mutate_exists_fails_without_changes() { // 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().never(); - let on_killed_account_ctx = MockDummyOnKilledAccount::on_killed_account_context(); - on_killed_account_ctx.expect().never(); - // Invoke the function under test. >::try_mutate_exists(account_id, |_maybe_data| -> Result<(), ()> { Err(()) }) .unwrap_err(); @@ -276,9 +254,5 @@ fn try_mutate_exists_fails_without_changes() { // Assert state changes. assert!(EvmSystem::account_exists(&account_id)); assert_eq!(EvmSystem::get(&account_id), data); - - // Assert mock invocations. - on_new_account_ctx.checkpoint(); - on_killed_account_ctx.checkpoint(); }); } From 0dd3df0f0ca47cb953ede2863a8682b9a4e97c23 Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Thu, 15 Jun 2023 17:08:18 +0300 Subject: [PATCH 05/11] Add comments for new tests --- frame/evm-system/src/tests.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/frame/evm-system/src/tests.rs b/frame/evm-system/src/tests.rs index ac74d76397..d59010eef9 100644 --- a/frame/evm-system/src/tests.rs +++ b/frame/evm-system/src/tests.rs @@ -125,6 +125,8 @@ fn inc_account_nonce_works() { }); } +/// 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(|| { @@ -164,6 +166,8 @@ fn try_mutate_exists_account_created() { }); } +/// 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(|| { @@ -194,6 +198,8 @@ fn try_mutate_exists_account_updated() { }); } +/// 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(|| { @@ -235,6 +241,8 @@ fn try_mutate_exists_account_removed() { }); } +/// 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(|| { From cb121200b09b90257072553c3988d746ccf8aef3 Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Fri, 16 Jun 2023 09:14:24 +0300 Subject: [PATCH 06/11] More explicitly handle (none,false) case --- frame/evm-system/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frame/evm-system/src/lib.rs b/frame/evm-system/src/lib.rs index e1f1587932..25ee8dfdb4 100644 --- a/frame/evm-system/src/lib.rs +++ b/frame/evm-system/src/lib.rs @@ -202,7 +202,9 @@ impl StoredMap<::AccountId, ::AccountData> Account::::remove(k); Self::on_killed_account(k.clone()); } - _ => {} + (None, false) => { + // Do nothing. + } } Ok(result) From f2eea5392775b795628fb3a93216fb654b0b739f Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Fri, 16 Jun 2023 09:17:03 +0300 Subject: [PATCH 07/11] Rename some_data back to maybe_account_data --- frame/evm-system/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/evm-system/src/lib.rs b/frame/evm-system/src/lib.rs index 25ee8dfdb4..ad38522983 100644 --- a/frame/evm-system/src/lib.rs +++ b/frame/evm-system/src/lib.rs @@ -182,15 +182,15 @@ impl StoredMap<::AccountId, ::AccountData> k: &::AccountId, f: impl FnOnce(&mut Option<::AccountData>) -> Result, ) -> Result { - let (mut some_data, was_providing) = if Self::account_exists(k) { + let (mut maybe_account_data, was_providing) = if Self::account_exists(k) { (Some(Account::::get(k).data), true) } else { (None, false) }; - let result = f(&mut some_data)?; + let result = f(&mut maybe_account_data)?; - match (some_data, was_providing) { + match (maybe_account_data, was_providing) { (Some(data), false) => { Account::::mutate(k, |a| a.data = data); Self::on_created_account(k.clone()); From 4025010496d54697898e1ecd2c400f16d8e81a20 Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Fri, 16 Jun 2023 09:19:19 +0300 Subject: [PATCH 08/11] Add data changes for try_mutate_exists_fails_without_changes test --- frame/evm-system/src/tests.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frame/evm-system/src/tests.rs b/frame/evm-system/src/tests.rs index d59010eef9..c86dbee736 100644 --- a/frame/evm-system/src/tests.rs +++ b/frame/evm-system/src/tests.rs @@ -256,8 +256,11 @@ fn try_mutate_exists_fails_without_changes() { assert!(EvmSystem::account_exists(&account_id)); // Invoke the function under test. - >::try_mutate_exists(account_id, |_maybe_data| -> Result<(), ()> { Err(()) }) - .unwrap_err(); + >::try_mutate_exists(account_id, |maybe_data| -> Result<(), ()> { + *maybe_data = None; + Err(()) + }) + .unwrap_err(); // Assert state changes. assert!(EvmSystem::account_exists(&account_id)); From 3b2e4baa36e9f9509a2aec822dc97ae1be5e2574 Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Fri, 16 Jun 2023 09:36:29 +0300 Subject: [PATCH 09/11] Add try_mutate_exists_account_not_created test --- frame/evm-system/src/tests.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/frame/evm-system/src/tests.rs b/frame/evm-system/src/tests.rs index c86dbee736..fd56763e45 100644 --- a/frame/evm-system/src/tests.rs +++ b/frame/evm-system/src/tests.rs @@ -267,3 +267,31 @@ fn try_mutate_exists_fails_without_changes() { assert_eq!(EvmSystem::get(&account_id), data); }); } + +/// This test verifies that try_mutate_exists works as expected in case data wasn't providing +/// and error happens during data mutation. +#[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. + assert_noop!( + >::try_mutate_exists(account_id, |maybe_data| -> Result<(), ()> { + *maybe_data = None; + Err(()) + }), + () + ); + + // Assert state changes. + assert!(!EvmSystem::account_exists(&account_id)); + }); +} From a46878d951d39f5c95aa789fe2c0210341464daa Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Fri, 16 Jun 2023 09:38:53 +0300 Subject: [PATCH 10/11] Add assert_noop to check state chages --- frame/evm-system/src/tests.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/frame/evm-system/src/tests.rs b/frame/evm-system/src/tests.rs index fd56763e45..09aeea82bd 100644 --- a/frame/evm-system/src/tests.rs +++ b/frame/evm-system/src/tests.rs @@ -256,11 +256,13 @@ fn try_mutate_exists_fails_without_changes() { assert!(EvmSystem::account_exists(&account_id)); // Invoke the function under test. - >::try_mutate_exists(account_id, |maybe_data| -> Result<(), ()> { - *maybe_data = None; - Err(()) - }) - .unwrap_err(); + assert_noop!( + >::try_mutate_exists(account_id, |maybe_data| -> Result<(), ()> { + *maybe_data = None; + Err(()) + }), + () + ); // Assert state changes. assert!(EvmSystem::account_exists(&account_id)); From 792f40c6046b8205d14fb7c2ff249a3d1f0108c5 Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Fri, 16 Jun 2023 09:57:51 +0300 Subject: [PATCH 11/11] Return success for try_mutate_exists_account_not_created test --- frame/evm-system/src/tests.rs | 46 +++++++++++++++++------------------ 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/frame/evm-system/src/tests.rs b/frame/evm-system/src/tests.rs index 09aeea82bd..218d1a1ae2 100644 --- a/frame/evm-system/src/tests.rs +++ b/frame/evm-system/src/tests.rs @@ -241,48 +241,45 @@ fn try_mutate_exists_account_removed() { }); } -/// This test verifies that try_mutate_exists works as expected in case getting error -/// during data mutation. +/// 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_fails_without_changes() { +fn try_mutate_exists_account_not_created() { new_test_ext().execute_with(|| { // Prepare test data. let account_id = H160::from_str("1000000000000000000000000000000000000001").unwrap(); - let nonce = 1; - let data = 1; - >::insert(account_id.clone(), AccountInfo { nonce, data }); // Check test preconditions. - assert!(EvmSystem::account_exists(&account_id)); + assert!(!EvmSystem::account_exists(&account_id)); + + // Set block number to enable events. + System::set_block_number(1); // Invoke the function under test. - assert_noop!( - >::try_mutate_exists(account_id, |maybe_data| -> Result<(), ()> { - *maybe_data = None; - Err(()) - }), - () - ); + >::try_mutate_exists(account_id, |maybe_data| -> Result<(), ()> { + *maybe_data = None; + Ok(()) + }) + .unwrap(); // Assert state changes. - assert!(EvmSystem::account_exists(&account_id)); - assert_eq!(EvmSystem::get(&account_id), data); + assert!(!EvmSystem::account_exists(&account_id)); }); } -/// This test verifies that try_mutate_exists works as expected in case data wasn't providing -/// and error happens during data mutation. +/// This test verifies that try_mutate_exists works as expected in case getting error +/// during data mutation. #[test] -fn try_mutate_exists_account_not_created() { +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; + >::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); + assert!(EvmSystem::account_exists(&account_id)); // Invoke the function under test. assert_noop!( @@ -294,6 +291,7 @@ fn try_mutate_exists_account_not_created() { ); // Assert state changes. - assert!(!EvmSystem::account_exists(&account_id)); + assert!(EvmSystem::account_exists(&account_id)); + assert_eq!(EvmSystem::get(&account_id), data); }); }