Implement transfer_asset on CurrencyAdapter (#4549)

* Implement transfer_asset on CurrencyAdapter

* Use Currency::transfer in transfer_asset

* Add a test to assert that Currency::transfer was indeed used

* Remove superfluous balance conversion to u128

* Rename transfer_asset and beam_asset

Co-authored-by: Giles Cope <gilescope@gmail.com>

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

@@ -17,8 +17,8 @@
 //! Adapters to work with `frame_support::traits::Currency` through XCM.
 
 use frame_support::traits::{ExistenceRequirement::AllowDeath, Get, WithdrawReasons};
-use sp_runtime::traits::{CheckedSub, SaturatedConversion};
+use sp_runtime::traits::CheckedSub;
-use sp_std::{convert::TryInto, marker::PhantomData, result};
+use sp_std::{marker::PhantomData, result};
 use xcm::latest::{Error as XcmError, MultiAsset, MultiLocation, Result};
 use xcm_executor::{
 	traits::{Convert, MatchesFungible, TransactAsset},
@@ -31,8 +31,6 @@ enum Error {
 	AssetNotFound,
 	/// `MultiLocation` to `AccountId` conversion failed.
 	AccountIdConversionFailed,
-	/// `u128` amount to currency `Balance` conversion failed.
-	AmountToBalanceConversionFailed,
 }
 
 impl From<Error> for XcmError {
@@ -41,8 +39,6 @@ impl From<Error> for XcmError {
 		match e {
 			Error::AssetNotFound => XcmError::AssetNotFound,
 			Error::AccountIdConversionFailed => FailedToTransactAsset("AccountIdConversionFailed"),
-			Error::AmountToBalanceConversionFailed =>
-				FailedToTransactAsset("AmountToBalanceConversionFailed"),
 		}
 	}
 }
@@ -149,27 +145,37 @@ impl<
 	fn deposit_asset(what: &MultiAsset, who: &MultiLocation) -> Result {
 		log::trace!(target: "xcm::currency_adapter", "deposit_asset what: {:?}, who: {:?}", what, who);
 		// Check we handle this asset.
-		let amount: u128 =
+		let amount = Matcher::matches_fungible(&what).ok_or(Error::AssetNotFound)?;
-			Matcher::matches_fungible(&what).ok_or(Error::AssetNotFound)?.saturated_into();
 		let who =
 			AccountIdConverter::convert_ref(who).map_err(|()| Error::AccountIdConversionFailed)?;
-		let balance_amount =
+		let _imbalance = Currency::deposit_creating(&who, amount);
-			amount.try_into().map_err(|_| Error::AmountToBalanceConversionFailed)?;
-		let _imbalance = Currency::deposit_creating(&who, balance_amount);
 		Ok(())
 	}
 
 	fn withdraw_asset(what: &MultiAsset, who: &MultiLocation) -> result::Result<Assets, XcmError> {
 		log::trace!(target: "xcm::currency_adapter", "withdraw_asset what: {:?}, who: {:?}", what, who);
 		// Check we handle this asset.
-		let amount: u128 =
+		let amount = Matcher::matches_fungible(what).ok_or(Error::AssetNotFound)?;
-			Matcher::matches_fungible(what).ok_or(Error::AssetNotFound)?.saturated_into();
 		let who =
 			AccountIdConverter::convert_ref(who).map_err(|()| Error::AccountIdConversionFailed)?;
-		let balance_amount =
+		Currency::withdraw(&who, amount, WithdrawReasons::TRANSFER, AllowDeath)
-			amount.try_into().map_err(|_| Error::AmountToBalanceConversionFailed)?;
-		Currency::withdraw(&who, balance_amount, WithdrawReasons::TRANSFER, AllowDeath)
 			.map_err(|e| XcmError::FailedToTransactAsset(e.into()))?;
 		Ok(what.clone().into())
 	}
+
+	fn internal_transfer_asset(
+		asset: &MultiAsset,
+		from: &MultiLocation,
+		to: &MultiLocation,
+	) -> result::Result<Assets, XcmError> {
+		log::trace!(target: "xcm::currency_adapter", "internal_transfer_asset asset: {:?}, from: {:?}, to: {:?}", asset, from, to);
+		let amount = Matcher::matches_fungible(asset).ok_or(Error::AssetNotFound)?;
+		let from =
+			AccountIdConverter::convert_ref(from).map_err(|()| Error::AccountIdConversionFailed)?;
+		let to =
+			AccountIdConverter::convert_ref(to).map_err(|()| Error::AccountIdConversionFailed)?;
+		Currency::transfer(&from, &to, amount, AllowDeath)
+			.map_err(|e| XcmError::FailedToTransactAsset(e.into()))?;
+		Ok(asset.clone().into())
+	}
 }

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

@@ -118,14 +118,14 @@ impl<
 		AccountId: Clone, // can't get away without it since Currency is generic over it.
 	> TransactAsset for FungiblesTransferAdapter<Assets, Matcher, AccountIdConverter, AccountId>
 {
-	fn transfer_asset(
+	fn internal_transfer_asset(
 		what: &MultiAsset,
 		from: &MultiLocation,
 		to: &MultiLocation,
 	) -> result::Result<xcm_executor::Assets, XcmError> {
 		log::trace!(
 			target: "xcm::fungibles_adapter",
-			"transfer_asset what: {:?}, from: {:?}, to: {:?}",
+			"internal_transfer_asset what: {:?}, from: {:?}, to: {:?}",
 			what, from, to
 		);
 		// Check we handle this asset.
@@ -325,12 +325,12 @@ impl<
 		>::withdraw_asset(what, who)
 	}
 
-	fn transfer_asset(
+	fn internal_transfer_asset(
 		what: &MultiAsset,
 		from: &MultiLocation,
 		to: &MultiLocation,
 	) -> result::Result<xcm_executor::Assets, XcmError> {
-		FungiblesTransferAdapter::<Assets, Matcher, AccountIdConverter, AccountId>::transfer_asset(
+		FungiblesTransferAdapter::<Assets, Matcher, AccountIdConverter, AccountId>::internal_transfer_asset(
 			what, from, to,
 		)
 	}

polkadot/xcm/xcm-builder/tests/mock/mod.rs

@@ -123,7 +123,7 @@ parameter_types! {
 pub type SovereignAccountOf =
 	(ChildParachainConvertsVia<ParaId, AccountId>, AccountId32Aliases<KusamaNetwork, AccountId>);
 
-pub type LocalAssetTransactor = XcmCurrencyAdapter<
+pub type LocalCurrencyAdapter = XcmCurrencyAdapter<
 	Balances,
 	IsConcrete<KsmLocation>,
 	SovereignAccountOf,
@@ -131,6 +131,8 @@ pub type LocalAssetTransactor = XcmCurrencyAdapter<
 	CheckAccount,
 >;
 
+pub type LocalAssetTransactor = (LocalCurrencyAdapter,);
+
 type LocalOriginConverter = (
 	SovereignSignedViaLocation<SovereignAccountOf, Origin>,
 	ChildParachainAsNative<origin::Origin, Origin>,

polkadot/xcm/xcm-builder/tests/scenarios.rs

@@ -17,7 +17,8 @@
 mod mock;
 
 use mock::{
-	kusama_like_with_balances, AccountId, Balance, Balances, BaseXcmWeight, XcmConfig, CENTS,
+	kusama_like_with_balances, AccountId, Balance, Balances, BaseXcmWeight, System, XcmConfig,
+	CENTS,
 };
 use polkadot_parachain::primitives::Id as ParaId;
 use sp_runtime::traits::AccountIdConversion;
@@ -66,6 +67,36 @@ fn withdraw_and_deposit_works() {
 	});
 }
 
+/// Scenario:
+/// Alice simply wants to transfer funds to Bob's account via XCM.
+///
+/// Asserts that the balances are updated correctly and the correct events are fired.
+#[test]
+fn transfer_asset_works() {
+	let bob = AccountId::new([1u8; 32]);
+	let balances = vec![(ALICE, INITIAL_BALANCE), (bob.clone(), INITIAL_BALANCE)];
+	kusama_like_with_balances(balances).execute_with(|| {
+		let amount = REGISTER_AMOUNT;
+		let weight = BaseXcmWeight::get();
+		// Use `execute_xcm_in_credit` here to pass through the barrier
+		let r = XcmExecutor::<XcmConfig>::execute_xcm_in_credit(
+			AccountId32 { network: NetworkId::Any, id: ALICE.into() },
+			Xcm(vec![TransferAsset {
+				assets: (Here, amount).into(),
+				beneficiary: AccountId32 { network: NetworkId::Any, id: bob.clone().into() }.into(),
+			}]),
+			weight,
+			weight,
+		);
+		System::assert_last_event(
+			pallet_balances::Event::Transfer { from: ALICE, to: bob.clone(), amount }.into(),
+		);
+		assert_eq!(r, Outcome::Complete(weight));
+		assert_eq!(Balances::free_balance(ALICE), INITIAL_BALANCE - amount);
+		assert_eq!(Balances::free_balance(bob), INITIAL_BALANCE + amount);
+	});
+}
+
 /// Scenario:
 /// A parachain wants to be notified that a transfer worked correctly.
 /// It includes a `QueryHolding` order after the deposit to get notified on success.

polkadot/xcm/xcm-executor/src/lib.rs

@@ -297,7 +297,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				// Take `assets` from the origin account (on-chain) and place into dest account.
 				let origin = self.origin.as_ref().ok_or(XcmError::BadOrigin)?;
 				for asset in assets.inner() {
-					Config::AssetTransactor::beam_asset(&asset, origin, &beneficiary)?;
+					Config::AssetTransactor::transfer_asset(&asset, origin, &beneficiary)?;
 				}
 				Ok(())
 			},
@@ -305,7 +305,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				let origin = self.origin.as_ref().ok_or(XcmError::BadOrigin)?;
 				// Take `assets` from the origin account (on-chain) and place into dest account.
 				for asset in assets.inner() {
-					Config::AssetTransactor::beam_asset(asset, origin, &dest)?;
+					Config::AssetTransactor::transfer_asset(asset, origin, &dest)?;
 				}
 				let ancestry = Config::LocationInverter::ancestry();
 				assets.reanchor(&dest, &ancestry).map_err(|()| XcmError::MultiLocationFull)?;

polkadot/xcm/xcm-executor/src/traits/transact_asset.rs

@@ -78,7 +78,13 @@ pub trait TransactAsset {
 	/// Move an `asset` `from` one location in `to` another location.
 	///
 	/// Returns `XcmError::FailedToTransactAsset` if transfer failed.
-	fn transfer_asset(
+	///
+	/// ## Notes
+	/// This function is meant to only be implemented by the type implementing `TransactAsset`, and
+	/// not be called directly. Most common API usages will instead call `transfer_asset`, which in
+	/// turn has a default implementation that calls `internal_transfer_asset`. As such, **please
+	/// do not call this method directly unless you know what you're doing**.
+	fn internal_transfer_asset(
 		_asset: &MultiAsset,
 		_from: &MultiLocation,
 		_to: &MultiLocation,
@@ -88,13 +94,14 @@ pub trait TransactAsset {
 
 	/// Move an `asset` `from` one location in `to` another location.
 	///
-	/// Attempts to use `transfer_asset` and if not available then falls back to using a two-part withdraw/deposit.
+	/// Attempts to use `internal_transfer_asset` and if not available then falls back to using a
-	fn beam_asset(
+	/// two-part withdraw/deposit.
+	fn transfer_asset(
 		asset: &MultiAsset,
 		from: &MultiLocation,
 		to: &MultiLocation,
 	) -> Result<Assets, XcmError> {
-		match Self::transfer_asset(asset, from, to) {
+		match Self::internal_transfer_asset(asset, from, to) {
 			Err(XcmError::Unimplemented) => {
 				let assets = Self::withdraw_asset(asset, from)?;
 				// Not a very forgiving attitude; once we implement roll-backs then it'll be nicer.
@@ -168,19 +175,19 @@ impl TransactAsset for Tuple {
 		Err(XcmError::AssetNotFound)
 	}
 
-	fn transfer_asset(
+	fn internal_transfer_asset(
 		what: &MultiAsset,
 		from: &MultiLocation,
 		to: &MultiLocation,
 	) -> Result<Assets, XcmError> {
 		for_tuples!( #(
-			match Tuple::transfer_asset(what, from, to) {
+			match Tuple::internal_transfer_asset(what, from, to) {
 				Err(XcmError::AssetNotFound) | Err(XcmError::Unimplemented) => (),
 				r => return r,
 			}
 		)* );
 		log::trace!(
-			target: "xcm::TransactAsset::transfer_asset",
+			target: "xcm::TransactAsset::internal_transfer_asset",
 			"did not transfer asset: what: {:?}, from: {:?}, to: {:?}",
 			what,
 			from,
@@ -212,7 +219,7 @@ mod tests {
 			Err(XcmError::AssetNotFound)
 		}
 
-		fn transfer_asset(
+		fn internal_transfer_asset(
 			_what: &MultiAsset,
 			_from: &MultiLocation,
 			_to: &MultiLocation,
@@ -235,7 +242,7 @@ mod tests {
 			Err(XcmError::Overflow)
 		}
 
-		fn transfer_asset(
+		fn internal_transfer_asset(
 			_what: &MultiAsset,
 			_from: &MultiLocation,
 			_to: &MultiLocation,
@@ -258,7 +265,7 @@ mod tests {
 			Ok(Assets::default())
 		}
 
-		fn transfer_asset(
+		fn internal_transfer_asset(
 			_what: &MultiAsset,
 			_from: &MultiLocation,
 			_to: &MultiLocation,