Skip to content
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.

Sign up for GitHub

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

Asset hubs - Add all assets in pool with native to XcmPaymentApi #523

Merged
merged 18 commits into from
Jan 24, 2025
Merged

Asset hubs - Add all assets in pool with native to XcmPaymentApi #523

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
d3305c9
feat(XcmPaymentApi): add all assets in pool with native to runtime API
franciscoaguirre Jan 2, 2025
fb02724
Merge branch 'main' into acceptable-payment-assets-in-asset-hubs
franciscoaguirre Jan 7, 2025
339c39b
doc: modify CHANGELOG
franciscoaguirre Jan 7, 2025
5f6bdfd
Merge branch 'main' into acceptable-payment-assets-in-asset-hubs
franciscoaguirre Jan 9, 2025
ed18df7
Merge branch 'main' into acceptable-payment-assets-in-asset-hubs
franciscoaguirre Jan 13, 2025
61e64a1
fix: XcmPaymentApi not allowing any version other than latest
franciscoaguirre Jan 13, 2025
1425995
fix: XcmPaymentApi not allowing any version other than latest
franciscoaguirre Jan 13, 2025
3dbc837
Merge branch 'main' into acceptable-payment-assets-in-asset-hubs
franciscoaguirre Jan 13, 2025
79c404e
fix: wrong change in encointer
franciscoaguirre Jan 13, 2025
eb40c0a
test: add tests for xcm payment api in every runtime
franciscoaguirre Jan 14, 2025
7bf7cb6
fix: fmt
franciscoaguirre Jan 14, 2025
e72c9be
Merge branch 'main' into acceptable-payment-assets-in-asset-hubs
franciscoaguirre Jan 14, 2025
3edb5fd
Merge branch 'main' into acceptable-payment-assets-in-asset-hubs
franciscoaguirre Jan 16, 2025
a05f054
doc: bump
franciscoaguirre Jan 17, 2025
7dcf165
Merge branch 'main' into acceptable-payment-assets-in-asset-hubs
bkchr Jan 20, 2025
f58c28b
Merge branch 'main' into acceptable-payment-assets-in-asset-hubs
franciscoaguirre Jan 23, 2025
a16f702
Merge branch 'main' into acceptable-payment-assets-in-asset-hubs
franciscoaguirre Jan 24, 2025
df17977
Merge branch 'main' into acceptable-payment-assets-in-asset-hubs
franciscoaguirre Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
### Added

- Location conversion tests for relays and parachains ([polkadot-fellows/runtimes#487](https://github.com/polkadot-fellows/runtimes/pull/487))
- Asset Hubs: XcmPaymentApi now returns all assets in a pool with the native token as acceptable for fee payment ([polkadot-fellows/runtimes#523](https://github.com/polkadot-fellows/runtimes/pull/523))

- ParaRegistration proxy for Polkadot and Kusama ([polkadot-fellows/runtimes#520](https://github.com/polkadot-fellows/runtimes/pull/520))

Expand Down
17 changes: 9 additions & 8 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ asset-hub-kusama-runtime = { path = "system-parachains/asset-hubs/asset-hub-kusa
asset-hub-polkadot-emulated-chain = { path = "integration-tests/emulated/chains/parachains/assets/asset-hub-polkadot" }
asset-hub-polkadot-runtime = { path = "system-parachains/asset-hubs/asset-hub-polkadot" }
asset-test-utils = { version = "18.0.0" }
assets-common = { version = "0.18.0", default-features = false }
assets-common = { version = "0.18.3", default-features = false }
authority-discovery-primitives = { version = "34.0.0", default-features = false, package = "sp-authority-discovery" }
babe-primitives = { version = "0.40.0", default-features = false, package = "sp-consensus-babe" }
beefy-primitives = { version = "22.1.0", default-features = false, package = "sp-consensus-beefy" }
Expand Down
3 changes: 2 additions & 1 deletion relay/kusama/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2638,7 +2638,8 @@ sp_api::impl_runtime_apis! {
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == xcm_config::TokenLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Expand Down
3 changes: 2 additions & 1 deletion relay/polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2829,7 +2829,8 @@ sp_api::impl_runtime_apis! {
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == xcm_config::TokenLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Expand Down
34 changes: 27 additions & 7 deletions system-parachains/asset-hubs/asset-hub-kusama/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1263,19 +1263,39 @@ impl_runtime_apis! {

impl xcm_runtime_apis::fees::XcmPaymentApi<Block> for Runtime {
fn query_acceptable_payment_assets(xcm_version: xcm::Version) -> Result<Vec<VersionedAssetId>, XcmPaymentApiError> {
let acceptable_assets = vec![AssetId(xcm_config::KsmLocation::get())];
let native_asset = xcm_config::KsmLocation::get();
// We accept the native asset to pay fees.
let mut acceptable_assets = vec![AssetId(native_asset.clone())];
// We also accept all assets in a pool with the native token.
acceptable_assets.extend(
assets_common::PoolAdapter::<Runtime>::get_assets_in_pool_with(native_asset)
.map_err(|()| XcmPaymentApiError::VersionedConversionFailed)?
);
PolkadotXcm::query_acceptable_payment_assets(xcm_version, acceptable_assets)
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
Ok(asset_id) if asset_id.0 == xcm_config::KsmLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
let native_asset = xcm_config::KsmLocation::get();
let fee_in_native = WeightToFee::weight_to_fee(&weight);
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == native_asset => {
// for native asset
Ok(fee_in_native)
},
Ok(asset_id) => {
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(
asset_id.0.clone(),
native_asset,
fee_in_native,
true, // We include the fee.
) {
Ok(swapped_in_native)
} else {
log::trace!(target: "xcm::xcm_runtime_apis", "query_weight_to_asset_fee - unhandled asset_id: {asset_id:?}!");
Err(XcmPaymentApiError::AssetNotFound)
}
},
Err(_) => {
log::trace!(target: "xcm::xcm_runtime_apis", "query_weight_to_asset_fee - failed to convert asset: {asset:?}!");
Expand Down
34 changes: 27 additions & 7 deletions system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1230,19 +1230,39 @@ impl_runtime_apis! {

impl xcm_runtime_apis::fees::XcmPaymentApi<Block> for Runtime {
fn query_acceptable_payment_assets(xcm_version: xcm::Version) -> Result<Vec<VersionedAssetId>, XcmPaymentApiError> {
let acceptable_assets = vec![AssetId(xcm_config::DotLocation::get())];
let native_asset = xcm_config::DotLocation::get();
// We accept the native asset to pay fees.
let mut acceptable_assets = vec![AssetId(native_asset.clone())];
// We also accept all assets in a pool with the native token.
acceptable_assets.extend(
assets_common::PoolAdapter::<Runtime>::get_assets_in_pool_with(native_asset)
.map_err(|()| XcmPaymentApiError::VersionedConversionFailed)?
);
PolkadotXcm::query_acceptable_payment_assets(xcm_version, acceptable_assets)
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
Ok(asset_id) if asset_id.0 == xcm_config::DotLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
let native_asset = xcm_config::DotLocation::get();
let fee_in_native = WeightToFee::weight_to_fee(&weight);
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == native_asset => {
// for native asset
Ok(fee_in_native)
},
Ok(asset_id) => {
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the naming of the function is really not that great. IMO something like simulate_swap would have been better instead of the clunky name.

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.

We are passing in the amount in the native asset (weight to fee returns the native asset) and then we want to get the price in the target asset (asset_in). Given that it looks correct, but yeah hard to follow.

asset_id.0.clone(),
native_asset,
fee_in_native,
true, // We include the fee.
) {
Ok(swapped_in_native)
} else {
log::trace!(target: "xcm::xcm_runtime_apis", "query_weight_to_asset_fee - unhandled asset_id: {asset_id:?}!");
Err(XcmPaymentApiError::AssetNotFound)
}
},
Err(_) => {
log::trace!(target: "xcm::xcm_runtime_apis", "query_weight_to_asset_fee - failed to convert asset: {asset:?}!");
Expand Down
3 changes: 2 additions & 1 deletion system-parachains/bridge-hubs/bridge-hub-kusama/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,8 @@ impl_runtime_apis! {
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == xcm_config::KsmRelayLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Expand Down
3 changes: 2 additions & 1 deletion system-parachains/bridge-hubs/bridge-hub-polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,8 @@ impl_runtime_apis! {
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == xcm_config::DotRelayLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Expand Down
3 changes: 2 additions & 1 deletion system-parachains/collectives/collectives-polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -980,7 +980,8 @@ impl_runtime_apis! {
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == xcm_config::DotLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Expand Down
3 changes: 2 additions & 1 deletion system-parachains/coretime/coretime-kusama/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,8 @@ impl_runtime_apis! {
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == xcm_config::KsmRelayLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Expand Down
3 changes: 2 additions & 1 deletion system-parachains/coretime/coretime-polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,8 @@ impl_runtime_apis! {
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == xcm_config::DotRelayLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Expand Down
3 changes: 2 additions & 1 deletion system-parachains/encointer/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -988,7 +988,8 @@ impl_runtime_apis! {
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<XcmAssetId>() {
let latest_asset_id: Result<XcmAssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == xcm_config::KsmLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Expand Down
3 changes: 2 additions & 1 deletion system-parachains/people/people-kusama/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,8 @@ impl_runtime_apis! {
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == xcm_config::RelayLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Expand Down
3 changes: 2 additions & 1 deletion system-parachains/people/people-polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,8 @@ impl_runtime_apis! {
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
match asset.try_as::<AssetId>() {
let latest_asset_id: Result<AssetId, ()> = asset.clone().try_into();
match latest_asset_id {
Ok(asset_id) if asset_id.0 == xcm_config::RelayLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Expand Down
Loading