Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix removing account logic at pallet-evm-system #111

Merged
merged 11 commits into from
Apr 5, 2024
Merged

Fix removing account logic at pallet-evm-system #111

merged 11 commits into from
Apr 5, 2024

Conversation

dmitrylavrenov
Copy link

No description provided.

@dmitrylavrenov dmitrylavrenov requested a review from MOZGIII March 27, 2024 17:21
@MOZGIII
Copy link

MOZGIII commented Mar 27, 2024

I don't get it, where's the actual fix?

@MOZGIII
Copy link

MOZGIII commented Mar 27, 2024

For what it's worth, it seems like remove_acount should just call try_mutate_exists and set the state to default inside the handler - not the other way around.

@dmitrylavrenov dmitrylavrenov force-pushed the fix-bug-with-remove-account branch from be9a89f to dbe91ad Compare March 28, 2024 16:05
@dmitrylavrenov dmitrylavrenov force-pushed the fix-bug-with-remove-account branch 3 times, most recently from 9a103be to 5a6e2e3 Compare March 28, 2024 16:39
@dmitrylavrenov dmitrylavrenov force-pushed the fix-bug-with-remove-account branch from 5a6e2e3 to 60bbc02 Compare March 28, 2024 16:44
@dmitrylavrenov dmitrylavrenov requested review from MOZGIII and removed request for MOZGIII March 28, 2024 16:46
@dmitrylavrenov
Copy link
Author

@MOZGIII The fix implementation has been improved and checked with introduced evm_system_removing_account_non_zero_balance test.

@dmitrylavrenov dmitrylavrenov force-pushed the fix-bug-with-remove-account branch from 80f7854 to 037ec15 Compare March 28, 2024 16:56
@MOZGIII
Copy link

MOZGIII commented Mar 31, 2024 •
edited
Loading

Let's add an E2E test with a contract self-destruct.

MOZGIII
MOZGIII requested changes Mar 31, 2024
View reviewed changes
Copy link

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add the E2E test and adjust the status types. Also, I need to figure out why do we switch from assert_noop to assert_eq at some of the tests.

@dmitrylavrenov dmitrylavrenov mentioned this pull request Apr 4, 2024
Merged
@dmitrylavrenov
Copy link
Author

Let's add the E2E test and adjust the status types. Also, I need to figure out why do we switch from assert_noop to assert_eq at some of the tests.

humanode-network/humanode#971

MOZGIII
MOZGIII reviewed Apr 4, 2024
View reviewed changes
rust-toolchain.toml Outdated Show resolved Hide resolved
@dmitrylavrenov dmitrylavrenov requested a review from MOZGIII April 5, 2024 08:12
@dmitrylavrenov dmitrylavrenov merged commit ac5c341 into locked/polkadot-v0.9.40 Apr 5, 2024
@dmitrylavrenov dmitrylavrenov deleted the fix-bug-with-remove-account branch April 5, 2024 08:31
dmitrylavrenov added a commit that referenced this pull request Apr 10, 2024
@dmitrylavrenov
Fix removing account logic at pallet-evm-system (#111)
dbf67b0 
* 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
dmitrylavrenov added a commit that referenced this pull request Apr 10, 2024
@dmitrylavrenov
Fix removing account logic at pallet-evm-system (#111) (#116)
34516f5 
* 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
dmitrylavrenov added a commit that referenced this pull request Apr 19, 2024
@dmitrylavrenov
Fix removing account logic at pallet-evm-system (#111) (#116)
d6d2437 
* 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
dmitrylavrenov added a commit that referenced this pull request Apr 19, 2024
@dmitrylavrenov
Fix removing account logic at pallet-evm-system (#111) (#116) (#119)
3f42f8f 
* 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
dmitrylavrenov added a commit that referenced this pull request Oct 21, 2024
@dmitrylavrenov
Fix removing account logic at pallet-evm-system (#111) (#116) (#119)
e4f5a8e 
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MOZGIII MOZGIII MOZGIII approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dmitrylavrenov @MOZGIII