[pallet_xcm::reserve_transfer_assets] NotHoldingFees issue in case of paid XcmRouter is used

xcm/pallet-xcm/src/mock.rs

@@ -16,23 +16,25 @@
 
 use codec::Encode;
 use frame_support::{
-	construct_runtime, parameter_types,
-	traits::{ConstU32, Everything, Nothing},
+	construct_runtime, match_types, parameter_types,
+	traits::{ConstU32, Everything, EverythingBut, Nothing},
 	weights::Weight,
 };
 use frame_system::EnsureRoot;
 use polkadot_parachain::primitives::Id as ParaId;
 use polkadot_runtime_parachains::origin;
 use sp_core::H256;
 use sp_runtime::{traits::IdentityLookup, AccountId32, BuildStorage};
-pub use sp_std::{cell::RefCell, fmt::Debug, marker::PhantomData};
+pub use sp_std::{
+	cell::RefCell, collections::btree_map::BTreeMap, fmt::Debug, marker::PhantomData,
+};
 use xcm::prelude::*;
 use xcm_builder::{
 	AccountId32Aliases, AllowKnownQueryResponses, AllowSubscriptionsFrom,
 	AllowTopLevelPaidExecutionFrom, Case, ChildParachainAsNative, ChildParachainConvertsVia,
 	ChildSystemParachainAsSuperuser, CurrencyAdapter as XcmCurrencyAdapter, FixedRateOfFungible,
 	FixedWeightBounds, IsConcrete, SignedAccountId32AsNative, SignedToAccountId32,
-	SovereignSignedViaLocation, TakeWeightCredit,
+	SovereignSignedViaLocation, TakeWeightCredit, XcmFeesToAccount,
 };
 use xcm_executor::XcmExecutor;
 
@@ -154,7 +156,7 @@ pub(crate) fn take_sent_xcm() -> Vec<(MultiLocation, Xcm<()>)> {
 		r
 	})
 }
-/// Sender that never returns error, always sends
+/// Sender that never returns error.
 pub struct TestSendXcm;
 impl SendXcm for TestSendXcm {
 	type Ticket = (MultiLocation, Xcm<()>);
@@ -193,6 +195,38 @@ impl SendXcm for TestSendXcmErrX8 {
 	}
 }
 
+parameter_types! {
+	pub Para3000: u32 = 3000;
+	pub Para3000Location: MultiLocation = Parachain(Para3000::get()).into();
+	pub Para3000PaymentAmount: u128 = 1;
+	pub Para3000PaymentMultiAssets: MultiAssets = MultiAssets::from(MultiAsset::from((Here, Para3000PaymentAmount::get())));
+}
+/// Sender only sends to `Parachain(3000)` destination requiring payment.
+pub struct TestPaidForPara3000SendXcm;
+impl SendXcm for TestPaidForPara3000SendXcm {
+	type Ticket = (MultiLocation, Xcm<()>);
+	fn validate(
+		dest: &mut Option<MultiLocation>,
+		msg: &mut Option<Xcm<()>>,
+	) -> SendResult<(MultiLocation, Xcm<()>)> {
+		if let Some(dest) = dest.as_ref() {
+			if !dest.eq(&Para3000Location::get()) {
+				return Err(SendError::NotApplicable)
+			}
+		} else {
+			return Err(SendError::NotApplicable)
+		}
+
+		let pair = (dest.take().unwrap(), msg.take().unwrap());
+		Ok((pair, Para3000PaymentMultiAssets::get()))
+	}
+	fn deliver(pair: (MultiLocation, Xcm<()>)) -> Result<XcmHash, SendError> {
+		let hash = fake_message_hash(&pair.1);
+		SENT_XCM.with(|q| q.borrow_mut().push(pair));
+		Ok(hash)
+	}
+}
+
 parameter_types! {
 	pub const BlockHashCount: u64 = 250;
 }
@@ -271,6 +305,14 @@ parameter_types! {
 	pub TrustedAssets: (MultiAssetFilter, MultiLocation) = (All.into(), Here.into());
 	pub const MaxInstructions: u32 = 100;
 	pub const MaxAssetsIntoHolding: u32 = 64;
+	pub XcmFeesTargetAccount: AccountId = AccountId::new([167u8; 32]);
+}
+
+pub const XCM_FEES_NOT_WAIVED_USER_ACCOUNT: [u8; 32] = [37u8; 32];
+match_types! {
+	pub type XcmFeesNotWaivedLocations: impl Contains<MultiLocation> = {
+		MultiLocation { parents: 0, interior: X1(Junction::AccountId32 {network: None, id: XCM_FEES_NOT_WAIVED_USER_ACCOUNT})}
+	};
 }
 
 pub type Barrier = (
@@ -283,7 +325,7 @@ pub type Barrier = (
 pub struct XcmConfig;
 impl xcm_executor::Config for XcmConfig {
 	type RuntimeCall = RuntimeCall;
-	type XcmSender = TestSendXcm;
+	type XcmSender = (TestPaidForPara3000SendXcm, TestSendXcm);
 	type AssetTransactor = LocalAssetTransactor;
 	type OriginConverter = LocalOriginConverter;
 	type IsReserve = ();
@@ -300,7 +342,12 @@ impl xcm_executor::Config for XcmConfig {
 	type SubscriptionService = XcmPallet;
 	type PalletInstancesInfo = AllPalletsWithSystem;
 	type MaxAssetsIntoHolding = MaxAssetsIntoHolding;
-	type FeeManager = ();
+	type FeeManager = XcmFeesToAccount<
+		Self,
+		EverythingBut<XcmFeesNotWaivedLocations>,
+		AccountId,
+		XcmFeesTargetAccount,
+	>;
 	type MessageExporter = ();
 	type UniversalAliases = Nothing;
 	type CallDispatcher = RuntimeCall;
@@ -322,7 +369,7 @@ parameter_types! {
 impl pallet_xcm::Config for Test {
 	type RuntimeEvent = RuntimeEvent;
 	type SendXcmOrigin = xcm_builder::EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
-	type XcmRouter = (TestSendXcmErrX8, TestSendXcm);
+	type XcmRouter = (TestSendXcmErrX8, TestPaidForPara3000SendXcm, TestSendXcm);
 	type ExecuteXcmOrigin = xcm_builder::EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
 	type XcmExecuteFilter = Everything;
 	type XcmExecutor = XcmExecutor<XcmConfig>;

xcm/pallet-xcm/src/tests.rs

@@ -526,6 +526,74 @@ fn reserve_transfer_assets_works() {
 	});
 }
 
+/// Test `reserve_transfer_assets_with_paid_router_works`
+///
+/// Asserts that the sender's balance is decreased and the beneficiary's balance
+/// is increased. Verifies the correct message is sent and event is emitted.
+/// Verifies that XCM router fees (`SendXcm::validate` -> `MultiAssets`) are withdrawn from correct user account
+/// and deposited to a correct target account (`XcmFeesTargetAccount`).
+#[test]
+fn reserve_transfer_assets_with_paid_router_works() {
+	let user_account = AccountId::from(XCM_FEES_NOT_WAIVED_USER_ACCOUNT);
+	let paid_para_id = Para3000::get();
+	let balances = vec![
+		(user_account.clone(), INITIAL_BALANCE),
+		(ParaId::from(paid_para_id).into_account_truncating(), INITIAL_BALANCE),
+		(XcmFeesTargetAccount::get(), INITIAL_BALANCE),
+	];
+	new_test_ext_with_balances(balances).execute_with(|| {
+		let xcm_router_fee_amount = Para3000PaymentAmount::get();
+		let weight = BaseXcmWeight::get() * 2;
+		let dest: MultiLocation =
+			Junction::AccountId32 { network: None, id: user_account.clone().into() }.into();
+		assert_eq!(Balances::total_balance(&user_account), INITIAL_BALANCE);
+		assert_ok!(XcmPallet::reserve_transfer_assets(
+			RuntimeOrigin::signed(user_account.clone()),
+			Box::new(Parachain(paid_para_id).into()),
+			Box::new(dest.clone().into()),
+			Box::new((Here, SEND_AMOUNT).into()),
+			0,
+		));
+		// check event
+		assert_eq!(
+			last_event(),
+			RuntimeEvent::XcmPallet(crate::Event::Attempted { outcome: Outcome::Complete(weight) })
+		);
+
+		// XCM_FEES_NOT_WAIVED_USER_ACCOUNT spent amount
+		assert_eq!(
+			Balances::free_balance(user_account),
+			INITIAL_BALANCE - SEND_AMOUNT - xcm_router_fee_amount
+		);
+		// Destination account (parachain account) has amount
+		let para_acc: AccountId = ParaId::from(paid_para_id).into_account_truncating();
+		assert_eq!(Balances::free_balance(para_acc), INITIAL_BALANCE + SEND_AMOUNT);
+		// XcmFeesTargetAccount where should lend xcm_router_fee_amount
+		assert_eq!(
+			Balances::free_balance(XcmFeesTargetAccount::get()),
+			INITIAL_BALANCE + xcm_router_fee_amount
+		);
+		assert_eq!(
+			sent_xcm(),
+			vec![(
+				Parachain(paid_para_id).into(),
+				Xcm(vec![
+					ReserveAssetDeposited((Parent, SEND_AMOUNT).into()),
+					ClearOrigin,
+					buy_limited_execution((Parent, SEND_AMOUNT), Weight::from_parts(4000, 4000)),
+					DepositAsset { assets: AllCounted(1).into(), beneficiary: dest },
+				]),
+			)]
+		);
+		let versioned_sent = VersionedXcm::from(sent_xcm().into_iter().next().unwrap().1);
+		let _check_v2_ok: xcm::v2::Xcm<()> = versioned_sent.try_into().unwrap();
+		assert_eq!(
+			last_event(),
+			RuntimeEvent::XcmPallet(crate::Event::Attempted { outcome: Outcome::Complete(weight) })
+		);
+	});
+}
+
 /// Test `limited_reserve_transfer_assets`
 ///
 /// Asserts that the sender's balance is decreased and the beneficiary's balance

xcm/xcm-builder/src/fee_handling.rs

@@ -37,16 +37,8 @@ impl<
 	> FeeManager for XcmFeesToAccount<XcmConfig, WaivedLocations, AccountId, ReceiverAccount>
 {
 	fn is_waived(origin: Option<&MultiLocation>, _: FeeReason) -> bool {
-		#[cfg(feature = "runtime-benchmarks")]
-		{
-			let _ = origin;
-			true
-		}
-		#[cfg(not(feature = "runtime-benchmarks"))]
-		{
-			let Some(loc) = origin else { return false };
-			WaivedLocations::contains(loc)
-		}
+		let Some(loc) = origin else { return false };
+		WaivedLocations::contains(loc)
 	}
 
 	fn handle_fee(fees: MultiAssets, context: Option<&XcmContext>) {

xcm/xcm-executor/src/lib.rs

@@ -404,10 +404,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
 		reason: FeeReason,
 	) -> Result<XcmHash, XcmError> {
 		let (ticket, fee) = validate_send::<Config::XcmSender>(dest, msg)?;
-		if !Config::FeeManager::is_waived(self.origin_ref(), reason) {
-			let paid = self.holding.try_take(fee.into()).map_err(|_| XcmError::NotHoldingFees)?;
-			Config::FeeManager::handle_fee(paid.into(), Some(&self.context));
-		}
+		self.take_fee(fee, reason)?;
 		Config::XcmSender::deliver(ticket).map_err(Into::into)
 	}
 
@@ -985,12 +982,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
 		let QueryResponseInfo { destination, query_id, max_weight } = info;
 		let instruction = QueryResponse { query_id, response, max_weight, querier };
 		let message = Xcm(vec![instruction]);
-		let (ticket, fee) = validate_send::<Config::XcmSender>(destination, message)?;
-		if !Config::FeeManager::is_waived(self.origin_ref(), fee_reason) {
-			let paid = self.holding.try_take(fee.into()).map_err(|_| XcmError::NotHoldingFees)?;
-			Config::FeeManager::handle_fee(paid.into(), Some(&self.context));
-		}
-		Config::XcmSender::deliver(ticket).map_err(Into::into)
+		self.send(destination, message, fee_reason)
 	}
 
 	fn try_reanchor(