Skip to content

xcm: Do not require Asset to be sorted on decode#9842

Merged
bkchr merged 3 commits intomasterfrom
bkchr-xcm-assets-sort
Sep 27, 2025
Merged

xcm: Do not require Asset to be sorted on decode#9842
bkchr merged 3 commits intomasterfrom
bkchr-xcm-assets-sort

Conversation

@bkchr
Copy link
Member

@bkchr bkchr commented Sep 25, 2025

Asset was requiring that all the assets are sorted at decoding. This is quite confusing for people writingg frontends, because this is not really documented anywhere. There are also only at max 20 assets available, we can just make everyones life easier and always sort the assets after decoding.

`Asset` was requiring that all the assets are sorted at decoding. This is quite
confusing for people writingg frontends, because this is not really documented anywhere.
There are also only at max 20 assets available, we can just make everyones life easier
and always sort the assets after decoding.
@bkchr bkchr requested a review from a team as a code owner September 25, 2025 20:17
@bkchr bkchr added the T6-XCM This PR/Issue is related to XCM. label Sep 25, 2025
@bkchr
Copy link
Member Author

bkchr commented Sep 25, 2025

/cmd prdoc --audience runtime_user --bump patch

Copy link
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.

Thanks for this 😁

@karolk91
Copy link
Contributor

karolk91 commented Sep 26, 2025

Something to take into consideration: there are extrinsics (like reserve_transfer_assets) that also take fee_asset_item as argument, which is u32 index provided by the caller i.e. the position of the asset to be used for fees

Copy link
Contributor

@claravanstaden claravanstaden left a comment

Choose a reason for hiding this comment

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

Thankful! ❤️

@bkchr
Copy link
Member Author

bkchr commented Sep 26, 2025

Something to take into consideration: there are extrinsics (like reserve_transfer_assets) that also take fee_asset_item as argument, which is u32 index provided by the caller i.e. the position of the asset to be used for fees

That is really sub-optimal :)

@jak-pan
Copy link
Contributor

jak-pan commented Sep 26, 2025

OMG thank you @bkchr. This was 💩 for ages.

@bkchr bkchr enabled auto-merge September 26, 2025 12:51
@bkchr bkchr added this pull request to the merge queue Sep 26, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 26, 2025
@bkchr bkchr enabled auto-merge September 26, 2025 19:03
@claravanstaden
Copy link
Contributor

@bkchr just checking if this will still work?

Something to take into consideration: there are extrinsics (like reserve_transfer_assets) that also take fee_asset_item as argument, which is u32 index provided by the caller i.e. the position of the asset to be used for fees

That is really sub-optimal :)

@bkchr
Copy link
Member Author

bkchr commented Sep 27, 2025

@bkchr just checking if this will still work?

Something to take into consideration: there are extrinsics (like reserve_transfer_assets) that also take fee_asset_item as argument, which is u32 index provided by the caller i.e. the position of the asset to be used for fees

That is really sub-optimal :)

As long as the assets are ordered, yes. But we discussed this in the Xcm channel and all these methods which take a fixed index are deprecated and should be replaced by assetid or something like this to not be bound to the ordering of the assets when addressing them.

@bkchr bkchr added this pull request to the merge queue Sep 27, 2025
@claravanstaden
Copy link
Contributor

Great, thanks @bkchr. I missed the thread, but I have read it now - I agree. Link to the thread, incase anyone else comes across this PR.

Merged via the queue into master with commit 5e28de7 Sep 27, 2025
247 of 252 checks passed
@bkchr bkchr deleted the bkchr-xcm-assets-sort branch September 27, 2025 07:07
bee344 pushed a commit that referenced this pull request Oct 7, 2025
`Asset` was requiring that all the assets are sorted at decoding. This
is quite confusing for people writingg frontends, because this is not
really documented anywhere. There are also only at max 20 assets
available, we can just make everyones life easier and always sort the
assets after decoding.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
`Asset` was requiring that all the assets are sorted at decoding. This
is quite confusing for people writingg frontends, because this is not
really documented anywhere. There are also only at max 20 assets
available, we can just make everyones life easier and always sort the
assets after decoding.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Nov 12, 2025
… specify asset for fees (#10243)

Multiple pallet-xcm calls use `u32` index as a way to specify which
asset from the `assets` (also an arg of the call) is to be used for fees
purposes. This PR brings **major API change (breaking change)** that
proposes usage of `VersionedAssetId` instead

Affected pallet-xcm calls: `teleport_assets`, `reserve_transfer_assets`,
`limited_reserve_transfer_assets`, `limited_teleport_assets`,
`transfer_assets`

This is follow-up change to the:
#9842, that aims to
remove the requirement of the client to provide sorted list of Assets to
the APIs (often a point failures). With the mentioned change sorting
happens on the runtime side and `u32` index (provided by the client) can
become invalid after sorting (this PR aims that problem)

Relevant
[dicussion](https://matrix.to/#/!nYxxyvMNMUaniRIokh:parity.io/$gAcAEznmQL6LhUlvGPHSaePUV4cZtxoco2I6ap8Cn3Q?via=parity.io&via=matrix.org&via=web3.foundation)
on XCM public element channel

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
0xRVE pushed a commit that referenced this pull request Nov 18, 2025
… specify asset for fees (#10243)

Multiple pallet-xcm calls use `u32` index as a way to specify which
asset from the `assets` (also an arg of the call) is to be used for fees
purposes. This PR brings **major API change (breaking change)** that
proposes usage of `VersionedAssetId` instead

Affected pallet-xcm calls: `teleport_assets`, `reserve_transfer_assets`,
`limited_reserve_transfer_assets`, `limited_teleport_assets`,
`transfer_assets`

This is follow-up change to the:
#9842, that aims to
remove the requirement of the client to provide sorted list of Assets to
the APIs (often a point failures). With the mentioned change sorting
happens on the runtime side and `u32` index (provided by the client) can
become invalid after sorting (this PR aims that problem)

Relevant
[dicussion](https://matrix.to/#/!nYxxyvMNMUaniRIokh:parity.io/$gAcAEznmQL6LhUlvGPHSaePUV4cZtxoco2I6ap8Cn3Q?via=parity.io&via=matrix.org&via=web3.foundation)
on XCM public element channel

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
karolk91 added a commit that referenced this pull request Nov 28, 2025
github-merge-queue bot pushed a commit that referenced this pull request Dec 4, 2025
Reverts #9842

Following on the discussion at
#10391, we don't want
to sort at decode level but rather to not require sorting at all among
the codebase
karolk91 added a commit that referenced this pull request Dec 4, 2025
Reverts #9842

Following on the discussion at
#10391, we don't want
to sort at decode level but rather to not require sorting at all among
the codebase

(cherry picked from commit 34034a9)
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.

6 participants