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

Change assetId type in SignerPayloadJSON to HexString #5967

Merged
merged 4 commits into from
Sep 2, 2024

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Aug 31, 2024

closes: #5965

The following is a BREAKING CHANGE

The assetId within the SignerPayloadJSON will now be transmitted as a HexString.

@TarikGul TarikGul merged commit 1e604cc into master Sep 2, 2024
4 checks passed
@TarikGul TarikGul deleted the tg-asset-hexstring branch September 2, 2024 17:24
@carlosala
Copy link

Hi! Just saw this. I think it could be a great idea to make asHex directly with the option, without making unwrap. Then we have the exact hexstring that will make part of the final extrinsic. What do you think?

@TarikGul
Copy link
Member Author

TarikGul commented Sep 3, 2024

Hi! Just saw this. I think it could be a great idea to make asHex directly with the option, without making unwrap. Then we have the exact hexstring that will make part of the final extrinsic. What do you think?

Yea totally works, I was on the fence on what to do so this adds some clarity to the decision. So basically undoing the unwrap, and keeping it as an Option<T> but as HexString?

@TarikGul
Copy link
Member Author

TarikGul commented Sep 3, 2024

There is one annoying thing that I am remembering now, its that the ExtrinsicPayload type in PJS doesn't know how to handle the AssetId when the Option type is passed in as a hex if one wants to reconstruct the extrinsic... I'll need to fix this today.

@carlosala
Copy link

I mean, if the type is Option<MultiLocation> it should take the option-prepended hex, right?

@TarikGul
Copy link
Member Author

TarikGul commented Sep 3, 2024

I totally agree, I am more just babbling to myself before. Unfortunately, i have realized it's not as straightforward as I wished.

@TarikGul
Copy link
Member Author

TarikGul commented Sep 3, 2024

@carlosala got a working solution, its a bit hacky, but IMO its the safest option, returns the assetId as it should be (Option<TAssetConversion> = Option<MultiLocation | AssetId> for backwards compatibility), and doesn't break any major workflows other than the actual SignerPayloadJSON.

@carlosala
Copy link

carlosala commented Sep 4, 2024

Sounds good 👍🏻

@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 Sep 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Pack assetId as HexString in SignerPayloadJSON
4 participants