-
Notifications
You must be signed in to change notification settings - Fork 159
feat: add pallets/foreign-assets endpoint #1314
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
Conversation
- add controller, service & reponse type for foreign assets - update chain-config - update docs
- In /pallets/foreign-assets/ endpoint removed `asset-info` since it is not necessary. - In the docs removed the array notation from the `200` response of multiple endpoints (including foreign assets) since there is no outer array in the result.
- Added in the response the asset's name & symbol in human readable format.
- Retrieved metadata based on MultiLocation key obtained by asset info entries - Changed types in IForeignAssetInfo
- Added a test (tinkernet) for foreign asset - Added metadata & registry for Asset Hub Kusama
Gitlab test should be fixed now, just need to fix the merge conflict, and pull from master. |
- Resolved conflicts by keeping both changes - Reordered the pallets endpoints in docs so they are in alphabetical order (almost) - Rebuilt docs
@@ -0,0 +1,12 @@ | |||
// '{"parents":"1","interior":{"X2":[{"Parachain":"2125"},{"GeneralIndex":"0"}]}}'; |
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.
Do we still need this comment? Also, Im thinking we should add the copyright to this file. I see a few other mock data files that we missed the copyright for as well that we can update at some point.
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.
- removed the comment and added the copyright text
- in some other files that it was missing, I added the copyright text
- in some other files that the copyright was with hear
2017-2022
I updated it to2017-2023
but not all.
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.
in some other files that the copyright was with hear 2017-2022 I updated it to 2017-2023 but not all.
Yea lets try to save the rest for a seperate PR, it just adds a lot of bloat to the review.
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.
Awesome PR!! Just one small question/nit.
src/types/responses/ForeignAssets.ts
Outdated
import { IAt } from '.'; | ||
|
||
export interface IForeignAssetInfo { | ||
foreignAssetInfo: PalletAssetsAssetDetails | AnyJson; |
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.
These types should have AnyJson
removed from 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.
I replaced with T
/Codec
like I had it in the beginning. If I remove completely and only leave types PalletAssetsAssetDetails
and AssetMetadata
it does not work.
I am now reading the next comment so I will change again.
|
||
const [{ number }, foreignAssetInfo] = await Promise.all([ | ||
api.rpc.chain.getHeader(hash), | ||
api.query.foreignAssets.asset.entries(), |
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.
Similar to above we need to correctly type the entries method. This is because both assetStorageKeyData
, and assetInfo
will have there own specific types.
assetStorageKeyData: StorageKey<AnyTuple>
assetInfo: Option<PalletAssetsAssetDetails>
// Incorrect:
api.query.foreignAssets.asset.entries();
// Correct:
api.query.foreignAssets.asset.entries<Option<PalletAssetsAssetDetails>>()
// This is a little tricky but the above will map out to Promise<[StorageKey<AnyTuple>, Option<PalletAssetsAssetDetails>][]> once we input the correct type.
This will all ensure that the above is typed correctly so we wont need to type cast anymore and we can clean up all the types within the service.
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.
That is amazing!!! Thank you so much @TarikGul for spending the time to explain and showcase this. I was fighting with the types quite a bit and now I get the logic! 🙏 🙏 🙏
foreignAssetMultiLocation | ||
); | ||
|
||
if (assetMetadata) { |
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 dont think it's assetMetadata we need to check here, but instead if (assetInfo.isSome)
, and then we need to unwrap the value when we key it into foreignAssetInfo: assetInfo.unwrap()
.
But then we need to handle the case where the info is not available in an else statement. Such that if it doesn't exist we return an empty object.
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 some notes on how we can update the type accuracy so we dont need to type cast or have Union types with AnyJson
. I think it should make our testing and endpoint a bit more accurate.
Co-authored-by: Tarik Gul <[email protected]>
Co-authored-by: Tarik Gul <[email protected]>
Co-authored-by: Tarik Gul <[email protected]>
Co-authored-by: Tarik Gul <[email protected]>
Co-authored-by: Tarik Gul <[email protected]>
- Replaced types to avoid type casts based on Tarik's suggestion - Added the copyright in some files that was missing - Updated the copyright year in some of the files
JSON.parse(foreignAssetMultiLocationStr) | ||
); | ||
|
||
const assetMetadata = await api.query.foreignAssets.metadata< |
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.
assetMetadata
is not an Option
it's just AssetMetadata
, assetInfo
in the only type that is going to be wrapped in an option.
src/types/responses/ForeignAssets.ts
Outdated
|
||
export interface IForeignAssetInfo { | ||
foreignAssetInfo: Option<PalletAssetsAssetDetails>; | ||
foreignAssetMetadata: Option<PalletAssetsAssetMetadata>; |
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.
This needs to be changed to just AssetMetadata
, I would actually also prefer if we also just use AssetMetadata
here because its the generic type for an Asset where as PalletAssetsAssetMetadata
is referring to the Asset Pallet.
Edit: I know we talked and said either was fine, but I think its more clear to just use the AssetsMetadata here.
- Changed the copyright year in the files related to this PR
- Added the DOT asset in the test file (.spec) - Added the AssetInfo and AssetMetadata for the DOT asset in the mock data (metadata currently have empty & zero values because those are the values in Asset Hub right now - to be changed) - Added again the storageKeyMultilocation in TypeFactory since indexType is not included in InterfaceTypes (as defined in storageKey)
- Replaced PalletAssetsAssetMetadata with AssetMetadata - Removed also Option from assetMetadata
* https://github.com/paritytech/asset-transfer-api-registry/blob/main/src/createRegistry.ts#L193-L238 | ||
*/ | ||
for (const [assetStorageKeyData, assetInfo] of foreignAssetInfo) { | ||
console.log(assetInfo.toRawType()); |
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.
Ahh this was my bad, but lets remove the console.log here
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.
Just one tiny little nit with removing the console.log I pushed up by accident. Other than that, really great job!! LGTM
Relates to: #1287
Description
Adds the endpoint
/pallets/foreign-assets/
which returns an array with the available foreign assets and their correspondingAssetDetails
andAssetMetadata
.Query Params
at
: Which block to query, it will default to the latest finalized block. Accepts a hash or blocknumberSample Response
While connected to
Kusama Asset Hub
and requesting/pallets/foreign-assets/
, the return result is/should be the following :Todos