Various minor fixes (#13945)

* Fix: Incorrect implementation of can_reserve check

* Fix: Incorrect migration of consumer counting for existing accounts with frozen amounts

* Fix: Inconsistent implementation between assets can_deposit and new_account

* Fixes

* Fixes

* Another fix

* Update tests.rs

* Update fungible_tests.rs

* Use `can_accrue_consumers` in the body of `can_inc_consumer`

---------

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: parity-processbot <>

substrate/frame/assets/src/functions.rs

@@ -138,7 +138,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			if amount < details.min_balance {
 				return DepositConsequence::BelowMinimum
 			}
-			if !details.is_sufficient && !frame_system::Pallet::<T>::can_inc_consumer(who) {
+			if !details.is_sufficient && !frame_system::Pallet::<T>::can_accrue_consumers(who, 2) {
 				return DepositConsequence::CannotCreate
 			}
 			if details.is_sufficient && details.sufficients.checked_add(1).is_none() {

substrate/frame/assets/src/tests.rs

@@ -21,7 +21,7 @@ use super::*;
 use crate::{mock::*, Error};
 use frame_support::{
 	assert_noop, assert_ok,
-	traits::{fungibles::InspectEnumerable, Currency},
+	traits::{fungibles::InspectEnumerable, tokens::Preservation::Protect, Currency},
 };
 use pallet_balances::Error as BalancesError;
 use sp_io::storage;
@@ -33,6 +33,25 @@ fn asset_ids() -> Vec<u32> {
 	s
 }
 
+#[test]
+fn transfer_should_never_burn() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, false, 1));
+		Balances::make_free_balance_be(&1, 100);
+		Balances::make_free_balance_be(&2, 100);
+
+		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
+		assert_eq!(Assets::balance(0, 1), 100);
+
+		while System::inc_consumers(&2).is_ok() {}
+		let _ = System::dec_consumers(&2);
+		// Exactly one consumer ref remaining.
+
+		let _ = <Assets as fungibles::Mutate<_>>::transfer(0, &1, &2, 50, Protect);
+		assert_eq!(Assets::balance(0, 1) + Assets::balance(0, 2), 100);
+	});
+}
+
 #[test]
 fn basic_minting_should_work() {
 	new_test_ext().execute_with(|| {
@@ -57,10 +76,7 @@ fn minting_too_many_insufficient_assets_fails() {
 		Balances::make_free_balance_be(&1, 100);
 		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
 		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 1, 1, 100));
-		assert_noop!(
-			Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100),
-			Error::<Test>::UnavailableConsumer
-		);
+		assert_noop!(Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100), TokenError::CannotCreate);
 
 		Balances::make_free_balance_be(&2, 1);
 		assert_ok!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 100));
@@ -78,10 +94,7 @@ fn minting_insufficient_asset_with_deposit_should_work_when_consumers_exhausted(
 		Balances::make_free_balance_be(&1, 100);
 		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
 		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 1, 1, 100));
-		assert_noop!(
-			Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100),
-			Error::<Test>::UnavailableConsumer
-		);
+		assert_noop!(Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100), TokenError::CannotCreate);
 
 		assert_ok!(Assets::touch(RuntimeOrigin::signed(1), 2));
 		assert_eq!(Balances::reserved_balance(&1), 10);
@@ -1322,3 +1335,23 @@ fn asset_create_and_destroy_is_reverted_if_callback_fails() {
 		);
 	});
 }
+
+#[test]
+fn multiple_transfer_alls_work_ok() {
+	new_test_ext().execute_with(|| {
+		// Only run PoC when the system pallet is enabled, since the underlying bug is in the
+		// system pallet it won't work with BalancesAccountStore
+		// Start with a balance of 100
+		Balances::force_set_balance(RuntimeOrigin::root(), 1, 100).unwrap();
+		// Emulate a sufficient, in reality this could be reached by transferring a sufficient
+		// asset to the account
+		assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1));
+		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
+		// Spend the same balance multiple times
+		assert_ok!(Balances::transfer_all(RuntimeOrigin::signed(1), 1337, false));
+		assert_ok!(Balances::transfer_all(RuntimeOrigin::signed(1), 1337, false));
+
+		assert_eq!(Balances::free_balance(&1), 0);
+		assert_eq!(Balances::free_balance(&1337), 100);
+	});
+}

substrate/frame/balances/src/impl_currency.rs

@@ -490,7 +490,9 @@ where
 			return true
 		}
 		Self::account(who).free.checked_sub(&value).map_or(false, |new_balance| {
-			Self::ensure_can_withdraw(who, value, WithdrawReasons::RESERVE, new_balance).is_ok()
+			new_balance >= T::ExistentialDeposit::get() &&
+				Self::ensure_can_withdraw(who, value, WithdrawReasons::RESERVE, new_balance)
+					.is_ok()
 		})
 	}
 

substrate/frame/balances/src/lib.rs

@@ -788,7 +788,7 @@ pub mod pallet {
 				return false
 			}
 			a.flags.set_new_logic();
-			if !a.reserved.is_zero() || !a.frozen.is_zero() {
+			if !a.reserved.is_zero() && a.frozen.is_zero() {
 				if system::Pallet::<T>::providers(who) == 0 {
 					// Gah!! We have no provider refs :(
 					// This shouldn't practically happen, but we need a failsafe anyway: let's give

substrate/frame/balances/src/tests/currency_tests.rs

@@ -1180,6 +1180,16 @@ fn named_reserve_should_work() {
 	});
 }
 
+#[test]
+fn reserve_must_succeed_if_can_reserve_does() {
+	ExtBuilder::default().build_and_execute_with(|| {
+		let _ = Balances::deposit_creating(&1, 1);
+		let _ = Balances::deposit_creating(&2, 2);
+		assert!(Balances::can_reserve(&1, 1) == Balances::reserve(&1, 1).is_ok());
+		assert!(Balances::can_reserve(&2, 1) == Balances::reserve(&2, 1).is_ok());
+	});
+}
+
 #[test]
 fn reserved_named_to_yourself_should_work() {
 	ExtBuilder::default().build_and_execute_with(|| {

substrate/frame/balances/src/tests/fungible_tests.rs

@@ -398,6 +398,31 @@ fn unholding_frees_hold_slot() {
 		});
 }
 
+#[test]
+fn sufficients_work_properly_with_reference_counting() {
+	ExtBuilder::default()
+		.existential_deposit(1)
+		.monied(true)
+		.build_and_execute_with(|| {
+			// Only run PoC when the system pallet is enabled, since the underlying bug is in the
+			// system pallet it won't work with BalancesAccountStore
+			if UseSystem::get() {
+				// Start with a balance of 100
+				<Balances as fungible::Mutate<_>>::set_balance(&1, 100);
+				// Emulate a sufficient, in reality this could be reached by transferring a
+				// sufficient asset to the account
+				System::inc_sufficients(&1);
+				// Spend the same balance multiple times
+				assert_ok!(<Balances as fungible::Mutate<_>>::transfer(&1, &1337, 100, Expendable));
+				assert_eq!(Balances::free_balance(&1), 0);
+				assert_noop!(
+					<Balances as fungible::Mutate<_>>::transfer(&1, &1337, 100, Expendable),
+					TokenError::FundsUnavailable
+				);
+			}
+		});
+}
+
 #[test]
 fn emit_events_with_changing_freezes() {
 	ExtBuilder::default().build_and_execute_with(|| {

substrate/frame/system/src/lib.rs

@@ -1222,10 +1222,20 @@ impl<T: Config> Pallet<T> {
 		a.consumers == 0 || a.providers > 1
 	}
 
-	/// True if the account has at least one provider reference.
-	pub fn can_inc_consumer(who: &T::AccountId) -> bool {
+	/// True if the account has at least one provider reference and adding `amount` consumer
+	/// references would not take it above the the maximum.
+	pub fn can_accrue_consumers(who: &T::AccountId, amount: u32) -> bool {
 		let a = Account::<T>::get(who);
-		a.providers > 0 && a.consumers < T::MaxConsumers::max_consumers()
+		match a.consumers.checked_add(amount) {
+			Some(c) => a.providers > 0 && c <= T::MaxConsumers::max_consumers(),
+			None => false,
+		}
+	}
+
+	/// True if the account has at least one provider reference and fewer consumer references than
+	/// the maximum.
+	pub fn can_inc_consumer(who: &T::AccountId) -> bool {
+		Self::can_accrue_consumers(who, 1)
 	}
 
 	/// Deposits an event into this block's event record.
@@ -1679,8 +1689,10 @@ impl<T: Config> StoredMap<T::AccountId, T::AccountData> for Pallet<T> {
 		let is_default = account.data == T::AccountData::default();
 		let mut some_data = if is_default { None } else { Some(account.data) };
 		let result = f(&mut some_data)?;
-		if Self::providers(k) > 0 {
+		if Self::providers(k) > 0 || Self::sufficients(k) > 0 {
 			Account::<T>::mutate(k, |a| a.data = some_data.unwrap_or_default());
+		} else {
+			Account::<T>::remove(k)
 		}
 		Ok(result)
 	}