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

feat: add pool-assets endpoints #1338

Merged
merged 3 commits into from
Oct 20, 2023
Merged

feat: add pool-assets endpoints #1338

merged 3 commits into from
Oct 20, 2023

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Oct 9, 2023

Description

Relates to: 1323

Adding the following endpoints :

  • /pallets/pool-assets/{assetId}/asset-info
  • /accounts/{accountId}/pool-asset-balances
  • /accounts/{accountId}/pool-asset-approvals

Query Parameters per Endpoint

For the endpoint /pallets/pool-assets/assetId/asset-info

  • at : Which block to query, it will default to the latest finalized block. Accepts a hash or blocknumber.

For the endpoint /accounts/{accountId}/pool-asset-balances

  • at: same as above.
  • assets: it accepts an assetId or an array of assetId's, e.g. ?assets[]=1&assets[]=2&assets[]=3.

For the endpoint /accounts/{accountId}/pool-asset-approvals

  • at: same as above.
  • delegate: the delegate account associated with the ApprovalKey which is tied to a Approval.
  • assetId: The assetId associated with the AssetApproval e.g. assets=id1

Sample Response per Endpoint

Request
http://127.0.0.1:8080/pallets/pool-assets/32/asset-info

Response

{
  "at": {
    "hash": "0xfe8fc0e90619ea1cf07e0ec07f6596ee11a318f2f047b8bc2adcbe544e69b818",
    "height": "5512016"
  },
  "assetInfo": {
    "owner": "5CuM1234Md2mMDnPSstq4o2TG3ujqySLtzP5Yeg7tjKwXBL8",
    "issuer": "5CuM1234Md2mMDnPSstq4o2TG3ujqySLtzP5Yeg7tjKwXBL8",
    "admin": "5CuM1234Md2mMDnPSstq4o2TG3ujqySLtzP5Yeg7tjKwXBL8",
    "freezer": "5CuM1234Md2mMDnPSstq4o2TG3ujqySLtzP5Yeg7tjKwXBL8",
    "supply": "9350825",
    "deposit": "0",
    "minBalance": "1",
    "isSufficient": false,
    "accounts": "2",
    "sufficients": "0",
    "approvals": "0",
    "status": "Live"
  },
  "assetMetaData": {
    "deposit": "0",
    "name": "0x",
    "symbol": "0x",
    "decimals": "0",
    "isFrozen": false
  }
}

Request
http://127.0.0.1:8080/accounts/5EiTAQ4cPoLTvwL6DE5BFSvKuwegjGwYGef1Mn6iwZTqBos9/pool-asset-balances

Response

{
  "at": {
    "hash": "0x0f8e5e30881da7a03b3b47325e417e260297ac765f6997a45be75f066a8a873a",
    "height": "5512744"
  },
  "pool-assets": [
    {
      "assetId": "0",
      "balance": "100",
      "isFrozen": "isFrozen does not exist for this runtime",
      "isSufficient": false
    },
    {
      "assetId": "10",
      "balance": "0",
      "isFrozen": false,
      "isSufficient": false
    },
    {
      "assetId": "4",
      "balance": "0",
      "isFrozen": false,
      "isSufficient": false
    },
    {
      "assetId": "21",
      "balance": "0",
      "isFrozen": false,
      "isSufficient": false
    },
   ...
   ...
   ...
   ...

Request
http://127.0.0.1:8080/accounts/5EiTAQ4cPoLTvwL6DE5BFSvKuwegjGwYGef1Mn6iwZTqBos9/pool-asset-balances?assets[]=4&assets[]=21

Response

{
  "at": {
    "hash": "0x291b348a508f28850e9e6b60b090f179c0e25b950c3449afcef1cdcc0ccee144",
    "height": "5512754"
  },
  "pool-assets": [
    {
      "assetId": "4",
      "balance": "0",
      "isFrozen": false,
      "isSufficient": false
    },
    {
      "assetId": "21",
      "balance": "0",
      "isFrozen": false,
      "isSufficient": false
    }
  ]
}

Request
http://127.0.0.1:8080/accounts/5D8Rj3PcZaTDETw2tK67APJVXEubgo7du83kaFXvju3ASToj/pool-asset-approvals?assetId=123&delegate=5FgrRXw3KjHaUY1PogXXdtnnYvgVG7pEML7mAdvqa4EFmqex

Response

{
  "at": {
    "hash": "0xc2c760726a44e18033fcfedb3a42407f1b43c41bf7e09382d50fae941f0fa671",
    "height": "5512935"
  },
  "amount": null,
  "deposit": null
}

Additional Fixes

  • Slightly changed the function name in AccountsAssetsController so it is aligned with the endpoint name.
  • Renamed a variable in the .spec file of Asset-Conversion so it is aligned with the naming in the two other mock api files.
  • Changed the expected response in the PalletsAssetsService test since in AssetInfo we receive status: 'Live' instead of isFrozen
    • For this, also replaced the rococoRegistry with the KusamaAssetHubRegistry
    • Replaced the returned type (AssetDetails) for AssetInfo in MockAssetData withPalletAssetsAssetDetails (checked with toRawType() ).
  • Some minor fixes in the mock block file of Westend.
  • Replaced isFrozen with Status in the foreignAssetInfo*** (mockAssetHubKusamaData file)

Todos

  • Added 1 controller & service for balances and approvals
  • Added 1 controller & service for asset-info
  • In chain-config : Added these endpoints only in assetHubWestendControllers
  • Updated Documentation & rebuilt docs
  • Added Tests
  • Added Mock Data based on Westend Asset Hub
  • Run linter
  • Run all tests

@Imod7 Imod7 requested a review from a team as a code owner October 9, 2023 23:00
/**
* @param keys Extract `assetId`s from an array of storage keys
*/
extractPoolAssetIds(keys: StorageKey<[AssetId]>[]): AssetId[] {
Copy link
Member

Choose a reason for hiding this comment

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

Just for the sake of consistency lets make this a private function as well.

): Promise<void> => {
const hash = await this.getHashFromAt(at);

const assetsSplit = (assets as string)?.split(',') || [];
Copy link
Member

Choose a reason for hiding this comment

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

So my only concern with this is it's not following Express convention of dealing with query params as an array, and will add two different ways to within sidecar to add the query param. I think for this we should stick to the conventional method of using expresses built in format.

format: unsignedInteger or $hex
- name: assets
in: query
description: A list of comma separated AssetId's to be queried. If not supplied, defaults to providing
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the comment about changing the query param structure.

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Overall, amazing job! I do think we should change the one format to fit within expresses supported structure for array query params.

Copy link
Collaborator

@marshacb marshacb left a comment

Choose a reason for hiding this comment

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

awesome PR! LGTM

@Imod7 Imod7 merged commit 02c4a3f into master Oct 20, 2023
14 checks passed
@Imod7 Imod7 deleted the domi-pool-assets branch October 20, 2023 06:10
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants