From cc291d02f19bcde6d015c9f72104afaba4ffece2 Mon Sep 17 00:00:00 2001 From: Gav Date: Tue, 11 Apr 2023 23:25:43 +0100 Subject: [PATCH 1/9] repatriate_reserved should respect freezes --- frame/balances/src/impl_currency.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/frame/balances/src/impl_currency.rs b/frame/balances/src/impl_currency.rs index 790a29f004764..a9c3eee02dd94 100644 --- a/frame/balances/src/impl_currency.rs +++ b/frame/balances/src/impl_currency.rs @@ -590,12 +590,22 @@ 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 may not repatriate any funds which would lead the total balance to be less than the + /// frozen amount. fn repatriate_reserved( slashed: &T::AccountId, beneficiary: &T::AccountId, value: Self::Balance, status: Status, ) -> Result { + if value > + >::reducible_total_balance_on_hold( + slashed, + Fortitude::Polite, + ) { + return Err(TokenError::FundsUnavailable.into()) + } let actual = Self::do_transfer_reserved(slashed, beneficiary, value, true, status)?; Ok(value.saturating_sub(actual)) } From bc191f93e0f0298fcc18e238e01254b51a0807cf Mon Sep 17 00:00:00 2001 From: Gav Date: Tue, 11 Apr 2023 23:26:43 +0100 Subject: [PATCH 2/9] Docs --- frame/balances/src/types.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/balances/src/types.rs b/frame/balances/src/types.rs index c96e1e44b4165..3b2f60f7218b0 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 thr 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. From 812c3c0eb18c1bc1d395b9462c01f233054ee018 Mon Sep 17 00:00:00 2001 From: Gav Date: Wed, 12 Apr 2023 12:04:13 +0100 Subject: [PATCH 3/9] Fix and clean --- frame/balances/src/impl_currency.rs | 16 +++++----------- frame/balances/src/lib.rs | 21 +++++++++++++-------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/frame/balances/src/impl_currency.rs b/frame/balances/src/impl_currency.rs index a9c3eee02dd94..9ed04ab4ff374 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, Fortitude::Polite, Precision::BestEffort, BalanceStatus as Status}, Currency, DefensiveSaturating, ExistenceRequirement, ExistenceRequirement::AllowDeath, Get, Imbalance, LockIdentifier, LockableCurrency, NamedReservableCurrency, @@ -591,22 +591,16 @@ where /// - the value to be moved is zero; or /// - the `slashed` id equal to `beneficiary` and the `status` is `Reserved`. /// - /// This may not repatriate any funds which would lead the total balance to be less than the - /// frozen amount. + /// 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 { - if value > - >::reducible_total_balance_on_hold( - slashed, - Fortitude::Polite, - ) { - return Err(TokenError::FundsUnavailable.into()) - } - 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..75247af217b7a 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -205,7 +205,7 @@ 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,7 +1100,7 @@ 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 @@ -1111,7 +1111,8 @@ pub mod pallet { slashed: &T::AccountId, beneficiary: &T::AccountId, value: T::Balance, - best_effort: bool, + precision: Precision, + fortitude: Fortitude, status: Status, ) -> Result { if value.is_zero() { @@ -1132,11 +1133,15 @@ pub mod pallet { Self::try_mutate_account( slashed, |from_account, _| -> Result { - let actual = cmp::min(from_account.reserved, value); - ensure!( - best_effort || actual == value, - Error::::InsufficientBalance + 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); match status { Status::Free => to_account.free = to_account @@ -1149,7 +1154,7 @@ pub mod pallet { .checked_add(&actual) .ok_or(ArithmeticError::Overflow)?, } - from_account.reserved -= actual; + from_account.reserved.saturating_reduce(actual); Ok(actual) }, ) From 05d02f040ed3c8a431b1db1481dd4a51751116ac Mon Sep 17 00:00:00 2001 From: Gav Date: Wed, 12 Apr 2023 12:09:25 +0100 Subject: [PATCH 4/9] Formatting --- frame/balances/src/impl_currency.rs | 5 +++-- frame/balances/src/lib.rs | 13 ++++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/frame/balances/src/impl_currency.rs b/frame/balances/src/impl_currency.rs index 9ed04ab4ff374..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, Fortitude::Polite, Precision::BestEffort, BalanceStatus as Status}, + tokens::{fungible, BalanceStatus as Status, Fortitude::Polite, Precision::BestEffort}, Currency, DefensiveSaturating, ExistenceRequirement, ExistenceRequirement::AllowDeath, Get, Imbalance, LockIdentifier, LockableCurrency, NamedReservableCurrency, @@ -600,7 +600,8 @@ where value: Self::Balance, status: Status, ) -> Result { - let actual = Self::do_transfer_reserved(slashed, beneficiary, value, BestEffort, Polite, 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 75247af217b7a..37ae81b612ce8 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, tokens::Precision}}; + use frame_support::{ + pallet_prelude::*, + traits::{fungible::Credit, tokens::Precision}, + }; use frame_system::pallet_prelude::*; pub type CreditOf = Credit<::AccountId, Pallet>; @@ -1133,10 +1136,10 @@ pub mod pallet { Self::try_mutate_account( slashed, |from_account, _| -> Result { - let max = >::reducible_total_balance_on_hold( - slashed, - fortitude, - ); + let max = + >::reducible_total_balance_on_hold( + slashed, fortitude, + ); let actual = match precision { Precision::BestEffort => value.min(max), Precision::Exact => value, From 1d58fe7a80e17791084184649dd9c484b524c0dc Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Fri, 14 Apr 2023 16:26:06 +0100 Subject: [PATCH 5/9] Update frame/balances/src/types.rs Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com> --- frame/balances/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/balances/src/types.rs b/frame/balances/src/types.rs index 3b2f60f7218b0..389124402a8f1 100644 --- a/frame/balances/src/types.rs +++ b/frame/balances/src/types.rs @@ -103,7 +103,7 @@ pub struct AccountData { /// reserves API. pub reserved: Balance, /// The amount that `free + reserved` may not drop below when reducing the balance, except for - /// actions where the account owner cannot reasonably benefit from thr balance reduction, such + /// 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- From 967c166bf704780dfec534ec45e715eda4654cc9 Mon Sep 17 00:00:00 2001 From: Gav Date: Fri, 14 Apr 2023 16:37:04 +0100 Subject: [PATCH 6/9] Fix --- frame/balances/src/lib.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 37ae81b612ce8..0aba2a1427d01 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -1105,9 +1105,7 @@ pub mod pallet { /// Move the reserved balance of one account into the balance of another, according to /// `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( @@ -1123,9 +1121,18 @@ pub mod pallet { } 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))), + return if precision == Precision::BestEffort { + match status { + Status::Free => Ok(value.saturating_sub(Self::unreserve(slashed, value))), + Status::Reserved => Ok(Self::reserved_balance(slashed).min(value)), + } + } else if Self::reserved_balance(slashed) >= value { + if status == Status::Free { + Self::unreserve(slashed, value); + } + Ok(value) + } else { + Err(TokenError::FundsUnavailable.into()) } } From 439cd0adcf05c07e851bee2cf6c87904bd329b15 Mon Sep 17 00:00:00 2001 From: Gav Date: Fri, 14 Apr 2023 16:42:35 +0100 Subject: [PATCH 7/9] Simplify --- frame/balances/src/lib.rs | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 0aba2a1427d01..7f400c188c650 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -1120,21 +1120,20 @@ pub mod pallet { 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, + }; if slashed == beneficiary { - return if precision == Precision::BestEffort { - match status { - Status::Free => Ok(value.saturating_sub(Self::unreserve(slashed, value))), - Status::Reserved => Ok(Self::reserved_balance(slashed).min(value)), - } - } else if Self::reserved_balance(slashed) >= value { - if status == Status::Free { - Self::unreserve(slashed, value); - } - Ok(value) - } else { - Err(TokenError::FundsUnavailable.into()) + return match status { + Status::Free => Ok(actual.saturating_sub(Self::unreserve(slashed, actual))), + Status::Reserved => Ok(actual), } } + ensure!(actual <= max, TokenError::FundsUnavailable); let ((actual, maybe_dust_1), maybe_dust_2) = Self::try_mutate_account( beneficiary, @@ -1143,15 +1142,6 @@ pub mod pallet { Self::try_mutate_account( slashed, |from_account, _| -> Result { - 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); match status { Status::Free => to_account.free = to_account From b8d979de3afe3a8d32e13b37d746023f04d3275f Mon Sep 17 00:00:00 2001 From: Gav Date: Fri, 14 Apr 2023 16:43:26 +0100 Subject: [PATCH 8/9] Fixes --- frame/balances/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 7f400c188c650..7707a9a674ba2 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -1127,13 +1127,13 @@ pub mod pallet { Precision::BestEffort => value.min(max), Precision::Exact => value, }; + ensure!(actual <= max, TokenError::FundsUnavailable); if slashed == beneficiary { return match status { Status::Free => Ok(actual.saturating_sub(Self::unreserve(slashed, actual))), Status::Reserved => Ok(actual), } } - ensure!(actual <= max, TokenError::FundsUnavailable); let ((actual, maybe_dust_1), maybe_dust_2) = Self::try_mutate_account( beneficiary, From aad8a67fdfd16490c61338315f7d869d7a8787bb Mon Sep 17 00:00:00 2001 From: Gav Date: Fri, 14 Apr 2023 16:44:37 +0100 Subject: [PATCH 9/9] Fixes --- frame/balances/src/lib.rs | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 7707a9a674ba2..74aec1f2d259b 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -1135,29 +1135,26 @@ pub mod pallet { } } - 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 { - 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(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(()) + }) }, )?;