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

feat(runtime): Account for programs locked balance #3908

Closed
wants to merge 2 commits into from

Conversation

ekovalev
Copy link
Member

@ekovalev ekovalev commented Apr 20, 2024

A necessary prerequisite for the staking builtin actor to be added. Being able to participate in staking for programs would mean they'll have part of their balances locked while bonding funds. This hasn't been accounted for so far - all checks of programs' funds availability have been based on their free_balance which can produce incorrect estimations and lead to errors in transfers.

@ekovalev ekovalev added A0-pleasereview PR is ready to be reviewed by the team D2-node Gear Node C1-feature Feature request labels Apr 20, 2024
.map_err(|e| match e {
DispatchError::Token(TokenError::BelowMinimum) => Error::<T>::InsufficientDeposit,
DispatchError::Arithmetic(ArithmeticError::Overflow) => Error::<T>::Overflow,
_ => unreachable!("Unexpected error: {e:?}"),
Copy link
Member

Choose a reason for hiding this comment

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

what cases could cause this branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case only these two are possible. We just need to account for other potential DispatchError variants to make it compile.

Copy link
Member

Choose a reason for hiding this comment

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

Please no

Other variants of DispatchError can be added and this will lead to panic, even if it is impossible now

Please make exhausted match or even better return error and panic in calling code (if absolutely unavoidable)

@@ -377,7 +383,15 @@ where
ProgramStorageOf::<T>::remove_program_pages(program_id, memory_infix);

let program_account = program_id.cast();
let balance = CurrencyOf::<T>::free_balance(&program_account);

// Only `reducible_balance` is allowed to be transferred out, even if a part of the
Copy link
Member

Choose a reason for hiding this comment

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

does reducible balance include ED?

Copy link
Member Author

Choose a reason for hiding this comment

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

Depends on Preservation requirement. If Preservation::Expendable is supplied then the account is allowed to die therefore the ED is included in the reducible_balance. Otherwise it's not

// free balance includes the ED
assert_eq!(CurrencyOf::<Test>::free_balance(program_account), value);

// reducible balance for preserved account doesn't include the ED
Copy link
Member

Choose a reason for hiding this comment

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

program should be able to operate of the ED.
e.g.
ed = 10
program has 15, while 2 is frozen
program must be able to send 13

Copy link
Member

Choose a reason for hiding this comment

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

test it pls

Copy link
Member Author

Choose a reason for hiding this comment

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

if the idea is to always keep a program's account alive then it won't be able to touch the ED (provided we get the reducible_balance with Preservation::Preserve parameter. Otherwise we can pass the Preservation::Expandable as the preservation requirement, thereby allowing the program's account to be wiped as dust in case the balance drops below the ED

@breathx
Copy link
Member

breathx commented Apr 26, 2024

Blocked by #3924

@breathx breathx added A5-dontmerge PR should not be merged yet and removed A0-pleasereview PR is ready to be reviewed by the team labels Apr 26, 2024
@breathx
Copy link
Member

breathx commented May 14, 2024

Implemented inside #3924 solution

@breathx breathx closed this May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A5-dontmerge PR should not be merged yet C1-feature Feature request D2-node Gear Node
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants