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.

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

refactor(core): Guarantee system accounts existence for programs #3961

Merged
merged 28 commits into from
Jun 24, 2024

Conversation

ekovalev
Copy link
Member

@ekovalev ekovalev commented May 13, 2024

Resolves #3924.

The idea is to separate the notion of value ownership/transfer by actors in Actor space from the underlying system accounts management level (e.g. Substrate's frame-system and balances pallets).
Programs (a.k.a. actors) should not care about their "wallets" implementation details and whether or not they require things like existential deposits to stay alive or have to respect limits on transferred value amounts imposed by the system level. Instead, the protocol must guarantee an invariant that for any active program there always is an account able to store any amount of value at any point throughout the program's lifetime.

This change, once implemented, renders useless numerous checks of a value validity in balances transfers - any value should be valid for a transfer and not cause program's account removal.

Specifically, the PR implements:

  • Charging the existential deposit to user's balance at the time of a program creation in Gear::upload_program and Gear::create_program extrinsics
    • 2 tests
  • Validating that existential deposit can be charged to a program's balance in case another program being created using the gr_create_program syscall (by decrementing the value counter) and further actual transfer of the ED in JournalNote::StoreNewPrograms handler
    • Track the case when code does not exist, so that refund must be made
    • 2 tests
  • The existential deposit is transferred to the program inheritor upon the gr_exit syscall (program deactivation)
    • 1 test
  • Removal of value limits (various check_value) in all send-like extrinsics and all backend send-like syscalls
    • 2 tests: extrinsics/syscalls (for syscalls could be implemented as core-processor/ext unit tests)
  • Ensures all transfers in the system are made with Existence::AllowDeath requirements (since the platform guarantees the account will not die as long as the program remains in active state)
  • Implements migrations to deposit ED from Treasury for each program that already exists in storage
    • try-runtime tests that after migrations all program that are in "Active" status have accounts with at least ED on them
  • Fix existing tests

@ekovalev ekovalev added the C2-refactoring Refactoring proposal label May 13, 2024
@ekovalev ekovalev force-pushed the ek-programs-accounts-management branch from 53c13ff to 255c089 Compare May 14, 2024 11:33
@breathx breathx added the A0-pleasereview PR is ready to be reviewed by the team label May 16, 2024
@ekovalev ekovalev force-pushed the ek-programs-accounts-management branch 3 times, most recently from 84c4177 to 7cf1ad5 Compare May 24, 2024 22:33
@breathx
Copy link
Member

breathx commented May 26, 2024

Seems ready to be reviewed once fixed benches?

@ekovalev
Copy link
Member Author

Seems ready to be reviewed once fixed benches?

Yes, pretty much.

@ekovalev ekovalev force-pushed the ek-programs-accounts-management branch from 4f41138 to 989e2fe Compare May 28, 2024 09:24
@ekovalev ekovalev marked this pull request as ready for review May 28, 2024 09:40
@ekovalev ekovalev force-pushed the ek-programs-accounts-management branch from 989e2fe to e196360 Compare May 29, 2024 13:08
Copy link
Member

@ukint-vs ukint-vs left a comment

Choose a reason for hiding this comment

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

lgtm.

pallets/gear-program/src/migrations/v7.rs Outdated Show resolved Hide resolved
@NikVolf
Copy link
Member

NikVolf commented May 30, 2024

merge master @ekovalev ?

@NikVolf
Copy link
Member

NikVolf commented May 30, 2024

@breathx final thoughts?

@ekovalev ekovalev requested a review from techraed May 31, 2024 09:55
@ekovalev ekovalev added the D1-core Gear Core label May 31, 2024
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

tests to be re-reviewed once applied suggested changes

otherwise well done

core-processor/src/ext.rs Outdated Show resolved Hide resolved
examples/syscall-error/src/wasm.rs Show resolved Hide resolved
gtest/src/manager.rs Outdated Show resolved Hide resolved
pallets/gear-bank/src/lib.rs Outdated Show resolved Hide resolved
pallets/gear-bank/src/lib.rs Show resolved Hide resolved
pallets/gear/src/manager/journal.rs Outdated Show resolved Hide resolved
examples/create-program-reentrance/src/wasm.rs Outdated Show resolved Hide resolved
pallets/gear/src/tests.rs Outdated Show resolved Hide resolved
pallets/gear/src/tests.rs Outdated Show resolved Hide resolved
@breathx breathx added A3-gotissues PR occurred to have issues after the review and removed A0-pleasereview PR is ready to be reviewed by the team labels Jun 3, 2024
pallets/gear/src/manager/journal.rs Show resolved Hide resolved
core-processor/src/ext.rs Outdated Show resolved Hide resolved
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

Im only worried now about match error branches and panics. Otherwise seems awesome 👍🏻 great job

pallets/gear-bank/src/lib.rs Show resolved Hide resolved
pallets/gear/src/internal.rs Outdated Show resolved Hide resolved
pallets/gear/src/internal.rs Show resolved Hide resolved
pallets/gear/src/manager/mod.rs Show resolved Hide resolved
pallets/gear/src/tests.rs Outdated Show resolved Hide resolved
@breathx breathx added the A0-pleasereview PR is ready to be reviewed by the team label Jun 11, 2024
@breathx
Copy link
Member

breathx commented Jun 11, 2024

@techraed kindly asking you to double check the changes and my comments above

Copy link
Member

@techraed techraed left a comment

Choose a reason for hiding this comment

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

Awesome!

I remember we agreed to use locks and to introduce integrational test, which tests that underlying Currency implementor locks & account existence invariants works as expected.

So, could you, please, introduce the test in this PR?

examples/create-program-reentrance/src/wasm.rs Outdated Show resolved Hide resolved
gtest/src/manager.rs Outdated Show resolved Hide resolved
pallets/gear-bank/src/lib.rs Show resolved Hide resolved
@breathx breathx merged commit d26eb33 into master Jun 24, 2024
10 checks passed
@breathx breathx deleted the ek-programs-accounts-management branch June 24, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview PR is ready to be reviewed by the team A3-gotissues PR occurred to have issues after the review C2-refactoring Refactoring proposal D1-core Gear Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: programs' frame_system::AccountInfo permanent existence
5 participants