relax `XcmFeeToAccount` trait bound on `AccountId` #4959

ozgunozerk · 2024-07-05T21:39:13Z

Configuring FeeManager enforces the boundary Into<[u8; 32]> for the AccountId type.

Here is how it works currently:

Configuration:

    type FeeManager = XcmFeeManagerFromComponents<
        IsChildSystemParachain<primitives::Id>,
        XcmFeeToAccount<Self::AssetTransactor, AccountId, TreasuryAccount>,
    >;

XcmToFeeAccount struct:

/// A `HandleFee` implementation that simply deposits the fees into a specific on-chain
/// `ReceiverAccount`.
///
/// It reuses the `AssetTransactor` configured on the XCM executor to deposit fee assets. If
/// the `AssetTransactor` returns an error while calling `deposit_asset`, then a warning will be
/// logged and the fee burned.
pub struct XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount>(
	PhantomData<(AssetTransactor, AccountId, ReceiverAccount)>,
);

impl<
		AssetTransactor: TransactAsset,
		AccountId: Clone + Into<[u8; 32]>,
		ReceiverAccount: Get<AccountId>,
	> HandleFee for XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount>
{
	fn handle_fee(fee: Assets, context: Option<&XcmContext>, _reason: FeeReason) -> Assets {
		deposit_or_burn_fee::<AssetTransactor, _>(fee, context, ReceiverAccount::get());

		Assets::new()
	}
}

deposit_or_burn_fee() function:

/// Try to deposit the given fee in the specified account.
/// Burns the fee in case of a failure.
pub fn deposit_or_burn_fee<AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 32]>>(
	fee: Assets,
	context: Option<&XcmContext>,
	receiver: AccountId,
) {
	let dest = AccountId32 { network: None, id: receiver.into() }.into();
	for asset in fee.into_inner() {
		if let Err(e) = AssetTransactor::deposit_asset(&asset, &dest, context) {
			log::trace!(
				target: "xcm::fees",
				"`AssetTransactor::deposit_asset` returned error: {:?}. Burning fee: {:?}. \
				They might be burned.",
				e, asset,
			);
		}
	}
}

In order to use another AccountId type (for example, 20 byte addresses for compatibility with Ethereum or Bitcoin), one has to duplicate the code as the following (roughly changing every 32 to 20):

/// A `HandleFee` implementation that simply deposits the fees into a specific on-chain
/// `ReceiverAccount`.
///
/// It reuses the `AssetTransactor` configured on the XCM executor to deposit fee assets. If
/// the `AssetTransactor` returns an error while calling `deposit_asset`, then a warning will be
/// logged and the fee burned.
pub struct XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount>(
    PhantomData<(AssetTransactor, AccountId, ReceiverAccount)>,
);
impl<
        AssetTransactor: TransactAsset,
        AccountId: Clone + Into<[u8; 20]>,
        ReceiverAccount: Get<AccountId>,
    > HandleFee for XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount>
{
    fn handle_fee(fee: XcmAssets, context: Option<&XcmContext>, _reason: FeeReason) -> XcmAssets {
        deposit_or_burn_fee::<AssetTransactor, _>(fee, context, ReceiverAccount::get());

        XcmAssets::new()
    }
}

pub fn deposit_or_burn_fee<AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 20]>>(
    fee: XcmAssets,
    context: Option<&XcmContext>,
    receiver: AccountId,
) {
    let dest = AccountKey20 { network: None, key: receiver.into() }.into();
    for asset in fee.into_inner() {
        if let Err(e) = AssetTransactor::deposit_asset(&asset, &dest, context) {
            log::trace!(
                target: "xcm::fees",
                "`AssetTransactor::deposit_asset` returned error: {:?}. Burning fee: {:?}. \
                They might be burned.",
                e, asset,
            );
        }
    }
}

This results in code duplication, which can be avoided simply by relaxing the trait enforced by XcmFeeToAccount.

In this PR, I propose to introduce a new trait called IntoLocation to be able to express both Into<[u8; 32]> and Into<[u8; 20]> should be accepted (and every other AccountId type as long as they implement this trait).

Currently, deposit_or_burn_fee() function converts the receiver: AccountId to a location. I think converting an account to Location should not be the responsibility of deposit_or_burn_fee() function.

This trait also decouples the conversion of AccountId to Location, from deposit_or_burn_fee() function. And exposes IntoLocation trait. Thus, allowing everyone to come up with their AccountId type and make it compatible for configuring FeeManager.

Note 1: if there is a better file/location to put IntoLocation, I'm all ears

Note 2: making deposit_or_burn_fee or XcmToFeeAccount generic was not possible from what I understood, due to Rust currently do not support a way to express the generic should implement either trait A or trait B (since the compiler cannot guarantee they won't overlap). In this case, they are Into<[u8; 32]> and Into<[u8; 20]>.
See this and this.

Note 3: I should also submit a PR to frontier that implements IntoLocation for AccountId20 if this PR gets accepted.

Summary

this new trait:

decouples the conversion of AccountId to Location, from deposit_or_burn_fee() function
makes XcmFeeToAccount accept every possible AccountId type as long as they they implement IntoLocation
backwards compatible
keeps the API simple and clean while making it less restrictive

@franciscoaguirre and @gupnik are already aware of the issue, so tagging them here for visibility.

polkadot/xcm/xcm-builder/src/fee_handling.rs

github-actions · 2024-07-08T09:27:52Z

Review required! Latest push from author must always be reviewed

bkchr · 2024-07-08T13:44:45Z

polkadot/xcm/xcm-builder/src/fee_handling.rs

+
+/// Try to deposit the given fee in the specified account.
+/// Burns the fee in case of a failure.
+pub fn deposit_or_burn_xcm_fee<AssetTransactor: TransactAsset>(


Does this needs to be public?

Also the old implementation should call this one internally.

I just copy pasted the old one. Didn't change the visibility settings, assuming there was a reason behind them. I can make both of them (the new and the old one) private, no worries

Seems like this function (the old one) is exported in xcm-builder/src/lib.rs. It is also used in polkadot-sdk/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs.

So, I think we should keep both of them public, I'll export the new one, and put deprecated on the old one.

Removed the old function. Also replaced XcmFeeToAcocunt with SendXcmFeeToAccount across the repo. If these changes are not meant to be done by now, I can revert them.

polkadot/xcm/xcm-builder/src/fee_handling.rs

franciscoaguirre · 2024-07-09T08:20:39Z

You need to put #[allow(deprecated)] in xcm-builder/SRC/fee_handling.rs to get the CI to pass

ozgunozerk · 2024-07-16T12:06:44Z

Hi @franciscoaguirre, what are the next steps for this PR?

franciscoaguirre · 2024-07-16T12:20:03Z

Just some clippy errors but the PR is fine

polkadot/xcm/xcm-builder/src/fee_handling.rs

Co-authored-by: Branislav Kontur <[email protected]>

paritytech-cicd-pr · 2024-07-17T11:02:18Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6718587

acatangiu · 2024-07-17T15:40:34Z

The CI pipeline was cancelled due to failure one of the required jobs. Job name: cargo-clippy Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6718587

You still have this failing compilation ^

acatangiu · 2024-07-17T15:41:32Z

The CI pipeline was cancelled due to failure one of the required jobs. Job name: cargo-clippy Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6718587

You still have this failing compilation ^

You should be able to reproduce locally: cargo test -r -p pallet-xcm

ozgunozerk · 2024-07-17T17:12:01Z

You should be able to reproduce locally: cargo test -r -p pallet-xcm

Yes, sorry, I was just running cargo clippy to see if there is any issue.

The issue was due to From<Location> is not implemented for the type AccountId32. I've fixed the error by implementing it.

Note: now staging-xcm imports sp-runtime crate to implement From<AccountId32> for Location trait. This way, runtime developers won't have cast between AccountId32 and [u8; 32] to benefit from this new change.
The other solution was to cast into() to every AccountId32 instance in every runtime affected by this, and I believe it would be ugly.

ozgunozerk · 2024-07-18T12:20:29Z

@acatangiu clippy problem is fixed, also reverted the fmt changes introduced by format on save so that fmt workflow wouldn't fail. Should be good to go from what I can tell

acatangiu

Changes are good, you just have to make the CI happy - you can check the failed checks on their details button to find out what/why's failing.

polkadot/xcm/src/v4/location.rs

ozgunozerk · 2024-07-18T16:44:37Z

Changes are good, you just have to make the CI happy - you can check the failed checks on their details button to find out what/why's failing.

Yep, I know. As I explained, my bad for previously thinking that cargo clippy should be good enough. And thanks for guiding me.

About semver check that is failing now:

error[E0658]: `#[diagnostic]` attribute name space is experimental

this error seems a bit cryptic. It refers to serdes source files. Any idea?

acatangiu · 2024-07-18T17:00:48Z

Changes are good, you just have to make the CI happy - you can check the failed checks on their details button to find out what/why's failing.

Yep, I know. As I explained, my bad for previously thinking that cargo clippy should be good enough. And thanks for guiding me.

About semver check that is failing now:
error[E0658]: `#[diagnostic]` attribute name space is experimental
this error seems a bit cryptic. It refers to serdes source files. Any idea?

this failure is unrelated probably coming from some upstream serde change - it is currently failing on all PRs...

you can ignore this one, we'll fix it at CI level. you can check the others

ozgunozerk · 2024-07-18T17:14:59Z

Fixed the docs related failure. Aside from 5 pending ones from the previous check, all the others seem to be passing 🎉

@franciscoaguirre

Fixes paritytech#4960 Configuring `FeeManager` enforces the boundary `Into<[u8; 32]>` for the `AccountId` type. Here is how it works currently: Configuration: ```rust type FeeManager = XcmFeeManagerFromComponents< IsChildSystemParachain<primitives::Id>, XcmFeeToAccount<Self::AssetTransactor, AccountId, TreasuryAccount>, >; ``` `XcmToFeeAccount` struct: ```rust /// A `HandleFee` implementation that simply deposits the fees into a specific on-chain /// `ReceiverAccount`. /// /// It reuses the `AssetTransactor` configured on the XCM executor to deposit fee assets. If /// the `AssetTransactor` returns an error while calling `deposit_asset`, then a warning will be /// logged and the fee burned. pub struct XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount>( PhantomData<(AssetTransactor, AccountId, ReceiverAccount)>, ); impl< AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 32]>, ReceiverAccount: Get<AccountId>, > HandleFee for XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount> { fn handle_fee(fee: Assets, context: Option<&XcmContext>, _reason: FeeReason) -> Assets { deposit_or_burn_fee::<AssetTransactor, _>(fee, context, ReceiverAccount::get()); Assets::new() } } ``` `deposit_or_burn_fee()` function: ```rust /// Try to deposit the given fee in the specified account. /// Burns the fee in case of a failure. pub fn deposit_or_burn_fee<AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 32]>>( fee: Assets, context: Option<&XcmContext>, receiver: AccountId, ) { let dest = AccountId32 { network: None, id: receiver.into() }.into(); for asset in fee.into_inner() { if let Err(e) = AssetTransactor::deposit_asset(&asset, &dest, context) { log::trace!( target: "xcm::fees", "`AssetTransactor::deposit_asset` returned error: {:?}. Burning fee: {:?}. \ They might be burned.", e, asset, ); } } } ``` --- In order to use **another** `AccountId` type (for example, 20 byte addresses for compatibility with Ethereum or Bitcoin), one has to duplicate the code as the following (roughly changing every `32` to `20`): ```rust /// A `HandleFee` implementation that simply deposits the fees into a specific on-chain /// `ReceiverAccount`. /// /// It reuses the `AssetTransactor` configured on the XCM executor to deposit fee assets. If /// the `AssetTransactor` returns an error while calling `deposit_asset`, then a warning will be /// logged and the fee burned. pub struct XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount>( PhantomData<(AssetTransactor, AccountId, ReceiverAccount)>, ); impl< AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 20]>, ReceiverAccount: Get<AccountId>, > HandleFee for XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount> { fn handle_fee(fee: XcmAssets, context: Option<&XcmContext>, _reason: FeeReason) -> XcmAssets { deposit_or_burn_fee::<AssetTransactor, _>(fee, context, ReceiverAccount::get()); XcmAssets::new() } } pub fn deposit_or_burn_fee<AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 20]>>( fee: XcmAssets, context: Option<&XcmContext>, receiver: AccountId, ) { let dest = AccountKey20 { network: None, key: receiver.into() }.into(); for asset in fee.into_inner() { if let Err(e) = AssetTransactor::deposit_asset(&asset, &dest, context) { log::trace!( target: "xcm::fees", "`AssetTransactor::deposit_asset` returned error: {:?}. Burning fee: {:?}. \ They might be burned.", e, asset, ); } } } ``` --- This results in code duplication, which can be avoided simply by relaxing the trait enforced by `XcmFeeToAccount`. In this PR, I propose to introduce a new trait called `IntoLocation` to be able to express both `Into<[u8; 32]>` and `Into<[u8; 20]>` should be accepted (and every other `AccountId` type as long as they implement this trait). Currently, `deposit_or_burn_fee()` function converts the `receiver: AccountId` to a location. I think converting an account to `Location` should not be the responsibility of `deposit_or_burn_fee()` function. This trait also decouples the conversion of `AccountId` to `Location`, from `deposit_or_burn_fee()` function. And exposes `IntoLocation` trait. Thus, allowing everyone to come up with their `AccountId` type and make it compatible for configuring `FeeManager`. --- Note 1: if there is a better file/location to put `IntoLocation`, I'm all ears Note 2: making `deposit_or_burn_fee` or `XcmToFeeAccount` generic was not possible from what I understood, due to Rust currently do not support a way to express the generic should implement either `trait A` or `trait B` (since the compiler cannot guarantee they won't overlap). In this case, they are `Into<[u8; 32]>` and `Into<[u8; 20]>`. See [this](rust-lang/rust#20400) and [this](rust-lang/rfcs#1672 (comment)). Note 3: I should also submit a PR to `frontier` that implements `IntoLocation` for `AccountId20` if this PR gets accepted. ### Summary this new trait: - decouples the conversion of `AccountId` to `Location`, from `deposit_or_burn_fee()` function - makes `XcmFeeToAccount` accept every possible `AccountId` type as long as they they implement `IntoLocation` - backwards compatible - keeps the API simple and clean while making it less restrictive @franciscoaguirre and @gupnik are already aware of the issue, so tagging them here for visibility. --------- Co-authored-by: Francisco Aguirre <[email protected]> Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: Adrian Catangiu <[email protected]> Co-authored-by: command-bot <>

make xcmfeetoaccount generic over account type

1a10642

ozgunozerk requested a review from a team as a code owner July 5, 2024 21:39

ozgunozerk changed the title ~~make XcmFeeToAccount generic over account type~~ relax XcmFeeToAccount trait bound on AccountId Jul 5, 2024

ozgunozerk mentioned this pull request Jul 5, 2024

Make XcmFeeToAccount less restrictive #4960

Closed

2 tasks

bkchr approved these changes Jul 6, 2024

View reviewed changes

polkadot/xcm/xcm-builder/src/fee_handling.rs Outdated Show resolved Hide resolved

reverse IntoLocation, and create new type instead

fc29435

github-actions bot requested a review from bkchr July 8, 2024 09:27

bkchr approved these changes Jul 8, 2024

View reviewed changes

bkchr requested a review from franciscoaguirre July 8, 2024 13:46

bkchr added the T6-XCM This PR/Issue is related to XCM. label Jul 8, 2024

ozgunozerk added 2 commits July 8, 2024 17:30

suggestions addressed

9a0fcc6

suggestions addressed

5945c0a

github-actions bot requested a review from bkchr July 8, 2024 14:30

franciscoaguirre approved these changes Jul 8, 2024

View reviewed changes

polkadot/xcm/xcm-builder/src/fee_handling.rs Show resolved Hide resolved

Merge branch 'master' into ozgun-into-location

5f28260

ozgunozerk mentioned this pull request Jul 9, 2024

Deprecation tracking issue for XcmFeeToAccount #4979

Open

7 tasks

deprecation time added

87c0935

github-actions bot requested a review from franciscoaguirre July 9, 2024 06:25

ci fix

437547b

franciscoaguirre approved these changes Jul 16, 2024

View reviewed changes

bkontur approved these changes Jul 16, 2024

View reviewed changes

polkadot/xcm/xcm-builder/src/fee_handling.rs Outdated Show resolved Hide resolved

Update polkadot/xcm/xcm-builder/src/fee_handling.rs

d272cbe

Co-authored-by: Branislav Kontur <[email protected]>

github-actions bot requested review from bkontur and franciscoaguirre July 16, 2024 13:47

import fix

1425c60

bkontur approved these changes Jul 16, 2024

View reviewed changes

suggestion

9960530

github-actions bot requested a review from acatangiu July 17, 2024 15:25

Merge branch 'master' into ozgun-into-location

a9f5915

ozgunozerk added 2 commits July 17, 2024 20:06

impl From<AccountId32> for Location

8a58ec6

prdoc update

5a9337d

fmt changes reverted

dc10342

acatangiu approved these changes Jul 18, 2024

View reviewed changes

polkadot/xcm/src/v4/location.rs Outdated Show resolved Hide resolved

polkadot/xcm/src/v4/location.rs Outdated Show resolved Hide resolved

polkadot/xcm/src/v4/location.rs Outdated Show resolved Hide resolved

polkadot/xcm/src/v4/location.rs Outdated Show resolved Hide resolved

acatangiu added 2 commits July 18, 2024 19:32

Apply suggestions from code review

99104b6

Merge branch 'master' into ozgun-into-location

1afb530

remove docs since we already implemented the trait

14c55df

github-actions bot requested a review from acatangiu July 18, 2024 17:10

Merge branch 'master' into ozgun-into-location

5d77ef3

acatangiu enabled auto-merge July 19, 2024 10:34

acatangiu added this pull request to the merge queue Jul 19, 2024

Merged via the queue into paritytech:master with commit f8f70b3 Jul 19, 2024
156 of 160 checks passed

This was referenced Jul 21, 2024

Implement From<AccountId20> for Location polkadot-evm/frontier#1471

Closed

🎁 [Feature Request]: Get rid of duplicated code for AccountId20 compatibility OpenZeppelin/polkadot-runtime-templates#258

Open

ggwpez mentioned this pull request Sep 9, 2024

Parent issue for stable2409 LTS release #5583

Closed

github-actions bot mentioned this pull request Oct 8, 2024

Update polkadot-sdk from stable2407 to stable2409 moonbeam-foundation/moonbeam#2994

Open

ozgunozerk mentioned this pull request Oct 24, 2024

re-export XcmFeeToAccount #6210

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

relax `XcmFeeToAccount` trait bound on `AccountId` #4959

relax `XcmFeeToAccount` trait bound on `AccountId` #4959

ozgunozerk commented Jul 5, 2024 •

edited

Loading

github-actions bot commented Jul 8, 2024

bkchr Jul 8, 2024

bkchr Jul 8, 2024

ozgunozerk Jul 8, 2024

ozgunozerk Jul 8, 2024

ozgunozerk Jul 8, 2024 •

edited

Loading

franciscoaguirre commented Jul 9, 2024

ozgunozerk commented Jul 16, 2024

franciscoaguirre commented Jul 16, 2024

paritytech-cicd-pr commented Jul 17, 2024

acatangiu commented Jul 17, 2024

acatangiu commented Jul 17, 2024

ozgunozerk commented Jul 17, 2024 •

edited

Loading

ozgunozerk commented Jul 18, 2024

acatangiu left a comment

ozgunozerk commented Jul 18, 2024

acatangiu commented Jul 18, 2024 •

edited

Loading

ozgunozerk commented Jul 18, 2024 •

edited

Loading

relax XcmFeeToAccount trait bound on AccountId #4959

relax XcmFeeToAccount trait bound on AccountId #4959

Conversation

ozgunozerk commented Jul 5, 2024 • edited Loading

Summary

github-actions bot commented Jul 8, 2024

bkchr Jul 8, 2024

Choose a reason for hiding this comment

bkchr Jul 8, 2024

Choose a reason for hiding this comment

ozgunozerk Jul 8, 2024

Choose a reason for hiding this comment

ozgunozerk Jul 8, 2024

Choose a reason for hiding this comment

ozgunozerk Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

franciscoaguirre commented Jul 9, 2024

ozgunozerk commented Jul 16, 2024

franciscoaguirre commented Jul 16, 2024

paritytech-cicd-pr commented Jul 17, 2024

acatangiu commented Jul 17, 2024

acatangiu commented Jul 17, 2024

ozgunozerk commented Jul 17, 2024 • edited Loading

ozgunozerk commented Jul 18, 2024

acatangiu left a comment

Choose a reason for hiding this comment

ozgunozerk commented Jul 18, 2024

acatangiu commented Jul 18, 2024 • edited Loading

ozgunozerk commented Jul 18, 2024 • edited Loading

relax `XcmFeeToAccount` trait bound on `AccountId` #4959

relax `XcmFeeToAccount` trait bound on `AccountId` #4959

ozgunozerk commented Jul 5, 2024 •

edited

Loading

ozgunozerk Jul 8, 2024 •

edited

Loading

ozgunozerk commented Jul 17, 2024 •

edited

Loading

acatangiu commented Jul 18, 2024 •

edited

Loading

ozgunozerk commented Jul 18, 2024 •

edited

Loading