Asset hubs - Add all assets in pool with native to XcmPaymentApi #523
Conversation
franciscoaguirre
changed the title
There was a problem hiding this comment.
@franciscoaguirre please add these tests for all runtimes and verify the new APIs work properly
|
Bumping It would be best if we get #519 merged before this. I'll still add it to try to have as few conflicts as possible. |
| log::trace!(target: "xcm::xcm_runtime_apis", "query_weight_to_asset_fee - unhandled asset_id: {asset_id:?}!"); | ||
| Err(XcmPaymentApiError::AssetNotFound) | ||
| // 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( |
There was a problem hiding this comment.
This function has a pretty suboptimal signature. Way too easy to mix up arguments (not introduced here, just noticed that it makes it unnecessary hard to review correctness).
There was a problem hiding this comment.
Note: Not even function docs nor naming of parameters helps here in any way. What is asset1? What is asset2? I need to read the code of the trait implementation to figure this out.
There was a problem hiding this comment.
In what asset is the provided amount? Based on the implementation of the called get_amount_in, I am inferring that the amount is the amount we want to receive in the native token and that the first parameter is indeed the asset_in, but that seems to contradict the other code path. I am sure I am just holding it wrong, but the point is, I should not need to read a particular implementation of a trait function to check that the parameters have been passed correctly (because I see that they are too generic for the type checker to be able to catch that).
Note: This is by no means unique to this code, we have plenty of similar examples in the parachains/polkadot code base. I am just using this opportunity to raise awareness in a broader scope: If we improved here, our code quality and also our speed of reviews could be greatly enhanced as everything starts to become locally reasonable: I should not need to study implementation details of code that is not part of that PR to be able to infer that it is correct.
The
XcmPaymentApihas two functions,query_acceptable_payment_assetsandquery_weight_to_asset_fee, which allow clients to know which assets can be used to pay for fees.On the asset hubs fees can be paid both with the native asset (DOT, KSM) and with any asset in a liquidity pool with the native asset. However, these functions were only returning the native asset.
This PR adds all other assets that can be used for fee payment to this API.
Also fixed an issue where these APIs wouldn't accept any version other than latest.