Fix delivery fees issue (#3085)

This fix aims to solve an issue in Kusama that resulted in failed
reserve asset transfers.
It will be ported later to master as well.
The issue is that during multi-hop XCMs, like reserve asset transfers
where the reserve is not the sender nor the destination, there are no
assets available to pay for delivery fees.
This fix makes sure to deduct delivery fees from the assets being
transferred.

- [x] code changes required for fix now complete
- [x] local testing done shows issue as fixed
- [x] regression test added
- [x] PR reviewed
- [x] CI passing - currently issue with CI not running on non-master PR
- [ ] xcm-executor patch version 4.0.1 released
- [ ] xcm-executor patch version 4.0.1 included in
https://github.com/polkadot-fellows/runtimes/
- [ ] Kusama and Polkadot runtimes patch released

---------

Signed-off-by: Adrian Catangiu <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
Co-authored-by: Keith Yeung <[email protected]>

cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs

@@ -49,9 +49,9 @@ use xcm::latest::prelude::*;
 use xcm_builder::{
 	AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses,
 	AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, AsPrefixedGeneralIndex,
-	ConvertedConcreteId, CurrencyAdapter, DenyReserveTransferToRelayChain, DenyThenTry,
-	EnsureXcmOrigin, FixedWeightBounds, FrameTransactionalProcessor, FungiblesAdapter, IsConcrete,
-	LocalMint, NativeAsset, NoChecking, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative,
+	ConvertedConcreteId, CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds,
+	FrameTransactionalProcessor, FungiblesAdapter, IsConcrete, LocalMint,
+	NativeAsset, NoChecking, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative,
 	SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative,
 	SignedToAccountId32, SovereignSignedViaLocation, StartsWith, TakeWeightCredit,
 	TrailingSetTopicAsId, UsingComponents, WithComputedOrigin, WithUniqueTopic,
@@ -190,34 +190,29 @@ match_types! {
 	};
 }
 
-pub type Barrier = TrailingSetTopicAsId<
-	DenyThenTry<
-		DenyReserveTransferToRelayChain,
+pub type Barrier = TrailingSetTopicAsId<(
+	TakeWeightCredit,
+	// Expected responses are OK.
+	AllowKnownQueryResponses<PolkadotXcm>,
+	// Allow XCMs with some computed origins to pass through.
+	WithComputedOrigin<
 		(
-			TakeWeightCredit,
-			// Expected responses are OK.
-			AllowKnownQueryResponses<PolkadotXcm>,
-			// Allow XCMs with some computed origins to pass through.
-			WithComputedOrigin<
-				(
-					// If the message is one that immediately attempts to pay for execution, then
-					// allow it.
-					AllowTopLevelPaidExecutionFrom<Everything>,
-					// System Assets parachain, parent and its exec plurality get free
-					// execution
-					AllowExplicitUnpaidExecutionFrom<(
-						CommonGoodAssetsParachain,
-						ParentOrParentsExecutivePlurality,
-					)>,
-					// Subscriptions for version tracking are OK.
-					AllowSubscriptionsFrom<Everything>,
-				),
-				UniversalLocation,
-				ConstU32<8>,
-			>,
+			// If the message is one that immediately attempts to pay for execution, then
+			// allow it.
+			AllowTopLevelPaidExecutionFrom<Everything>,
+			// System Assets parachain, parent and its exec plurality get free
+			// execution
+			AllowExplicitUnpaidExecutionFrom<(
+				CommonGoodAssetsParachain,
+				ParentOrParentsExecutivePlurality,
+			)>,
+			// Subscriptions for version tracking are OK.
+			AllowSubscriptionsFrom<Everything>,
 		),
+		UniversalLocation,
+		ConstU32<8>,
 	>,
->;
+)>;
 
 /// Type alias to conveniently refer to `frame_system`'s `Config::AccountId`.
 pub type AccountIdOf<R> = <R as frame_system::Config>::AccountId;
@@ -314,8 +309,6 @@ pub type Reserves = (
 	NativeAssetFrom<SystemAssetHubLocation>,
 	AssetPrefixFrom<EthereumLocation, SystemAssetHubLocation>,
 );
-pub type TrustedTeleporters =
-	(AssetFromChain<LocalTeleportableToAssetHub, SystemAssetHubLocation>,);
 
 pub struct XcmConfig;
 impl xcm_executor::Config for XcmConfig {
@@ -326,7 +319,7 @@ impl xcm_executor::Config for XcmConfig {
 	type OriginConverter = XcmOriginToTransactDispatchOrigin;
 	type IsReserve = Reserves;
 	// no teleport trust established with other chains
-	type IsTeleporter = TrustedTeleporters;
+	type IsTeleporter = ();
 	type UniversalLocation = UniversalLocation;
 	type Barrier = Barrier;
 	type Weigher = FixedWeightBounds<UnitWeightCost, RuntimeCall, MaxInstructions>;
@@ -366,7 +359,7 @@ impl pallet_xcm::Config for Runtime {
 	type SendXcmOrigin = EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
 	type XcmRouter = XcmRouter;
 	type ExecuteXcmOrigin = EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
-	type XcmExecuteFilter = Nothing;
+	type XcmExecuteFilter = Everything;
 	// ^ Disable dispatchable execute on the XCM pallet.
 	// Needs to be `Everything` for local testing.
 	type XcmExecutor = XcmExecutor<XcmConfig>;

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

@@ -829,16 +829,32 @@ impl<Config: config::Config> XcmExecutor<Config> {
 			DepositReserveAsset { assets, dest, xcm } => {
 				let old_holding = self.holding.clone();
 				let result = Config::TransactionalProcessor::process(|| {
+					// we need to do this take/put cycle to solve wildcards and get exact assets to be
+					// weighed
+					let to_weigh = self.holding.saturating_take(assets.clone());
+					self.holding.subsume_assets(to_weigh.clone());
+
+					let mut message_to_weigh =
+						vec![ReserveAssetDeposited(to_weigh.into()), ClearOrigin];
+					message_to_weigh.extend(xcm.0.clone().into_iter());
+					let (_, fee) =
+						validate_send::<Config::XcmSender>(dest.clone(), Xcm(message_to_weigh))?;
+					// set aside fee to be charged by XcmSender
+					let parked_fee = self.holding.saturating_take(fee.into());
+
+					// now take assets to deposit (excluding parked_fee)
 					let deposited = self.holding.saturating_take(assets);
 					for asset in deposited.assets_iter() {
 						Config::AssetTransactor::deposit_asset(&asset, &dest, Some(&self.context))?;
 					}
-					// Note that we pass `None` as `maybe_failed_bin` and drop any assets which
-					// cannot be reanchored  because we have already called `deposit_asset` on all
-					// assets.
+					// Note that we pass `None` as `maybe_failed_bin` and drop any assets which cannot
+					// be reanchored  because we have already called `deposit_asset` on all assets.
 					let assets = Self::reanchored(deposited, &dest, None);
 					let mut message = vec![ReserveAssetDeposited(assets), ClearOrigin];
 					message.extend(xcm.0.into_iter());
+
+					// put back parked_fee in holding register to be charged by XcmSender
+					self.holding.subsume_assets(parked_fee);
 					self.send(dest, Xcm(message), FeeReason::DepositReserveAsset)?;
 					Ok(())
 				});