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

runtime/common/Cargo.toml

@@ -50,6 +50,9 @@ runtime-parachains = { package = "polkadot-runtime-parachains", path = "../parac
 
 slot-range-helper = { path = "slot_range_helper", default-features = false }
 xcm = { path = "../../xcm", default-features = false }
+xcm-executor = { path = "../../xcm/xcm-executor", default-features = false, optional = true }
+
+pallet-xcm-benchmarks = { path = "../../xcm/pallet-xcm-benchmarks", default-features = false, optional = true }
 
 [dev-dependencies]
 hex-literal = "0.4.1"
@@ -111,7 +114,9 @@ runtime-benchmarks = [
 	"frame-system/runtime-benchmarks",
 	"runtime-parachains/runtime-benchmarks",
 	"pallet-babe/runtime-benchmarks",
-	"pallet-fast-unstake/runtime-benchmarks"
+	"pallet-fast-unstake/runtime-benchmarks",
+	"pallet-xcm-benchmarks/runtime-benchmarks",
+	"xcm-executor",
 ]
 try-runtime = [
 	"runtime-parachains/try-runtime",

runtime/common/src/xcm_sender.rs

@@ -109,6 +109,96 @@ impl<T: configuration::Config + dmp::Config, W: xcm::WrapVersion, P: PriceForPar
 	}
 }
 
+/// Implementation of `pallet_xcm_benchmarks::EnsureDelivery` which helps to ensure delivery to the
+/// `ParaId` parachain (sibling or child). Deposits existential deposit for origin (if needed).
+/// Deposits estimated fee to the origin account (if needed).
+/// Allows to trigger additional logic for specific `ParaId` (e.g. open HRMP channel) (if neeeded).
+#[cfg(feature = "runtime-benchmarks")]
+pub struct ToParachainDeliveryHelper<
+	XcmConfig,
+	ExistentialDeposit,
+	PriceForDelivery,
+	ParaId,
+	ToParaIdHelper,
+>(
+	sp_std::marker::PhantomData<(
+		XcmConfig,
+		ExistentialDeposit,
+		PriceForDelivery,
+		ParaId,
+		ToParaIdHelper,
+	)>,
+);
+
+#[cfg(feature = "runtime-benchmarks")]
+impl<
+		XcmConfig: xcm_executor::Config,
+		ExistentialDeposit: Get<Option<MultiAsset>>,
+		PriceForDelivery: PriceForParachainDelivery,
+		Parachain: Get<ParaId>,
+		ToParachainHelper: EnsureForParachain,
+	> pallet_xcm_benchmarks::EnsureDelivery
+	for ToParachainDeliveryHelper<
+		XcmConfig,
+		ExistentialDeposit,
+		PriceForDelivery,
+		Parachain,
+		ToParachainHelper,
+	>
+{
+	fn ensure_successful_delivery(
+		origin_ref: &MultiLocation,
+		_dest: &MultiLocation,
+		fee_reason: xcm_executor::traits::FeeReason,
+	) -> (Option<xcm_executor::FeesMode>, Option<MultiAssets>) {
+		use xcm_executor::{
+			traits::{FeeManager, TransactAsset},
+			FeesMode,
+		};
+
+		let mut fees_mode = None;
+		if !XcmConfig::FeeManager::is_waived(Some(origin_ref), fee_reason) {
+			// if not waived, we need to set up accounts for paying and receiving fees
+
+			// mint ED to origin if needed
+			if let Some(ed) = ExistentialDeposit::get() {
+				XcmConfig::AssetTransactor::deposit_asset(&ed, &origin_ref, None).unwrap();
+			}
+
+			// overestimate delivery fee
+			let overestimated_xcm = vec![ClearOrigin; 128].into();
+			let overestimated_fees = PriceForDelivery::price_for_parachain_delivery(
+				Parachain::get(),
+				&overestimated_xcm,
+			);
+
+			// mint overestimated fee to origin
+			for fee in overestimated_fees.inner() {
+				XcmConfig::AssetTransactor::deposit_asset(&fee, &origin_ref, None).unwrap();
+			}
+
+			// allow more initialization for target parachain
+			ToParachainHelper::ensure(Parachain::get());
+
+			// expected worst case - direct withdraw
+			fees_mode = Some(FeesMode { jit_withdraw: true });
+		}
+		(fees_mode, None)
+	}
+}
+
+/// Ensure more initialization for `ParaId`. (e.g. open HRMP channels, ...)
+#[cfg(feature = "runtime-benchmarks")]
+pub trait EnsureForParachain {
+	fn ensure(para_id: ParaId);
+}
+#[cfg(feature = "runtime-benchmarks")]
+impl EnsureForParachain for () {
+	fn ensure(_para_id: ParaId) {
+		// doing nothing
+	}
+}
+
 #[cfg(test)]
 mod tests {
 	use super::*;

runtime/kusama/src/lib.rs

@@ -2117,9 +2117,24 @@ sp_api::impl_runtime_apis! {
 			impl pallet_nomination_pools_benchmarking::Config for Runtime {}
 			impl runtime_parachains::disputes::slashing::benchmarking::Config for Runtime {}
 
+			parameter_types! {
+				pub ExistentialDepositMultiAsset: Option<MultiAsset> = Some((
+					TokenLocation::get(),
+					ExistentialDeposit::get()
+				).into());
+				pub ToParachain: ParaId = kusama_runtime_constants::system_parachain::ASSET_HUB_ID.into();
+			}
+
 			impl pallet_xcm_benchmarks::Config for Runtime {
 				type XcmConfig = XcmConfig;
 				type AccountIdConverter = SovereignAccountOf;
+				type DeliveryHelper = runtime_common::xcm_sender::ToParachainDeliveryHelper<
+					XcmConfig,
+					ExistentialDepositMultiAsset,
+					xcm_config::PriceForChildParachainDelivery,
+					ToParachain,
+					(),
+				>;
 				fn valid_destination() -> Result<MultiLocation, BenchmarkError> {
 					Ok(AssetHub::get())
 				}

runtime/kusama/src/xcm_config.rs

@@ -115,15 +115,14 @@ parameter_types! {
 	pub const BaseDeliveryFee: u128 = CENTS.saturating_mul(3);
 }
 
+pub type PriceForChildParachainDelivery =
+	ExponentialPrice<FeeAssetId, BaseDeliveryFee, TransactionByteFee, Dmp>;
+
 /// The XCM router. When we want to send an XCM message, we use this type. It amalgamates all of our
 /// individual routers.
 pub type XcmRouter = WithUniqueTopic<(
 	// Only one router so far - use DMP to communicate with child parachains.
-	ChildParachainRouter<
-		Runtime,
-		XcmPallet,
-		ExponentialPrice<FeeAssetId, BaseDeliveryFee, TransactionByteFee, Dmp>,
-	>,
+	ChildParachainRouter<Runtime, XcmPallet, PriceForChildParachainDelivery>,
 )>;
 
 parameter_types! {
@@ -146,6 +145,9 @@ match_types! {
 	pub type OnlyParachains: impl Contains<MultiLocation> = {
 		MultiLocation { parents: 0, interior: X1(Parachain(_)) }
 	};
+	pub type HereLocation: impl Contains<MultiLocation> = {
+		MultiLocation { parents: 0, interior: Here }
+	};
 }
 
 /// The barriers one of which must be passed for an XCM message to be executed.
@@ -346,7 +348,8 @@ impl xcm_executor::Config for XcmConfig {
 	type SubscriptionService = XcmPallet;
 	type PalletInstancesInfo = AllPalletsWithSystem;
 	type MaxAssetsIntoHolding = MaxAssetsIntoHolding;
-	type FeeManager = XcmFeesToAccount<Self, SystemParachains, AccountId, TreasuryAccount>;
+	type FeeManager =
+		XcmFeesToAccount<Self, (SystemParachains, HereLocation), AccountId, TreasuryAccount>;
 	// No bridges yet...
 	type MessageExporter = ();
 	type UniversalAliases = Nothing;

runtime/polkadot/src/lib.rs

@@ -2076,9 +2076,24 @@ sp_api::impl_runtime_apis! {
 			let treasury_key = frame_system::Account::<Runtime>::hashed_key_for(Treasury::account_id());
 			whitelist.push(treasury_key.to_vec().into());
 
+			parameter_types! {
+				pub ExistentialDepositMultiAsset: Option<MultiAsset> = Some((
+					TokenLocation::get(),
+					ExistentialDeposit::get()
+				).into());
+				pub ToParachain: ParaId = polkadot_runtime_constants::system_parachain::ASSET_HUB_ID.into();
+			}
+
 			impl pallet_xcm_benchmarks::Config for Runtime {
 				type XcmConfig = XcmConfig;
 				type AccountIdConverter = SovereignAccountOf;
+				type DeliveryHelper = runtime_common::xcm_sender::ToParachainDeliveryHelper<
+					XcmConfig,
+					ExistentialDepositMultiAsset,
+					xcm_config::PriceForChildParachainDelivery,
+					ToParachain,
+					(),
+				>;
 				fn valid_destination() -> Result<MultiLocation, BenchmarkError> {
 					Ok(AssetHubLocation::get())
 				}

runtime/polkadot/src/xcm_config.rs

@@ -120,15 +120,14 @@ parameter_types! {
 	pub const BaseDeliveryFee: u128 = CENTS.saturating_mul(3);
 }
 
+pub type PriceForChildParachainDelivery =
+	ExponentialPrice<FeeAssetId, BaseDeliveryFee, TransactionByteFee, Dmp>;
+
 /// The XCM router. When we want to send an XCM message, we use this type. It amalgamates all of our
 /// individual routers.
 pub type XcmRouter = (
 	// Only one router so far - use DMP to communicate with child parachains.
-	ChildParachainRouter<
-		Runtime,
-		XcmPallet,
-		ExponentialPrice<FeeAssetId, BaseDeliveryFee, TransactionByteFee, Dmp>,
-	>,
+	ChildParachainRouter<Runtime, XcmPallet, PriceForChildParachainDelivery>,
 );
 
 parameter_types! {
@@ -157,6 +156,9 @@ match_types! {
 		MultiLocation { parents: 0, interior: X1(Parachain(COLLECTIVES_ID)) } |
 		MultiLocation { parents: 0, interior: X2(Parachain(COLLECTIVES_ID), Plurality { id: BodyId::Technical, .. }) }
 	};
+	pub type HereLocation: impl Contains<MultiLocation> = {
+		MultiLocation { parents: 0, interior: Here }
+	};
 }
 
 /// The barriers one of which must be passed for an XCM message to be executed.
@@ -347,7 +349,8 @@ impl xcm_executor::Config for XcmConfig {
 	type SubscriptionService = XcmPallet;
 	type PalletInstancesInfo = AllPalletsWithSystem;
 	type MaxAssetsIntoHolding = MaxAssetsIntoHolding;
-	type FeeManager = XcmFeesToAccount<Self, SystemParachains, AccountId, TreasuryAccount>;
+	type FeeManager =
+		XcmFeesToAccount<Self, (SystemParachains, HereLocation), AccountId, TreasuryAccount>;
 	// No bridges yet...
 	type MessageExporter = ();
 	type UniversalAliases = Nothing;

runtime/rococo/src/lib.rs

@@ -2094,11 +2094,26 @@ sp_api::impl_runtime_apis! {
 				LocalCheckAccount, LocationConverter, AssetHub, TokenLocation, XcmConfig,
 			};
 
+			parameter_types! {
+				pub ExistentialDepositMultiAsset: Option<MultiAsset> = Some((
+					TokenLocation::get(),
+					ExistentialDeposit::get()
+				).into());
+				pub ToParachain: ParaId = rococo_runtime_constants::system_parachain::ASSET_HUB_ID.into();
+			}
+
 			impl frame_system_benchmarking::Config for Runtime {}
 			impl frame_benchmarking::baseline::Config for Runtime {}
 			impl pallet_xcm_benchmarks::Config for Runtime {
 				type XcmConfig = XcmConfig;
 				type AccountIdConverter = LocationConverter;
+				type DeliveryHelper = runtime_common::xcm_sender::ToParachainDeliveryHelper<
+					XcmConfig,
+					ExistentialDepositMultiAsset,
+					xcm_config::PriceForChildParachainDelivery,
+					ToParachain,
+					(),
+				>;
 				fn valid_destination() -> Result<MultiLocation, BenchmarkError> {
 					Ok(AssetHub::get())
 				}
@@ -2116,10 +2131,7 @@ sp_api::impl_runtime_apis! {
 					AssetHub::get(),
 					MultiAsset { fun: Fungible(1 * UNITS), id: Concrete(TokenLocation::get()) },
 				));
-				pub const TrustedReserve: Option<(MultiLocation, MultiAsset)> = Some((
-					AssetHub::get(),
-					MultiAsset { fun: Fungible(1 * UNITS), id: Concrete(TokenLocation::get()) },
-				));
+				pub const TrustedReserve: Option<(MultiLocation, MultiAsset)> = None;
 			}
 
 			impl pallet_xcm_benchmarks::fungible::Config for Runtime {

runtime/rococo/src/xcm_config.rs

@@ -96,15 +96,14 @@ parameter_types! {
 	pub const BaseDeliveryFee: u128 = CENTS.saturating_mul(3);
 }
 
+pub type PriceForChildParachainDelivery =
+	ExponentialPrice<FeeAssetId, BaseDeliveryFee, TransactionByteFee, Dmp>;
+
 /// The XCM router. When we want to send an XCM message, we use this type. It amalgamates all of our
 /// individual routers.
 pub type XcmRouter = WithUniqueTopic<(
 	// Only one router so far - use DMP to communicate with child parachains.
-	ChildParachainRouter<
-		Runtime,
-		XcmPallet,
-		ExponentialPrice<FeeAssetId, BaseDeliveryFee, TransactionByteFee, Dmp>,
-	>,
+	ChildParachainRouter<Runtime, XcmPallet, PriceForChildParachainDelivery>,
 )>;
 
 parameter_types! {
@@ -140,6 +139,9 @@ match_types! {
 	pub type OnlyParachains: impl Contains<MultiLocation> = {
 		MultiLocation { parents: 0, interior: X1(Parachain(_)) }
 	};
+	pub type HereLocation: impl Contains<MultiLocation> = {
+		MultiLocation { parents: 0, interior: Here }
+	};
 }
 
 /// The barriers one of which must be passed for an XCM message to be executed.
@@ -319,7 +321,8 @@ impl xcm_executor::Config for XcmConfig {
 	type SubscriptionService = XcmPallet;
 	type PalletInstancesInfo = AllPalletsWithSystem;
 	type MaxAssetsIntoHolding = MaxAssetsIntoHolding;
-	type FeeManager = XcmFeesToAccount<Self, SystemParachains, AccountId, TreasuryAccount>;
+	type FeeManager =
+		XcmFeesToAccount<Self, (SystemParachains, HereLocation), AccountId, TreasuryAccount>;
 	type MessageExporter = ();
 	type UniversalAliases = Nothing;
 	type CallDispatcher = WithOriginFilter<SafeCallFilter>;

runtime/westend/src/lib.rs

@@ -1860,6 +1860,7 @@ sp_api::impl_runtime_apis! {
 			impl pallet_xcm_benchmarks::Config for Runtime {
 				type XcmConfig = xcm_config::XcmConfig;
 				type AccountIdConverter = xcm_config::LocationConverter;
+				type DeliveryHelper = ();
 				fn valid_destination() -> Result<MultiLocation, BenchmarkError> {
 					Ok(AssetHub::get())
 				}

xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs

@@ -24,7 +24,7 @@ use frame_support::{
 use sp_runtime::traits::{Bounded, Zero};
 use sp_std::{prelude::*, vec};
 use xcm::latest::prelude::*;
-use xcm_executor::traits::{ConvertLocation, TransactAsset};
+use xcm_executor::traits::{ConvertLocation, FeeReason, TransactAsset};
 
 benchmarks_instance_pallet! {
 	where_clause { where
@@ -87,12 +87,28 @@ benchmarks_instance_pallet! {
 		let dest_location = T::valid_destination()?;
 		let dest_account = T::AccountIdConverter::convert_location(&dest_location).unwrap();
 
+		use crate::EnsureDelivery;
+		let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery(
+			&sender_location,
+			&dest_location,
+			FeeReason::TransferReserveAsset
+		);
+		let sender_account_balance_before = T::TransactAsset::balance(&sender_account);
+
 		let asset = T::get_multi_asset();
 		<AssetTransactorOf<T>>::deposit_asset(&asset, &sender_location, None).unwrap();
+		assert!(T::TransactAsset::balance(&sender_account) > sender_account_balance_before);
 		let assets: MultiAssets = vec![ asset ].into();
 		assert!(T::TransactAsset::balance(&dest_account).is_zero());
 
 		let mut executor = new_executor::<T>(sender_location);
+		if let Some(expected_fees_mode) = expected_fees_mode {
+			executor.set_fees_mode(expected_fees_mode);
+		}
+		if let Some(expected_assets_in_holding) = expected_assets_in_holding {
+			executor.set_holding(expected_assets_in_holding.into());
+		}
+
 		let instruction = Instruction::TransferReserveAsset {
 			assets,
 			dest: dest_location,
@@ -102,7 +118,7 @@ benchmarks_instance_pallet! {
 	}: {
 		executor.bench_process(xcm)?;
 	} verify {
-		assert!(T::TransactAsset::balance(&sender_account).is_zero());
+		assert!(T::TransactAsset::balance(&sender_account) <= sender_account_balance_before);
 		assert!(!T::TransactAsset::balance(&dest_account).is_zero());
 		// TODO: Check sender queue is not empty. #4426
 	}