Skip to content
This repository has been 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

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Feb 17, 2023

Motivation:
Allow AssetTransactor to work with multiple FungiblesAdapter for multiple pallet_assets instances. E.g. in Statemine/t we need to have multiple instances of pallet_assets (TrustBackedAssets + ForeignAssets):
pub type AssetTransactors = (CurrencyTransactor, FungiblesTransactor, ForeignFungiblesTransactor);
e.g. https://github.com/paritytech/cumulus/pull/2167/files#diff-07287c62dd91150d975f7504bd14cc62d9889c44f7ba8ca44c30c03b05d5b071R141

Problem:
Combination (FungiblesAdapter<Assets1, ConvertedConcreteId<..>, ...>, FungiblesAdapter<Assets2, ConvertedConcreteId<..>, ...>) with ConvertedConcreteId for ConvertAssetId returns AssetIdConversionFailed (which is mapped to XcmError::FailedToTransactAsset("AssetIdConversionFailed")) does not work.
Because [impl TransactAsset for Tuple {] does continuation only for cases: Err(XcmError::AssetNotFound) | Err(XcmError::Unimplemented) => (),
so if first [(FungiblesAdapter<Assets1, ConvertedConcreteId<..>, ...>] fails on [AssetIdConversionFailed], then we dont give chance to other Adapter/FungiblesAdapter in tuple chain
so it is not possible to have multiple pallet_assets instances with its own FungiblesAdapter.

For example 1 CurrencyAdapter works in TransactAsset tuple chain, because it does Matcher::matches_fungible(what).ok_or(Error::AssetNotFound)?
For example 2 Cumulus's TakeFirstAssetTrader https://github.com/paritytech/cumulus/blob/master/primitives/utility/src/lib.rs#L168 does the same: Matcher::matches_fungibles(&first).map_err(|_| XcmError::AssetNotFound)?;

So, as a temporary hack to pass, I created wrapping matcher which converts MatcherError::AssetIdConversionFailed -> MatcherError::AssetNotFound
https://github.com/paritytech/cumulus/pull/2167/files#diff-aa9ec9164c4412c613444721bada9490dd697084f621658cba0a8db2ae360a13R33-R46
https://github.com/paritytech/cumulus/pull/2167/files#diff-07287c62dd91150d975f7504bd14cc62d9889c44f7ba8ca44c30c03b05d5b071L94-R107

Possible solutions:

  1. simpliest - proposed in this PR - just to change mapping Error::AssetIdConversionFailed => XcmError::AssetNotFound
  2. change impl ConvertedConcreteId here
  • MatchError::AssetIdConversionFailed -> MatchError::AssetNotFound
  • potentialy, then we dont need AssetIdConversionFailed
  • and all Matcher::matches_fungible will be aligned and mapped to the AssetNotFound
  1. write new ConvertedConcreteId with fix from 2.
  2. extend TransactAsset tuple hanlding with XcmError::FailedToTransactAsset("AssetIdConversionFailed"), but this does not seem a good solution
  3. other solutions?

@bkontur bkontur added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. T6-XCM This PR/Issue is related to XCM. labels Feb 17, 2023
@bkontur
Copy link
Contributor Author

bkontur commented Feb 22, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased


// 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

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).

@bkontur
Copy link
Contributor Author

bkontur commented Feb 28, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@@ -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

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

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)

Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Looks good to me

@bkontur
Copy link
Contributor Author

bkontur commented Mar 2, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkontur bkontur merged commit 3dd8331 into master Mar 2, 2023
@bkontur bkontur deleted the bko-xcm-fungible-adapter-weight-trader branch March 2, 2023 15:50
ordian added a commit that referenced this pull request Mar 7, 2023
* master: (27 commits)
  bump `zombienet` version to v1.3.37 (#6773)
  Bump `blake2b_simd` to 1.0.1 (#6829)
  changelog: update template for new label behavior (E3/E4) (#6804)
  Companion for paritytech/substrate#12828 (#6380)
  Don't send `ActiveLeaves` from leaves in db on startup in Overseer (#6727)
  Polkadot XCM Body constants (#6788)
  Decrease expected peer count in zombinenet tests (#6826)
  Additional tracing in `provisioner`, `vote_selection` and `dispute-coordinator` (#6775)
  Change node-key for bootnodes (#6772)
  Change handle_import_statements to FatalResult (#6820)
  Introduce XCM matcher for writing barriers (#6756)
  Freeze note on `SessionInfo`. (#6818)
  Bump parity-db (#6816)
  Removing Outdated References to Misbehavior Arbitration Subsystem (#6814)
  Forgotten re-export for `MatchedConvertedConcreteId` (#6815)
  Companion for substrate#13509: bump API versions of {Beefy,Mmr}Api (#6809)
  Migrate to `Weight::from_parts` (#6794)
  [XCM] Multiple `FungiblesAdapter`s support + `WeightTrader::buy_weight` more accurate error (#6739)
  Get rid of unnecessary cloning and work. (#6808)
  changelog: fix migration listing (#6806)
  ...
ordian added a commit that referenced this pull request Mar 7, 2023
* master: (27 commits)
  bump `zombienet` version to v1.3.37 (#6773)
  Bump `blake2b_simd` to 1.0.1 (#6829)
  changelog: update template for new label behavior (E3/E4) (#6804)
  Companion for paritytech/substrate#12828 (#6380)
  Don't send `ActiveLeaves` from leaves in db on startup in Overseer (#6727)
  Polkadot XCM Body constants (#6788)
  Decrease expected peer count in zombinenet tests (#6826)
  Additional tracing in `provisioner`, `vote_selection` and `dispute-coordinator` (#6775)
  Change node-key for bootnodes (#6772)
  Change handle_import_statements to FatalResult (#6820)
  Introduce XCM matcher for writing barriers (#6756)
  Freeze note on `SessionInfo`. (#6818)
  Bump parity-db (#6816)
  Removing Outdated References to Misbehavior Arbitration Subsystem (#6814)
  Forgotten re-export for `MatchedConvertedConcreteId` (#6815)
  Companion for substrate#13509: bump API versions of {Beefy,Mmr}Api (#6809)
  Migrate to `Weight::from_parts` (#6794)
  [XCM] Multiple `FungiblesAdapter`s support + `WeightTrader::buy_weight` more accurate error (#6739)
  Get rid of unnecessary cloning and work. (#6808)
  changelog: fix migration listing (#6806)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants