Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Named reserve #7778

Merged
merged 43 commits into from
Jun 4, 2021
Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
d0b265b
add NamedReservableCurrency
xlc Dec 22, 2020
d13c224
move currency related trait and types into a new file
xlc Dec 23, 2020
597b342
implement NamedReservableCurrency
xlc Dec 23, 2020
47643ad
remove empty reserves
xlc Dec 23, 2020
91e0b59
Update frame/support/src/traits.rs
xlc Dec 23, 2020
22e9e69
Merge remote-tracking branch 'origin/master' into named-reserve
xlc Jan 5, 2021
281bd90
fix build
xlc Jan 6, 2021
52cb8b1
bump year
xlc Jan 6, 2021
59e9b01
add MaxReserves
xlc Jan 6, 2021
747c318
repatriate_reserved_named should put reserved fund into named reserved
xlc Jan 11, 2021
cd63175
Merge remote-tracking branch 'origin/master' into named-reserve
xlc Jan 11, 2021
e504b60
add tests
xlc Jan 11, 2021
b029920
Merge remote-tracking branch 'origin/master' into named-reserve
xlc Jan 12, 2021
3f9a057
add some docs
xlc Jan 12, 2021
734363b
Merge remote-tracking branch 'origin/master' into named-reserve
xlc Jan 17, 2021
995dac0
fix warning
xlc Jan 17, 2021
0a04dce
Merge remote-tracking branch 'origin/master' into named-reserve
xlc Feb 9, 2021
e1644d6
Update lib.rs
gavofyork Feb 26, 2021
1ce677e
Merge remote-tracking branch 'origin/master' into named-reserve
xlc Feb 26, 2021
ddfc2b0
fix test
xlc Feb 26, 2021
66a2517
fix test
xlc Feb 26, 2021
0301c7f
Merge remote-tracking branch 'origin/master' into named-reserve
xlc Feb 28, 2021
dcaf864
fix
xlc Feb 28, 2021
772258c
fix
xlc Feb 28, 2021
b107903
triggier CI
xlc Mar 1, 2021
29f821e
Merge remote-tracking branch 'origin/master' into named-reserve
xlc Mar 22, 2021
b6c55e1
Merge branch 'master' into sw/named-reserve-fix
shaunxw Jun 1, 2021
77ccea7
Move NamedReservableCurrency.
shaunxw Jun 1, 2021
5942996
Use strongly bounded vec for reserves.
shaunxw Jun 1, 2021
4f9154a
Fix test.
shaunxw Jun 1, 2021
baf4a0f
Merge pull request #1 from shaunxw/sw/named-reserve-fix
xlc Jun 1, 2021
b19dc12
remove duplicated file
xlc Jun 1, 2021
72fb670
trigger CI
xlc Jun 1, 2021
d852a81
Make `ReserveIdentifier` assosicated type
xlc Jun 1, 2021
73a0d6f
add helpers
xlc Jun 1, 2021
2ee5092
make ReserveIdentifier assosicated type
xlc Jun 1, 2021
7614ecc
fix
xlc Jun 1, 2021
e6a1d9d
Merge remote-tracking branch 'origin/master' into named-reserve
xlc Jun 1, 2021
643110f
update
xlc Jun 1, 2021
49e18f1
trigger CI
xlc Jun 2, 2021
44d9823
Apply suggestions from code review
xlc Jun 2, 2021
28ff17f
trigger CI
xlc Jun 2, 2021
7ba4dc1
Apply suggestions from code review
gavofyork Jun 4, 2021
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: 2 additions & 0 deletions bin/node-template/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,8 @@ parameter_types! {

impl pallet_balances::Config for Runtime {
type MaxLocks = MaxLocks;
type MaxReserves = ();
type ReserveIdentifier = [u8; 8];
/// The type for recording an account's balance.
type Balance = Balance;
/// The ubiquitous event type.
Expand Down
3 changes: 3 additions & 0 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,10 +375,13 @@ parameter_types! {
// For weight estimation, we assume that the most locks on an individual account will be 50.
// This number may need to be adjusted in the future if this assumption no longer holds true.
pub const MaxLocks: u32 = 50;
pub const MaxReserves: u32 = 50;
}

impl pallet_balances::Config for Runtime {
type MaxLocks = MaxLocks;
type MaxReserves = MaxReserves;
type ReserveIdentifier = [u8; 8];
type Balance = Balance;
type DustRemoval = ();
type Event = Event;
Expand Down
2 changes: 2 additions & 0 deletions frame/assets/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ impl pallet_balances::Config for Test {
type AccountStore = System;
type WeightInfo = ();
type MaxLocks = ();
type MaxReserves = ();
type ReserveIdentifier = [u8; 8];
}

parameter_types! {
Expand Down
2 changes: 2 additions & 0 deletions frame/atomic-swap/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ parameter_types! {
}
impl pallet_balances::Config for Test {
type MaxLocks = ();
type MaxReserves = ();
type ReserveIdentifier = [u8; 8];
type Balance = u64;
type DustRemoval = ();
type Event = Event;
Expand Down
2 changes: 2 additions & 0 deletions frame/babe/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ parameter_types! {

impl pallet_balances::Config for Test {
type MaxLocks = ();
type MaxReserves = ();
type ReserveIdentifier = [u8; 8];
type Balance = u128;
type DustRemoval = ();
type Event = Event;
Expand Down
225 changes: 222 additions & 3 deletions frame/balances/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
//! - [`Currency`](frame_support::traits::Currency): Functions for dealing with a
//! fungible assets system.
//! - [`ReservableCurrency`](frame_support::traits::ReservableCurrency):
//! - [`NamedReservableCurrency`](frame_support::traits::NamedReservableCurrency):
//! Functions for dealing with assets that can be reserved from an account.
//! - [`LockableCurrency`](frame_support::traits::LockableCurrency): Functions for
//! dealing with accounts that allow liquidity restrictions.
Expand Down Expand Up @@ -163,9 +164,9 @@ use frame_support::{
traits::{
Currency, OnUnbalanced, TryDrop, StoredMap, MaxEncodedLen,
WithdrawReasons, LockIdentifier, LockableCurrency, ExistenceRequirement,
Imbalance, SignedImbalance, ReservableCurrency, Get, ExistenceRequirement::KeepAlive,
ExistenceRequirement::AllowDeath,
tokens::{fungible, DepositConsequence, WithdrawConsequence, BalanceStatus as Status}
Imbalance, SignedImbalance, ReservableCurrency, Get, ExistenceRequirement::{AllowDeath, KeepAlive},
NamedReservableCurrency,
tokens::{fungible, DepositConsequence, WithdrawConsequence, BalanceStatus as Status},
}
};
#[cfg(feature = "std")]
Expand Down Expand Up @@ -214,6 +215,12 @@ pub mod pallet {
/// The maximum number of locks that should exist on an account.
/// Not strictly enforced, but used for weight estimation.
type MaxLocks: Get<u32>;

/// The maximum number of named reserves that can exist on an account.
type MaxReserves: Get<u32>;

/// The id type for named reserves.
type ReserveIdentifier: Parameter + Member + MaxEncodedLen + Ord + Copy;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to make ReserveIdentifier configurable?

Sounds like it will be a lot harder to use.

LockIdentifier is the same and defined in the primitives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requested by @gavofyork

#7778 (comment)
#7778 (comment)

I think the goal is make transition to fungible named holds in future easier.

Copy link
Member

@gavofyork gavofyork Jun 4, 2021

Choose a reason for hiding this comment

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

The goal is to ensure that the number of values of ReserveIdentifier (and thus the number of reserves than can possibly be in place at once) is capped at compile time through Rust's type system. This will obviate the need for TooManyReserves error type and MaxReserves parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will obviate the need for TooManyReserves error type and MaxReserves parameter.

May I ask why the MaxReserves parameter was still necessary after all?

}

#[pallet::pallet]
Expand Down Expand Up @@ -409,6 +416,8 @@ pub mod pallet {
ExistingVestingSchedule,
/// Beneficiary account must pre-exist
DeadAccount,
/// Number of named reserves exceed MaxReserves
TooManyReserves,
}

/// The total units issued in the system.
Expand Down Expand Up @@ -444,6 +453,17 @@ pub mod pallet {
ConstU32<300_000>,
>;

/// Named reserves on some account balances.
#[pallet::storage]
#[pallet::getter(fn reserves)]
pub type Reserves<T: Config<I>, I: 'static = ()> = StorageMap<
_,
Blake2_128Concat,
T::AccountId,
BoundedVec<ReserveData<T::ReserveIdentifier, T::Balance>, T::MaxReserves>,
ValueQuery
>;

/// Storage version of the pallet.
///
/// This is set to v2.0.0 for new networks.
Expand Down Expand Up @@ -560,6 +580,15 @@ pub struct BalanceLock<Balance> {
pub reasons: Reasons,
}

/// Store named reserved balance
gavofyork marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, MaxEncodedLen)]
pub struct ReserveData<ReserveIdentifier, Balance> {
/// The identifier for the named reserve
gavofyork marked this conversation as resolved.
Show resolved Hide resolved
pub id: ReserveIdentifier,
/// The amount of the named reserve
gavofyork marked this conversation as resolved.
Show resolved Hide resolved
pub amount: Balance,
}

/// All balance information for an account.
#[derive(Encode, Decode, Clone, PartialEq, Eq, Default, RuntimeDebug, MaxEncodedLen)]
pub struct AccountData<Balance> {
Expand All @@ -575,6 +604,7 @@ pub struct AccountData<Balance> {
///
/// This balance is a 'reserve' balance that other subsystems use in order to set aside tokens
/// that are still 'owned' by the account holder, but which are suspendable.
/// This includes named reserve and unnamed reserve.
pub reserved: Balance,
/// The amount that `free` may not drop below when withdrawing for *anything except transaction
/// fee payment*.
Expand Down Expand Up @@ -1648,6 +1678,195 @@ impl<T: Config<I>, I: 'static> ReservableCurrency<T::AccountId> for Pallet<T, I>
}
}

impl<T: Config<I>, I: 'static> NamedReservableCurrency<T::AccountId> for Pallet<T, I> where
T::Balance: MaybeSerializeDeserialize + Debug
{
type ReserveIdentifier = T::ReserveIdentifier;

fn reserved_balance_named(id: &Self::ReserveIdentifier, who: &T::AccountId) -> Self::Balance {
let reserves = Self::reserves(who);
Copy link
Member

Choose a reason for hiding this comment

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

Note that despite the binary search, this is still O(named_reservations) (due to needing the full deserialisation of Self::reserves).

reserves
.binary_search_by_key(id, |data| data.id)
.map(|index| reserves[index].amount)
.unwrap_or_default()
}

/// Move `value` from the free balance from `who` to a named reserve balance.
///
/// Is a no-op if value to be reserved is zero.
fn reserve_named(id: &Self::ReserveIdentifier, who: &T::AccountId, value: Self::Balance) -> DispatchResult {
if value.is_zero() { return Ok(()) }

Reserves::<T, I>::try_mutate(who, |reserves| -> DispatchResult {
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(index) => {
// this add can't overflow but just to be defensive.
reserves[index].amount = reserves[index].amount.saturating_add(value);
},
Err(index) => {
reserves.try_insert(index, ReserveData {
id: id.clone(),
amount: value
}).map_err(|_| Error::<T, I>::TooManyReserves)?;
},
};
<Self as ReservableCurrency<_>>::reserve(who, value)?;
Ok(())
})
}

/// Unreserve some funds, returning any amount that was unable to be unreserved.
///
/// Is a no-op if the value to be unreserved is zero.
fn unreserve_named(id: &Self::ReserveIdentifier, who: &T::AccountId, value: Self::Balance) -> Self::Balance {
if value.is_zero() { return Zero::zero() }

Reserves::<T, I>::mutate_exists(who, |maybe_reserves| -> Self::Balance {
if let Some(reserves) = maybe_reserves.as_mut() {
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(index) => {
let to_change = cmp::min(reserves[index].amount, value);

let remain = <Self as ReservableCurrency<_>>::unreserve(who, to_change);

// remain should always be zero but just to be defensive here
Copy link
Contributor

@kianenigma kianenigma Jan 11, 2021

Choose a reason for hiding this comment

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

Idea: write a wrapper for all of these defensive unwrap_or_default or saturating_something that we do that will still do the same thing, but flip an on-chain emergency button or log something.

@shawntabrizi I think you once mentioned a similar idea. Did it ever make it into an issue? Else I think it can be.

let actual = to_change.saturating_sub(remain);

// `actual <= to_change` and `to_change <= amount`; qed;
reserves[index].amount -= actual;

if reserves[index].amount.is_zero() {
if reserves.len() == 1 {
// no more named reserves
*maybe_reserves = None;
} else {
// remove this named reserve
reserves.remove(index);
}
}

value - actual
},
Err(_) => {
value
},
}
} else {
value
}
})
}

/// Slash from reserved balance, returning the negative imbalance created,
/// and any amount that was unable to be slashed.
///
/// Is a no-op if the value to be slashed is zero.
fn slash_reserved_named(
Copy link
Contributor

@kianenigma kianenigma Jan 11, 2021

Choose a reason for hiding this comment

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

Could foresee having a trait NamedReserve or sth and then dropping the named postifx, but then I would be worried that by changing an import (use traits::ReservableCurrency -> use traits::NamedReseve) the semantics of T::Currency::slash would get flipped, which can lead to a pretty bad bug.

But correct me if I am wrong, I think rustc is luckily not smart about this and even if only one of the traits is in scope, it would demand the universal function invocation syntax?

sth to consider overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I think it is possible to create a struct NamedReserveAdpator<NamedReservableCurrency, Get<ReserveIdentifier>> that converts NamedReservableCurrency into ReservableCurrency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xlc marked this conversation as resolved.
Show resolved Hide resolved
id: &Self::ReserveIdentifier,
who: &T::AccountId,
value: Self::Balance
) -> (Self::NegativeImbalance, Self::Balance) {
if value.is_zero() { return (NegativeImbalance::zero(), Zero::zero()) }

Reserves::<T, I>::mutate(who, |reserves| -> (Self::NegativeImbalance, Self::Balance) {
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(index) => {
let to_change = cmp::min(reserves[index].amount, value);

let (imb, remain) = <Self as ReservableCurrency<_>>::slash_reserved(who, to_change);

// remain should always be zero but just to be defensive here
let actual = to_change.saturating_sub(remain);

// `actual <= to_change` and `to_change <= amount`; qed;
reserves[index].amount -= actual;

(imb, value - actual)
},
Err(_) => {
(NegativeImbalance::zero(), value)
},
}
})
}

/// Move the reserved balance of one account into the balance of another, according to `status`.
/// If `status` is `Reserved`, the balance will be reserved with given `id`.
///
/// Is a no-op if:
/// - the value to be moved is zero; or
/// - the `slashed` id equal to `beneficiary` and the `status` is `Reserved`.
fn repatriate_reserved_named(
xlc marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn repatriate_reserved_named(
fn repatriate_named_reserve(

probably doesnt matter, but name here is strange

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 simply add _named to all the methods of ReserveCurrency. Not sure if we want consistent naming convention or better method names.

id: &Self::ReserveIdentifier,
slashed: &T::AccountId,
beneficiary: &T::AccountId,
value: Self::Balance,
status: Status,
) -> Result<Self::Balance, DispatchError> {
if value.is_zero() { return Ok(Zero::zero()) }

if slashed == beneficiary {
return match status {
Status::Free => Ok(Self::unreserve_named(id, slashed, value)),
Status::Reserved => Ok(value.saturating_sub(Self::reserved_balance_named(id, slashed))),
};
}

Reserves::<T, I>::try_mutate(slashed, |reserves| -> Result<Self::Balance, DispatchError> {
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(index) => {
let to_change = cmp::min(reserves[index].amount, value);

let actual = if status == Status::Reserved {
// make it the reserved under same identifier
Reserves::<T, I>::try_mutate(beneficiary, |reserves| -> Result<T::Balance, DispatchError> {
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(index) => {
let remain = <Self as ReservableCurrency<_>>::repatriate_reserved(slashed, beneficiary, to_change, status)?;

// remain should always be zero but just to be defensive here
let actual = to_change.saturating_sub(remain);

// this add can't overflow but just to be defensive.
reserves[index].amount = reserves[index].amount.saturating_add(actual);

Ok(actual)
},
Err(index) => {
let remain = <Self as ReservableCurrency<_>>::repatriate_reserved(slashed, beneficiary, to_change, status)?;

// remain should always be zero but just to be defensive here
let actual = to_change.saturating_sub(remain);

reserves.try_insert(index, ReserveData {
id: id.clone(),
amount: actual
}).map_err(|_| Error::<T, I>::TooManyReserves)?;

Ok(actual)
},
}
})?
} else {
let remain = <Self as ReservableCurrency<_>>::repatriate_reserved(slashed, beneficiary, to_change, status)?;

// remain should always be zero but just to be defensive here
to_change.saturating_sub(remain)
};

// `actual <= to_change` and `to_change <= amount`; qed;
reserves[index].amount -= actual;

Ok(value - actual)
},
Err(_) => {
Ok(value)
},
}
})
}
}

impl<T: Config<I>, I: 'static> LockableCurrency<T::AccountId> for Pallet<T, I>
where
T::Balance: MaybeSerializeDeserialize + Debug
Expand Down
Loading