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.

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

Support different assets for delivery fees #4375

Closed
wants to merge 52 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
eea690b
feat(xcm-executor): add AssetConverter config item to support multipl…
franciscoaguirre May 8, 2024
5d74482
fix: implement AssetConverter in all runtimes
franciscoaguirre May 8, 2024
8e9df56
feat: add AssetConverters copying the traders
franciscoaguirre May 8, 2024
3ed0430
test(asset-hub-westend): add tests for paying delivery fees with diff…
franciscoaguirre May 8, 2024
da83c68
fix: update lock
franciscoaguirre May 8, 2024
efab825
fix(pallet-xcm): remove jit withdrawal to allow paying delivery fees …
franciscoaguirre May 11, 2024
da7b054
doc: add TODOs I'm considering
franciscoaguirre May 11, 2024
bdfbeaf
feat: start with SwapAssetConverter
franciscoaguirre May 11, 2024
a81ba18
Merge branch 'master' into different-assets-delivery-fees
franciscoaguirre May 15, 2024
c69095f
feat: SwapAssetConverter. Some tests failing
franciscoaguirre May 22, 2024
0193e60
fix(asset-hub-rococo-integration-tests): delivery fee taken from holding
franciscoaguirre May 22, 2024
0b0437c
fix(integration-tests): stick to jit withdrawal for now
franciscoaguirre May 22, 2024
03c01ff
Merge branch 'master' into different-assets-delivery-fees
franciscoaguirre Jul 1, 2024
502ebf6
chore: update Cargo.lock
franciscoaguirre Jul 1, 2024
550d92a
fix: fmt
franciscoaguirre Jul 1, 2024
ba024d1
feat(xcm-executor): remove all logic regarding local execution
franciscoaguirre Jul 1, 2024
69d1043
feat: complete tests
franciscoaguirre Jul 2, 2024
820160e
fix: fmt
franciscoaguirre Jul 2, 2024
677129d
Merge branch 'master' into different-assets-delivery-fees
franciscoaguirre Jul 2, 2024
99c9749
feat: remove support for sufficient asset converters
franciscoaguirre Jul 2, 2024
05b620d
feat(asset-hub-rococo): add a PoolAssetsConverter
franciscoaguirre Jul 2, 2024
522e7c7
feat(emulated-tests): add USDT to genesis and create WND<>USDT pool i…
franciscoaguirre Jul 3, 2024
629dcce
doc: prdoc
franciscoaguirre Jul 3, 2024
b6f3b6e
doc(asset-hub-westend-integration-tests): change the docs for a test
franciscoaguirre Jul 3, 2024
7ffd02a
chore(xcm-executor): change order of associated types in config
franciscoaguirre Jul 3, 2024
69c22ce
fix: fmt
franciscoaguirre Jul 3, 2024
b485475
Merge branch 'master' into different-assets-delivery-fees
franciscoaguirre Jul 3, 2024
df196fe
feat: remove AssetConverter and use AssetExchanger instead
franciscoaguirre Jul 4, 2024
1639489
doc: prdoc
franciscoaguirre Jul 4, 2024
9875ee2
doc: prdoc
franciscoaguirre Jul 4, 2024
c3d1554
fix: clippy and feedback
franciscoaguirre Jul 4, 2024
ab890aa
Merge branch 'master' into different-assets-delivery-fees
franciscoaguirre Jul 4, 2024
e2107f8
fix: zepter and taplo
franciscoaguirre Jul 4, 2024
84d0f14
Merge branch 'master' into different-assets-delivery-fees
franciscoaguirre Jul 5, 2024
77b0cc8
feat(xcm-executor): not error in case there are no delivery fees
franciscoaguirre Jul 5, 2024
2888826
chore(xcm-executor): deduplicate swapping logic in take_fees
franciscoaguirre Jul 5, 2024
90807aa
fix(assets-common): remove duplicated feature flags
franciscoaguirre Jul 5, 2024
10dd60f
chore(xcm-executor): remove default implementation for AssetExchange
franciscoaguirre Jul 5, 2024
49d76b9
doc: update prdoc crate bumps
franciscoaguirre Jul 5, 2024
a2d3b68
Apply suggestions from code review
acatangiu Jul 9, 2024
d4d6055
fix executor
acatangiu Jul 9, 2024
8f266c4
clean up tests
acatangiu Jul 9, 2024
11a03ee
Merge branch 'master' into different-assets-delivery-fees
franciscoaguirre Jul 17, 2024
b0e1c60
chore: address some docs and naming feedback
franciscoaguirre Jul 17, 2024
18e8b7c
fix(staging-xcm-executor): move log to tracing
franciscoaguirre Jul 18, 2024
d1baffc
chore: address more feedback
franciscoaguirre Jul 18, 2024
28553fb
Merge branch 'master' into different-assets-delivery-fees
franciscoaguirre Jul 18, 2024
d576a76
fix: fmt
franciscoaguirre Jul 18, 2024
1605b8a
Merge branch 'master' into different-assets-delivery-fees
franciscoaguirre Jul 22, 2024
5e1c999
fix: fmt
franciscoaguirre Jul 22, 2024
1fe6c08
feat(xcm-builder): rename FungiblesPoolAdapter to SingleAssetExchange…
franciscoaguirre Jul 22, 2024
184b2eb
feat(xcm-builder): tests for SingleAssetExchangeAdapter
franciscoaguirre Jul 24, 2024
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
Prev Previous commit
Next Next commit
chore(xcm-executor): remove default implementation for AssetExchange
  • Loading branch information
franciscoaguirre committed Jul 5, 2024
commit 10dd60fc0c6b0f675cac903a84ecbc3e67a56398
4 changes: 4 additions & 0 deletions polkadot/xcm/xcm-builder/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ impl AssetExchange for TestAssetExchanger {
) -> Result<AssetsInHolding, AssetsInHolding> {
Ok(want.clone().into())
}

fn quote_exchange_price(_: &Asset, _: &Asset, _: bool) -> Option<u128> {
None
}
}

pub struct TestPalletsInfo;
Expand Down
4 changes: 4 additions & 0 deletions polkadot/xcm/xcm-builder/src/tests/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,10 @@ impl AssetExchange for TestAssetExchange {
EXCHANGE_ASSETS.with(|l| l.replace(have));
Ok(get)
}

fn quote_exchange_price(_: &Asset, _: &Asset, _: bool) -> Option<u128> {
None
}
}

pub struct SiblingPrefix;
Expand Down
14 changes: 11 additions & 3 deletions polkadot/xcm/xcm-executor/src/traits/asset_exchange.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,7 @@ pub trait AssetExchange {
/// - `asset1` The first asset.
/// - `asset2` The second asset.
/// - `maximal`: If `true`, then all of `asset1` should be used.
fn quote_exchange_price(_asset1: &Asset, _asset2: &Asset, _maximal: bool) -> Option<u128> {
None
}
fn quote_exchange_price(asset1: &Asset, asset2: &Asset, maximal: bool) -> Option<u128>;
Copy link
Contributor

Choose a reason for hiding this comment

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

The API is not clear to me. Exact asset1 for asset2, asset1 for exact asset2? if first for example, what the fungibility amount of asset2 implies in this case? Also should new function be more symmetric with above function, where it accepts in and out assets as a collection of assets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it makes sense to accept collections of assets here.

The API was meant to mimic the QuotePrice trait, but using maximal: bool to distinguish the two cases.

Copy link
Contributor

@gui1117 gui1117 Jul 21, 2024

Choose a reason for hiding this comment

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

I also think the non-symmetricity with exchange_asset makes it difficult to use in more general cases.

Let's say you want to quote the price for 2 assets, from 2 other assets, to later call exchange_asset (a more general case than the current PR). Then there is no way to use this quote_exchange_price to get the expected requirement for give to get a successful execution of exchange_asset.

maybe something like this could be better

/// Returns a vector of minimal assets in holding which would execute
/// a successful `exchange_asset(_, _, give, want, _)`
fn exchange_asset_minimal_needed(give: &[AssetId], want: &Assets) -> Vec<AssetsInHolding>

Note that we could also support non-fungibles with a more complex give parameter maybe.
I'm not sure if we should also give the Option<Location> information

Then the tuple implementation would just be:

{
	for_tuples!( #(
		Tuple::exchange_asset_minimal_needed(give, want)
	)* )
		.flatten()
}

To avoid allocations we could put this vector in parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it depends on whether we want to keep the structure of Assets or limit it to only one asset at a time. Using Assets is more flexible, as we allow implementations to do more things. This adapter only takes the first asset for exchanging, but it's only of many.

Anyway, I decided to split this PR into two, one containing only the adapter and the additional quote method, the other one containing the tweaks to the executor using the AssetExchanger.

We can keep this discussion going on the PR for the adapter.

}

#[impl_trait_for_tuples::impl_for_tuples(30)]
Expand All @@ -64,4 +62,14 @@ impl AssetExchange for Tuple {
)* );
Err(give)
}

fn quote_exchange_price(asset1: &Asset, asset2: &Asset, maximal: bool) -> Option<u128> {
for_tuples!( #(
match Tuple::quote_exchange_price(asset1, asset2, maximal) {
Some(amount) => return Some(amount),
None => {}
}
)* );
None
}
}