Skip to content
Snippets Groups Projects
user avatar
Özgün Özerk authored
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](https://github.com/rust-lang/rust/issues/20400) and
[this](https://github.com/rust-lang/rfcs/pull/1672#issuecomment-262152934).

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: default avatarFrancisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: default avatarBranislav Kontur <bkontur@gmail.com>
Co-authored-by: default avatarAdrian Catangiu <adrian@parity.io>
Co-authored-by: command-bot <>
f8f70b37
Name Last commit Last update
..
constants
src
Cargo.toml
build.rs