Make indices module economically safe

frame/balances/src/lib.rs

@@ -165,7 +165,7 @@ use frame_support::{
 		Currency, OnReapAccount, OnUnbalanced, TryDrop, StoredMap,
 		WithdrawReason, WithdrawReasons, LockIdentifier, LockableCurrency, ExistenceRequirement,
 		Imbalance, SignedImbalance, ReservableCurrency, Get, ExistenceRequirement::KeepAlive,
-		ExistenceRequirement::AllowDeath, IsDeadAccount
+		ExistenceRequirement::AllowDeath, IsDeadAccount, BalanceStatus as Status
 	}
 };
 use sp_runtime::{
@@ -1082,10 +1082,10 @@ impl<T: Trait<I>, I: Instance> Currency<T::AccountId> for Module<T, I> where
 	fn deposit_into_existing(
 		who: &T::AccountId,
 		value: Self::Balance
-	) -> result::Result<Self::PositiveImbalance, DispatchError> {
+	) -> Result<Self::PositiveImbalance, DispatchError> {
 		if value.is_zero() { return Ok(PositiveImbalance::zero()) }
 
-		Self::try_mutate_account(who, |account| -> DispatchResult {
+		Self::try_mutate_account(who, |account| -> Result<Self::PositiveImbalance, DispatchError> {
 			ensure!(!account.total().is_zero(), Error::<T, I>::DeadAccount);
 			account.free = account.free.checked_add(&value).ok_or(Error::<T, I>::Overflow)?;
 			Ok(PositiveImbalance::new(value))
@@ -1241,13 +1241,14 @@ impl<T: Trait<I>, I: Instance> ReservableCurrency<T::AccountId> for Module<T, I>
 		})
 	}
 
-	/// Move the reserved balance of one account into the free balance of another.
+	/// 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.
 	fn repatriate_reserved(
 		slashed: &T::AccountId,
 		beneficiary: &T::AccountId,
 		value: Self::Balance,
+		status: Status,
 	) -> Result<Self::Balance, DispatchError> {
 		if value.is_zero() { return Ok (Zero::zero()) }
 
@@ -1259,7 +1260,10 @@ impl<T: Trait<I>, I: Instance> ReservableCurrency<T::AccountId> for Module<T, I>
 			ensure!(!to_account.total().is_zero(), Error::<T, I>::DeadAccount);
 			Self::try_mutate_account(slashed, |from_account| -> Result<Self::Balance, DispatchError> {
 				let actual = cmp::min(from_account.reserved, value);
-				to_account.free = to_account.free.checked_add(&actual).ok_or(Error::<T, I>::Overflow)?;
+				match status {
+					Status::Free => to_account.free = to_account.free.checked_add(&actual).ok_or(Error::<T, I>::Overflow)?,
+					Status::Reserved => to_account.reserved = to_account.reserved.checked_add(&actual).ok_or(Error::<T, I>::Overflow)?,
+				}
 				from_account.reserved -= actual;
 				Ok(value - actual)
 			})

frame/balances/src/tests.rs

@@ -444,25 +444,39 @@ macro_rules! decl_tests {
 		}
 
 		#[test]
-		fn transferring_reserved_balance_should_work() {
+		fn repatriating_reserved_balance_should_work() {
 			<$ext_builder>::default().build().execute_with(|| {
 				let _ = Balances::deposit_creating(&1, 110);
 				let _ = Balances::deposit_creating(&2, 1);
 				assert_ok!(Balances::reserve(&1, 110));
-				assert_ok!(Balances::repatriate_reserved(&1, &2, 41), 0);
+				assert_ok!(Balances::repatriate_reserved(&1, &2, 41, Status::Free), 0);
 				assert_eq!(Balances::reserved_balance(1), 69);
 				assert_eq!(Balances::free_balance(1), 0);
 				assert_eq!(Balances::reserved_balance(2), 0);
 				assert_eq!(Balances::free_balance(2), 42);
 			});
 		}
 
+		#[test]
+		fn transferring_reserved_balance_should_work() {
+			<$ext_builder>::default().build().execute_with(|| {
+				let _ = Balances::deposit_creating(&1, 110);
+				let _ = Balances::deposit_creating(&2, 1);
+				assert_ok!(Balances::reserve(&1, 110));
+				assert_ok!(Balances::repatriate_reserved(&1, &2, 41, Status::Reserved), 0);
+				assert_eq!(Balances::reserved_balance(1), 69);
+				assert_eq!(Balances::free_balance(1), 0);
+				assert_eq!(Balances::reserved_balance(2), 41);
+				assert_eq!(Balances::free_balance(2), 1);
+			});
+		}
+
 		#[test]
 		fn transferring_reserved_balance_to_nonexistent_should_fail() {
 			<$ext_builder>::default().build().execute_with(|| {
 				let _ = Balances::deposit_creating(&1, 111);
 				assert_ok!(Balances::reserve(&1, 111));
-				assert_noop!(Balances::repatriate_reserved(&1, &2, 42), Error::<$test, _>::DeadAccount);
+				assert_noop!(Balances::repatriate_reserved(&1, &2, 42, Status::Free), Error::<$test, _>::DeadAccount);
 			});
 		}
 
@@ -472,7 +486,7 @@ macro_rules! decl_tests {
 				let _ = Balances::deposit_creating(&1, 110);
 				let _ = Balances::deposit_creating(&2, 1);
 				assert_ok!(Balances::reserve(&1, 41));
-				assert_ok!(Balances::repatriate_reserved(&1, &2, 69), 28);
+				assert_ok!(Balances::repatriate_reserved(&1, &2, 69, Status::Free), 28);
 				assert_eq!(Balances::reserved_balance(1), 0);
 				assert_eq!(Balances::free_balance(1), 69);
 				assert_eq!(Balances::reserved_balance(2), 0);

frame/democracy/src/lib.rs

@@ -30,7 +30,7 @@ use frame_support::{
 	weights::SimpleDispatchInfo,
 	traits::{
 		Currency, ReservableCurrency, LockableCurrency, WithdrawReason, LockIdentifier, Get,
-		OnReapAccount, OnUnbalanced
+		OnReapAccount, OnUnbalanced, BalanceStatus
 	}
 };
 use frame_system::{self as system, ensure_signed, ensure_root};
@@ -802,7 +802,7 @@ decl_module! {
 			let queue = <DispatchQueue<T>>::get();
 			ensure!(!queue.iter().any(|item| &item.1 == &proposal_hash), Error::<T>::Imminent);
 
-			let _ = T::Currency::repatriate_reserved(&old, &who, deposit);
+			let _ = T::Currency::repatriate_reserved(&old, &who, deposit, BalanceStatus::Free);
 			<Preimages<T>>::remove(&proposal_hash);
 			Self::deposit_event(RawEvent::PreimageReaped(proposal_hash, old, deposit, who));
 		}

frame/elections-phragmen/src/lib.rs

@@ -88,7 +88,7 @@ use frame_support::{
 	decl_storage, decl_event, ensure, decl_module, decl_error, weights::SimpleDispatchInfo,
 	traits::{
 		Currency, Get, LockableCurrency, LockIdentifier, ReservableCurrency, WithdrawReasons,
-		ChangeMembers, OnUnbalanced, WithdrawReason, Contains
+		ChangeMembers, OnUnbalanced, WithdrawReason, Contains, BalanceStatus
 	}
 };
 use sp_phragmen::ExtendedBalance;
@@ -311,7 +311,7 @@ decl_module! {
 			let valid = Self::is_defunct_voter(&target);
 			if valid {
 				// reporter will get the voting bond of the target
-				T::Currency::repatriate_reserved(&target, &reporter, T::VotingBond::get())?;
+				T::Currency::repatriate_reserved(&target, &reporter, T::VotingBond::get(), BalanceStatus::Free)?;
 				// remove the target. They are defunct.
 				Self::do_remove_voter(&target, false);
 			} else {

frame/elections/src/lib.rs

@@ -32,7 +32,7 @@ use frame_support::{
 	decl_storage, decl_event, ensure, decl_module, decl_error,
 	weights::SimpleDispatchInfo,
 	traits::{
-		Currency, ExistenceRequirement, Get, LockableCurrency, LockIdentifier,
+		Currency, ExistenceRequirement, Get, LockableCurrency, LockIdentifier, BalanceStatus,
 		OnUnbalanced, ReservableCurrency, WithdrawReason, WithdrawReasons, ChangeMembers
 	}
 };
@@ -501,7 +501,7 @@ decl_module! {
 			if valid {
 				// This only fails if `reporter` doesn't exist, which it clearly must do since its
 				// the origin. Still, it's no more harmful to propagate any error at this point.
-				T::Currency::repatriate_reserved(&who, &reporter, T::VotingBond::get())?;
+				T::Currency::repatriate_reserved(&who, &reporter, T::VotingBond::get(), BalanceStatus::Free)?;
 				Self::deposit_event(RawEvent::VoterReaped(who, reporter));
 			} else {
 				let imbalance = T::Currency::slash_reserved(&reporter, T::VotingBond::get()).0;

frame/generic-asset/src/lib.rs

@@ -166,7 +166,7 @@ use frame_support::{
 	decl_event, decl_module, decl_storage, ensure, decl_error,
 	traits::{
 		Currency, ExistenceRequirement, Imbalance, LockIdentifier, LockableCurrency, ReservableCurrency,
-		SignedImbalance, WithdrawReason, WithdrawReasons, TryDrop,
+		SignedImbalance, WithdrawReason, WithdrawReasons, TryDrop, BalanceStatus,
 	},
 	Parameter, StorageMap,
 };
@@ -714,8 +714,8 @@ impl<T: Trait> Module<T> {
 		}
 	}
 
-	/// Move up to `amount` from reserved balance of account `who` to free balance of account
-	/// `beneficiary`.
+	/// Move up to `amount` from reserved balance of account `who` to balance of account
+	/// `beneficiary`, either free or reserved depending on `status`.
 	///
 	/// As much funds up to `amount` will be moved as possible. If this is less than `amount`, then
 	/// the `remaining` would be returned, else `Zero::zero()`.
@@ -726,13 +726,23 @@ impl<T: Trait> Module<T> {
 		who: &T::AccountId,
 		beneficiary: &T::AccountId,
 		amount: T::Balance,
+		status: BalanceStatus,
 	) -> T::Balance {
 		let b = Self::reserved_balance(asset_id, who);
 		let slash = sp_std::cmp::min(b, amount);
 
-		let original_free_balance = Self::free_balance(asset_id, beneficiary);
-		let new_free_balance = original_free_balance + slash;
-		Self::set_free_balance(asset_id, beneficiary, new_free_balance);
+		match status {
+			BalanceStatus::Free => {
+				let original_free_balance = Self::free_balance(asset_id, beneficiary);
+				let new_free_balance = original_free_balance + slash;
+				Self::set_free_balance(asset_id, beneficiary, new_free_balance);
+			}
+			BalanceStatus::Reserved => {
+				let original_reserved_balance = Self::reserved_balance(asset_id, beneficiary);
+				let new_reserved_balance = original_reserved_balance + slash;
+				Self::set_reserved_balance(asset_id, beneficiary, new_reserved_balance);
+			}
+		}
 
 		let new_reserve_balance = b - slash;
 		Self::set_reserved_balance(asset_id, who, new_reserve_balance);
@@ -1288,8 +1298,9 @@ where
 		slashed: &T::AccountId,
 		beneficiary: &T::AccountId,
 		value: Self::Balance,
+		status: BalanceStatus,
 	) -> result::Result<Self::Balance, DispatchError> {
-		Ok(<Module<T>>::repatriate_reserved(&U::asset_id(), slashed, beneficiary, value))
+		Ok(<Module<T>>::repatriate_reserved(&U::asset_id(), slashed, beneficiary, value, status))
 	}
 }
 

frame/generic-asset/src/tests.rs

@@ -556,7 +556,7 @@ fn slash_reserved_should_return_none() {
 fn repatriate_reserved_return_amount_substracted_by_slash_amount() {
 	ExtBuilder::default().build().execute_with(|| {
 		GenericAsset::set_reserved_balance(&1, &0, 100);
-		assert_eq!(GenericAsset::repatriate_reserved(&1, &0, &1, 130), 30);
+		assert_eq!(GenericAsset::repatriate_reserved(&1, &0, &1, 130, BalanceStatus::Free), 30);
 	});
 }
 
@@ -571,7 +571,7 @@ fn repatriate_reserved_return_amount_substracted_by_slash_amount() {
 fn repatriate_reserved_return_none() {
 	ExtBuilder::default().build().execute_with(|| {
 		GenericAsset::set_reserved_balance(&1, &0, 100);
-		assert_eq!(GenericAsset::repatriate_reserved(&1, &0, &1, 90), 0);
+		assert_eq!(GenericAsset::repatriate_reserved(&1, &0, &1, 90, BalanceStatus::Free), 0);
 	});
 }
 

frame/identity/src/lib.rs

@@ -74,7 +74,7 @@ use sp_runtime::traits::{StaticLookup, EnsureOrigin, Zero, AppendZerosInput};
 use frame_support::{
 	decl_module, decl_event, decl_storage, ensure, decl_error,
 	traits::{Currency, ReservableCurrency, OnUnbalanced, Get},
-	weights::SimpleDispatchInfo,
+	weights::SimpleDispatchInfo, BalanceStatus,
 };
 use frame_system::{self as system, ensure_signed, ensure_root};
 
@@ -821,7 +821,7 @@ decl_module! {
 			match id.judgements.binary_search_by_key(&reg_index, |x| x.0) {
 				Ok(position) => {
 					if let Judgement::FeePaid(fee) = id.judgements[position].1 {
-						let _ = T::Currency::repatriate_reserved(&target, &sender, fee);
+						let _ = T::Currency::repatriate_reserved(&target, &sender, fee, BalanceStatus::Free);
 					}
 					id.judgements[position] = item
 				}

frame/indices/Cargo.toml

@@ -16,6 +16,9 @@ sp-core = { version = "2.0.0", default-features = false, path = "../../primitive
 frame-support = { version = "2.0.0", default-features = false, path = "../support" }
 frame-system = { version = "2.0.0", default-features = false, path = "../system" }
 
+[dev-dependencies]
+pallet-balances = { version = "2.0.0", default-features = false, path = "../balances" }
+
 [features]
 default = ["std"]
 std = [
@@ -28,4 +31,5 @@ std = [
 	"frame-support/std",
 	"sp-runtime/std",
 	"frame-system/std",
+	"pallet-balances/std",
 ]