Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make XcmFeeToAccount less restrictive #4960

Closed
2 tasks done
ozgunozerk opened this issue Jul 5, 2024 · 0 comments · Fixed by #4959
Closed
2 tasks done

Make XcmFeeToAccount less restrictive #4960

ozgunozerk opened this issue Jul 5, 2024 · 0 comments · Fixed by #4959
Labels
I5-enhancement An additional feature request. I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@ozgunozerk
Copy link
Contributor

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Motivation

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.

Request

Relax the trait enforced by XcmFeeToAccount.

Solution

Submitted a #4959

Are you willing to help with this request?

Yes!

@ozgunozerk ozgunozerk added the I5-enhancement An additional feature request. label Jul 5, 2024
@github-actions github-actions bot added the I10-unconfirmed Issue might be valid, but it's not yet known. label Jul 5, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 19, 2024
Fixes #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 <>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
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 <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant