Improve asset conversion by xlc · Pull Request #9892 · paritytech/polkadot-sdk

xlc · 2025-09-30T23:21:57Z

added view functions #7374
added trait MutateLiquidity to allow other pallets to manage liquidity #9765

xlc · 2025-09-30T23:26:09Z

/cmd fmt

xlc · 2025-09-30T23:30:06Z

/cmd prdoc --audience runtime_dev --bump minor

…time_dev --bump minor'

xlc · 2025-10-01T04:02:40Z

/cmd fmt

xlc · 2025-10-01T04:19:41Z

https://github.com/paritytech/polkadot-sdk/actions/runs/18151037215/job/51662079600?pr=9892

do we care semvar for test runtimes? they shouldn't be published anyway?

substrate/frame/asset-conversion/src/swap.rs

substrate/frame/asset-conversion/src/liquidity.rs

substrate/frame/asset-conversion/src/lib.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

gui1117

Looks good to me,

I think the major breaking change is unnecessary if this pallet is widely used.
If it is only used mostly by us then we can break the function signature.

gui1117 · 2025-10-03T00:37:52Z

substrate/frame/asset-conversion/src/swap.rs

-			)
-		})?;
-		Ok(amount_in)
+		Self::do_swap_tokens_for_exact_tokens(


Side note, I still think we should do something about functions that returns a dirty storage on error.

Maybe we should try to standardize a suffix: _mroe: must rollback on error.
And we use it every.

I thought about this before and my conclusion is just we should require all non view only pallet public methods are protected with #[transactional].

And with pallet level fuzzing setup (which we don't have), it is easy to verify.

gui1117 · 2025-10-03T00:40:42Z

prdoc/pr_9892.prdoc

+    Add `trait MutateLiquidity` for pallet-asset-conversion.
+crates:
+- name: pallet-asset-conversion
+  bump: minor


This is major change for the rust library, we change the functions signature.

I guess there is nothing wrong to return a Result type for get_reserve in view function so I updated to avoid breaking change.

gui1117 · 2025-10-03T00:51:20Z

substrate/frame/asset-conversion/src/lib.rs

+		#[pallet::weight(T::WeightInfo::swap_exact_tokens_for_tokens(path.len() as u32))]
+		pub fn swap_exact_tokens_for_tokens(
+			origin: OriginFor<T>,
+			path: Vec<Box<T::AssetKind>>,


For curiosity, why did we make it a Vec of Box in the past?
The only reason I know to use Box for arguments is to reduce the type size of the runtime call enum.
Is there another reason?
I also ask because just after we copy again the whole value into a Vec<AssetKind>.

blame traced back to #1677 and original code was BoundedVec<T::MultiAssetId, T::MaxSwapPathLength> @muharem could you help?

substrate/frame/asset-conversion/src/lib.rs

gui1117 · 2025-10-03T01:07:47Z

substrate/frame/asset-conversion/src/liquidity.rs

+			amount2_min_receive,
+			withdraw_to,
+		)
+	}


We could have one test that calls into them, but also they are straightforward so it is good to me.

gui1117 · 2025-10-03T01:14:37Z

substrate/frame/asset-conversion/src/lib.rs

-			asset2: T::AssetKind,
-		) -> Result<(T::Balance, T::Balance), Error<T>> {
-			let pool_account = T::PoolLocator::pool_address(&asset1, &asset2)
-				.map_err(|_| Error::<T>::InvalidAssetPair)?;


we no longer make a distinction for this error and now always return PoolNoFound. But I think it is ok. As both error mean the pool doesn't exist.

gui1117 · 2025-10-03T01:15:12Z

substrate/frame/asset-conversion/src/lib.rs

-		pub fn get_reserves(
-			asset1: T::AssetKind,
-			asset2: T::AssetKind,
-		) -> Result<(T::Balance, T::Balance), Error<T>> {


We could keep this function and introduce get_reserves_ok to avoid a breaking change.

Co-authored-by: Guillaume Thiolliere <guillaume.thiolliere@parity.io>

claravanstaden · 2025-10-03T07:28:52Z

substrate/frame/asset-conversion/src/lib.rs

+		/// Swap the exact amount of `asset1` into `asset2`.
+		/// `amount_out_min` param allows you to specify the min amount of the `asset2`
+		/// you're happy to receive.
+		///
+		/// [`AssetConversionApi::quote_price_exact_tokens_for_tokens`] runtime call can be called
+		/// for a quote.
+		#[pallet::call_index(3)]
+		#[pallet::weight(T::WeightInfo::swap_exact_tokens_for_tokens(path.len() as u32))]
+		pub fn swap_exact_tokens_for_tokens(


Why did you decide to change the ordering of these? Makes it hard to see the diff and what changed

I think it is just that the function body as been moved outside of the call into a regular function, so the logic can be reused for the trait implementation.

@gui1117 it was already in a helper function do_swap_exact_tokens_for_tokens, so it looks like this method was just moved up in the file.

maybe I'm wrong, but I think it is just how git is showing the diff, the calls were ordered by call index and still is.

yeah it is git diff got confused. the only "reordering" is the body moved from calls to helper methods and that's larger chunk of matching compare to just the call method declaration

added view functions #7374 added trait `MutateLiquidity` to allow other pallets to manage liquidity #9765 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Guillaume Thiolliere <guillaume.thiolliere@parity.io>

xlc added 2 commits October 1, 2025 11:03

add view function for asset-conversion

4c607d8

trait to manage liquidty for asset conversion

50ac66f

xlc requested a review from a team as a code owner September 30, 2025 23:21

Merge branch 'master' into improve-asset-conversion

48eafdc

Update from github-actions[bot] running command 'fmt'

fbd22c8

github-actions bot and others added 5 commits September 30, 2025 23:32

Update from github-actions[bot] running command 'prdoc --audience run…

3fa2375

…time_dev --bump minor'

fix

3056ba8

update prdoc

49abb4e

fix

cec203d

fix

9292226

Update from github-actions[bot] running command 'fmt'

081bfc7

bkchr reviewed Oct 1, 2025

View reviewed changes

xlc and others added 5 commits October 2, 2025 19:47

Update substrate/frame/asset-conversion/src/lib.rs

25f7bb9

Co-authored-by: Bastian Köcher <git@kchr.de>

update comments

cac5d1c

improve

a0084b3

Merge branch 'master' into improve-asset-conversion

b47cfbc

add docs

87f10f1

gui1117 approved these changes Oct 3, 2025

View reviewed changes

Update substrate/frame/asset-conversion/src/lib.rs

dc5d515

Co-authored-by: Guillaume Thiolliere <guillaume.thiolliere@parity.io>

joepetrowski requested a review from muharem October 3, 2025 04:38

claravanstaden reviewed Oct 3, 2025

View reviewed changes

claravanstaden approved these changes Oct 3, 2025

View reviewed changes

gui1117 mentioned this pull request Oct 3, 2025

add RewardsPool trait #9857

Merged

xlc added 2 commits October 6, 2025 11:04

Merge branch 'master' into improve-asset-conversion

397fe98

avoid breaking change

113d66d

bkchr approved these changes Oct 6, 2025

View reviewed changes

bkchr added this pull request to the merge queue Oct 6, 2025

bkchr added the T2-pallets This PR/Issue is related to a particular pallet. label Oct 6, 2025

Merged via the queue into paritytech:master with commit be423a3 Oct 6, 2025
258 of 261 checks passed

xlc deleted the improve-asset-conversion branch October 6, 2025 10:14

pgherveou added this to [preview] release tracker Mar 20, 2026

github-project-automation bot moved this to Done in [preview] release tracker Mar 20, 2026

pgherveou removed this from [preview] release tracker Mar 20, 2026

pgherveou added this to [preview] release tracker Mar 20, 2026

github-project-automation bot moved this to Done in [preview] release tracker Mar 20, 2026

Conversation

xlc commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xlc commented Sep 30, 2025

Uh oh!

xlc commented Sep 30, 2025

Uh oh!

xlc commented Oct 1, 2025

Uh oh!

xlc commented Oct 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gui1117 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gui1117 Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xlc commented Sep 30, 2025 •

edited

Loading

gui1117 left a comment •

edited

Loading

gui1117 Oct 3, 2025 •

edited

Loading