-
Ö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:
Francisco Aguirre <franciscoaguirreperez@gmail.com> Co-authored-by:
Branislav Kontur <bkontur@gmail.com> Co-authored-by:
Adrian Catangiu <adrian@parity.io> Co-authored-by: command-bot <>
Unverifiedf8f70b37
Code owners
Assign users and groups as approvers for specific file changes. Learn more.