update XcmPaymentApi query_delivery_fees to return the fee based on V…#8297
update XcmPaymentApi query_delivery_fees to return the fee based on V…#8297jpserrat wants to merge 2 commits intoparitytech:masterfrom
Conversation
…ersionedAssetId parameter
…rrat/query-delivery-fees-v2
| 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)?; |
There was a problem hiding this comment.
I would rather modify the helper query_delivery_fees in pallet-xcm so that other runtimes integrating this can just use that.
There was a problem hiding this comment.
For this, I would need to add a new dependency, right? I added it into the implementation of XcmPaymentApi due to this comment.
It would have been much simpler to add it in pallet-xcm, but from what I understand, we’re going to let the runtime decide how to implement this.
For the query_weight_to_asset_fee, we also do the conversion in the runtime. This way, we can keep things more consistent by leaving all the conversions to be implemented by the runtime (I’m assuming that’s the case—I don’t know all the implementations 🤣).
Anyway, I’m going to leave this decision to you. Also, let me know if I got all wrong.
There was a problem hiding this comment.
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
| } | ||
|
|
||
| fn query_delivery_fees(destination: VersionedLocation, message: VersionedXcm<()>) -> Result<VersionedAssets, XcmPaymentApiError> { | ||
| fn query_delivery_fees(destination: VersionedLocation, message: VersionedXcm<()>, _: VersionedAssetId) -> Result<VersionedAssets, XcmPaymentApiError> { |
There was a problem hiding this comment.
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 ({ parents: 1, interior: Here })
| /// These always come in a specific asset, defined by the parameter `asset`. | ||
| /// | ||
| /// # Arguments | ||
| /// * `asset`: The asset to charge fees in. |
There was a problem hiding this comment.
Move to end of argument list
There was a problem hiding this comment.
The function signature currently is (destination: VersionedLocation, message: VersionedXcm<()>, asset: VersionedAssetId). And in the argument list it looks like the order is starting from the last.
- asset
- message
- destination
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?
|
Thank you for contributing! 😁 Left some comments |
|
Thank you for all the comments Francisco! |
| let fee_in_native = PolkadotXcm::query_delivery_fees(destination, message)?; | ||
| let asset_id: Result<AssetId, ()> = asset.clone().try_into(); | ||
| let native_asset = xcm_config::WestendLocation::get(); | ||
| match asset_id { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| Runtime::query_delivery_fees( | ||
| destination_to_query.clone(), | ||
| remote_message.clone(), | ||
| VersionedAssetId::from(AssetId(Location::parent())) |
There was a problem hiding this comment.
You may want to use macro overloading to parametrise this macro too, e.g. #8392
|
I'm closing this in favor of #9963 |
|
I've just noticed that you responded Francisco. Thank you for the continuation, I'll take a look at what you did to learn a little bit more! Sorry for not finishing this. |
Adds an `asset_id` parameter to `XcmPaymentApi.query_delivery_fees` so fees can be estimated in assets other than the native one. Fixes #7061 Continuation of #8297 ## TODO - [x] Add parameter to runtime API - [x] Add parameter to pallet-xcm helper - [x] Test querying delivery fee in an asset other than native --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Adrian Catangiu <adrian@parity.io>
4 participants
Closes #7061
@acatangiu Two questions.
VersionedAssetIdwithout any use. Should I implement this for all of them?