-
Notifications
You must be signed in to change notification settings - Fork 409
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
feat(extension-ui): extrinsic asset id #1331
Conversation
b5cf741
to
24f7143
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Some things to note from when we spoke:
- works when sending an X2 pool asset location as the optional
assetId
(e.g.{parents: 0, interior:{X2:[{PalletInstance:50},{GeneralIndex:41}]}}
- supporting only the
X2
junction may be limiting and could be expanded to support other location junctions (if it turns out to be necessary) but if this is out of scope for this fix another issue can be created - PJS apps doesn't have the
assetId
appear in the extension UI due to the assetId value not being a part of thepayload
and would require additional changes
We should open an issue in polkadot-sdk to get that error changed as well. "Invalid Transaction: Payment" is far too vague, and doesn't tell you that there is no liquidity pool for this asset. |
return null; | ||
} | ||
|
||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the return value of the AssetId I think we can just return the whole MultiLocation
, and shouldn't extrapolate any specific, this will keep it true to the original input.
cc: @IkerAlus Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we limiting this to only asset IDs as XCMV2 junction objects with the junctions PalletInstance
and GeneralIndex
? Is there a limitation I am not aware of?
From UX perspective and considering only AH, indeed, most of the cases of users paying fees in other tokens beyond DOT will have an asset ID in the mentioned form, but actually any asset with an existing pool against DOT (asset conversion pallet) will be able to do this, for example any foreign asset sent to AH.
The general solution will be to allow any Multilocation
as asset ID.
Edit: Perhaps i should have put this as a general comment in the review, consider it in this way @ryanleecode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We thought of this but I think this XCMV2 object covers 99% of usecases. This change doesn't affect the actual functionality, so if the user provides say and interior with length 3/4/5/6/7/8, it will still go through. This function is just to display it in the UI.
For the other types, its ambigious how it would be displayed in the UI. should we just assume the last element in the array is always GeneralIndex
and it is the asset id?
We could also display instances where the user is choosing to pay int he native token using the "Here" XCM injuction, but that also defeats the purpose of using ChargeAssetTxPayment
signed extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks! Indeed, it makes sense to only show these cases in the UI, particularly considering it won't crash and simply not show the general case.
Answering the original question from @TarikGul : I also think it is better to return the whole Multilocation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the whole multilocation considered the Asset though? So should we show the whole multilocation?
return false; | ||
} | ||
|
||
return 'parents' in assetId && 'interior' in assetId && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove most of this and just ensure the assetId field exists.
Side note: Upon construction of the Extrinisic the api will also check the validity of the structure of the assetId passed in based on the metadata provided by the SignedExtension fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is an option, we can just use nullish optional chaining
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple discussion points on the following. Thank so much for the PR.
@ryanleecode Looks really solid, nice job! For the expendable could we do something like: <tr>
<td className='label'>{t('asset')}</td>
<td className='data'>
<details>
<summary>{'{...}'}</summary>
<pre>{someValueForJSOn}</pre>
</details>
</td>
</tr> |
the thing i dont like about this is you need that extra click to see the actual asset id where as before it was even worse is if the xcm is really big u might even need to scroll down. (not the case in the picture below, but imagine if it was a X3 xcm). |
I think there is two key points here that i see:
|
Ideally there should be a tool out there to parse the MultiLocation into its written format. I haven't found one such tool so I've opened an issue in xcm-tools to request this: paraspell/xcm-tools#188 |
I agree it will be nicer to show the location in the written format. Assuming we go this way, will the interface show the whole location at first sight? or will the user still need to do the extra click? |
Shows the written format and can be expanded to show the whole thing. |
So I looked into the One main thing is xcm-tools uses polkadot-js as direct dependency meaning the package will always need to be in sync with our updates which brings further maintenance, and potential complications. @Tbaut Any thoughts about the design. My thought is we just keep the raw JSON as the displayed output with the dropdown, and avoid any additional maintenance of deps in the future. |
Its possible they make it a standalone package (they already have a monorepo structure in place). in theory the package should require no dependencies on its own, since the XCM format is known. |
@TarikGul Its in their pipeline for early April. paraspell/xcm-tools#188 (comment) Shall we wait or get this merged with |
Happy to get this in, and we can create a tracking issue to follow this up. |
nice, I think the JSON preview is good enough. |
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. |
Fixes: #1314
Adds
assetId
in the extrinsic UI. Assumes the assetId is an XCMV2 junction object with the junctionsPalletInstance
andGeneralIndex
. Otherwise it won't render and neither will it crash because any errors are swallowed tonull
.