Introduce ReservableCurrency Trait in Balances Module (#2124)

* Introduce `ReservableCurrency`

* Update Docs for `ReservableCurrency`

* Update Tests

* Bump spec and rebuild wasm

* Extra: Add a note to `slash()`

Discussion in Riot clarified the behavior of `slash()` and `can_slash()`. Trying to sneak clarifying comments about it into this PR

* Update lib.rs

* Don't drop the periods!

CC @shawntabrizi

substrate/node/runtime/src/lib.rs

@@ -58,7 +58,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("node"),
 	impl_name: create_runtime_str!("substrate-node"),
 	authoring_version: 10,
-	spec_version: 44,
+	spec_version: 45,
 	impl_version: 45,
 	apis: RUNTIME_API_VERSIONS,
 };

substrate/srml/balances/src/lib.rs

@@ -62,6 +62,8 @@
 //!
 //! - [`Currency`](https://crates.parity.io/srml_support/traits/trait.Currency.html): Functions for dealing with a
 //! fungible assets system.
+//! - [`ReservableCurrency`](https://crates.parity.io/srml_support/traits/trait.ReservableCurrency.html):
+//! Functions for dealing with assets that can be reserved from an account.
 //! - [`LockableCurrency`](https://crates.parity.io/srml_support/traits/trait.LockableCurrency.html): Functions for
 //! dealing with accounts that allow liquidity restrictions.
 //! - [`Imbalance`](https://crates.parity.io/srml_support/traits/trait.Imbalance.html): Functions for handling
@@ -176,7 +178,7 @@ use srml_support::{StorageValue, StorageMap, Parameter, decl_event, decl_storage
 use srml_support::traits::{
 	UpdateBalanceOutcome, Currency, OnFreeBalanceZero, MakePayment, OnUnbalanced,
 	WithdrawReason, WithdrawReasons, LockIdentifier, LockableCurrency, ExistenceRequirement,
-	Imbalance, SignedImbalance
+	Imbalance, SignedImbalance, ReservableCurrency
 };
 use srml_support::dispatch::Result;
 use primitives::traits::{
@@ -661,14 +663,6 @@ where
 		Self::free_balance(who) >= value
 	}
 
-	fn can_reserve(who: &T::AccountId, value: Self::Balance) -> bool {
-		Self::free_balance(who)
-			.checked_sub(&value)
-			.map_or(false, |new_balance|
-				Self::ensure_can_withdraw(who, value, WithdrawReason::Reserve, new_balance).is_ok()
-			)
-	}
-
 	fn total_issuance() -> Self::Balance {
 		<TotalIssuance<T, I>>::get()
 	}
@@ -681,10 +675,6 @@ where
 		<FreeBalance<T, I>>::get(who)
 	}
 
-	fn reserved_balance(who: &T::AccountId) -> Self::Balance {
-		<ReservedBalance<T, I>>::get(who)
-	}
-
 	fn ensure_can_withdraw(
 		who: &T::AccountId,
 		_amount: T::Balance,
@@ -775,6 +765,10 @@ where
 		let free_slash = cmp::min(free_balance, value);
 		Self::set_free_balance(who, free_balance - free_slash);
 		let remaining_slash = value - free_slash;
+		// NOTE: `slash()` prefers free balance, but assumes that reserve balance can be drawn
+		// from in extreme circumstances. `can_slash()` should be used prior to `slash()` is avoid having
+		// to draw from reserved funds, however we err on the side of punishment if things are inconsistent
+		// or `can_slash` wasn't used appropriately.
 		if !remaining_slash.is_zero() {
 			let reserved_balance = Self::reserved_balance(who);
 			let reserved_slash = cmp::min(reserved_balance, remaining_slash);
@@ -852,6 +846,23 @@ where
 		};
 		(imbalance, outcome)
 	}
+}
+
+impl<T: Trait<I>, I: Instance> ReservableCurrency<T::AccountId> for Module<T, I>
+where
+	T::Balance: MaybeSerializeDebug
+{
+	fn can_reserve(who: &T::AccountId, value: Self::Balance) -> bool {
+		Self::free_balance(who)
+			.checked_sub(&value)
+			.map_or(false, |new_balance|
+				Self::ensure_can_withdraw(who, value, WithdrawReason::Reserve, new_balance).is_ok()
+			)
+	}
+
+	fn reserved_balance(who: &T::AccountId) -> Self::Balance {
+		<ReservedBalance<T, I>>::get(who)
+	}
 
 	fn reserve(who: &T::AccountId, value: Self::Balance) -> result::Result<(), &'static str> {
 		let b = Self::free_balance(who);

substrate/srml/balances/src/tests.rs

@@ -23,7 +23,8 @@ use mock::{Balances, ExtBuilder, Runtime, System};
 use runtime_io::with_externalities;
 use srml_support::{
 	assert_noop, assert_ok, assert_err,
-	traits::{LockableCurrency, LockIdentifier, WithdrawReason, WithdrawReasons, Currency, MakePayment}
+	traits::{LockableCurrency, LockIdentifier, WithdrawReason, WithdrawReasons,
+	Currency, MakePayment, ReservableCurrency}
 };
 
 const ID_1: LockIdentifier = *b"1       ";
@@ -101,17 +102,17 @@ fn lock_reasons_should_work() {
 	with_externalities(&mut ExtBuilder::default().existential_deposit(1).monied(true).transaction_fees(0, 1).build(), || {
 		Balances::set_lock(ID_1, &1, 10, u64::max_value(), WithdrawReason::Transfer.into());
 		assert_noop!(<Balances as Currency<_>>::transfer(&1, &2, 1), "account liquidity restrictions prevent withdrawal");
-		assert_ok!(<Balances as Currency<_>>::reserve(&1, 1));
+		assert_ok!(<Balances as ReservableCurrency<_>>::reserve(&1, 1));
 		assert_ok!(<Balances as MakePayment<_>>::make_payment(&1, 1));
 
 		Balances::set_lock(ID_1, &1, 10, u64::max_value(), WithdrawReason::Reserve.into());
 		assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1));
-		assert_noop!(<Balances as Currency<_>>::reserve(&1, 1), "account liquidity restrictions prevent withdrawal");
+		assert_noop!(<Balances as ReservableCurrency<_>>::reserve(&1, 1), "account liquidity restrictions prevent withdrawal");
 		assert_ok!(<Balances as MakePayment<_>>::make_payment(&1, 1));
 
 		Balances::set_lock(ID_1, &1, 10, u64::max_value(), WithdrawReason::TransactionPayment.into());
 		assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1));
-		assert_ok!(<Balances as Currency<_>>::reserve(&1, 1));
+		assert_ok!(<Balances as ReservableCurrency<_>>::reserve(&1, 1));
 		assert_noop!(<Balances as MakePayment<_>>::make_payment(&1, 1), "account liquidity restrictions prevent withdrawal");
 	});
 }

substrate/srml/council/src/seats.rs

@@ -21,7 +21,7 @@ use primitives::traits::{Zero, One, As, StaticLookup};
 use runtime_io::print;
 use srml_support::{
 	StorageValue, StorageMap, dispatch::Result, decl_storage, decl_event, ensure,
-	traits::{Currency, OnUnbalanced}
+	traits::{Currency, ReservableCurrency, OnUnbalanced}
 };
 use democracy;
 use system::{self, ensure_signed};

substrate/srml/democracy/src/lib.rs

@@ -24,7 +24,7 @@ use primitives::traits::{Zero, As, Bounded};
 use parity_codec::{Encode, Decode};
 use srml_support::{StorageValue, StorageMap, Parameter, Dispatchable, IsSubType, EnumerableStorageMap};
 use srml_support::{decl_module, decl_storage, decl_event, ensure};
-use srml_support::traits::{Currency, LockableCurrency, WithdrawReason, LockIdentifier};
+use srml_support::traits::{Currency, ReservableCurrency, LockableCurrency, WithdrawReason, LockIdentifier};
 use srml_support::dispatch::Result;
 use system::ensure_signed;
 
@@ -72,7 +72,7 @@ impl Vote {
 type BalanceOf<T> = <<T as Trait>::Currency as Currency<<T as system::Trait>::AccountId>>::Balance;
 
 pub trait Trait: system::Trait + Sized {
-	type Currency: LockableCurrency<<Self as system::Trait>::AccountId, Moment=Self::BlockNumber>;
+	type Currency: ReservableCurrency<Self::AccountId> + LockableCurrency<Self::AccountId, Moment=Self::BlockNumber>;
 
 	type Proposal: Parameter + Dispatchable<Origin=Self::Origin> + IsSubType<Module<Self>>;
 

substrate/srml/staking/src/tests.rs

@@ -24,7 +24,7 @@ use phragmen;
 use primitives::Perquintill;
 use srml_support::{assert_ok, assert_noop, EnumerableStorageMap};
 use mock::{Balances, Session, Staking, System, Timestamp, Test, ExtBuilder, Origin};
-use srml_support::traits::Currency;
+use srml_support::traits::{Currency, ReservableCurrency};
 
 #[test]
 fn basic_setup_works() {

substrate/srml/support/src/traits.rs

@@ -249,10 +249,6 @@ pub trait Currency<AccountId> {
 	/// balance changes in the meantime and only the reserved balance is not taken into account.
 	fn can_slash(who: &AccountId, value: Self::Balance) -> bool;
 
-	/// Same result as `reserve(who, value)` (but without the side-effects) assuming there
-	/// are no balance changes in the meantime.
-	fn can_reserve(who: &AccountId, value: Self::Balance) -> bool;
-
 	/// The total amount of issuance in the system.
 	fn total_issuance() -> Self::Balance;
 
@@ -273,19 +269,6 @@ pub trait Currency<AccountId> {
 	/// collapsed to zero if it ever becomes less than `ExistentialDeposit`.
 	fn free_balance(who: &AccountId) -> Self::Balance;
 
-	/// The amount of the balance of a given account that is externally reserved; this can still get
-	/// slashed, but gets slashed last of all.
-	///
-	/// This balance is a 'reserve' balance that other subsystems use in order to set aside tokens
-	/// that are still 'owned' by the account holder, but which are suspendable.
-	///
-	/// When this balance falls below the value of `ExistentialDeposit`, then this 'reserve account'
-	/// is deleted: specifically, `ReservedBalance`.
-	///
-	/// `system::AccountNonce` is also deleted if `FreeBalance` is also zero (it also gets
-	/// collapsed to zero if it ever becomes less than `ExistentialDeposit`.
-	fn reserved_balance(who: &AccountId) -> Self::Balance;
-
 	/// Returns `Ok` iff the account is able to make a withdrawal of the given amount
 	/// for the given reason. Basically, it's just a dry-run of `withdraw`.
 	///
@@ -361,6 +344,36 @@ pub trait Currency<AccountId> {
 		SignedImbalance<Self::Balance, Self::PositiveImbalance>,
 		UpdateBalanceOutcome,
 	);
+}
+
+/// A currency where funds can be reserved from the user.
+pub trait ReservableCurrency<AccountId>: Currency<AccountId> {
+	/// Same result as `reserve(who, value)` (but without the side-effects) assuming there
+	/// are no balance changes in the meantime.
+	fn can_reserve(who: &AccountId, value: Self::Balance) -> bool;
+
+	/// Deducts up to `value` from reserved balance of `who`. This function cannot fail.
+	///
+	/// As much funds up to `value` will be deducted as possible. If the reserve balance of `who`
+	/// is less than `value`, then a non-zero second item will be returned.
+	fn slash_reserved(
+		who: &AccountId,
+		value: Self::Balance
+	) -> (Self::NegativeImbalance, Self::Balance);
+
+	/// The amount of the balance of a given account that is externally reserved; this can still get
+	/// slashed, but gets slashed last of all.
+	///
+	/// This balance is a 'reserve' balance that other subsystems use in order to set aside tokens
+	/// that are still 'owned' by the account holder, but which are suspendable.
+	///
+	/// When this balance falls below the value of `ExistentialDeposit`, then this 'reserve account'
+	/// is deleted: specifically, `ReservedBalance`.
+	///
+	/// `system::AccountNonce` is also deleted if `FreeBalance` is also zero (it also gets
+	/// collapsed to zero if it ever becomes less than `ExistentialDeposit`.
+	fn reserved_balance(who: &AccountId) -> Self::Balance;
+
 
 	/// Moves `value` from balance to reserved balance.
 	///
@@ -380,15 +393,6 @@ pub trait Currency<AccountId> {
 	/// invoke `on_reserved_too_low` and could reap the account.
 	fn unreserve(who: &AccountId, value: Self::Balance) -> Self::Balance;
 
-	/// Deducts up to `value` from reserved balance of `who`. This function cannot fail.
-	///
-	/// As much funds up to `value` will be deducted as possible. If the reserve balance of `who`
-	/// is less than `value`, then a non-zero second item will be returned.
-	fn slash_reserved(
-		who: &AccountId,
-		value: Self::Balance
-	) -> (Self::NegativeImbalance, Self::Balance);
-
 	/// Moves up to `value` from reserved balance of account `slashed` to free balance of account
 	/// `beneficiary`. `beneficiary` must exist for this to succeed. If it does not, `Err` will be
 	/// returned.

substrate/srml/treasury/src/lib.rs

@@ -22,7 +22,7 @@
 use serde_derive::{Serialize, Deserialize};
 use rstd::prelude::*;
 use srml_support::{StorageValue, StorageMap, decl_module, decl_storage, decl_event, ensure};
-use srml_support::traits::{Currency, OnDilution, OnUnbalanced, Imbalance};
+use srml_support::traits::{Currency, ReservableCurrency, OnDilution, OnUnbalanced, Imbalance};
 use runtime_primitives::{Permill, traits::{Zero, EnsureOrigin, StaticLookup}};
 use parity_codec::{Encode, Decode};
 use system::ensure_signed;
@@ -33,7 +33,7 @@ type NegativeImbalanceOf<T> = <<T as Trait>::Currency as Currency<<T as system::
 
 pub trait Trait: system::Trait {
 	/// The staking balance.
-	type Currency: Currency<Self::AccountId>;
+	type Currency: Currency<Self::AccountId> + ReservableCurrency<Self::AccountId>;
 
 	/// Origin from which approvals must come.
 	type ApproveOrigin: EnsureOrigin<Self::Origin>;