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

Make on_unbalanceds work with fungibles imbalances #4564

Merged
merged 8 commits into from
Jul 23, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cumulus/parachains/common/src/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ where
AccountIdOf<R>: From<polkadot_primitives::AccountId> + Into<polkadot_primitives::AccountId>,
<R as frame_system::Config>::RuntimeEvent: From<pallet_balances::Event<R>>,
{
fn on_unbalanceds<B>(
fn on_unbalanceds(
mut fees_then_tips: impl Iterator<
Item = fungible::Credit<R::AccountId, pallet_balances::Pallet<R>>,
>,
Expand Down
2 changes: 1 addition & 1 deletion polkadot/runtime/common/src/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ where
<R as frame_system::Config>::AccountId: From<polkadot_primitives::AccountId>,
<R as frame_system::Config>::AccountId: Into<polkadot_primitives::AccountId>,
{
fn on_unbalanceds<B>(
fn on_unbalanceds(
mut fees_then_tips: impl Iterator<Item = Credit<R::AccountId, pallet_balances::Pallet<R>>>,
) {
if let Some(fees) = fees_then_tips.next() {
Expand Down
23 changes: 23 additions & 0 deletions prdoc/pr_4564.prdoc
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
Copy link
Member

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.

- name: pallet-balances
bump: patch
muharem marked this conversation as resolved.
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
2 changes: 1 addition & 1 deletion substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ type NegativeImbalance = <Balances as Currency<AccountId>>::NegativeImbalance;

pub struct DealWithFees;
impl OnUnbalanced<NegativeImbalance> for DealWithFees {
fn on_unbalanceds<B>(mut fees_then_tips: impl Iterator<Item = NegativeImbalance>) {
fn on_unbalanceds(mut fees_then_tips: impl Iterator<Item = NegativeImbalance>) {
if let Some(fees) = fees_then_tips.next() {
// for fees, 80% to treasury, 20% to author
let mut split = fees.ration(80, 20);
Expand Down
14 changes: 13 additions & 1 deletion substrate/frame/balances/src/impl_currency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub use imbalances::{NegativeImbalance, PositiveImbalance};
mod imbalances {
use super::{result, Config, Imbalance, RuntimeDebug, Saturating, TryDrop, Zero};
use core::mem;
use frame_support::traits::SameOrOther;
use frame_support::traits::{tokens::imbalance::TryMerge, SameOrOther};

/// Opaque, move-only struct with private fields that serves as a token denoting that
/// funds have been created without any equal and opposite accounting.
Expand Down Expand Up @@ -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))
Copy link
Member

@ggwpez ggwpez Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use super::{super::Imbalance as ImbalanceT, Balanced, *};
use crate::traits::{
fungibles,
misc::{SameOrOther, TryDrop},
tokens::{AssetId, Balance},
tokens::{imbalance::TryMerge, AssetId, Balance},
};
use core::marker::PhantomData;
use frame_support_procedural::{EqNoBound, PartialEqNoBound, RuntimeDebugNoBound};
Expand Down Expand Up @@ -157,6 +157,14 @@ impl<B: Balance, OnDrop: HandleImbalanceDrop<B>, OppositeOnDrop: HandleImbalance
}
}

impl<B: Balance, OnDrop: HandleImbalanceDrop<B>, OppositeOnDrop: HandleImbalanceDrop<B>> TryMerge
for Imbalance<B, OnDrop, OppositeOnDrop>
{
fn try_merge(self, other: Self) -> Result<Self, (Self, Self)> {
Ok(self.merge(other))
}
}

/// Converts a `fungibles` `imbalance` instance to an instance of a `fungible` imbalance type.
///
/// This function facilitates imbalance conversions within the implementations of
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ use super::*;
use crate::traits::{
fungible,
misc::{SameOrOther, TryDrop},
tokens::{imbalance::Imbalance as ImbalanceT, AssetId, Balance},
tokens::{
imbalance::{Imbalance as ImbalanceT, TryMerge},
AssetId, Balance,
},
};
use core::marker::PhantomData;
use frame_support_procedural::{EqNoBound, PartialEqNoBound, RuntimeDebugNoBound};
Expand Down Expand Up @@ -176,6 +179,18 @@ impl<
}
}

impl<
A: AssetId,
B: Balance,
OnDrop: HandleImbalanceDrop<A, B>,
OppositeOnDrop: HandleImbalanceDrop<A, B>,
> TryMerge for Imbalance<A, B, OnDrop, OppositeOnDrop>
{
fn try_merge(self, other: Self) -> Result<Self, (Self, Self)> {
self.merge(other)
}
}

/// Converts a `fungible` `imbalance` instance to an instance of a `fungibles` imbalance type using
/// a specified `asset`.
///
Expand Down
16 changes: 15 additions & 1 deletion substrate/frame/support/src/traits/tokens/imbalance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ pub use split_two_ways::SplitTwoWays;
///
/// You can always retrieve the raw balance value using `peek`.
#[must_use]
pub trait Imbalance<Balance>: Sized + TryDrop + Default {
pub trait Imbalance<Balance>: Sized + TryDrop + Default + TryMerge {
/// The oppositely imbalanced type. They come in pairs.
type Opposite: Imbalance<Balance>;

Expand Down Expand Up @@ -182,6 +182,13 @@ pub trait Imbalance<Balance>: Sized + TryDrop + Default {
fn peek(&self) -> Balance;
}

/// Try to merge two imbalances.
pub trait TryMerge: Sized {
/// Consume `self` and an `other` to return a new instance that combines both. Errors with
/// Err(self, other) if the imbalances cannot be merged (e.g. imbalances of different assets).
fn try_merge(self, other: Self) -> Result<Self, (Self, Self)>;
}

#[cfg(feature = "std")]
impl<Balance: Default> Imbalance<Balance> for () {
type Opposite = ();
Expand Down Expand Up @@ -236,3 +243,10 @@ impl<Balance: Default> Imbalance<Balance> for () {
Default::default()
}
}

#[cfg(feature = "std")]
impl TryMerge for () {
fn try_merge(self, _: Self) -> Result<Self, (Self, Self)> {
Ok(())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the advantage of calling on_unbalanced on the merged result vs calling it on every balance individually?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay seems fine to me.

}
}

/// Handler for some imbalance. Infallible.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ pub struct DealWithFees;
impl OnUnbalanced<fungible::Credit<<Runtime as frame_system::Config>::AccountId, Balances>>
for DealWithFees
{
fn on_unbalanceds<B>(
fn on_unbalanceds(
mut fees_then_tips: impl Iterator<
Item = fungible::Credit<<Runtime as frame_system::Config>::AccountId, Balances>,
>,
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/transaction-payment/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub struct DealWithFees;
impl OnUnbalanced<fungible::Credit<<Runtime as frame_system::Config>::AccountId, Balances>>
for DealWithFees
{
fn on_unbalanceds<B>(
fn on_unbalanceds(
mut fees_then_tips: impl Iterator<
Item = fungible::Credit<<Runtime as frame_system::Config>::AccountId, Balances>,
>,
Expand Down
Loading