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

Add ConsiderationFromLegacy traits #2274

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
94 changes: 93 additions & 1 deletion substrate/frame/support/src/traits/tokens/fungible/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,45 @@ use crate::{
traits::{Consideration, Footprint},
};

/// Extension for the `Consideration` trait to migrate legacy `Currency` deposits.
///
/// Provides a `new_from_exact` method for those types using a `fungible` balance frozen,
/// This method is useful when a new ticket needs to be created with a precise balance, instead of
/// deriving it from a footprint.
pub trait FreezeConsiderationFromLegacy<A, F>: Consideration<A>
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
where
A: 'static,
F: 'static + MutateFreeze<A>,
{
/// Create a ticket for a `new` balance attributable to `who`. This ticket *must* ultimately
/// be consumed through `update` or `drop` once a footprint changes or is removed.
fn new_from_exact(_who: &A, _new: F::Balance) -> Result<Option<Self>, DispatchError>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do these return Result<Option<Self>, DispatchError> instead of Result<Self, DispatchError>?

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 believe I implemented that to return None as the default impl. As mentioned earlier, ConsiderationFromLegacy will be required for those pallets that need it during the migration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I'm still not convinced

  1. this feels like a hack to pass a type indirectly to a migration.. As discussed in dm, I'd like to check if it's possible to pass this type directly to the migration if indeed it is only needed in the migration, not the pallet
  2. there are genuine use cases for when you do want to use this directly in the pallet, e.g. @gupnik is working on a lazy migration right now where we're discussing potentially using it

Copy link
Member

Choose a reason for hiding this comment

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

I guess the default impl would never be used and is only meant to make it compile once all currency related crates were removed from the runtime config.
But then it should indeed return an Error and not None.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah would feel much better returning an error to me instead of the None 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I still would like to see if possible to avoid the default impl (and polluting the pallet config) by passing this type directly to the migration it is used in tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liamaharon @ggwpez I replaced None with Err. It should be now good to go.

For reference, here an example of the new method being used: https://github.com/paritytech/polkadot-sdk/pull/1789/files#diff-489bbb1c7c1b1b8803ab344f63079bdc1b7d67db3d2d8809d6a3a755443619f0R77-R107

where
Self: Sized,
{
Ok(None)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This default impl feels like a footgun to me

Suggested change
Self: Sized,
{
Ok(None)
}
Self: Sized;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replied why of the default impl in the next comment

}

/// Extension for `Consideration` trait.
/// Provides a `new_from_exact` method for those types using a `fungible` balance placed on hold.
/// This method is useful when a new ticket needs to be created with a precise balance, instead of
/// deriving it from a footprint.
pub trait HoldConsiderationFromLegacy<A, F>: Consideration<A>
where
A: 'static,
F: 'static + MutateHold<A>,
{
/// Create a ticket for a `new` balance attributable to `who`. This ticket *must* ultimately
/// be consumed through `update` or `drop` once a footprint changes or is removed.
fn new_from_exact(_who: &A, _new: F::Balance) -> Result<Option<Self>, DispatchError>
where
Self: Sized,
{
Ok(None)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same, not sure why anyone would want to use this default impl

Suggested change
Self: Sized,
{
Ok(None)
}
Self: Sized;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The FRAME pallet that makes use of it on its Config would look something like this:

  type Consideration: Consideration<Self::AccountId> + HoldConsiderationFromLegacy<Self::AccountId, BalanceOf<T>>

The default impl is for 3rd parties that make use of the pallet, so then they do not have to manually implement it.

}

/// Consideration method using a `fungible` balance frozen as the cost exacted for the footprint.
///
/// The aggregate amount frozen under `R::get()` for any account which has multiple tickets,
Expand Down Expand Up @@ -118,7 +157,21 @@ impl<
}
}

/// Consideration method using a `fungible` balance frozen as the cost exacted for the footprint.
impl<
A: 'static,
F: 'static + MutateFreeze<A>,
R: 'static + Get<F::Id>,
D: 'static + Convert<Footprint, F::Balance>,
> FreezeConsiderationFromLegacy<A, F> for FreezeConsideration<A, F, R, D>
{
fn new_from_exact(who: &A, new: F::Balance) -> Result<Option<Self>, DispatchError> {
F::increase_frozen(&R::get(), who, new)?;
Ok(Some(Self(new, PhantomData)))
}
}

/// Consideration method using a `fungible` balance placed on hold as the cost exacted for the
/// footprint.
#[derive(
CloneNoBound,
EqNoBound,
Expand Down Expand Up @@ -163,6 +216,19 @@ impl<
}
}

impl<
A: 'static,
F: 'static + MutateHold<A>,
R: 'static + Get<F::Reason>,
D: 'static + Convert<Footprint, F::Balance>,
> HoldConsiderationFromLegacy<A, F> for HoldConsideration<A, F, R, D>
{
fn new_from_exact(who: &A, new: F::Balance) -> Result<Option<Self>, DispatchError> {
F::hold(&R::get(), who, new)?;
Ok(Some(Self(new, PhantomData)))
}
}

/// Basic consideration method using a `fungible` balance frozen as the cost exacted for the
/// footprint.
///
Expand Down Expand Up @@ -202,6 +268,19 @@ impl<
}
}

impl<
A: 'static,
Fx: 'static + MutateFreeze<A>,
Rx: 'static + Get<Fx::Id>,
D: 'static + Convert<Footprint, Fx::Balance>,
> FreezeConsiderationFromLegacy<A, Fx> for LoneFreezeConsideration<A, Fx, Rx, D>
{
fn new_from_exact(who: &A, new: Fx::Balance) -> Result<Option<Self>, DispatchError> {
ensure!(Fx::balance_frozen(&Rx::get(), who).is_zero(), DispatchError::Unavailable);
Fx::set_frozen(&Rx::get(), who, new, Polite).map(|_| Some(Self(PhantomData)))
}
}

/// Basic consideration method using a `fungible` balance placed on hold as the cost exacted for the
/// footprint.
///
Expand Down Expand Up @@ -243,3 +322,16 @@ impl<
let _ = F::burn_all_held(&R::get(), who, BestEffort, Force);
}
}

impl<
A: 'static,
F: 'static + MutateHold<A>,
R: 'static + Get<F::Reason>,
D: 'static + Convert<Footprint, F::Balance>,
> HoldConsiderationFromLegacy<A, F> for LoneHoldConsideration<A, F, R, D>
{
fn new_from_exact(who: &A, new: F::Balance) -> Result<Option<Self>, DispatchError> {
ensure!(F::balance_on_hold(&R::get(), who).is_zero(), DispatchError::Unavailable);
F::set_on_hold(&R::get(), who, new).map(|_| Some(Self(PhantomData)))
}
}