Too permissive XCM token matching inside tuples

[mrshiposha]

The current implementation of token matchers for tuples seems too permissive. When bundled in a tuple, the matchers should let the next matcher execute only on AssetNotFound or Unimplemented errors, as the Asset Transactors do, not on every error.

Otherwise, there could be a situation where some of the matchers fail with a prohibiting error, potentially polluting the storage in the process. However, the matching process won't halt and will fall through the rest of the matchers. Some of the remaining matchers could succeed, consequently triggering the Asset Transactor to do something. Yet, we have additional changes on-chain because of the first failed matcher.

This could lead to hard-to-debug errors.

Suggested change

I suggest changing the following implementations:

• MatchesFungibles
• MatchesNonFungibles

To something similar to this:

match Tuple::matches_fungibles(a) { Err(AssetNotFound | Unimplemented) => (), o => return o }

Inconsistent return types

Also, it feels inconsistent that Matches<Singular> (Fungible / NonFungible) and Matches<Plural> (Fungibles / NonFungibles) have different return types. The <Singular> matchers return an Option, while the <Plural> ones return a Result.
Maybe a Result return type for every matcher would be better?

[bkontur]

@mrshiposha Long time ago, I was fixing also something around this "fungible matching" stuff. I got your points, but let's find a better/simpler/generic solution here.

Just few of my thoughts for the beginning:

fn matches_fungible(a: &Asset) -> Option<Balance>;
vs 
fn matches_fungibles(a: &Asset) -> Result<(AssetId, Balance);

• they both match single asset a: &Asset and return single balance or (assetId, balance), so the plural matches_fungibles makes is kind of not that clear
• pallet_xcm has support for locking assets, but actually, only supports one asset e.g. native token - uses MatchesFungible, but there is an older issue for supporting multiple asssets
  • I guess we need here support for pallet_assets and freezing Define and implement Hold for Assets #4315 and Implement pallet-assets-freezer #3951
  • so the CurrencyMatcher should be changed MatchesFungible to MatchesFungibles
• even the native asset (pallet_balances) has known assetId from the XCM point of view
  • everywhere we need just MatchesFungible -> Option<Balance>, we should be able to use MatchesFungibles -> Result<(AssetId, Balance) and just ignore AssetId part

So before we start aligning/changing api/signatures, let's think about first if we really need both XCM traits (MatchesFungible, MatchesFungibles). I think that the XCM does not (and should not) make a difference between assets (native asset from pallet_balances vs assets from pallet_assets). The XCM should see (I mean just fungible) an asset as "tuple" (assetId=Location + balance amount).

I am (almost) sure, that just one trait (as a combination of both) should be enough:

pub trait MatchesFungible<AssetId, Balance> {
	fn matches_fungible(a: &Asset) -> result::Result<(AssetId, Balance), Error>;
}

@mrshiposha Thank you for raising this issue. Feel free to continue/contribute with a PR. I am happy to help you, but let's discuss and make a "plan" first. :)

[mrshiposha]

I am (almost) sure, that just one trait (as a combination of both) should be enough

I think so, too.

However, I think it'd be confusing if the adapter could ignore the AssetId (generic parameter) value.
Another thing that induces confusion is that we have two entities that we call AssetId:

1. XCM AssetId type
2. chain-specific AssetId (like the one in the fungibles::Inspect).
   The matcher's AssetId generic parameter is actually the second one, the chain-specific one. It's not apparent that the adapter will ignore it.
   Also, NFTs can naturally identified on-chain using only one ID, like Core Time NFTs or Aventus NFTs. Moreover, one chain can host different NFT solutions (pallets/contracts). Each of them could provide different NFT kinds: in-class or classless ones.
   Of course, within XCM, these things are identified using both XCM AssetId and the fungibility. However, Matchers convert the XCM identification to the chain-specific identification (which can differ from XCM's and usually is. Chains seldom use XCM IDs for internal operations).

Maybe we could define that single trait more generally?

pub trait MatchesAsset<Id> {
	fn matches_asset(a: &Asset) -> Result<Id, Error>;
}

It seems to me to be more general since we can always define the Id as a tuple.
It is consistent with both fungibles (as the Id can identify a concrete balance) and non-fungibles.

The adapters could use the trait like this:

• MatchesAsset<Fungible::Balance>
• MatchesAsset<(Assets::AssetId, Assets::Balance)>

For non-fungibles, it will be similar.

This would require changing the matcher trait implementations since they are generic, and we can't have several generic implementations of the same trait for the same type.
Yet, we can add wrapper types for things like IsConcrete and MatchedConvertedConcreteId.

This way, we can cover fungibles and non-fungibles with a uniform matcher trait.

For example:

// Wrapper types - they will be plugged into the adapters in the Runtime config.
pub struct MatchFungible<MatcherSettings>(PhantomData<MatcherSettings>);
pub struct MatchNonFungible<MatcherSettings>(PhantomData<MatcherSettings>);

// impl: IsConcrete
pub struct IsConcrete<T>(PhantomData<T>);
impl<T: Get<Location>, B: TryFrom<u128>> MatchesAsset<B> for MatchFungible<IsConcrete<B>> {
	fn matches_asset(a: &Asset) -> Result<B, Error> {
		match (&a.id, &a.fun) {
			(AssetId(ref id), Fungible(ref amount)) if id == &T::get() => {
				(*amount).try_into().map_err(|_| MatchError::AssetNotHandled)
			},
			_ => Err(Error::AssetNotHandled),
		}
	}
}

impl<T: Get<Location>, I: TryFrom<AssetInstance>> MatchesAsset<I> for MatchNonFungible<IsConcrete<I>> {
	fn matches_asset(a: &Asset) -> Result<I, Error> {
		match (&a.id, &a.fun) {
			(AssetId(id), NonFungible(instance)) if id == &T::get() => {
				(*instance).try_into().map_err(|_| MatchError::AssetNotHandled)
			},
			_ => Err(Error::AssetNotHandled),
		}
	}
}

// impl: ConvertedConcreteId
impl<
	AssetId: Clone,
	Balance: Clone,
	ConvertAssetId: MaybeEquivalence<Location, AssetId>,
	ConvertBalance: MaybeEquivalence<u128, Balance>,
> MatchesAsset<(AssetId, Balance)> for MatchFungible<ConvertedConcreteId<AssetId, Balance, ConvertAssetId, ConvertBalance>> {
	/* ...match fungibles... */
}

impl<
	ClassId: Clone,
	InstanceId: Clone,
	ConvertClassId: MaybeEquivalence<Location, ClassId>,
	ConvertInstanceId: MaybeEquivalence<AssetInstance, InstanceId>,
> MatchesAsset<(ClassId, InstanceId)>
	for MatchNonFungible<ConvertedConcreteId<ClassId, InstanceId, ConvertClassId, ConvertInstanceId>> {
	/* ...match non-fungibles... */
}