-
Notifications
You must be signed in to change notification settings - Fork 1.2k
update XcmPaymentApi query_delivery_fees to return the fee based on V… #8297
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,7 +101,7 @@ use assets_common::{ | |
| use polkadot_runtime_common::{BlockHashCount, SlowAdjustingFeeUpdate}; | ||
| use weights::{BlockExecutionWeight, ExtrinsicBaseWeight, RocksDbWeight}; | ||
| use xcm::{ | ||
| latest::prelude::AssetId, | ||
| latest::prelude::{AssetId, Asset, Assets as XcmAssets, Fungible}, | ||
| prelude::{VersionedAsset, VersionedAssetId, VersionedAssets, VersionedLocation, VersionedXcm}, | ||
| Version as XcmVersion, | ||
| }; | ||
|
|
@@ -111,7 +111,7 @@ use frame_support::traits::PalletInfoAccess; | |
|
|
||
| #[cfg(feature = "runtime-benchmarks")] | ||
| use xcm::latest::prelude::{ | ||
| Asset, Assets as XcmAssets, Fungible, Here, InteriorLocation, Junction, Junction::*, Location, | ||
| Here, InteriorLocation, Junction, Junction::*, Location, | ||
| NetworkId, NonFungible, Parent, ParentThen, Response, XCM_VERSION, | ||
| }; | ||
|
|
||
|
|
@@ -1725,8 +1725,49 @@ impl_runtime_apis! { | |
| PolkadotXcm::query_xcm_weight(message) | ||
| } | ||
|
|
||
| fn query_delivery_fees(destination: VersionedLocation, message: VersionedXcm<()>) -> Result<VersionedAssets, XcmPaymentApiError> { | ||
| PolkadotXcm::query_delivery_fees(destination, message) | ||
| fn query_delivery_fees(destination: VersionedLocation, message: VersionedXcm<()>, asset: VersionedAssetId) -> Result<VersionedAssets, XcmPaymentApiError> { | ||
| let fee_in_native = PolkadotXcm::query_delivery_fees(destination, message)?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rather modify the helper
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this, I would need to add a new dependency, right? I added it into the implementation of For the Anyway, I’m going to leave this decision to you. Also, let me know if I got all wrong.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The runtime decides how to implement this, but we've seen in #8281 an example of using an existing configuration item to move more things into the helper function and make these APIs simpler to implement for teams |
||
| let asset_id: Result<AssetId, ()> = asset.clone().try_into(); | ||
| let native_asset = xcm_config::WestendLocation::get(); | ||
| match asset_id { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for adding this new functionality. I've noticed that the tests currently cover only the native asset branch. Would you mind adding tests for the remaining branches to ensure comprehensive coverage?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the review! I didn't add test, because I'm waiting for @franciscoaguirre response about where the conversion should be implement. If you have any directions on this would be great, should I keep it where it is or should I move it to pallet-xcm |
||
| // For native asset there is no need to convert. | ||
| Ok(asset_id) if asset_id.0 == native_asset => { | ||
| Ok(fee_in_native) | ||
| }, | ||
| Ok(asset_id) => { | ||
| let fee: XcmAssets = fee_in_native.clone().try_into().map_err(|()| { | ||
| log::trace!(target: "xcm::xcm_runtime_apis", "query_delivery_fees - failed to convert fee: {fee_in_native:?}!"); | ||
| XcmPaymentApiError::VersionedConversionFailed | ||
| })?; | ||
|
|
||
| // Convert Fee to Fungible | ||
| let Fungible(balance) = fee.inner()[0].fun else { | ||
| unreachable!("fee is fungible"); | ||
| }; | ||
|
|
||
| // Try to get current price of `asset_id` in `native_asset`. | ||
| if let Ok(Some(swapped_in_native)) = assets_common::PoolAdapter::<Runtime>::quote_price_tokens_for_exact_tokens( | ||
| asset_id.0.clone(), | ||
| native_asset, | ||
| balance, | ||
| true, | ||
| ) { | ||
| // convert Balance to VersionedAssets | ||
| let converted_asset = Asset { | ||
| id: asset_id, | ||
| fun: Fungible(swapped_in_native), | ||
| }; | ||
| Ok(vec![converted_asset].into()) | ||
| } else { | ||
| log::trace!(target: "xcm::xcm_runtime_apis", "query_delivery_fees - unhandled asset_id: {asset_id:?}!"); | ||
| Err(XcmPaymentApiError::AssetNotFound) | ||
| } | ||
| }, | ||
| Err(_) => { | ||
| log::trace!(target: "xcm::xcm_runtime_apis", "query_delivery_fees - failed to convert asset: {asset:?}!"); | ||
| Err(XcmPaymentApiError::VersionedConversionFailed) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -835,7 +835,7 @@ impl_runtime_apis! { | |
| PolkadotXcm::query_xcm_weight(message) | ||
| } | ||
|
|
||
| fn query_delivery_fees(destination: VersionedLocation, message: VersionedXcm<()>) -> Result<VersionedAssets, XcmPaymentApiError> { | ||
| fn query_delivery_fees(destination: VersionedLocation, message: VersionedXcm<()>, _: VersionedAssetId) -> Result<VersionedAssets, XcmPaymentApiError> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even though bridge hub doesn't support multiple assets for fee payment, I would still throw an error if the asset is not the expected one ( |
||
| PolkadotXcm::query_delivery_fees(destination, message) | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ sp_api::decl_runtime_apis! { | |
| /// | ||
| /// To determine the execution weight of the calls required for | ||
| /// [`xcm::latest::Instruction::Transact`] instruction, `TransactionPaymentCallApi` can be used. | ||
| #[api_version(2)] | ||
| pub trait XcmPaymentApi { | ||
| /// Returns a list of acceptable payment assets. | ||
| /// | ||
|
|
@@ -58,14 +59,26 @@ sp_api::decl_runtime_apis! { | |
| fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, Error>; | ||
|
|
||
| /// Get delivery fees for sending a specific `message` to a `destination`. | ||
| /// These always come in a specific asset, defined by the chain. | ||
| /// These always come in a specific asset, defined by the chain. Version 1 | ||
| /// | ||
| /// # Arguments | ||
| /// * `message`: The message that'll be sent, necessary because most delivery fees are based on the | ||
| /// size of the message. | ||
| /// * `destination`: The destination to send the message to. Different destinations may use | ||
| /// different senders that charge different fees. | ||
| #[changed_in(2)] | ||
| fn query_delivery_fees(destination: VersionedLocation, message: VersionedXcm<()>) -> Result<VersionedAssets, Error>; | ||
|
|
||
| /// Get delivery fees for sending a specific `message` to a `destination`. | ||
| /// These always come in a specific asset, defined by the parameter `asset`. | ||
| /// | ||
| /// # Arguments | ||
| /// * `asset`: The asset to charge fees in. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move to end of argument list
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function signature currently is
This is why I left this as the first argument in the list instead of put it in at the end. What do you think? |
||
| /// * `message`: The message that'll be sent, necessary because most delivery fees are based on the | ||
| /// size of the message. | ||
| /// * `destination`: The destination to send the message to. Different destinations may use | ||
| /// different senders that charge different fees. | ||
| fn query_delivery_fees(destination: VersionedLocation, message: VersionedXcm<()>, asset: VersionedAssetId) -> Result<VersionedAssets, Error>; | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to use macro overloading to parametrise this macro too, e.g. #8392