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

fix: ChargeAssetTxPayment to work with Asset Conversion on Westend's and Kusama's Asset Hubs #5752

Merged
merged 7 commits into from
Nov 16, 2023

Conversation

bee344
Copy link
Member

@bee344 bee344 commented Nov 14, 2023

Since the addition of the new pallet-asset-conversion to Kusama Asset Hub and Westend Asset Hub, the ChargeAssetTxPayment mutated to accept a MultiLocation as the ID of the asset used to pay the tx fees, which in turn broke the decoding of the blocks in which it was present. What this PR does is fix that by modifying the signed extension for the cases of KAH and WAH, by integrating an alias that modifies the SE to use AssetId or MultiLocation depending on the runtime version. At the same time, as the introduction of the pallet was at different times on KAH and WAH and was not added in Polkadot Asset Hub, it adds type override files for each of them, and leaves the statemint one without change.
It also modifies SignerOptions to accept an object for the SE assetId, so that a MultiLocation can be passed and the SE be used in a straightforward manner.

Block retrieved with substrate-sidecar pre-fix:

Block retrieved with substrate-sidecar after fix:

Closes Issue 5750 and 5734 (probably).

@TarikGul
Copy link
Member

TarikGul commented Nov 14, 2023

I can confirm this fixes the decoding issue we've been seeing with the assetId as a multilocation.

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Seems sane, thank you.

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Some small nigglies identified by CI, see https://github.com/polkadot-js/api/actions/runs/6870177012/job/18703510196?pr=5752#step:5:1294

It basically just needs a default type definition for the added type - generally we probably want to point this to the latest definition, which also means that the higher-version specs don't need the override.

So basically -

  1. add a default to point to Option<MultiLocation>
  2. keep the override for lower versions (as required)
  3. remove the override for current versions (meaning we don't need to keep adding them into the future)

@bee344
Copy link
Member Author

bee344 commented Nov 15, 2023

@jacogr I added the default types as you suggested and it solved the issues, but I also had to modify the Signer test to account for the assetId being a MultiLocation

@jacogr
Copy link
Member

jacogr commented Nov 15, 2023

@bee344 All good, I appreciate your persistence :)

@bee344 bee344 requested a review from jacogr November 15, 2023 23:31
@jacogr jacogr merged commit bc81ca4 into polkadot-js:master Nov 16, 2023
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Nov 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Statemine blocks 5753828, 5753849 can not be decoded
4 participants