From c5637bda913f7c4ed101855440ea1f14eac855c6 Mon Sep 17 00:00:00 2001
From: Branislav Kontur <bkontur@gmail.com>
Date: Thu, 2 Mar 2023 16:50:12 +0100
Subject: [PATCH] [XCM] Multiple `FungiblesAdapter`s support +
`WeightTrader::buy_weight` more accurate error (#6739)
* Change ParaId->Sibling for `SiblingParachainConvertsVia`
* [XCM] Multiple `FungiblesAdapter`s support + `WeightTrader::buy_weight` more accurate error
* Added test for `ConvertedConcreteId` with `AsPrefixedGeneralIndex`
* Solution 3. - new MatchedConvertedConcreteId with matching capabilities
* Review fixes
* Renamed `AssetNotFound` -> `AssetNotHandled`
---------
Co-authored-by: parity-processbot <>
---
.../xcm/xcm-builder/src/asset_conversion.rs | 211 +++++++++++++++++-
.../xcm/xcm-builder/src/currency_adapter.rs | 16 +-
.../xcm-executor/src/traits/token_matching.rs | 11 +-
.../xcm/xcm-executor/src/traits/weight.rs | 21 +-
.../xcm-simulator/example/src/parachain.rs | 2 +-
5 files changed, 238 insertions(+), 23 deletions(-)
diff --git a/polkadot/xcm/xcm-builder/src/asset_conversion.rs b/polkadot/xcm/xcm-builder/src/asset_conversion.rs
index 1a13f1ffb44..cadbb8381cf 100644
--- a/polkadot/xcm/xcm-builder/src/asset_conversion.rs
+++ b/polkadot/xcm/xcm-builder/src/asset_conversion.rs
@@ -16,7 +16,7 @@
//! Adapters to work with `frame_support::traits::tokens::fungibles` through XCM.
-use frame_support::traits::Get;
+use frame_support::traits::{Contains, Get};
use sp_std::{borrow::Borrow, marker::PhantomData, prelude::*, result};
use xcm::latest::prelude::*;
use xcm_executor::traits::{Convert, Error as MatchError, MatchesFungibles, MatchesNonFungibles};
@@ -69,7 +69,7 @@ impl<
fn matches_fungibles(a: &MultiAsset) -> result::Result<(AssetId, Balance), MatchError> {
let (amount, id) = match (&a.fun, &a.id) {
(Fungible(ref amount), Concrete(ref id)) => (amount, id),
- _ => return Err(MatchError::AssetNotFound),
+ _ => return Err(MatchError::AssetNotHandled),
};
let what =
ConvertAssetId::convert_ref(id).map_err(|_| MatchError::AssetIdConversionFailed)?;
@@ -89,7 +89,7 @@ impl<
fn matches_nonfungibles(a: &MultiAsset) -> result::Result<(ClassId, InstanceId), MatchError> {
let (instance, class) = match (&a.fun, &a.id) {
(NonFungible(ref instance), Concrete(ref class)) => (instance, class),
- _ => return Err(MatchError::AssetNotFound),
+ _ => return Err(MatchError::AssetNotHandled),
};
let what =
ConvertClassId::convert_ref(class).map_err(|_| MatchError::AssetIdConversionFailed)?;
@@ -113,7 +113,7 @@ impl<
fn matches_fungibles(a: &MultiAsset) -> result::Result<(AssetId, Balance), MatchError> {
let (amount, id) = match (&a.fun, &a.id) {
(Fungible(ref amount), Abstract(ref id)) => (amount, id),
- _ => return Err(MatchError::AssetNotFound),
+ _ => return Err(MatchError::AssetNotHandled),
};
let what =
ConvertAssetId::convert_ref(id).map_err(|_| MatchError::AssetIdConversionFailed)?;
@@ -133,7 +133,7 @@ impl<
fn matches_nonfungibles(a: &MultiAsset) -> result::Result<(ClassId, InstanceId), MatchError> {
let (instance, class) = match (&a.fun, &a.id) {
(NonFungible(ref instance), Abstract(ref class)) => (instance, class),
- _ => return Err(MatchError::AssetNotFound),
+ _ => return Err(MatchError::AssetNotHandled),
};
let what =
ConvertClassId::convert_ref(class).map_err(|_| MatchError::AssetIdConversionFailed)?;
@@ -147,3 +147,204 @@ impl<
pub type ConvertedConcreteAssetId<A, B, C, O> = ConvertedConcreteId<A, B, C, O>;
#[deprecated = "Use `ConvertedAbstractId` instead"]
pub type ConvertedAbstractAssetId<A, B, C, O> = ConvertedAbstractId<A, B, C, O>;
+
+pub struct MatchedConvertedConcreteId<AssetId, Balance, MatchAssetId, ConvertAssetId, ConvertOther>(
+ PhantomData<(AssetId, Balance, MatchAssetId, ConvertAssetId, ConvertOther)>,
+);
+impl<
+ AssetId: Clone,
+ Balance: Clone,
+ MatchAssetId: Contains<MultiLocation>,
+ ConvertAssetId: Convert<MultiLocation, AssetId>,
+ ConvertBalance: Convert<u128, Balance>,
+ > MatchesFungibles<AssetId, Balance>
+ for MatchedConvertedConcreteId<AssetId, Balance, MatchAssetId, ConvertAssetId, ConvertBalance>
+{
+ fn matches_fungibles(a: &MultiAsset) -> result::Result<(AssetId, Balance), MatchError> {
+ let (amount, id) = match (&a.fun, &a.id) {
+ (Fungible(ref amount), Concrete(ref id)) if MatchAssetId::contains(id) => (amount, id),
+ _ => return Err(MatchError::AssetNotHandled),
+ };
+ let what =
+ ConvertAssetId::convert_ref(id).map_err(|_| MatchError::AssetIdConversionFailed)?;
+ let amount = ConvertBalance::convert_ref(amount)
+ .map_err(|_| MatchError::AmountToBalanceConversionFailed)?;
+ Ok((what, amount))
+ }
+}
+impl<
+ ClassId: Clone,
+ InstanceId: Clone,
+ MatchClassId: Contains<MultiLocation>,
+ ConvertClassId: Convert<MultiLocation, ClassId>,
+ ConvertInstanceId: Convert<AssetInstance, InstanceId>,
+ > MatchesNonFungibles<ClassId, InstanceId>
+ for MatchedConvertedConcreteId<ClassId, InstanceId, MatchClassId, ConvertClassId, ConvertInstanceId>
+{
+ fn matches_nonfungibles(a: &MultiAsset) -> result::Result<(ClassId, InstanceId), MatchError> {
+ let (instance, class) = match (&a.fun, &a.id) {
+ (NonFungible(ref instance), Concrete(ref class)) if MatchClassId::contains(class) =>
+ (instance, class),
+ _ => return Err(MatchError::AssetNotHandled),
+ };
+ let what =
+ ConvertClassId::convert_ref(class).map_err(|_| MatchError::AssetIdConversionFailed)?;
+ let instance = ConvertInstanceId::convert_ref(instance)
+ .map_err(|_| MatchError::InstanceConversionFailed)?;
+ Ok((what, instance))
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ use xcm_executor::traits::JustTry;
+
+ struct OnlyParentZero;
+ impl Contains<MultiLocation> for OnlyParentZero {
+ fn contains(a: &MultiLocation) -> bool {
+ match a {
+ MultiLocation { parents: 0, .. } => true,
+ _ => false,
+ }
+ }
+ }
+
+ #[test]
+ fn matched_converted_concrete_id_for_fungibles_works() {
+ type AssetIdForTrustBackedAssets = u32;
+ type Balance = u128;
+ frame_support::parameter_types! {
+ pub TrustBackedAssetsPalletLocation: MultiLocation = PalletInstance(50).into();
+ }
+
+ // ConvertedConcreteId cfg
+ type Converter = MatchedConvertedConcreteId<
+ AssetIdForTrustBackedAssets,
+ Balance,
+ OnlyParentZero,
+ AsPrefixedGeneralIndex<
+ TrustBackedAssetsPalletLocation,
+ AssetIdForTrustBackedAssets,
+ JustTry,
+ >,
+ JustTry,
+ >;
+ assert_eq!(
+ TrustBackedAssetsPalletLocation::get(),
+ MultiLocation { parents: 0, interior: X1(PalletInstance(50)) }
+ );
+
+ // err - does not match
+ assert_eq!(
+ Converter::matches_fungibles(&MultiAsset {
+ id: Concrete(MultiLocation::new(1, X2(PalletInstance(50), GeneralIndex(1)))),
+ fun: Fungible(12345),
+ }),
+ Err(MatchError::AssetNotHandled)
+ );
+
+ // err - matches, but convert fails
+ assert_eq!(
+ Converter::matches_fungibles(&MultiAsset {
+ id: Concrete(MultiLocation::new(
+ 0,
+ X2(PalletInstance(50), GeneralKey { length: 1, data: [1; 32] })
+ )),
+ fun: Fungible(12345),
+ }),
+ Err(MatchError::AssetIdConversionFailed)
+ );
+
+ // err - matches, but NonFungible
+ assert_eq!(
+ Converter::matches_fungibles(&MultiAsset {
+ id: Concrete(MultiLocation::new(0, X2(PalletInstance(50), GeneralIndex(1)))),
+ fun: NonFungible(Index(54321)),
+ }),
+ Err(MatchError::AssetNotHandled)
+ );
+
+ // ok
+ assert_eq!(
+ Converter::matches_fungibles(&MultiAsset {
+ id: Concrete(MultiLocation::new(0, X2(PalletInstance(50), GeneralIndex(1)))),
+ fun: Fungible(12345),
+ }),
+ Ok((1, 12345))
+ );
+ }
+
+ #[test]
+ fn matched_converted_concrete_id_for_nonfungibles_works() {
+ type ClassId = u32;
+ type ClassInstanceId = u64;
+ frame_support::parameter_types! {
+ pub TrustBackedAssetsPalletLocation: MultiLocation = PalletInstance(50).into();
+ }
+
+ // ConvertedConcreteId cfg
+ struct ClassInstanceIdConverter;
+ impl Convert<AssetInstance, ClassInstanceId> for ClassInstanceIdConverter {
+ fn convert_ref(value: impl Borrow<AssetInstance>) -> Result<ClassInstanceId, ()> {
+ value.borrow().clone().try_into().map_err(|_| ())
+ }
+
+ fn reverse_ref(value: impl Borrow<ClassInstanceId>) -> Result<AssetInstance, ()> {
+ Ok(AssetInstance::from(value.borrow().clone()))
+ }
+ }
+
+ type Converter = MatchedConvertedConcreteId<
+ ClassId,
+ ClassInstanceId,
+ OnlyParentZero,
+ AsPrefixedGeneralIndex<TrustBackedAssetsPalletLocation, ClassId, JustTry>,
+ ClassInstanceIdConverter,
+ >;
+ assert_eq!(
+ TrustBackedAssetsPalletLocation::get(),
+ MultiLocation { parents: 0, interior: X1(PalletInstance(50)) }
+ );
+
+ // err - does not match
+ assert_eq!(
+ Converter::matches_nonfungibles(&MultiAsset {
+ id: Concrete(MultiLocation::new(1, X2(PalletInstance(50), GeneralIndex(1)))),
+ fun: NonFungible(Index(54321)),
+ }),
+ Err(MatchError::AssetNotHandled)
+ );
+
+ // err - matches, but convert fails
+ assert_eq!(
+ Converter::matches_nonfungibles(&MultiAsset {
+ id: Concrete(MultiLocation::new(
+ 0,
+ X2(PalletInstance(50), GeneralKey { length: 1, data: [1; 32] })
+ )),
+ fun: NonFungible(Index(54321)),
+ }),
+ Err(MatchError::AssetIdConversionFailed)
+ );
+
+ // err - matches, but Fungible vs NonFungible
+ assert_eq!(
+ Converter::matches_nonfungibles(&MultiAsset {
+ id: Concrete(MultiLocation::new(0, X2(PalletInstance(50), GeneralIndex(1)))),
+ fun: Fungible(12345),
+ }),
+ Err(MatchError::AssetNotHandled)
+ );
+
+ // ok
+ assert_eq!(
+ Converter::matches_nonfungibles(&MultiAsset {
+ id: Concrete(MultiLocation::new(0, X2(PalletInstance(50), GeneralIndex(1)))),
+ fun: NonFungible(Index(54321)),
+ }),
+ Ok((1, 54321))
+ );
+ }
+}
diff --git a/polkadot/xcm/xcm-builder/src/currency_adapter.rs b/polkadot/xcm/xcm-builder/src/currency_adapter.rs
index 2815d8bd3c4..bb7ac7a648d 100644
--- a/polkadot/xcm/xcm-builder/src/currency_adapter.rs
+++ b/polkadot/xcm/xcm-builder/src/currency_adapter.rs
@@ -28,8 +28,8 @@ use xcm_executor::{
/// Asset transaction errors.
enum Error {
- /// Asset not found.
- AssetNotFound,
+ /// The given asset is not handled. (According to [`XcmError::AssetNotFound`])
+ AssetNotHandled,
/// `MultiLocation` to `AccountId` conversion failed.
AccountIdConversionFailed,
}
@@ -38,7 +38,7 @@ impl From<Error> for XcmError {
fn from(e: Error) -> Self {
use XcmError::FailedToTransactAsset;
match e {
- Error::AssetNotFound => XcmError::AssetNotFound,
+ Error::AssetNotHandled => XcmError::AssetNotFound,
Error::AccountIdConversionFailed => FailedToTransactAsset("AccountIdConversionFailed"),
}
}
@@ -143,7 +143,7 @@ impl<
log::trace!(target: "xcm::currency_adapter", "can_check_in origin: {:?}, what: {:?}", _origin, what);
// Check we handle this asset.
let amount: Currency::Balance =
- Matcher::matches_fungible(what).ok_or(Error::AssetNotFound)?;
+ Matcher::matches_fungible(what).ok_or(Error::AssetNotHandled)?;
match CheckedAccount::get() {
Some((checked_account, MintLocation::Local)) =>
Self::can_reduce_checked(checked_account, amount),
@@ -168,7 +168,7 @@ impl<
fn can_check_out(_dest: &MultiLocation, what: &MultiAsset, _context: &XcmContext) -> Result {
log::trace!(target: "xcm::currency_adapter", "check_out dest: {:?}, what: {:?}", _dest, what);
- let amount = Matcher::matches_fungible(what).ok_or(Error::AssetNotFound)?;
+ let amount = Matcher::matches_fungible(what).ok_or(Error::AssetNotHandled)?;
match CheckedAccount::get() {
Some((checked_account, MintLocation::Local)) =>
Self::can_accrue_checked(checked_account, amount),
@@ -194,7 +194,7 @@ impl<
fn deposit_asset(what: &MultiAsset, who: &MultiLocation, _context: &XcmContext) -> Result {
log::trace!(target: "xcm::currency_adapter", "deposit_asset what: {:?}, who: {:?}", what, who);
// Check we handle this asset.
- let amount = Matcher::matches_fungible(&what).ok_or(Error::AssetNotFound)?;
+ let amount = Matcher::matches_fungible(&what).ok_or(Error::AssetNotHandled)?;
let who =
AccountIdConverter::convert_ref(who).map_err(|()| Error::AccountIdConversionFailed)?;
let _imbalance = Currency::deposit_creating(&who, amount);
@@ -208,7 +208,7 @@ impl<
) -> result::Result<Assets, XcmError> {
log::trace!(target: "xcm::currency_adapter", "withdraw_asset what: {:?}, who: {:?}", what, who);
// Check we handle this asset.
- let amount = Matcher::matches_fungible(what).ok_or(Error::AssetNotFound)?;
+ let amount = Matcher::matches_fungible(what).ok_or(Error::AssetNotHandled)?;
let who =
AccountIdConverter::convert_ref(who).map_err(|()| Error::AccountIdConversionFailed)?;
Currency::withdraw(&who, amount, WithdrawReasons::TRANSFER, AllowDeath)
@@ -223,7 +223,7 @@ impl<
_context: &XcmContext,
) -> 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 amount = Matcher::matches_fungible(asset).ok_or(Error::AssetNotHandled)?;
let from =
AccountIdConverter::convert_ref(from).map_err(|()| Error::AccountIdConversionFailed)?;
let to =
diff --git a/polkadot/xcm/xcm-executor/src/traits/token_matching.rs b/polkadot/xcm/xcm-executor/src/traits/token_matching.rs
index befff6b1b72..f1bbbe68025 100644
--- a/polkadot/xcm/xcm-executor/src/traits/token_matching.rs
+++ b/polkadot/xcm/xcm-executor/src/traits/token_matching.rs
@@ -48,9 +48,10 @@ impl<Instance> MatchesNonFungible<Instance> for Tuple {
}
/// Errors associated with [`MatchesFungibles`] operation.
+#[derive(Debug, PartialEq, Eq)]
pub enum Error {
- /// Asset not found.
- AssetNotFound,
+ /// The given asset is not handled. (According to [`XcmError::AssetNotFound`])
+ AssetNotHandled,
/// `MultiLocation` to `AccountId` conversion failed.
AccountIdConversionFailed,
/// `u128` amount to currency `Balance` conversion failed.
@@ -65,7 +66,7 @@ impl From<Error> for XcmError {
fn from(e: Error) -> Self {
use XcmError::FailedToTransactAsset;
match e {
- Error::AssetNotFound => XcmError::AssetNotFound,
+ Error::AssetNotHandled => XcmError::AssetNotFound,
Error::AccountIdConversionFailed => FailedToTransactAsset("AccountIdConversionFailed"),
Error::AmountToBalanceConversionFailed =>
FailedToTransactAsset("AmountToBalanceConversionFailed"),
@@ -86,7 +87,7 @@ impl<AssetId, Balance> MatchesFungibles<AssetId, Balance> for Tuple {
match Tuple::matches_fungibles(a) { o @ Ok(_) => return o, _ => () }
)* );
log::trace!(target: "xcm::matches_fungibles", "did not match fungibles asset: {:?}", &a);
- Err(Error::AssetNotFound)
+ Err(Error::AssetNotHandled)
}
}
@@ -101,6 +102,6 @@ impl<AssetId, Instance> MatchesNonFungibles<AssetId, Instance> for Tuple {
match Tuple::matches_nonfungibles(a) { o @ Ok(_) => return o, _ => () }
)* );
log::trace!(target: "xcm::matches_non_fungibles", "did not match fungibles asset: {:?}", &a);
- Err(Error::AssetNotFound)
+ Err(Error::AssetNotHandled)
}
}
diff --git a/polkadot/xcm/xcm-executor/src/traits/weight.rs b/polkadot/xcm/xcm-executor/src/traits/weight.rs
index 76f411e5c1f..6e111f93e80 100644
--- a/polkadot/xcm/xcm-executor/src/traits/weight.rs
+++ b/polkadot/xcm/xcm-executor/src/traits/weight.rs
@@ -67,16 +67,29 @@ impl WeightTrader for Tuple {
}
fn buy_weight(&mut self, weight: Weight, payment: Assets) -> Result<Assets, XcmError> {
+ let mut too_expensive_error_found = false;
let mut last_error = None;
for_tuples!( #(
match Tuple.buy_weight(weight, payment.clone()) {
Ok(assets) => return Ok(assets),
- Err(e) => { last_error = Some(e) }
+ Err(e) => {
+ if let XcmError::TooExpensive = e {
+ too_expensive_error_found = true;
+ }
+ last_error = Some(e)
+ }
}
)* );
- let last_error = last_error.unwrap_or(XcmError::TooExpensive);
- log::trace!(target: "xcm::buy_weight", "last_error: {:?}", last_error);
- Err(last_error)
+
+ log::trace!(target: "xcm::buy_weight", "last_error: {:?}, too_expensive_error_found: {}", last_error, too_expensive_error_found);
+
+ // if we have multiple traders, and first one returns `TooExpensive` and others fail e.g. `AssetNotFound`
+ // then it is more accurate to return `TooExpensive` then `AssetNotFound`
+ Err(if too_expensive_error_found {
+ XcmError::TooExpensive
+ } else {
+ last_error.unwrap_or(XcmError::TooExpensive)
+ })
}
fn refund_weight(&mut self, weight: Weight) -> Option<MultiAsset> {
diff --git a/polkadot/xcm/xcm-simulator/example/src/parachain.rs b/polkadot/xcm/xcm-simulator/example/src/parachain.rs
index 029771bce10..e2a5364e908 100644
--- a/polkadot/xcm/xcm-simulator/example/src/parachain.rs
+++ b/polkadot/xcm/xcm-simulator/example/src/parachain.rs
@@ -49,7 +49,7 @@ use xcm_executor::{
};
pub type SovereignAccountOf = (
- SiblingParachainConvertsVia<ParaId, AccountId>,
+ SiblingParachainConvertsVia<Sibling, AccountId>,
AccountId32Aliases<RelayNetwork, AccountId>,
ParentIsPreset<AccountId>,
);
--
GitLab