-
Notifications
You must be signed in to change notification settings - Fork 151
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: update polkadot-js api to 7.2.1 #809
Conversation
return rococoRegistry.createType('AssetDetails', responseObj); | ||
}); | ||
|
||
export const assetsInfoKeysInjected = (): (() => Promise<AssetDetails>) => { |
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.
To highlight a change for reviewers. I use to directly inject the keys
field into the assetsInfo
in order to replicate the stucture of the object in polkadot-js. I used Object.assign() directly in the top layer of the file without enclosing it within its own function. I removed that by adding the exported assetsInfoKeysInjected
. This way we don't tamper with assetsInfo
ever within testing, but instead can have the keys if it's necessary with a shallow copy of the original object with the keys injected.
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.
Cool, I like this change!
Since JS is so awful at preventing things from being mutated I do like the "safe" option of returning objects and such from functions for tests; that way you know nothing else can ever mess with them!
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.
😺 Yea I totally agree. Glad I changed that in the refactoring in this PR, very thankful for the TS compiler in moments like this as well. Makes it super easy to cleanup.
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.
Left a couple of comments, and I didn't go too deep, but lgtm :) Do as you wish with the comments!
* Both `PalletAssetsAssetBalance`, and `LegacyPalletAssetsAssetBalance` have either, | ||
* a `sufficient` or `isSufficient` field exposed instead and are the types used | ||
* pre v9160. |
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 am confused, are both these legacy types in some respect? Also, or maybe related, how is reason
related to sufficient
/is_sufficient
, like, how does it replace them?
EDIT: reading further clarified it. Maybe worth clarifying a wee bit though.
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.
Yea it can be a little confusing. Thanks for the feedback. Ill go it and make it clearer.
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.
So I cleaned up the wording completely, I think its way more clear (a bit longer) and doesn't feel clunky when reading it.
I think the last wording I had made assumptions on how much the reader knew which I removed, and instead explained it more from the ground up.
This PR does a couple of things.
It refactors the asset mock data to keep code cleaner and DRY.
Updates polkadot-js api to 7.2.1 which introduces some new logic.
reasons
field is now added and replacessufficient
as of v9160. This is because the return type ofquery.assets.account()
went fromPalletAssetsAssetBalance
tooOption<PalletAssetsAssetAccount>
.There are 3 cases we look for, and I wrote them in the comments of the code.
It's good to note as well, once 9160 is released we can change the structure of the response to include all the contents of
reasons
. This PR is to make it compatible with Sidecar without breaking any code, and to allow us to catch up with p.jsref:
https://github.com/paritytech/substrate/pull/10382/files#diff-9acae09f48474b7f0b96e7a3d66644e0ce5179464cbb0e00671ad09aa3f73a5fR88