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

Kusama coretime revenue integration #384

Conversation

s0me0ne-unkn0wn
Copy link
Contributor

Based on #381

CC @ggwpez

Copy link
Contributor

@seadanda seadanda left a 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);
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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

@seadanda seadanda mentioned this pull request Jul 15, 2024
@s0me0ne-unkn0wn s0me0ne-unkn0wn marked this pull request as ready for review July 16, 2024 13:12
@s0me0ne-unkn0wn
Copy link
Contributor Author

@ggwpez will you just merge it into your branch? I didn't commit Cargo.lock checks as there are still some version inconsistencies, and I don't think going through the CI here makes sense.

@ggwpez ggwpez merged commit 36c9a7f into polkadot-fellows:oty-update-1-14 Jul 17, 2024
14 of 47 checks passed
@s0me0ne-unkn0wn s0me0ne-unkn0wn deleted the kusama-coretime-revenue branch July 17, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants