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

Frame: Consideration trait generic over Footprint and indicates zero cost #4596

Merged
merged 7 commits into from
Jun 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
19 changes: 11 additions & 8 deletions substrate/frame/preimage/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,9 @@ pub mod pallet {
type ManagerOrigin: EnsureOrigin<Self::RuntimeOrigin>;

/// A means of providing some cost while data is stored on-chain.
type Consideration: Consideration<Self::AccountId>;
///
/// Should never return a `None`, implying no cost for a non-empty preimage.
Copy link
Member

Choose a reason for hiding this comment

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

Why should it not do that? The implementor of the Consideration should be able to return as they see fit.
Otherwise it would probably need a Consideration and MaybeConsideration trait to disambiguate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. But I did now want to change the behaviour of this pallet, and make only a patch. Otherwise we need a migration for the preimage pallet.
Maybe I should rephrase it, that we do expect some cost for any non zero footprint, so it should not return none.

Copy link
Member

Choose a reason for hiding this comment

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

Okay i see. The maybe add a check to the integrity_test of the pallet that a zero sized preimage still needs a deposit.

type Consideration: Consideration<Self::AccountId, Footprint>;
}

#[pallet::pallet]
Expand Down Expand Up @@ -158,6 +160,8 @@ pub mod pallet {
TooMany,
/// Too few hashes were requested to be upgraded (i.e. zero).
TooFew,
/// No ticket with a cost was returned by [`Config::Consideration`] to store the preimage.
NoCost,
}

/// A reason for this pallet placing a hold on funds.
Expand Down Expand Up @@ -268,10 +272,10 @@ impl<T: Config> Pallet<T> {
// unreserve deposit
T::Currency::unreserve(&who, amount);
// take consideration
let Ok(ticket) =
let Ok(Some(ticket)) =
T::Consideration::new(&who, Footprint::from_parts(1, len as usize))
.defensive_proof("Unexpected inability to take deposit after unreserved")
else {
defensive!("None ticket or inability to take deposit after unreserved");
return true
};
RequestStatus::Unrequested { ticket: (who, ticket), len }
Expand All @@ -282,12 +286,10 @@ impl<T: Config> Pallet<T> {
T::Currency::unreserve(&who, deposit);
// take consideration
if let Some(len) = maybe_len {
let Ok(ticket) =
let Ok(Some(ticket)) =
T::Consideration::new(&who, Footprint::from_parts(1, len as usize))
.defensive_proof(
"Unexpected inability to take deposit after unreserved",
)
else {
defensive!("None ticket or inability to take deposit after unreserved");
return true
};
Some((who, ticket))
Expand Down Expand Up @@ -347,7 +349,8 @@ impl<T: Config> Pallet<T> {
RequestStatus::Requested { maybe_ticket: None, count: 1, maybe_len: Some(len) },
(None, Some(depositor)) => {
let ticket =
T::Consideration::new(depositor, Footprint::from_parts(1, len as usize))?;
T::Consideration::new(depositor, Footprint::from_parts(1, len as usize))?
.ok_or(Error::<T>::NoCost)?;
RequestStatus::Unrequested { ticket: (depositor.clone(), ticket), len }
},
};
Expand Down
26 changes: 15 additions & 11 deletions substrate/frame/support/src/traits/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ where
}

/// Some sort of cost taken from account temporarily in order to offset the cost to the chain of
/// holding some data [`Footprint`] in state.
/// holding some data `Footprint` (e.g. [`Footprint`]) in state.
///
/// The cost may be increased, reduced or dropped entirely as the footprint changes.
///
Expand All @@ -206,16 +206,20 @@ where
/// treated as one*. Don't type to duplicate it, and remember to drop it when you're done with
/// it.
#[must_use]
pub trait Consideration<AccountId>: Member + FullCodec + TypeInfo + MaxEncodedLen {
pub trait Consideration<AccountId, Footprint>:
Member + FullCodec + TypeInfo + MaxEncodedLen
{
/// Create a ticket for the `new` footprint attributable to `who`. This ticket *must* ultimately
/// be consumed through `update` or `drop` once the footprint changes or is removed.
fn new(who: &AccountId, new: Footprint) -> Result<Self, DispatchError>;
/// be consumed through `update` or `drop` once the footprint changes or is removed. `None`
/// implies no cost for a given footprint.
fn new(who: &AccountId, new: Footprint) -> Result<Option<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.

Self can also say for itself that there is no cost (with 0 balance for example), but this wont let a pallet to have no storage entry for such case.

gupnik marked this conversation as resolved.
Show resolved Hide resolved

/// Optionally consume an old ticket and alter the footprint, enforcing the new cost to `who`
/// and returning the new ticket (or an error if there was an issue).
/// and returning the new ticket (or an error if there was an issue). `None` implies no cost for
/// a given footprint.
///
/// For creating tickets and dropping them, you can use the simpler `new` and `drop` instead.
fn update(self, who: &AccountId, new: Footprint) -> Result<Self, DispatchError>;
fn update(self, who: &AccountId, new: Footprint) -> Result<Option<Self>, DispatchError>;

/// Consume a ticket for some `old` footprint attributable to `who` which should now been freed.
fn drop(self, who: &AccountId) -> Result<(), DispatchError>;
Expand All @@ -230,12 +234,12 @@ pub trait Consideration<AccountId>: Member + FullCodec + TypeInfo + MaxEncodedLe
}
}

impl<A> Consideration<A> for () {
fn new(_: &A, _: Footprint) -> Result<Self, DispatchError> {
Ok(())
impl<A, F> Consideration<A, F> for () {
fn new(_: &A, _: F) -> Result<Option<Self>, DispatchError> {
Ok(Some(()))
}
fn update(self, _: &A, _: Footprint) -> Result<(), DispatchError> {
Ok(())
fn update(self, _: &A, _: F) -> Result<Option<Self>, DispatchError> {
Ok(Some(()))
}
fn drop(self, _: &A) -> Result<(), DispatchError> {
Ok(())
Expand Down
113 changes: 79 additions & 34 deletions substrate/frame/support/src/traits/tokens/fungible/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,31 +198,40 @@ use crate::{
MaxEncodedLen,
RuntimeDebugNoBound,
)]
#[scale_info(skip_type_params(A, F, R, D))]
#[scale_info(skip_type_params(A, F, R, D, Fp))]
#[codec(mel_bound())]
pub struct FreezeConsideration<A, F, R, D>(F::Balance, PhantomData<fn() -> (A, R, D)>)
pub struct FreezeConsideration<A, F, R, D, Fp>(F::Balance, PhantomData<fn() -> (A, R, D, Fp)>)
where
F: MutateFreeze<A>;
impl<
A: 'static,
F: 'static + MutateFreeze<A>,
R: 'static + Get<F::Id>,
D: 'static + Convert<Footprint, F::Balance>,
> Consideration<A> for FreezeConsideration<A, F, R, D>
D: 'static + Convert<Fp, F::Balance>,
Fp: 'static,
> Consideration<A, Fp> for FreezeConsideration<A, F, R, D, Fp>
{
fn new(who: &A, footprint: Footprint) -> Result<Self, DispatchError> {
fn new(who: &A, footprint: Fp) -> Result<Option<Self>, DispatchError> {
let new = D::convert(footprint);
F::increase_frozen(&R::get(), who, new)?;
Ok(Self(new, PhantomData))
if new.is_zero() {
Ok(None)
} else {
F::increase_frozen(&R::get(), who, new)?;
Ok(Some(Self(new, PhantomData)))
}
}
fn update(self, who: &A, footprint: Footprint) -> Result<Self, DispatchError> {
fn update(self, who: &A, footprint: Fp) -> Result<Option<Self>, DispatchError> {
let new = D::convert(footprint);
if self.0 > new {
F::decrease_frozen(&R::get(), who, self.0 - new)?;
} else if new > self.0 {
F::increase_frozen(&R::get(), who, new - self.0)?;
}
Ok(Self(new, PhantomData))
if new.is_zero() {
Ok(None)
} else {
Ok(Some(Self(new, PhantomData)))
}
}
fn drop(self, who: &A) -> Result<(), DispatchError> {
F::decrease_frozen(&R::get(), who, self.0).map(|_| ())
Expand All @@ -240,31 +249,43 @@ impl<
MaxEncodedLen,
RuntimeDebugNoBound,
)]
#[scale_info(skip_type_params(A, F, R, D))]
#[scale_info(skip_type_params(A, F, R, D, Fp))]
#[codec(mel_bound())]
pub struct HoldConsideration<A, F, R, D>(F::Balance, PhantomData<fn() -> (A, R, D)>)
pub struct HoldConsideration<A, F, R, D, Fp = Footprint>(
F::Balance,
PhantomData<fn() -> (A, R, D, Fp)>,
)
where
F: MutateHold<A>;
impl<
A: 'static,
F: 'static + MutateHold<A>,
R: 'static + Get<F::Reason>,
D: 'static + Convert<Footprint, F::Balance>,
> Consideration<A> for HoldConsideration<A, F, R, D>
D: 'static + Convert<Fp, F::Balance>,
Fp: 'static,
> Consideration<A, Fp> for HoldConsideration<A, F, R, D, Fp>
{
fn new(who: &A, footprint: Footprint) -> Result<Self, DispatchError> {
fn new(who: &A, footprint: Fp) -> Result<Option<Self>, DispatchError> {
let new = D::convert(footprint);
F::hold(&R::get(), who, new)?;
Ok(Self(new, PhantomData))
if new.is_zero() {
Ok(None)
} else {
F::hold(&R::get(), who, new)?;
Ok(Some(Self(new, PhantomData)))
}
}
fn update(self, who: &A, footprint: Footprint) -> Result<Self, DispatchError> {
fn update(self, who: &A, footprint: Fp) -> Result<Option<Self>, DispatchError> {
let new = D::convert(footprint);
if self.0 > new {
F::release(&R::get(), who, self.0 - new, BestEffort)?;
} else if new > self.0 {
F::hold(&R::get(), who, new - self.0)?;
}
Ok(Self(new, PhantomData))
if new.is_zero() {
Ok(None)
} else {
Ok(Some(Self(new, PhantomData)))
}
}
fn drop(self, who: &A) -> Result<(), DispatchError> {
F::release(&R::get(), who, self.0, BestEffort).map(|_| ())
Expand All @@ -291,22 +312,34 @@ impl<
MaxEncodedLen,
RuntimeDebugNoBound,
)]
#[scale_info(skip_type_params(A, Fx, Rx, D))]
#[scale_info(skip_type_params(A, Fx, Rx, D, Fp))]
#[codec(mel_bound())]
pub struct LoneFreezeConsideration<A, Fx, Rx, D>(PhantomData<fn() -> (A, Fx, Rx, D)>);
pub struct LoneFreezeConsideration<A, Fx, Rx, D, Fp>(PhantomData<fn() -> (A, Fx, Rx, D, Fp)>);
impl<
A: 'static,
Fx: 'static + MutateFreeze<A>,
Rx: 'static + Get<Fx::Id>,
D: 'static + Convert<Footprint, Fx::Balance>,
> Consideration<A> for LoneFreezeConsideration<A, Fx, Rx, D>
D: 'static + Convert<Fp, Fx::Balance>,
Fp: 'static,
> Consideration<A, Fp> for LoneFreezeConsideration<A, Fx, Rx, D, Fp>
{
fn new(who: &A, footprint: Footprint) -> Result<Self, DispatchError> {
fn new(who: &A, footprint: Fp) -> Result<Option<Self>, DispatchError> {
ensure!(Fx::balance_frozen(&Rx::get(), who).is_zero(), DispatchError::Unavailable);
Fx::set_frozen(&Rx::get(), who, D::convert(footprint), Polite).map(|_| Self(PhantomData))
let new = D::convert(footprint);
if new.is_zero() {
Ok(None)
} else {
Fx::set_frozen(&Rx::get(), who, new, Polite).map(|_| Some(Self(PhantomData)))
}
}
fn update(self, who: &A, footprint: Footprint) -> Result<Self, DispatchError> {
Fx::set_frozen(&Rx::get(), who, D::convert(footprint), Polite).map(|_| Self(PhantomData))
fn update(self, who: &A, footprint: Fp) -> Result<Option<Self>, DispatchError> {
let new = D::convert(footprint);
let _ = Fx::set_frozen(&Rx::get(), who, new, Polite)?;
if new.is_zero() {
Ok(None)
} else {
Ok(Some(Self(PhantomData)))
}
gupnik marked this conversation as resolved.
Show resolved Hide resolved
}
fn drop(self, who: &A) -> Result<(), DispatchError> {
Fx::thaw(&Rx::get(), who).map(|_| ())
Expand All @@ -330,22 +363,34 @@ impl<
MaxEncodedLen,
RuntimeDebugNoBound,
)]
#[scale_info(skip_type_params(A, Fx, Rx, D))]
#[scale_info(skip_type_params(A, Fx, Rx, D, Fp))]
#[codec(mel_bound())]
pub struct LoneHoldConsideration<A, Fx, Rx, D>(PhantomData<fn() -> (A, Fx, Rx, D)>);
pub struct LoneHoldConsideration<A, Fx, Rx, D, Fp>(PhantomData<fn() -> (A, Fx, Rx, D, Fp)>);
impl<
A: 'static,
F: 'static + MutateHold<A>,
R: 'static + Get<F::Reason>,
D: 'static + Convert<Footprint, F::Balance>,
> Consideration<A> for LoneHoldConsideration<A, F, R, D>
D: 'static + Convert<Fp, F::Balance>,
Fp: 'static,
> Consideration<A, Fp> for LoneHoldConsideration<A, F, R, D, Fp>
{
fn new(who: &A, footprint: Footprint) -> Result<Self, DispatchError> {
fn new(who: &A, footprint: Fp) -> Result<Option<Self>, DispatchError> {
ensure!(F::balance_on_hold(&R::get(), who).is_zero(), DispatchError::Unavailable);
F::set_on_hold(&R::get(), who, D::convert(footprint)).map(|_| Self(PhantomData))
let new = D::convert(footprint);
if new.is_zero() {
Ok(None)
} else {
F::set_on_hold(&R::get(), who, new).map(|_| Some(Self(PhantomData)))
}
}
fn update(self, who: &A, footprint: Footprint) -> Result<Self, DispatchError> {
F::set_on_hold(&R::get(), who, D::convert(footprint)).map(|_| Self(PhantomData))
fn update(self, who: &A, footprint: Fp) -> Result<Option<Self>, DispatchError> {
let new = D::convert(footprint);
let _ = F::set_on_hold(&R::get(), who, new)?;
if new.is_zero() {
Ok(None)
} else {
Ok(Some(Self(PhantomData)))
}
gupnik marked this conversation as resolved.
Show resolved Hide resolved
}
fn drop(self, who: &A) -> Result<(), DispatchError> {
F::release_all(&R::get(), who, BestEffort).map(|_| ())
Expand Down
Loading