fix issues per review (#293)

* fix issues per review

* update

Author: xlc

authority/src/lib.rs

@@ -1,6 +1,6 @@
 //! # Authority
 //! A module to provide features for governance including dispatch method on
-//! behalf other accounts and schdule dispatchables.
+//! behalf of other accounts and schedule dispatchables.
 //!
 //! - [`Trait`](./trait.Trait.html)
 //! - [`Call`](./enum.Call.html)
@@ -9,7 +9,7 @@
 //! ## Overview
 //!
 //! Two functionalities are provided by this module:
-//! - schdule a dispatchable
+//! - schedule a dispatchable
 //! - dispatch method with on behalf of other origins
 
 #![cfg_attr(not(feature = "std"), no_std)]
@@ -93,7 +93,7 @@ pub type Origin<T> = DelayedOrigin<<T as frame_system::Trait>::BlockNumber, <T a
 
 /// Config for orml-authority
 pub trait AuthorityConfig<Origin, PalletsOrigin, BlockNumber> {
-	/// Check if the `origin` is allow to schedule a dispatchable call with a
+	/// Check if the `origin` is allowed to schedule a dispatchable call with a
 	/// given `priority`.
 	fn check_schedule_dispatch(origin: Origin, priority: Priority) -> DispatchResult;
 	/// Check if the `origin` is allow to fast track a scheduled task that
@@ -214,7 +214,7 @@ decl_module! {
 			Self::deposit_event(RawEvent::Dispatched(e.map(|_| ()).map_err(|e| e.error)));
 		}
 
-		/// Schdule a dispatchable to be dispatched at later block.
+		/// Schedule a dispatchable to be dispatched at later block.
 		/// This is the only way to dispatch a call with `DelayedOrigin`.
 		#[weight = T::WeightInfo::schedule_dispatch()]
 		pub fn schedule_dispatch(
@@ -265,6 +265,7 @@ decl_module! {
 		}
 
 		/// Fast track a scheduled dispatchable.
+		/// WARN: This is not fully implemented.
 		#[weight = T::WeightInfo::fast_track_scheduled_dispatch()]
 		pub fn fast_track_scheduled_dispatch(
 			origin,
@@ -294,6 +295,7 @@ decl_module! {
 		}
 
 		/// Delay a scheduled dispatchable.
+		/// WARN: This is not fully implemented.
 		#[weight = T::WeightInfo::delay_scheduled_dispatch()]
 		pub fn delay_scheduled_dispatch(
 			origin,

currencies/src/lib.rs

@@ -38,7 +38,7 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use codec::{Codec, FullCodec};
+use codec::Codec;
 use frame_support::{
 	decl_error, decl_event, decl_module, decl_storage,
 	traits::{
@@ -49,7 +49,7 @@ use frame_support::{
 };
 use frame_system::{ensure_root, ensure_signed};
 use sp_runtime::{
-	traits::{AtLeast32BitUnsigned, MaybeSerializeDeserialize, StaticLookup, Zero},
+	traits::{CheckedSub, MaybeSerializeDeserialize, StaticLookup, Zero},
 	DispatchError, DispatchResult,
 };
 use sp_std::{
@@ -542,102 +542,85 @@ where
 pub type NativeCurrencyOf<T> = Currency<T, <T as Trait>::GetNativeCurrencyId>;
 
 /// Adapt other currency traits implementation to `BasicCurrency`.
-pub struct BasicCurrencyAdapter<Currency, Balance, BalanceConvert, Amount, Moment>(
-	marker::PhantomData<(Currency, Balance, BalanceConvert, Amount, Moment)>,
-);
+pub struct BasicCurrencyAdapter<T, Currency, Amount, Moment>(marker::PhantomData<(T, Currency, Amount, Moment)>);
 
 type PalletBalanceOf<A, Currency> = <Currency as PalletCurrency<A>>::Balance;
 
 // Adapt `frame_support::traits::Currency`
-impl<AccountId, Currency, Balance, BalanceConvert, Amount, Moment> BasicCurrency<AccountId>
-	for BasicCurrencyAdapter<Currency, Balance, BalanceConvert, Amount, Moment>
+impl<T, AccountId, Currency, Amount, Moment> BasicCurrency<AccountId>
+	for BasicCurrencyAdapter<T, Currency, Amount, Moment>
 where
 	Currency: PalletCurrency<AccountId>,
-	Balance: AtLeast32BitUnsigned + FullCodec + Copy + MaybeSerializeDeserialize + Debug + Default,
-	BalanceConvert: From<PalletBalanceOf<AccountId, Currency>>
-		+ Into<PalletBalanceOf<AccountId, Currency>>
-		+ From<Balance>
-		+ Into<Balance>,
+	T: Trait,
 {
-	type Balance = Balance;
+	type Balance = PalletBalanceOf<AccountId, Currency>;
 
 	fn total_issuance() -> Self::Balance {
-		BalanceConvert::from(Currency::total_issuance()).into()
+		Currency::total_issuance()
 	}
 
 	fn total_balance(who: &AccountId) -> Self::Balance {
-		BalanceConvert::from(Currency::total_balance(who)).into()
+		Currency::total_balance(who)
 	}
 
 	fn free_balance(who: &AccountId) -> Self::Balance {
-		BalanceConvert::from(Currency::free_balance(who)).into()
+		Currency::free_balance(who)
 	}
 
 	fn ensure_can_withdraw(who: &AccountId, amount: Self::Balance) -> DispatchResult {
-		let new_balance_pallet = {
-			let new_balance = Self::free_balance(who)
-				.checked_sub(&amount)
-				.ok_or("InsufficientBalance")?;
-			BalanceConvert::from(new_balance).into()
-		};
-		let amount_pallet = BalanceConvert::from(amount).into();
-		Currency::ensure_can_withdraw(who, amount_pallet, WithdrawReasons::all(), new_balance_pallet)
+		let new_balance = Self::free_balance(who)
+			.checked_sub(&amount)
+			.ok_or(Error::<T>::BalanceTooLow)?;
+
+		Currency::ensure_can_withdraw(who, amount, WithdrawReasons::all(), new_balance)
 	}
 
 	fn transfer(from: &AccountId, to: &AccountId, amount: Self::Balance) -> DispatchResult {
-		let amount_pallet = BalanceConvert::from(amount).into();
-		Currency::transfer(from, to, amount_pallet, ExistenceRequirement::AllowDeath)
+		Currency::transfer(from, to, amount, ExistenceRequirement::AllowDeath)
 	}
 
 	fn deposit(who: &AccountId, amount: Self::Balance) -> DispatchResult {
-		let _ = Currency::deposit_creating(who, BalanceConvert::from(amount).into());
+		let _ = Currency::deposit_creating(who, amount);
 		Ok(())
 	}
 
 	fn withdraw(who: &AccountId, amount: Self::Balance) -> DispatchResult {
-		Currency::withdraw(
-			who,
-			BalanceConvert::from(amount).into(),
-			WithdrawReasons::all(),
-			ExistenceRequirement::AllowDeath,
-		)
-		.map(|_| ())
+		Currency::withdraw(who, amount, WithdrawReasons::all(), ExistenceRequirement::AllowDeath).map(|_| ())
 	}
 
 	fn can_slash(who: &AccountId, amount: Self::Balance) -> bool {
-		Currency::can_slash(who, BalanceConvert::from(amount).into())
+		Currency::can_slash(who, amount)
 	}
 
 	fn slash(who: &AccountId, amount: Self::Balance) -> Self::Balance {
-		let (_, gap) = Currency::slash(who, BalanceConvert::from(amount).into());
-		BalanceConvert::from(gap).into()
+		let (_, gap) = Currency::slash(who, amount);
+		gap
 	}
 }
 
 // Adapt `frame_support::traits::Currency`
-impl<AccountId, Currency, Balance, BalanceConvert, Amount, Moment> BasicCurrencyExtended<AccountId>
-	for BasicCurrencyAdapter<Currency, Balance, BalanceConvert, Amount, Moment>
+impl<T, AccountId, Currency, Amount, Moment> BasicCurrencyExtended<AccountId>
+	for BasicCurrencyAdapter<T, Currency, Amount, Moment>
 where
-	Balance: AtLeast32BitUnsigned + FullCodec + Copy + MaybeSerializeDeserialize + Debug + Default,
 	Amount: Signed
-		+ TryInto<Balance>
-		+ TryFrom<Balance>
+		+ TryInto<PalletBalanceOf<AccountId, Currency>>
+		+ TryFrom<PalletBalanceOf<AccountId, Currency>>
 		+ SimpleArithmetic
 		+ Codec
 		+ Copy
 		+ MaybeSerializeDeserialize
 		+ Debug
 		+ Default,
 	Currency: PalletCurrency<AccountId>,
-	BalanceConvert: From<PalletBalanceOf<AccountId, Currency>>
-		+ Into<PalletBalanceOf<AccountId, Currency>>
-		+ From<Balance>
-		+ Into<Balance>,
+	T: Trait,
 {
 	type Amount = Amount;
 
 	fn update_balance(who: &AccountId, by_amount: Self::Amount) -> DispatchResult {
-		let by_balance = by_amount.abs().try_into().map_err(|_| "AmountIntoBalanceFailed")?;
+		let by_balance = by_amount
+			.abs()
+			.try_into()
+			.map_err(|_| Error::<T>::AmountIntoBalanceFailed)?;
 		if by_amount.is_positive() {
 			Self::deposit(who, by_balance)
 		} else {
@@ -647,34 +630,20 @@ where
 }
 
 // Adapt `frame_support::traits::LockableCurrency`
-impl<AccountId, Currency, Balance, BalanceConvert, Amount, Moment> BasicLockableCurrency<AccountId>
-	for BasicCurrencyAdapter<Currency, Balance, BalanceConvert, Amount, Moment>
+impl<T, AccountId, Currency, Amount, Moment> BasicLockableCurrency<AccountId>
+	for BasicCurrencyAdapter<T, Currency, Amount, Moment>
 where
-	Balance: AtLeast32BitUnsigned + FullCodec + Copy + MaybeSerializeDeserialize + Debug + Default,
 	Currency: PalletLockableCurrency<AccountId>,
-	BalanceConvert: From<PalletBalanceOf<AccountId, Currency>>
-		+ Into<PalletBalanceOf<AccountId, Currency>>
-		+ From<Balance>
-		+ Into<Balance>,
+	T: Trait,
 {
 	type Moment = Moment;
 
 	fn set_lock(lock_id: LockIdentifier, who: &AccountId, amount: Self::Balance) {
-		Currency::set_lock(
-			lock_id,
-			who,
-			BalanceConvert::from(amount).into(),
-			WithdrawReasons::all(),
-		);
+		Currency::set_lock(lock_id, who, amount, WithdrawReasons::all());
 	}
 
 	fn extend_lock(lock_id: LockIdentifier, who: &AccountId, amount: Self::Balance) {
-		Currency::extend_lock(
-			lock_id,
-			who,
-			BalanceConvert::from(amount).into(),
-			WithdrawReasons::all(),
-		);
+		Currency::extend_lock(lock_id, who, amount, WithdrawReasons::all());
 	}
 
 	fn remove_lock(lock_id: LockIdentifier, who: &AccountId) {
@@ -683,35 +652,31 @@ where
 }
 
 // Adapt `frame_support::traits::ReservableCurrency`
-impl<AccountId, Currency, Balance, BalanceConvert, Amount, Moment> BasicReservableCurrency<AccountId>
-	for BasicCurrencyAdapter<Currency, Balance, BalanceConvert, Amount, Moment>
+impl<T, AccountId, Currency, Amount, Moment> BasicReservableCurrency<AccountId>
+	for BasicCurrencyAdapter<T, Currency, Amount, Moment>
 where
-	Balance: AtLeast32BitUnsigned + FullCodec + Copy + MaybeSerializeDeserialize + Debug + Default,
 	Currency: PalletReservableCurrency<AccountId>,
-	BalanceConvert: From<PalletBalanceOf<AccountId, Currency>>
-		+ Into<PalletBalanceOf<AccountId, Currency>>
-		+ From<Balance>
-		+ Into<Balance>,
+	T: Trait,
 {
 	fn can_reserve(who: &AccountId, value: Self::Balance) -> bool {
-		Currency::can_reserve(who, BalanceConvert::from(value).into())
+		Currency::can_reserve(who, value)
 	}
 
 	fn slash_reserved(who: &AccountId, value: Self::Balance) -> Self::Balance {
-		let (_, gap) = Currency::slash_reserved(who, BalanceConvert::from(value).into());
-		BalanceConvert::from(gap).into()
+		let (_, gap) = Currency::slash_reserved(who, value);
+		gap
 	}
 
 	fn reserved_balance(who: &AccountId) -> Self::Balance {
-		BalanceConvert::from(Currency::reserved_balance(who)).into()
+		Currency::reserved_balance(who)
 	}
 
 	fn reserve(who: &AccountId, value: Self::Balance) -> DispatchResult {
-		Currency::reserve(who, BalanceConvert::from(value).into())
+		Currency::reserve(who, value)
 	}
 
 	fn unreserve(who: &AccountId, value: Self::Balance) -> Self::Balance {
-		BalanceConvert::from(Currency::unreserve(who, BalanceConvert::from(value).into())).into()
+		Currency::unreserve(who, value)
 	}
 
 	fn repatriate_reserved(
@@ -720,7 +685,6 @@ where
 		value: Self::Balance,
 		status: BalanceStatus,
 	) -> result::Result<Self::Balance, DispatchError> {
-		Currency::repatriate_reserved(slashed, beneficiary, BalanceConvert::from(value).into(), status)
-			.map(|a| BalanceConvert::from(a).into())
+		Currency::repatriate_reserved(slashed, beneficiary, value, status)
 	}
 }

currencies/src/mock.rs

@@ -113,7 +113,7 @@ impl Trait for Runtime {
 }
 pub type Currencies = Module<Runtime>;
 pub type NativeCurrency = NativeCurrencyOf<Runtime>;
-pub type AdaptedBasicCurrency = BasicCurrencyAdapter<PalletBalances, Balance, Balance, i64, u64>;
+pub type AdaptedBasicCurrency = BasicCurrencyAdapter<Runtime, PalletBalances, i64, u64>;
 
 pub const ALICE: AccountId = 1;
 pub const BOB: AccountId = 2;

tokens/src/lib.rs

@@ -380,6 +380,7 @@ impl<T: Trait> MultiCurrency<T::AccountId> for Module<T> {
 		let to_balance = Self::free_balance(currency_id, to)
 			.checked_add(&amount)
 			.ok_or(Error::<T>::BalanceOverflow)?;
+		// Cannot underflow because ensure_can_withdraw check
 		Self::set_free_balance(currency_id, from, from_balance - amount);
 		Self::set_free_balance(currency_id, to, to_balance);
 		T::OnReceived::on_received(to, currency_id, amount);
@@ -411,6 +412,7 @@ impl<T: Trait> MultiCurrency<T::AccountId> for Module<T> {
 		}
 		Self::ensure_can_withdraw(currency_id, who, amount)?;
 
+		// Cannot underflow because ensure_can_withdraw check
 		<TotalIssuance<T>>::mutate(currency_id, |v| *v -= amount);
 		Self::set_free_balance(currency_id, who, Self::free_balance(currency_id, who) - amount);
 
@@ -439,20 +441,26 @@ impl<T: Trait> MultiCurrency<T::AccountId> for Module<T> {
 
 		let account = Self::accounts(who, currency_id);
 		let free_slashed_amount = account.free.min(amount);
+		// Cannot underflow becuase free_slashed_amount can never be greater than amount
 		let mut remaining_slash = amount - free_slashed_amount;
 
 		// slash free balance
 		if !free_slashed_amount.is_zero() {
+			// Cannot underflow becuase free_slashed_amount can never be greater than
+			// account.free
 			Self::set_free_balance(currency_id, who, account.free - free_slashed_amount);
 		}
 
 		// slash reserved balance
 		if !remaining_slash.is_zero() {
 			let reserved_slashed_amount = account.reserved.min(remaining_slash);
+			// Cannot underflow due to above line
 			remaining_slash -= reserved_slashed_amount;
 			Self::set_reserved_balance(currency_id, who, account.reserved - reserved_slashed_amount);
 		}
 
+		// Cannot underflow because the slashed value cannot be greater than total
+		// issuance
 		<TotalIssuance<T>>::mutate(currency_id, |v| *v -= amount - remaining_slash);
 		remaining_slash
 	}
@@ -577,6 +585,8 @@ impl<T: Trait> MultiReservableCurrency<T::AccountId> for Module<T> {
 
 		let account = Self::accounts(who, currency_id);
 		Self::set_free_balance(currency_id, who, account.free - value);
+		// Cannot overflow becuase total issuance is using the same balance type and
+		// this doesn't increase total issuance
 		Self::set_reserved_balance(currency_id, who, account.reserved + value);
 		Ok(())
 	}

utilities/src/lib.rs

@@ -33,7 +33,7 @@ pub fn with_transaction_result<R>(f: impl FnOnce() -> Result<R, DispatchError>)
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use frame_support::{assert_err, assert_ok, decl_module, decl_storage};
+	use frame_support::{assert_noop, assert_ok, decl_module, decl_storage};
 	use sp_io::TestExternalities;
 	use sp_runtime::{DispatchError, DispatchResult};
 
@@ -75,7 +75,7 @@ mod tests {
 			assert_eq!(Value::get(), 0);
 			assert_eq!(Map::get("val0"), 0);
 
-			assert_err!(
+			assert_noop!(
 				with_transaction_result(|| -> DispatchResult {
 					Value::set(99);
 					Map::insert("val0", 99);