Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

[XCM] Multiple FungiblesAdapters support + WeightTrader::buy_weight more accurate error #6739

Merged
merged 9 commits into from
Mar 2, 2023
Merged

[XCM] Multiple FungiblesAdapters support + WeightTrader::buy_weight more accurate error #6739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b5945ee
Change ParaId->Sibling for `SiblingParachainConvertsVia`
bkontur Feb 17, 2023
ea32b57
[XCM] Multiple `FungiblesAdapter`s support + `WeightTrader::buy_weigh…
bkontur Feb 17, 2023
9dc8504
Merge remote-tracking branch 'origin/master' into bko-xcm-fungible-ad…
Feb 22, 2023
a50366f
Added test for `ConvertedConcreteId` with `AsPrefixedGeneralIndex`
bkontur Feb 23, 2023
b1858f8
Solution 3. - new MatchedConvertedConcreteId with matching capabilities
bkontur Feb 24, 2023
b43f230
Review fixes
bkontur Feb 26, 2023
4d83269
Renamed `AssetNotFound` -> `AssetNotHandled`
bkontur Feb 28, 2023
66de9cb
Merge remote-tracking branch 'origin/master' into bko-xcm-fungible-ad…
Feb 28, 2023
b47f25b
Merge remote-tracking branch 'origin/master' into bko-xcm-fungible-ad…
Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
211 changes: 206 additions & 5 deletions xcm/xcm-builder/src/asset_conversion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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)?;
Expand All @@ -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)?;
Expand All @@ -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)?;
Expand All @@ -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)?;
Expand All @@ -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))
);
}
}
16 changes: 8 additions & 8 deletions xcm/xcm-builder/src/currency_adapter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand All @@ -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"),
}
}
Expand Down Expand Up @@ -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),
Expand All @@ -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),
Expand All @@ -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);
Expand All @@ -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)
Expand All @@ -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 =
Expand Down
11 changes: 6 additions & 5 deletions xcm/xcm-executor/src/traits/token_matching.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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"),
Expand All @@ -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)
}
}

Expand All @@ -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)
}
}
21 changes: 17 additions & 4 deletions xcm/xcm-executor/src/traits/weight.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

@KiChjang KiChjang Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code like this is why we should actually figure out a better error reporting system for XCM, such as using anyhow or failure, because IIRC both of these crates allow chaining of errors, so you may chain errors together and have the ability to look into errors and see what the root cause is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KiChjang
This buy_weight returns v3::XcmError, so we probably shouldnt modify it now, right?
Do you want to change error handling here or you mean it like future improvement, e.g. for v4?

If this change for buy_weight is blocking this PR, I can extract it to separate PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no changes are necessary, I was making a general comment actually, but is definitely something that we should look into soon (one of the new hires can look into this).

XcmError::TooExpensive
} else {
last_error.unwrap_or(XcmError::TooExpensive)
})
}

fn refund_weight(&mut self, weight: Weight) -> Option<MultiAsset> {
Expand Down
2 changes: 1 addition & 1 deletion xcm/xcm-simulator/example/src/parachain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ use xcm_executor::{
};

pub type SovereignAccountOf = (
SiblingParachainConvertsVia<ParaId, AccountId>,
SiblingParachainConvertsVia<Sibling, AccountId>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intended?

Copy link
Contributor Author

@bkontur bkontur Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, ParaId has different encoding then Sibling,
and e.g. everywhere in Cumulus there is used SiblingParachainConvertsVia<Sibling, and also everywhere ParaId is used by relaychain cfg like ChildParachainConvertsVia<ParaId,

I found it just by accident because of: https://github.com/paritytech/cumulus/pull/2167/files#diff-07287c62dd91150d975f7504bd14cc62d9889c44f7ba8ca44c30c03b05d5b071L389-R421 and I guess this was inspired by this example

Copy link
Member

@gavofyork gavofyork Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, ParaId has different encoding then Sibling,

It doesn't. But the change is ok - just slightly more explicit about the intent.

#[derive(
	Clone, Copy, Default, Encode, Decode, Eq, PartialEq, Ord, PartialOrd, RuntimeDebug, TypeInfo,
)]
pub struct Sibling(pub Id);

(Id is imported as ParaId)

AccountId32Aliases<RelayNetwork, AccountId>,
ParentIsPreset<AccountId>,
);
Expand Down