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 3 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
147 changes: 147 additions & 0 deletions frame/balances/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ use frame_support::{
WithdrawReasons, LockIdentifier, LockableCurrency, ExistenceRequirement,
Imbalance, SignedImbalance, ReservableCurrency, Get, ExistenceRequirement::KeepAlive,
ExistenceRequirement::AllowDeath, IsDeadAccount, BalanceStatus as Status,
ReserveIdentifier, NamedReservableCurrency,
}
};
use sp_runtime::{
Expand Down Expand Up @@ -322,6 +323,12 @@ pub struct BalanceLock<Balance> {
pub reasons: Reasons,
}

#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug)]
pub struct ReserveData<Balance> {
pub id: ReserveIdentifier,
pub amount: Balance,
}

/// All balance information for an account.
#[derive(Encode, Decode, Clone, PartialEq, Eq, Default, RuntimeDebug)]
pub struct AccountData<Balance> {
Expand Down Expand Up @@ -397,6 +404,8 @@ decl_storage! {
/// NOTE: Should only be accessed when setting, changing and freeing a lock.
pub Locks get(fn locks): map hasher(blake2_128_concat) T::AccountId => Vec<BalanceLock<T::Balance>>;

pub Reserves get(fn reserves): map hasher(blake2_128_concat) T::AccountId => Vec<ReserveData<T::Balance>>;

/// Storage version of the pallet.
///
/// This is set to v2.0.0 for new networks.
Expand Down Expand Up @@ -1219,6 +1228,144 @@ impl<T: Config<I>, I: Instance> ReservableCurrency<T::AccountId> for Module<T, I
}
}

impl<T: Config<I>, I: Instance> NamedReservableCurrency<T::AccountId> for Module<T, I> where
T::Balance: MaybeSerializeDeserialize + Debug
{
fn reserved_balance_named(id: &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(|idx| reserves[idx].amount)
.unwrap_or_default()
}

/// Move `value` from the free balance from `who` to their reserved balance.
xlc marked this conversation as resolved.
Show resolved Hide resolved
///
/// Is a no-op if value to be reserved is zero.
fn reserve_named(id: &ReserveIdentifier, who: &T::AccountId, value: Self::Balance) -> DispatchResult {
xlc marked this conversation as resolved.
Show resolved Hide resolved
<Self as ReservableCurrency<_>>::reserve(who, value)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm? you are doing a normal reserved and a named here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

named reserve is also a normal reserve so that the value from reserved_balance still make sense (reserved balance + free balance == total_balance)

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh okay I thought they were entirely distinct, and you could only do either Currency::reserved_balance (which would return only unnamed reserved stuff) and Currency::named_reserve_balance.

But I guess having them linked is beneficial as you can query the grand total reserved of an account which is useful.

Reserves::<T, I>::mutate(who, |reserves| {
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(idx) => {
// this add can't overflow but just to be defensive.
reserves[idx].amount = reserves[idx].amount.saturating_add(value)
},
Err(idx) => {
reserves.insert(idx, ReserveData {
id: id.clone(),
amount: 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: &ReserveIdentifier, who: &T::AccountId, value: Self::Balance) -> Self::Balance {
xlc marked this conversation as resolved.
Show resolved Hide resolved
if value.is_zero() { return Zero::zero() }

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

let remain = <Self as ReservableCurrency<_>>::unreserve(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[idx].amount -= actual;

value - actual
},
Err(_) => {
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: &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(idx) => {
let to_change = cmp::min(reserves[idx].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[idx].amount -= actual;

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

/// Move the reserved balance of one account into the balance of another, according to `status`.
///
/// 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: &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(idx) => {
let to_change = cmp::min(reserves[idx].amount, value);

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);

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

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

/// Implement `OnKilledAccount` to remove the local account, if using local account storage.
///
/// NOTE: You probably won't need to use this! This only needs to be "wired in" to System module
Expand Down
Loading