-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Introduce pallet-dap-satellite and BurnHandler to make tokens burn configurable #10597
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
base: master
Are you sure you want to change the base?
Conversation
4ecbd80 to
5025686
Compare
137b6f5 to
ee054ae
Compare
ee054ae to
1ad4d04
Compare
9d68694 to
85200d8
Compare
|
About #10597 (comment): I think we need to add this fix to the normal DAP pallet as well; any DOT that is stored in DAP is not part of the active issuance. |
| // Mark funds as inactive so they don't participate in governance voting. | ||
| // TODO: When implementing XCM transfer to AssetHub, call `reactivate(amount)` before | ||
| // sending. | ||
| T::Currency::deactivate(amount); |
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.
The original code of pallet-balances in this case will reduce the issuance. Is that correct? We are not increasing it
here, but marking something is inactive 🤔
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.
I think it's the correct behavior and no - we are not reducing the total issuance.
Current flow with BurnHandler = Dap / Dap satellite:
- decrease_balance(who) ( here https://github.com/paritytech/polkadot-sdk/blob/sigurpol-dap-satellite/substrate/frame/balances/src/impl_fungible.rs#L203-L203 in burn_from) -> removes from user's balance, does NOT touch total issuance
- increase_balance(satellite) - adds to satellite's balance, does NOT touch total issuance
- Total issuance: unchanged (funds moved to DAP, nothing created / destroyed)
- deactivate(amount) - increases InactiveIssuance storage
or maybe I am not understanding your comment correctly 😓
| ); | ||
|
|
||
| // Mark funds as inactive so they don't participate in governance voting. | ||
| // TODO: When implementing XCM transfer to AssetHub, call `reactivate(amount)` before |
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.
The flow of funds that go to DAP should be:
- Some pallet burns
- They come to satellite
- TI is not change (or am I missing sth?)
- These funds are inactive now
- Nothing wrt.
active/inactive/issuancechanges when they are sent from the satellite account to the main DAP account.
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.
improved the comment. AFAIK here we need to reactivate before sending and on DAP side we need to deactivate when receiving
kianenigma
left a comment
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.
I have doubts around how TI issuance is handled; Let me recap what I knew before this:
- Pallet balances implements
fungible::Mutate, which has asburn_frommethod. - This is used for both the
burntransaction, and if another pallet callsT::Currency::burn()(test should cover this too btw) - At the moment, this implementation is hardcoded in
pallet-balances, and does what is sensible: burn the funds, reduce TI. - Now we want
pallet-balancesto allow for customization of itsfungible::Mutate::burn_from. - I think in the new model, the whole implementation of
BurnFromshould come from atype BurnHandler, not just the destination part. - This will allow our new
BurnHandlerto manage TI as it wants, while allowing the originalDirectBurnto remain exactly as-is. - The PRdoc and PR description should say clearly that all existing users should use
type BurnHandler = pallet_balances::DirectBurnand it will be a noop.
It is part of this PR already after your previous comment. But if we decide NOT to ship this PR for 2.0.6 then yes, we need to add that fix in ad-hoc small PR targeting 2512 / 2.0.6 |
I think the current behavior is correct but your approach is definitely much cleaner so I will go for BurnHandler manage the whole burn_from as per #10597 (comment) |
Implemented here f91cb59 |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
| /// `DirectBurn<Balances>` or a custom implementation. | ||
| type BurnHandler = (); |
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.
Can't we set this to DirectBurn then a lot of tests and mocks don't need to change.
Other option is to make the impl for () be actually what DirectBurn does.
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.
I am cooking the latter as we speak since I realized we can make things much better assuming default = DirectBurn for all test and non Westend runtimes
| let mut split = | ||
| fees.ration(dap_percent.deconstruct() as u32, other_percent.deconstruct() as u32); | ||
| if let Some(tips) = fees_then_tips.next() { | ||
| // Tips go 100% to other handler. |
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.
Is this intentional?
I suppose most runtimes will want to configure this as "everything goes to DAP". In this case the tips still go whatever
is OtherHandler.
In a similar vein, while DealWithFeesSplit is useful, am I correct that in most real cases we want to use type DealWithFees = Dap, which directly uses the OnUnbalanced impl below?
| mod burn_handler; | ||
| mod deal_with_fees_split; | ||
| mod genesis; | ||
| mod on_unbalanced; |
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.
This is a good place to have the tests where:
- You create a fake pallet in the
mock.rsthat hastype Currencyin it. - It calls
T::Currency::burn - Burn ends up in DAP.
This PR introduces
pallet-dap-satellite, allowing system chains different from AssetHub to collect funds (to be periodically sent to DAP on AssetHub in a future PR) instead of burning them directly.It also introduces
BurnHandlertrait to make any burns configurable (direct burn vs redirect to a buffer).In Westend AH, RC and system chains, all burns and all fees now go to DAP or DAP satellite.
Changes
frame-support: New
BurnHandlertrait andDirectBurnstruct.DirectBurnimplementsBurnHandler::burn_fromto reduce total issuance.pallet-dap-satellite: New pallet that implements
BurnHandlerandOnUnbalancedby accumulating funds in a satellite account (total issuance unchanged).DAP and DAP satellite funds are excluded from
active_issuance.Fee split set to 100% DAP / DAP satellites on Westend system chains.
Redirect all burns to DAP or DAP satellite on Westend system chains
The
BurnHandler::burn_frommethod controls the entire burn operation, including balance reduction and issuance handling.Runtimes must explicitly configure this:
DirectBurn<Balances>— reduces total issuance viaset_total_issuance()pallet_dap::Pallet<Runtime>(AssetHub) orpallet_dap_satellite::Pallet<Runtime>(other system chains) — credits buffer viaincrease_balance, thendeactivate(amount). Total issuance unchanged.Note: non-native asset burns via pallet-assets use the default fungible implementation, not pallet-balances's BurnHandler.
🔜 Coming next
In a followup PR, we will hook the logic to periodically send funds from the satellite to the main DAP on AssetHub.
When doing so, we will also add related tests under
cumulus/parachains/integration-tests/emulated/tests. Doing it now without the XCM transfer, wouldn't provide any additional value vs runtime tests added in the scope of this PR.Related PRs / issues
Followup of #10576.
Close #10488.