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>

substrate/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<Self::Balance, DispatchError> {
-		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))
 	}
 }

substrate/frame/balances/src/lib.rs

@@ -205,7 +205,10 @@ type AccountIdLookupOf<T> = <<T as frame_system::Config>::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<T, I> = Credit<<T as frame_system::Config>::AccountId, Pallet<T, I>>;
@@ -1100,43 +1103,43 @@ 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<T::Balance, DispatchError> {
 			if value.is_zero() {
 				return Ok(Zero::zero())
 			}
 
+			let max = <Self as fungible::InspectHold<_>>::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<T::Balance>), DispatchError> {
+				|to_account, is_new| -> Result<((), Option<T::Balance>), DispatchError> {
 					ensure!(!is_new, Error::<T, I>::DeadAccount);
-					Self::try_mutate_account(
-						slashed,
-						|from_account, _| -> Result<T::Balance, DispatchError> {
-							let actual = cmp::min(from_account.reserved, value);
-							ensure!(
-								best_effort || actual == value,
-								Error::<T, I>::InsufficientBalance
-							);
+					Self::try_mutate_account(slashed, |from_account, _| -> DispatchResult {
 						match status {
 							Status::Free =>
 								to_account.free = to_account
@@ -1149,10 +1152,9 @@ pub mod pallet {
 									.checked_add(&actual)
 									.ok_or(ArithmeticError::Overflow)?,
 						}
-							from_account.reserved -= actual;
-							Ok(actual)
-						},
-					)
+						from_account.reserved.saturating_reduce(actual);
+						Ok(())
+					})
 				},
 			)?;
 

substrate/frame/balances/src/types.rs

@@ -102,9 +102,9 @@ pub struct AccountData<Balance> {
 	/// 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.