Skip to content
Snippets Groups Projects
  • Özgün Özerk's avatar
    relax `XcmFeeToAccount` trait bound on `AccountId` (#4959) · f8f70b37
    Ö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 <>
    Unverified
    f8f70b37
Code owners
Assign users and groups as approvers for specific file changes. Learn more.