Skip to content

Remove TakeFirstAssetTrader from AH Westend and Rococo#8376

Merged
x3c41a merged 43 commits intomasterfrom
rm_tak_first_asset_trader
May 2, 2025
Merged

Remove TakeFirstAssetTrader from AH Westend and Rococo#8376
x3c41a merged 43 commits intomasterfrom
rm_tak_first_asset_trader

Conversation

@x3c41a
Copy link
Copy Markdown
Contributor

@x3c41a x3c41a commented Apr 29, 2025

Description

Removed TakeFirstAssetTrader from Asset Hub Westend and Rococo. Improved macros, fixed tests.

This implies asset sufficiency no longer guarantees that weight can also be bought with it. SwapFirstAssetTrader is used instead which will attempt to swap some of the given asset for the required amount of native asset to buy weight. This may or may not succeed depending on whether there is a local pool present with enough liquidity to serve the swap.

Review notes

Additionally parametrised macro and fixed Westend test: weight swapping was failing at this line with around 100x difference thus had to modify the macro.

Fixes #8233

@x3c41a x3c41a added the T6-XCM This PR/Issue is related to XCM. label Apr 29, 2025
@x3c41a
Copy link
Copy Markdown
Contributor Author

x3c41a commented Apr 30, 2025

if it comes to
[Check publish build / check-publish-compile (pull_request)](https://github.com/paritytech/polkadot-sdk/actions/runs/14749062513/job/41402146630?pr=8376) failure, I ran cargo build -p parachain-template-node on my local machine and everything went well; might be unrelated to my change.

…idity pool (#8392)

Used macro overloading instead of two macros
@x3c41a x3c41a enabled auto-merge May 1, 2025 11:09
@x3c41a x3c41a changed the title Stopped using TakeFirstAssetTrader in AH Rococo & Westend Stopped using TakeFirstAssetTrader in AH Westend May 1, 2025
@x3c41a x3c41a disabled auto-merge May 1, 2025 12:16
@x3c41a x3c41a requested review from bkontur and franciscoaguirre May 1, 2025 13:02
@x3c41a
Copy link
Copy Markdown
Contributor Author

x3c41a commented May 1, 2025

I modified runtime test.

  1. I followed gut feeling and removed the tests which were testing take_first_trader functionality.
    Remove functionality --> remove its tests.
    Let me know in case I was wrong and we do need to keep those tests

  2. Also, I fixed test_asset_xcm_take_first_trader_not_possible_for_non_sufficient_assets by adding liquidity pool and extra checks to it -- please verify the checks make sense

Copy link
Copy Markdown
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

It's fine to remove those tests. We no longer use sufficient assets for fee payment, we always use liquidity pools, and we have tests for using liquidity pools to swap for fee payment already

@paritytech-workflow-stopper
Copy link
Copy Markdown

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/14798317288
Failed job name: fmt

@acatangiu acatangiu changed the title Stopped using TakeFirstAssetTrader in AH Westend Removed TakeFirstAssetTrader from configs. Improved macros, fixed tests. May 2, 2025
@acatangiu acatangiu changed the title Removed TakeFirstAssetTrader from configs. Improved macros, fixed tests. Remove TakeFirstAssetTrader from Asset Hubs configs. Improved macros, fixed tests. May 2, 2025
@acatangiu acatangiu changed the title Remove TakeFirstAssetTrader from Asset Hubs configs. Improved macros, fixed tests. Remove TakeFirstAssetTrader from Asset Hubs configs May 2, 2025
Co-authored-by: Adrian Catangiu <adrian@parity.io>
@acatangiu acatangiu changed the title Remove TakeFirstAssetTrader from Asset Hubs configs Remove TakeFirstAssetTrader from AH Westend and Rococo May 2, 2025
@x3c41a x3c41a enabled auto-merge May 2, 2025 16:42
@x3c41a x3c41a added this pull request to the merge queue May 2, 2025
Merged via the queue into master with commit e85f0ad May 2, 2025
238 of 250 checks passed
@x3c41a x3c41a deleted the rm_tak_first_asset_trader branch May 2, 2025 17:38
castillax pushed a commit that referenced this pull request May 12, 2025
## Description

Removed `TakeFirstAssetTrader` from Asset Hub Westend and Rococo.
Improved macros, fixed tests.

This implies asset sufficiency no longer guarantees that weight can also
be bought with it. `SwapFirstAssetTrader` is used instead which will
attempt to swap some of the given asset for the required amount of
native asset to buy weight. This may or may not succeed depending on
whether there is a local pool present with enough liquidity to serve the
swap.

## Review notes

Additionally parametrised macro and fixed Westend test: weight swapping
was failing at [this
line](https://github.com/paritytech/polkadot-sdk/blob/44ae6a8bebd23a8ffac02d71c6e74ee889c3ab00/substrate/frame/asset-conversion/src/lib.rs#L903)
with around 100x difference thus had to modify the macro.

Fixes #8233

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@bkontur bkontur mentioned this pull request Jun 9, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T6-XCM This PR/Issue is related to XCM.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Testnet Asset Hubs: remove XCM sufficient asset fee trader

5 participants