-
Notifications
You must be signed in to change notification settings - Fork 94
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
Kusama coretime revenue integration #384
Kusama coretime revenue integration #384
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a query about the period of the burn. Looks like cargo.lock needs an update too.
fn on_new_timeslice(_t: pallet_broker::Timeslice) { | ||
let stash = CoretimeBurnAccount::get(); | ||
let value = | ||
Balances::reducible_balance(&stash, Preservation::Expendable, Fortitude::Polite); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah also this - we don't cover the ED before payments go in, so I think this should take the whole balance to ensure that we are actually burning all revenue as the RFC states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I just replace reducible_balance
with total_balance
, will teleport logic succeed in checking the funds out and withdrawing them, considering that the account balance falls under the ED?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going through the logic now to understand how it works. The parachain side doesn't have CheckingAccount
, so can_check_out
will always succeed. Then, AssetTransactor::withdraw_asset
will call Fungible::burn_from(..., Expendable, Exact, Polite)
, which, in turn, will call for Fungible::reducible_balance(..., Expendable, Polite)
. That is, my check exactly matches the check withdraw_asset
will perform. Given that the account has an additional provider ref from OnUnbalanced
implementation, it should still return the total free balance of the account not reserving the ED: https://github.com/paritytech/polkadot-sdk/blob/926c1b6adcaa767f4887b5774ef1fdde75156dd9/substrate/frame/balances/src/impl_fungible.rs#L65-L66
To me, it seems sensible to keep it as is to match this reducible_balance
check to the checks performed by the withdraw_asset
implementation. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, missed your response in my notifications.
Maybe worth having this discussion on the main PR to include others - I'm happy to merge as is to not block, since it is a legit solution anyway
@ggwpez will you just merge it into your branch? I didn't commit |
36c9a7f
into
polkadot-fellows:oty-update-1-14
Based on #381
CC @ggwpez