[FRAME] Short-circuit fungible self transfer (#2118)

Changes:
- Change the fungible(s) logic to treat a self-transfer as No-OP (as
long as all pre-checks pass).

Note that the self-transfer case will not emit an event since no state
was changed.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

bridges/primitives/relayers/src/lib.rs

@@ -115,7 +115,7 @@ where
 impl<T, Relayer> PaymentProcedure<Relayer, T::Balance> for PayRewardFromAccount<T, Relayer>
 where
 	T: frame_support::traits::fungible::Mutate<Relayer>,
-	Relayer: Decode + Encode,
+	Relayer: Decode + Encode + Eq,
 {
 	type Error = sp_runtime::DispatchError;
 

cumulus/primitives/utility/src/lib.rs

@@ -102,7 +102,7 @@ struct AssetTraderRefunder {
 /// Important: Errors if the Trader is being called twice by 2 BuyExecution instructions
 /// Alternatively we could just return payment in the aforementioned case
 pub struct TakeFirstAssetTrader<
-	AccountId,
+	AccountId: Eq,
 	FeeCharger: ChargeWeightInFungibles<AccountId, ConcreteAssets>,
 	Matcher: MatchesFungibles<ConcreteAssets::AssetId, ConcreteAssets::Balance>,
 	ConcreteAssets: fungibles::Mutate<AccountId> + fungibles::Balanced<AccountId>,
@@ -112,7 +112,7 @@ pub struct TakeFirstAssetTrader<
 	PhantomData<(AccountId, FeeCharger, Matcher, ConcreteAssets, HandleRefund)>,
 );
 impl<
-		AccountId,
+		AccountId: Eq,
 		FeeCharger: ChargeWeightInFungibles<AccountId, ConcreteAssets>,
 		Matcher: MatchesFungibles<ConcreteAssets::AssetId, ConcreteAssets::Balance>,
 		ConcreteAssets: fungibles::Mutate<AccountId> + fungibles::Balanced<AccountId>,
@@ -241,7 +241,7 @@ impl<
 }
 
 impl<
-		AccountId,
+		AccountId: Eq,
 		FeeCharger: ChargeWeightInFungibles<AccountId, ConcreteAssets>,
 		Matcher: MatchesFungibles<ConcreteAssets::AssetId, ConcreteAssets::Balance>,
 		ConcreteAssets: fungibles::Mutate<AccountId> + fungibles::Balanced<AccountId>,

polkadot/xcm/xcm-builder/src/fungibles_adapter.rs

@@ -34,7 +34,7 @@ impl<
 		Assets: fungibles::Mutate<AccountId>,
 		Matcher: MatchesFungibles<Assets::AssetId, Assets::Balance>,
 		AccountIdConverter: ConvertLocation<AccountId>,
-		AccountId: Clone, // can't get away without it since Currency is generic over it.
+		AccountId: Eq + Clone, /* can't get away without it since Currency is generic over it. */
 	> TransactAsset for FungiblesTransferAdapter<Assets, Matcher, AccountIdConverter, AccountId>
 {
 	fn internal_transfer_asset(
@@ -150,7 +150,7 @@ impl<
 		Assets: fungibles::Mutate<AccountId>,
 		Matcher: MatchesFungibles<Assets::AssetId, Assets::Balance>,
 		AccountIdConverter: ConvertLocation<AccountId>,
-		AccountId: Clone, // can't get away without it since Currency is generic over it.
+		AccountId: Eq + Clone, /* can't get away without it since Currency is generic over it. */
 		CheckAsset: AssetChecking<Assets::AssetId>,
 		CheckingAccount: Get<AccountId>,
 	>
@@ -185,7 +185,7 @@ impl<
 		Assets: fungibles::Mutate<AccountId>,
 		Matcher: MatchesFungibles<Assets::AssetId, Assets::Balance>,
 		AccountIdConverter: ConvertLocation<AccountId>,
-		AccountId: Clone, // can't get away without it since Currency is generic over it.
+		AccountId: Eq + Clone, /* can't get away without it since Currency is generic over it. */
 		CheckAsset: AssetChecking<Assets::AssetId>,
 		CheckingAccount: Get<AccountId>,
 	> TransactAsset
@@ -325,7 +325,7 @@ impl<
 		Assets: fungibles::Mutate<AccountId>,
 		Matcher: MatchesFungibles<Assets::AssetId, Assets::Balance>,
 		AccountIdConverter: ConvertLocation<AccountId>,
-		AccountId: Clone, // can't get away without it since Currency is generic over it.
+		AccountId: Eq + Clone, /* can't get away without it since Currency is generic over it. */
 		CheckAsset: AssetChecking<Assets::AssetId>,
 		CheckingAccount: Get<AccountId>,
 	> TransactAsset

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

@@ -18,14 +18,18 @@
 //! Tests regarding the functionality of the `Currency` trait set implementations.
 
 use super::*;
-use crate::NegativeImbalance;
-use frame_support::traits::{
-	BalanceStatus::{Free, Reserved},
-	Currency,
-	ExistenceRequirement::{self, AllowDeath, KeepAlive},
-	Hooks, LockIdentifier, LockableCurrency, NamedReservableCurrency, ReservableCurrency,
-	WithdrawReasons,
+use crate::{Event, NegativeImbalance};
+use frame_support::{
+	traits::{
+		BalanceStatus::{Free, Reserved},
+		Currency,
+		ExistenceRequirement::{self, AllowDeath, KeepAlive},
+		Hooks, LockIdentifier, LockableCurrency, NamedReservableCurrency, ReservableCurrency,
+		WithdrawReasons,
+	},
+	StorageNoopGuard,
 };
+use frame_system::Event as SysEvent;
 
 const ID_1: LockIdentifier = *b"1       ";
 const ID_2: LockIdentifier = *b"2       ";
@@ -1363,3 +1367,39 @@ fn freezing_and_locking_should_work() {
 			assert_eq!(System::consumers(&1), 0);
 		});
 }
+
+#[test]
+fn self_transfer_noop() {
+	ExtBuilder::default().existential_deposit(100).build_and_execute_with(|| {
+		assert_eq!(Balances::total_issuance(), 0);
+		let _ = Balances::deposit_creating(&1, 100);
+
+		// The account is set up properly:
+		assert_eq!(
+			events(),
+			[
+				Event::Deposit { who: 1, amount: 100 }.into(),
+				SysEvent::NewAccount { account: 1 }.into(),
+				Event::Endowed { account: 1, free_balance: 100 }.into(),
+			]
+		);
+		assert_eq!(Balances::free_balance(1), 100);
+		assert_eq!(Balances::total_issuance(), 100);
+
+		// Transfers to self are No-OPs:
+		let _g = StorageNoopGuard::new();
+		for i in 0..200 {
+			let r = Balances::transfer_allow_death(Some(1).into(), 1, i);
+
+			if i <= 100 {
+				assert_ok!(r);
+			} else {
+				assert!(r.is_err());
+			}
+
+			assert!(events().is_empty());
+			assert_eq!(Balances::free_balance(1), 100, "Balance unchanged by self transfer");
+			assert_eq!(Balances::total_issuance(), 100, "TI unchanged by self transfers");
+		}
+	});
+}

substrate/frame/broker/src/test_fungibles.rs

@@ -168,7 +168,7 @@ where
 
 impl<
 		Instance: Get<u32>,
-		AccountId: Encode,
+		AccountId: Encode + Eq,
 		AssetId: tokens::AssetId + Copy,
 		MinimumBalance: TypedGet,
 		HoldReason,

substrate/frame/nis/src/lib.rs

@@ -148,7 +148,7 @@ impl<T> fungible::Unbalanced<T> for NoCounterpart<T> {
 	}
 	fn set_total_issuance(_: Self::Balance) {}
 }
-impl<T> FunMutate<T> for NoCounterpart<T> {}
+impl<T: Eq> FunMutate<T> for NoCounterpart<T> {}
 impl<T> Convert<Perquintill, u32> for NoCounterpart<T> {
 	fn convert(_: Perquintill) -> u32 {
 		0

substrate/frame/support/src/traits/tokens/fungible/item_of.rs

@@ -219,7 +219,7 @@ impl<
 impl<
 		F: fungibles::Mutate<AccountId>,
 		A: Get<<F as fungibles::Inspect<AccountId>>::AssetId>,
-		AccountId,
+		AccountId: Eq,
 	> Mutate<AccountId> for ItemOf<F, A, AccountId>
 {
 	fn mint_into(who: &AccountId, amount: Self::Balance) -> Result<Self::Balance, DispatchError> {

substrate/frame/support/src/traits/tokens/fungible/regular.rs

@@ -235,7 +235,10 @@ pub trait Unbalanced<AccountId>: Inspect<AccountId> {
 }
 
 /// Trait for providing a basic fungible asset.
-pub trait Mutate<AccountId>: Inspect<AccountId> + Unbalanced<AccountId> {
+pub trait Mutate<AccountId>: Inspect<AccountId> + Unbalanced<AccountId>
+where
+	AccountId: Eq,
+{
 	/// Increase the balance of `who` by exactly `amount`, minting new tokens. If that isn't
 	/// possible then an `Err` is returned and nothing is changed.
 	fn mint_into(who: &AccountId, amount: Self::Balance) -> Result<Self::Balance, DispatchError> {
@@ -303,6 +306,9 @@ pub trait Mutate<AccountId>: Inspect<AccountId> + Unbalanced<AccountId> {
 	}
 
 	/// Transfer funds from one account into another.
+	///
+	/// A transfer where the source and destination account are identical is treated as No-OP after
+	/// checking the preconditions.
 	fn transfer(
 		source: &AccountId,
 		dest: &AccountId,
@@ -311,6 +317,10 @@ pub trait Mutate<AccountId>: Inspect<AccountId> + Unbalanced<AccountId> {
 	) -> Result<Self::Balance, DispatchError> {
 		let _extra = Self::can_withdraw(source, amount).into_result(preservation != Expendable)?;
 		Self::can_deposit(dest, amount, Extant).into_result()?;
+		if source == dest {
+			return Ok(amount)
+		}
+
 		Self::decrease_balance(source, amount, BestEffort, preservation, Polite)?;
 		// This should never fail as we checked `can_deposit` earlier. But we do a best-effort
 		// anyway.

substrate/frame/support/src/traits/tokens/fungibles/regular.rs

@@ -250,7 +250,10 @@ pub trait Unbalanced<AccountId>: Inspect<AccountId> {
 }
 
 /// Trait for providing a basic fungible asset.
-pub trait Mutate<AccountId>: Inspect<AccountId> + Unbalanced<AccountId> {
+pub trait Mutate<AccountId>: Inspect<AccountId> + Unbalanced<AccountId>
+where
+	AccountId: Eq,
+{
 	/// Increase the balance of `who` by exactly `amount`, minting new tokens. If that isn't
 	/// possible then an `Err` is returned and nothing is changed.
 	fn mint_into(
@@ -353,6 +356,9 @@ pub trait Mutate<AccountId>: Inspect<AccountId> + Unbalanced<AccountId> {
 	}
 
 	/// Transfer funds from one account into another.
+	///
+	/// A transfer where the source and destination account are identical is treated as No-OP after
+	/// checking the preconditions.
 	fn transfer(
 		asset: Self::AssetId,
 		source: &AccountId,
@@ -363,6 +369,10 @@ pub trait Mutate<AccountId>: Inspect<AccountId> + Unbalanced<AccountId> {
 		let _extra = Self::can_withdraw(asset.clone(), source, amount)
 			.into_result(preservation != Expendable)?;
 		Self::can_deposit(asset.clone(), dest, amount, Extant).into_result()?;
+		if source == dest {
+			return Ok(amount)
+		}
+
 		Self::decrease_balance(asset.clone(), source, amount, BestEffort, preservation, Polite)?;
 		// This should never fail as we checked `can_deposit` earlier. But we do a best-effort
 		// anyway.

substrate/frame/support/src/traits/tokens/pay.rs

@@ -82,8 +82,13 @@ pub enum PaymentStatus {
 }
 
 /// Simple implementation of `Pay` which makes a payment from a "pot" - i.e. a single account.
-pub struct PayFromAccount<F, A>(sp_std::marker::PhantomData<(F, A)>);
-impl<A: TypedGet, F: fungible::Mutate<A::Type>> Pay for PayFromAccount<F, A> {
+pub struct PayFromAccount<F, A>(core::marker::PhantomData<(F, A)>);
+impl<A, F> Pay for PayFromAccount<F, A>
+where
+	A: TypedGet,
+	F: fungible::Mutate<A::Type>,
+	A::Type: Eq,
+{
 	type Balance = F::Balance;
 	type Beneficiary = A::Type;
 	type AssetKind = ();
@@ -110,11 +115,12 @@ impl<A: TypedGet, F: fungible::Mutate<A::Type>> Pay for PayFromAccount<F, A> {
 
 /// Simple implementation of `Pay` for assets which makes a payment from a "pot" - i.e. a single
 /// account.
-pub struct PayAssetFromAccount<F, A>(sp_std::marker::PhantomData<(F, A)>);
+pub struct PayAssetFromAccount<F, A>(core::marker::PhantomData<(F, A)>);
 impl<A, F> frame_support::traits::tokens::Pay for PayAssetFromAccount<F, A>
 where
 	A: TypedGet,
 	F: fungibles::Mutate<A::Type> + fungibles::Create<A::Type>,
+	A::Type: Eq,
 {
 	type Balance = F::Balance;
 	type Beneficiary = A::Type;