Make on_unbalanceds work with fungibles imbalances#4564
Conversation
muharem
added
the
T1-FRAME
|
|
||
| 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)) |
There was a problem hiding this comment.
I think conceptually it makes more sense to call try_merge from merge and just ignore the return value there. Then try-merge would fail on overflow instead of saturating.
There was a problem hiding this comment.
Can you explain why? It feels less correct to me to ignore an error from try_merge within merge. In the scope of merge we wont know for sure, that the try_merge wont return an error.
We probably fine with saturation since we do not expect it to overflow the max supply type. It may happened only if you create imbalances in some hacky way, and do not use fungibles api.
Then try-merge would fail on overflow instead of saturating.
To handle this we would need a separate impl for try_merge, since merge is infallible
There was a problem hiding this comment.
The difference is that we explicitly ignore the error - instead of merge pretending to be infallible. We already have messed up TI on production, so i think saturation could happen.
Anyway, i dont care so much in this case.
There was a problem hiding this comment.
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.
| } | ||
| } | ||
| if let Some(sum) = sum { | ||
| Self::on_unbalanced(sum) |
There was a problem hiding this comment.
Where is the advantage of calling on_unbalanced on the merged result vs calling it on every balance individually?
There was a problem hiding this comment.
We were merging tip and fee, to handle both with a single on_unbalanceds handler. This call would result a single deposit instead of two if the imbalances are identical.
Current open PR with impl of payment handler for the fees, requires two on unbalanced impl, because on_unbalanceds cannot handle fungibles iterator -
pr #4488
I do not like the breaking change, but also feel like this change needed, since we moving away from currency impls and using fungible and fungibles.
I also wanna have this decided, before I merge that PR to not break it again later.
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
|
|
||
| crates: | ||
| - name: frame-support | ||
| bump: major |
There was a problem hiding this comment.
Hm the CI suggest minor here, but a public trait was change, so should be major IMHO.
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
…4564) Make `on_unbalanceds` work with `fungibles` `imbalances`. 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`. ### Migration for `OnUnbalanced` trait implementations: In case if you have a custom implementation of `on_unbalanceds` trait function, remove it's `<B>` type argument. ### Migration for custom imbalance types: If you have your own imbalance types implementations, implement the `TryMerge` trait for it introduced with this update. The applicability of the `on_unbalanceds` function to fungibles imbalances is useful in cases like - [link](https://github.com/paritytech/polkadot-sdk/blob/3a8e675e9f6f283514c00c14d3d1d90ed5bf59c0/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs#L267) from paritytech#4488. --------- Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
github-project-automation
bot
moved this from Todo
to Done
in [preview] release tracker
4 participants
Make
on_unbalancedswork withfungiblesimbalances.The
fungiblesimbalancescannot be handled by the default implementation ofon_unbalancedsfrom theOnUnbalancedtrait. This is because thefungiblesimbalancestypes do not implement theImbalancetrait (and cannot with its current semantics). Theon_unbalancedsfunction requires only themergefunction for the imbalance type. In this PR, we provide theTryMergetrait, which can be implemented by all imbalance types and makeOnUnbalancedrequire it insteadImbalance.Migration for
OnUnbalancedtrait implementations:In case if you have a custom implementation of
on_unbalancedstrait function, remove it's<B>type argument.Migration for custom imbalance types:
If you have your own imbalance types implementations, implement the
TryMergetrait for it introduced with this update.The applicability of the
on_unbalancedsfunction to fungibles imbalances is useful in cases like - link from #4488.