From 785115b3a13901b0c708af8166430bcc9c71f28f Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Mon, 17 Apr 2023 19:13:55 +0100 Subject: [PATCH] Balances: repatriate_reserved should respect freezes (#13885) * repatriate_reserved should respect freezes * Docs * Fix and clean * Formatting * Update frame/balances/src/types.rs Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com> * Fix * Simplify * Fixes * Fixes --------- Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com> --- frame/balances/src/impl_currency.rs | 9 +++- frame/balances/src/lib.rs | 70 +++++++++++++++-------------- frame/balances/src/types.rs | 6 +-- 3 files changed, 46 insertions(+), 39 deletions(-) diff --git a/frame/balances/src/impl_currency.rs b/frame/balances/src/impl_currency.rs index 790a29f004764..329ea289f966e 100644 --- a/frame/balances/src/impl_currency.rs +++ b/frame/balances/src/impl_currency.rs @@ -22,7 +22,7 @@ use frame_support::{ ensure, pallet_prelude::DispatchResult, traits::{ - tokens::{fungible, BalanceStatus as Status}, + tokens::{fungible, BalanceStatus as Status, Fortitude::Polite, Precision::BestEffort}, Currency, DefensiveSaturating, ExistenceRequirement, ExistenceRequirement::AllowDeath, Get, Imbalance, LockIdentifier, LockableCurrency, NamedReservableCurrency, @@ -590,13 +590,18 @@ where /// Is a no-op if: /// - the value to be moved is zero; or /// - the `slashed` id equal to `beneficiary` and the `status` is `Reserved`. + /// + /// This is `Polite` and thus will not repatriate any funds which would lead the total balance + /// to be less than the frozen amount. Returns `Ok` with the actual amount of funds moved, + /// which may be less than `value` since the operation is done an a `BestEffort` basis. fn repatriate_reserved( slashed: &T::AccountId, beneficiary: &T::AccountId, value: Self::Balance, status: Status, ) -> Result { - let actual = Self::do_transfer_reserved(slashed, beneficiary, value, true, status)?; + let actual = + Self::do_transfer_reserved(slashed, beneficiary, value, BestEffort, Polite, status)?; Ok(value.saturating_sub(actual)) } } diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 029a170535a10..74aec1f2d259b 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -205,7 +205,10 @@ type AccountIdLookupOf = <::Lookup as StaticLookup #[frame_support::pallet] pub mod pallet { use super::*; - use frame_support::{pallet_prelude::*, traits::fungible::Credit}; + use frame_support::{ + pallet_prelude::*, + traits::{fungible::Credit, tokens::Precision}, + }; use frame_system::pallet_prelude::*; pub type CreditOf = Credit<::AccountId, Pallet>; @@ -1100,59 +1103,58 @@ pub mod pallet { } /// Move the reserved balance of one account into the balance of another, according to - /// `status`. + /// `status`. This will respect freezes/locks only if `fortitude` is `Polite`. /// - /// Is a no-op if: - /// - the value to be moved is zero; or - /// - the `slashed` id equal to `beneficiary` and the `status` is `Reserved`. + /// Is a no-op if the value to be moved is zero. /// /// NOTE: returns actual amount of transferred value in `Ok` case. pub(crate) fn do_transfer_reserved( slashed: &T::AccountId, beneficiary: &T::AccountId, value: T::Balance, - best_effort: bool, + precision: Precision, + fortitude: Fortitude, status: Status, ) -> Result { if value.is_zero() { return Ok(Zero::zero()) } + let max = >::reducible_total_balance_on_hold( + slashed, fortitude, + ); + let actual = match precision { + Precision::BestEffort => value.min(max), + Precision::Exact => value, + }; + ensure!(actual <= max, TokenError::FundsUnavailable); if slashed == beneficiary { return match status { - Status::Free => Ok(value.saturating_sub(Self::unreserve(slashed, value))), - Status::Reserved => Ok(value.saturating_sub(Self::reserved_balance(slashed))), + Status::Free => Ok(actual.saturating_sub(Self::unreserve(slashed, actual))), + Status::Reserved => Ok(actual), } } - let ((actual, maybe_dust_1), maybe_dust_2) = Self::try_mutate_account( + let ((_, maybe_dust_1), maybe_dust_2) = Self::try_mutate_account( beneficiary, - |to_account, is_new| -> Result<(T::Balance, Option), DispatchError> { + |to_account, is_new| -> Result<((), Option), DispatchError> { ensure!(!is_new, Error::::DeadAccount); - Self::try_mutate_account( - slashed, - |from_account, _| -> Result { - let actual = cmp::min(from_account.reserved, value); - ensure!( - best_effort || actual == value, - Error::::InsufficientBalance - ); - match status { - Status::Free => - to_account.free = to_account - .free - .checked_add(&actual) - .ok_or(ArithmeticError::Overflow)?, - Status::Reserved => - to_account.reserved = to_account - .reserved - .checked_add(&actual) - .ok_or(ArithmeticError::Overflow)?, - } - from_account.reserved -= actual; - Ok(actual) - }, - ) + Self::try_mutate_account(slashed, |from_account, _| -> DispatchResult { + match status { + Status::Free => + to_account.free = to_account + .free + .checked_add(&actual) + .ok_or(ArithmeticError::Overflow)?, + Status::Reserved => + to_account.reserved = to_account + .reserved + .checked_add(&actual) + .ok_or(ArithmeticError::Overflow)?, + } + from_account.reserved.saturating_reduce(actual); + Ok(()) + }) }, )?; diff --git a/frame/balances/src/types.rs b/frame/balances/src/types.rs index c96e1e44b4165..389124402a8f1 100644 --- a/frame/balances/src/types.rs +++ b/frame/balances/src/types.rs @@ -102,9 +102,9 @@ pub struct AccountData { /// This is the sum of all individual holds together with any sums still under the (deprecated) /// reserves API. pub reserved: Balance, - /// The amount that `free` may not drop below when reducing the balance, except for actions - /// where the account owner cannot reasonably benefit from thr balance reduction, such as - /// slashing. + /// The amount that `free + reserved` may not drop below when reducing the balance, except for + /// actions where the account owner cannot reasonably benefit from the balance reduction, such + /// as slashing. pub frozen: Balance, /// Extra information about this account. The MSB is a flag indicating whether the new ref- /// counting logic is in place for this account.