-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Make on_unbalanceds work with fungibles imbalances
#4564
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
Changes from 4 commits
56138a4
d2e511c
1b2e87e
fa6661e
f4b7b10
bc7d035
647f506
0cc2db6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| title: "Make `on_unbalanceds` work with `fungibles` `imbalances`" | ||
|
|
||
| doc: | ||
| - audience: Node Operator | ||
| description: | | ||
| The `fungibles` `imbalances` cannot be handled by the default implementation of `on_unbalanceds` from the `OnUnbalanced` trait. This is because the `fungibles` `imbalances` types do not implement the `Imbalance` trait (and cannot with its current semantics). The `on_unbalanceds` function requires only the `merge` function for the imbalance type. In this PR, we provide the `TryMerge` trait, which can be implemented by all imbalance types and make `OnUnbalanced` require it instead `Imbalance`. | ||
|
|
||
| This introduces a breaking change to the `OnUnbalanced` trait, but it only requires the removal of an unnecessary type parameter. | ||
|
|
||
| crates: | ||
| - name: frame-support | ||
| bump: major | ||
| - name: pallet-balances | ||
| bump: patch | ||
muharem marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| - name: pallet-asset-conversion-tx-payment | ||
| bump: patch | ||
| - name: pallet-transaction-payment | ||
| bump: patch | ||
| - name: kitchensink-runtime | ||
| bump: patch | ||
| - name: polkadot-runtime-common | ||
| bump: patch | ||
| - name: parachains-common | ||
muharem marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,7 @@ pub use imbalances::{NegativeImbalance, PositiveImbalance}; | |
| // of the inner member. | ||
| mod imbalances { | ||
| use super::{result, Config, Imbalance, RuntimeDebug, Saturating, TryDrop, Zero}; | ||
| use frame_support::traits::SameOrOther; | ||
| use frame_support::traits::{tokens::imbalance::TryMerge, SameOrOther}; | ||
| use sp_std::mem; | ||
|
|
||
| /// Opaque, move-only struct with private fields that serves as a token denoting that | ||
|
|
@@ -132,6 +132,12 @@ mod imbalances { | |
| } | ||
| } | ||
|
|
||
| impl<T: Config<I>, I: 'static> TryMerge for PositiveImbalance<T, I> { | ||
| fn try_merge(self, other: Self) -> Result<Self, (Self, Self)> { | ||
| Ok(self.merge(other)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think conceptually it makes more sense to call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why? It feels less correct to me to ignore an error from
To handle this we would need a separate impl for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The difference is that we explicitly ignore the error - instead of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, but I think that would be a bigger change, a behaviour change, and if needed should be explored and addressed in different PR. |
||
| } | ||
| } | ||
|
|
||
| impl<T: Config<I>, I: 'static> TryDrop for NegativeImbalance<T, I> { | ||
| fn try_drop(self) -> result::Result<(), Self> { | ||
| self.drop_zero() | ||
|
|
@@ -196,6 +202,12 @@ mod imbalances { | |
| } | ||
| } | ||
|
|
||
| impl<T: Config<I>, I: 'static> TryMerge for NegativeImbalance<T, I> { | ||
| fn try_merge(self, other: Self) -> Result<Self, (Self, Self)> { | ||
| Ok(self.merge(other)) | ||
| } | ||
| } | ||
|
|
||
| impl<T: Config<I>, I: 'static> Drop for PositiveImbalance<T, I> { | ||
| /// Basic drop handler will just square up the total issuance. | ||
| fn drop(&mut self) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -35,11 +35,26 @@ pub trait OnUnbalanced<Imbalance: TryDrop> { | |||
| /// Handler for some imbalances. The different imbalances might have different origins or | ||||
| /// meanings, dependent on the context. Will default to simply calling on_unbalanced for all | ||||
| /// of them. Infallible. | ||||
| fn on_unbalanceds<B>(amounts: impl Iterator<Item = Imbalance>) | ||||
| fn on_unbalanceds(mut amounts: impl Iterator<Item = Imbalance>) | ||||
| where | ||||
| Imbalance: crate::traits::Imbalance<B>, | ||||
| Imbalance: crate::traits::tokens::imbalance::TryMerge, | ||||
| { | ||||
| Self::on_unbalanced(amounts.fold(Imbalance::zero(), |i, x| x.merge(i))) | ||||
| let mut sum: Option<Imbalance> = None; | ||||
| while let Some(next) = amounts.next() { | ||||
| sum = match sum { | ||||
| Some(sum) => match sum.try_merge(next) { | ||||
| Ok(sum) => Some(sum), | ||||
| Err((sum, next)) => { | ||||
| Self::on_unbalanced(next); | ||||
| Some(sum) | ||||
| }, | ||||
| }, | ||||
| None => Some(next), | ||||
| } | ||||
| } | ||||
| if let Some(sum) = sum { | ||||
| Self::on_unbalanced(sum) | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is the advantage of calling
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We were merging Current open PR with impl of payment handler for the fees, requires two on unbalanced impl, because polkadot-sdk/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs Line 267 in 3a8e675
pr #4488 I do not like the breaking change, but also feel like this change needed, since we moving away from I also wanna have this decided, before I merge that PR to not break it again later.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay seems fine to me. |
||||
| } | ||||
| } | ||||
|
|
||||
| /// Handler for some imbalance. Infallible. | ||||
|
|
||||
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.
Hm the CI suggest minor here, but a public trait was change, so should be major IMHO.