-
Notifications
You must be signed in to change notification settings - Fork 796
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
Labels
I5-enhancement
An additional feature request.
I10-unconfirmed
Issue might be valid, but it's not yet known.
Comments
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.
Is there an existing issue?
Experiencing problems? Have you tried our Stack Exchange first?
Motivation
Configuring
FeeManager
enforces the boundaryInto<[u8; 32]>
for theAccountId
type.Here is how it works currently:
Configuration:
XcmToFeeAccount
struct:deposit_or_burn_fee()
function: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 every32
to20
):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!
The text was updated successfully, but these errors were encountered: