Skip to content

Commit

Permalink
Make ticket non-optional and add ensure_successful method to Consider…
Browse files Browse the repository at this point in the history 
…ation trait (#5359)

Make ticket non-optional and add ensure_successful method to
Consideration trait.

Reverts the optional return ticket type for the new function introduced
in
[polkadot-sdk/4596](#4596)
and adds a helper `ensure_successful` function for the runtime
benchmarks.
Since the existing FRAME pallet represents zero cost with a zero balance
rather than `None` in an option, maintaining the ticket type as a
non-optional balance is beneficial for backward compatibility and helps
avoid unnecessary migrations.
  • Loading branch information
@muharem
muharem authored Aug 14, 2024
1 parent e4f8a6d commit 00946b1
Show file tree
Hide file tree
Showing 6 changed files with 115 additions and 104 deletions.
21 changes: 21 additions & 0 deletions prdoc/pr_5359.prdoc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
title: Make ticket non-optional and add ensure_successful method to Consideration trait

doc:
- audience: Runtime Dev
description: |
Make ticket non-optional and add ensure_successful method to Consideration trait.

Reverts the optional return ticket type for the new function introduced in
[polkadot-sdk/4596](https://github.com/paritytech/polkadot-sdk/pull/4596) and adds a helper
`ensure_successful` function for the runtime benchmarks.
Since the existing FRAME pallet represents zero cost with a zero balance type rather than
`None` in an option, maintaining the ticket type as a non-optional balance is beneficial
for backward compatibility and helps avoid unnecessary migrations.

crates:
- name: frame-support
bump: major
- name: pallet-preimage
bump: major
- name: pallet-balances
bump: patch
41 changes: 23 additions & 18 deletions substrate/frame/balances/src/tests/fungible_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -518,24 +518,25 @@ fn freeze_consideration_works() {
let who = 4;
// freeze amount taken somewhere outside of our (Consideration) scope.
let extend_freeze = 15;
let zero_ticket = Consideration::new(&who, Footprint::from_parts(0, 0)).unwrap();
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0);

let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 10);

let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap().unwrap();
let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap();
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 4);

assert_ok!(Balances::increase_frozen(&TestId::Foo, &who, extend_freeze));
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 4 + extend_freeze);

let ticket = ticket.update(&who, Footprint::from_parts(8, 1)).unwrap().unwrap();
let ticket = ticket.update(&who, Footprint::from_parts(8, 1)).unwrap();
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 8 + extend_freeze);

assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), None);
assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), zero_ticket);
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0 + extend_freeze);

let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 10 + extend_freeze);

let _ = ticket.drop(&who).unwrap();
Expand All @@ -560,24 +561,26 @@ fn hold_consideration_works() {
let who = 4;
// hold amount taken somewhere outside of our (Consideration) scope.
let extend_hold = 15;

let zero_ticket = Consideration::new(&who, Footprint::from_parts(0, 0)).unwrap();
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0);

let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 10);

let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap().unwrap();
let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap();
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 4);

assert_ok!(Balances::hold(&TestId::Foo, &who, extend_hold));
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 4 + extend_hold);

let ticket = ticket.update(&who, Footprint::from_parts(8, 1)).unwrap().unwrap();
let ticket = ticket.update(&who, Footprint::from_parts(8, 1)).unwrap();
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 8 + extend_hold);

assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), None);
assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), zero_ticket);
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0 + extend_hold);

let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 10 + extend_hold);

let _ = ticket.drop(&who).unwrap();
Expand All @@ -600,21 +603,22 @@ fn lone_freeze_consideration_works() {
>;

let who = 4;
let zero_ticket = Consideration::new(&who, Footprint::from_parts(0, 0)).unwrap();
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0);

let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 10);

assert_ok!(Balances::increase_frozen(&TestId::Foo, &who, 5));
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 15);

let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap().unwrap();
let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap();
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 4);

assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), None);
assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), zero_ticket);
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0);

let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 10);

let _ = ticket.drop(&who).unwrap();
Expand All @@ -637,21 +641,22 @@ fn lone_hold_consideration_works() {
>;

let who = 4;
let zero_ticket = Consideration::new(&who, Footprint::from_parts(0, 0)).unwrap();
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0);

let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 10);

assert_ok!(Balances::hold(&TestId::Foo, &who, 5));
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 15);

let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap().unwrap();
let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap();
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 4);

assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), None);
assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), zero_ticket);
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0);

let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 10);

let _ = ticket.drop(&who).unwrap();
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/preimage/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ benchmarks! {
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
hash
) verify {
let ticket = TicketOf::<T>::new(&noter, Footprint { count: 1, size: MAX_SIZE as u64 }).unwrap().unwrap();
let ticket = TicketOf::<T>::new(&noter, Footprint { count: 1, size: MAX_SIZE as u64 }).unwrap();
let s = RequestStatus::Requested { maybe_ticket: Some((noter, ticket)), count: 1, maybe_len: Some(MAX_SIZE) };
assert_eq!(RequestStatusFor::<T>::get(&hash), Some(s));
}
Expand Down
17 changes: 7 additions & 10 deletions substrate/frame/preimage/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,6 @@ pub mod pallet {
type ManagerOrigin: EnsureOrigin<Self::RuntimeOrigin>;

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

Expand Down Expand Up @@ -162,8 +160,6 @@ 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 @@ -274,10 +270,10 @@ impl<T: Config> Pallet<T> {
// unreserve deposit
T::Currency::unreserve(&who, amount);
// take consideration
let Ok(Some(ticket)) =
let Ok(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 @@ -288,10 +284,12 @@ impl<T: Config> Pallet<T> {
T::Currency::unreserve(&who, deposit);
// take consideration
if let Some(len) = maybe_len {
let Ok(Some(ticket)) =
let Ok(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 @@ -351,8 +349,7 @@ 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))?
.ok_or(Error::<T>::NoCost)?;
T::Consideration::new(depositor, Footprint::from_parts(1, len as usize))?;
RequestStatus::Unrequested { ticket: (depositor.clone(), ticket), len }
},
};
Expand Down
26 changes: 15 additions & 11 deletions substrate/frame/support/src/traits/storage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ where
}

/// Some sort of cost taken from account temporarily in order to offset the cost to the chain of
/// holding some data `Footprint` (e.g. [`Footprint`]) in state.
/// holding some data [`Footprint`] in state.
///
/// The cost may be increased, reduced or dropped entirely as the footprint changes.
///
Expand All @@ -217,16 +217,14 @@ 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. `None`
/// implies no cost for a given footprint.
fn new(who: &AccountId, new: Footprint) -> Result<Option<Self>, DispatchError>;
/// be consumed through `update` or `drop` once the footprint changes or is removed.
fn new(who: &AccountId, new: Footprint) -> Result<Self, DispatchError>;

/// 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). `None` implies no cost for
/// a given footprint.
/// and returning the new ticket (or an error if there was an issue).
///
/// For creating tickets and dropping them, you can use the simpler `new` and `drop` instead.
fn update(self, who: &AccountId, new: Footprint) -> Result<Option<Self>, DispatchError>;
fn update(self, who: &AccountId, new: Footprint) -> Result<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 @@ -239,18 +237,24 @@ pub trait Consideration<AccountId, Footprint>:
fn burn(self, _: &AccountId) {
let _ = self;
}
/// Ensure that creating a ticket for a given account and footprint will be successful if done
/// immediately after this call.
#[cfg(feature = "runtime-benchmarks")]
fn ensure_successful(who: &AccountId, new: Footprint);
}

impl<A, F> Consideration<A, F> for () {
fn new(_: &A, _: F) -> Result<Option<Self>, DispatchError> {
Ok(Some(()))
fn new(_: &A, _: F) -> Result<Self, DispatchError> {
Ok(())
}
fn update(self, _: &A, _: F) -> Result<Option<Self>, DispatchError> {
Ok(Some(()))
fn update(self, _: &A, _: F) -> Result<(), DispatchError> {
Ok(())
}
fn drop(self, _: &A) -> Result<(), DispatchError> {
Ok(())
}
#[cfg(feature = "runtime-benchmarks")]
fn ensure_successful(_: &A, _: F) {}
}

macro_rules! impl_incrementable {
Expand Down
Loading

0 comments on commit 00946b1

Please sign in to comment.