From f7a11c3b44492cb8988f3005ee0dfe24afe8f709 Mon Sep 17 00:00:00 2001 From: Francisco Aguirre Date: Fri, 26 Jul 2024 22:17:18 +0200 Subject: [PATCH] chore(xcm-executor): refactor finding asset to pay delivery fees --- .../assets/asset-hub-rococo/src/xcm_config.rs | 15 +-- .../asset-hub-westend/src/xcm_config.rs | 14 +-- .../runtimes/testing/penpal/src/xcm_config.rs | 10 +- polkadot/xcm/xcm-executor/src/lib.rs | 92 ++++++++----------- 4 files changed, 61 insertions(+), 70 deletions(-) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs index 8c8e991eb850..a2882f54bd06 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs @@ -54,13 +54,13 @@ use xcm_builder::{ DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, FungiblesAdapter, GlobalConsensusParachainConvertsFor, HashedDescription, IsConcrete, LocalMint, - NetworkExportTableItem, NoChecking, NonFungiblesAdapter, ParentAsSuperuser, ParentIsPreset, - RelayChainAsNative, SiblingParachainAsNative, SiblingParachainConvertsVia, - SignedAccountId32AsNative, SignedToAccountId32, SovereignPaidRemoteExporter, + MatchedConvertedConcreteId, NetworkExportTableItem, NoChecking, NonFungiblesAdapter, + ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, + SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, + SignedToAccountId32, SingleAssetExchangeAdapter, SovereignPaidRemoteExporter, SovereignSignedViaLocation, StartsWith, StartsWithExplicitGlobalConsensus, TakeWeightCredit, - TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, - WithLatestLocationConverter, XcmFeeManagerFromComponents, SendXcmFeeToAccount, SingleAssetExchangeAdapter, - MatchedConvertedConcreteId, + TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, + WithLatestLocationConverter, WithUniqueTopic, XcmFeeManagerFromComponents, }; use xcm_executor::XcmExecutor; @@ -340,13 +340,14 @@ pub type PoolAssetsExchanger = SingleAssetExchangeAdapter< ( TrustBackedAssetsAsLocation, ForeignAssetsConvertedConcreteId, + // `ForeignAssetsConvertedConcreteId` excludes the relay token, so we add it back here. MatchedConvertedConcreteId< xcm::v3::Location, Balance, Equals, WithLatestLocationConverter, TryConvertInto, - >, // Adding this back in to match on relay token. + >, ), AccountId, >; diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs index 34ff951746a7..6f2acfdaf6bb 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs @@ -50,12 +50,13 @@ use xcm_builder::{ DenyReserveTransferToRelayChain, DenyThenTry, DescribeFamily, DescribePalletTerminal, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, FungiblesAdapter, GlobalConsensusParachainConvertsFor, HashedDescription, IsConcrete, LocalMint, - NetworkExportTableItem, NoChecking, NonFungiblesAdapter, ParentAsSuperuser, ParentIsPreset, - RelayChainAsNative, SiblingParachainAsNative, SiblingParachainConvertsVia, - SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, StartsWith, + MatchedConvertedConcreteId, NetworkExportTableItem, NoChecking, NonFungiblesAdapter, + ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, + SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, + SignedToAccountId32, SingleAssetExchangeAdapter, SovereignSignedViaLocation, StartsWith, StartsWithExplicitGlobalConsensus, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, - WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, XcmFeeManagerFromComponents, - WithLatestLocationConverter, SendXcmFeeToAccount, SingleAssetExchangeAdapter, MatchedConvertedConcreteId, + WeightInfoBounds, WithComputedOrigin, WithLatestLocationConverter, WithUniqueTopic, + XcmFeeManagerFromComponents, }; use xcm_executor::XcmExecutor; @@ -359,13 +360,14 @@ pub type PoolAssetsExchanger = SingleAssetExchangeAdapter< ( TrustBackedAssetsAsLocation, ForeignAssetsConvertedConcreteId, + // `ForeignAssetsConvertedConcreteId` excludes the relay token, so we add it back here. MatchedConvertedConcreteId< xcm::v3::Location, Balance, Equals, WithLatestLocationConverter, TryConvertInto, - >, // Adding this back in to match on relay token. + >, ), AccountId, >; diff --git a/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs b/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs index d202789017bf..99aadb33b840 100644 --- a/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs @@ -50,12 +50,12 @@ use xcm_builder::{ AccountId32Aliases, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, AsPrefixedGeneralIndex, ConvertedConcreteId, EnsureXcmOrigin, FixedWeightBounds, FrameTransactionalProcessor, - FungibleAdapter, FungiblesAdapter, SingleAssetExchangeAdapter, IsConcrete, LocalMint, NativeAsset, - NoChecking, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, + FungibleAdapter, FungiblesAdapter, IsConcrete, LocalMint, NativeAsset, NoChecking, + ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, - SignedToAccountId32, SovereignSignedViaLocation, StartsWith, TakeWeightCredit, - TrailingSetTopicAsId, UsingComponents, WithComputedOrigin, WithUniqueTopic, - XcmFeeManagerFromComponents, SendXcmFeeToAccount, + SignedToAccountId32, SingleAssetExchangeAdapter, SovereignSignedViaLocation, StartsWith, + TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, WithComputedOrigin, WithUniqueTopic, + XcmFeeManagerFromComponents, }; use xcm_executor::{traits::JustTry, XcmExecutor}; diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index 5576544cf421..fbc436aa2472 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -492,32 +492,8 @@ impl XcmExecutor { }; // If `BuyExecution` was called, we know we can try to use that asset for fees. // We get the asset the user wants to use to pay for fees. - let asset_to_pay_for_fees = if let Some(asset_used_for_fees) = &self.asset_used_for_fees { - if asset_used_for_fees != &asset_needed_for_fees.id { - match Config::AssetExchanger::quote_exchange_price( - &(asset_used_for_fees.clone(), 1u128).into(), - &asset_needed_for_fees, - false, - ) { - Some(necessary_amount) => - (asset_used_for_fees.clone(), necessary_amount).into(), - // If we can't convert, then we return the original asset. - // It will error later in any case. - None => { - tracing::trace!( - target: "xcm::take_fee", - ?asset_used_for_fees, - "Could not convert fees", - ); - asset_needed_for_fees.clone() - }, - } - } else { - asset_needed_for_fees.clone() - } - } else { - asset_needed_for_fees.clone() - }; + let asset_to_pay_for_fees = + self.calculate_asset_for_delivery_fees(asset_needed_for_fees.clone()); tracing::trace!(target: "xcm::fees", ?asset_to_pay_for_fees); // We withdraw or take from holding the asset the user wants to use for fee payment. let withdrawn_fee_asset = if self.fees_mode.jit_withdraw { @@ -567,6 +543,40 @@ impl XcmExecutor { Ok(()) } + /// Calculates the amount of `self.asset_used_for_fees` required to swap for + /// `asset_needed_for_fees`. + /// + /// The calculation is done by `Config::AssetExchanger`. + /// If `self.asset_used_for_fees` is not set, it will just return `asset_needed_for_fees`. + fn calculate_asset_for_delivery_fees(&self, asset_needed_for_fees: Asset) -> Asset { + if let Some(asset_wanted_for_fees) = &self.asset_used_for_fees { + if *asset_wanted_for_fees != asset_needed_for_fees.id { + match Config::AssetExchanger::quote_exchange_price( + &(asset_wanted_for_fees.clone(), 1u128).into(), + &asset_needed_for_fees, + false, // Minimal. + ) { + Some(necessary_amount) => + (asset_wanted_for_fees.clone(), necessary_amount).into(), + // If we can't convert, then we return the original asset. + // It will error later in any case. + None => { + tracing::trace!( + target: "xcm::take_fee", + ?asset_wanted_for_fees, + "Could not convert fees", + ); + asset_needed_for_fees.clone() + }, + } + } else { + asset_needed_for_fees + } + } else { + asset_needed_for_fees + } + } + /// Calculates what `local_querier` would be from the perspective of `destination`. fn to_querier( local_querier: Option, @@ -965,39 +975,17 @@ impl XcmExecutor { self.asset_used_for_fees, asset_needed_for_fees, ); let asset_to_pay_for_fees = - self.asset_used_for_fees.as_ref().unwrap_or(&asset_needed_for_fees.id); - let actual_asset_to_use_for_fees = - if asset_to_pay_for_fees != &asset_needed_for_fees.id { - // Get the correct amount of asset_to_pay_for_fees. - match Config::AssetExchanger::quote_exchange_price( - &(asset_to_pay_for_fees.clone(), 1u128).into(), - &asset_needed_for_fees, - false, - ) { - Some(necessary_amount) => - (asset_to_pay_for_fees.clone(), necessary_amount).into(), - None => { - tracing::trace!( - target: "xcm::take_fee", - ?asset_to_pay_for_fees, - "Could not convert fees", - ); - asset_needed_for_fees.clone() - }, - } - } else { - asset_needed_for_fees.clone() - }; + self.calculate_asset_for_delivery_fees(asset_needed_for_fees.clone()); // set aside fee to be charged by XcmSender let delivery_fee = - self.holding.saturating_take(actual_asset_to_use_for_fees.into()); + self.holding.saturating_take(asset_to_pay_for_fees.into()); tracing::trace!(target: "xcm::DepositReserveAsset", ?delivery_fee); Some(delivery_fee) } else { None }; - // now take assets to deposit (excluding transport_fee) - // TODO: We should be taking `assets - transport_fee` + // now take assets to deposit (excluding delivery_fee) + // TODO: We should be taking `assets - delivery_fee` let deposited = self.holding.saturating_take(assets); tracing::trace!(target: "xcm::DepositReserveAsset", ?deposited, "Assets except delivery fee"); for asset in deposited.assets_iter() {