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: assets endpoints #533

Merged
merged 80 commits into from
May 11, 2021
Merged

feat: assets endpoints #533

merged 80 commits into from
May 11, 2021

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented May 4, 2021

Assets Endpoints

closes: #491
closes: #545

GET assets/{assetId}/asset-info

Associated Controller: AssetsController
Associated Service: AssetsService
Response:

{
    at: { ... }
    assetInfo: {
        owner: AccountId,
        issuer: AccountId,
        admin: AccountId,
        freezer: AccountId,
        supply: TAssetBalance, // U64
        deposit: TAssetDepositBalance, // BalanceOf
        minBalance: TAssetBalance,
        isSufficient: bool, // polkadot-js way of saying boolean
        accounts: U32,
        sufficients: U32,
        approvals: U32,
        isFrozen: bool
    }
    assetMetadata: {
        deposit: TAssetDepositBalance,
        name: Bytes, // Polkadot-js type
        symbol: Bytes,
        decimals: U8,
        isFrozen: bool
    }
}

AssetInfo(AssetDetails): substrate ref | polkadot-js ref
AssetMetaData: substrate ref | polkadot-js ref

GET accounts/{accountId}/asset-balances?assets=assetId

Notes:

  1. Originally per the issue filed above, there were two different proposed responses depending on whether an assetId is provided as a parameter. I changed it slightly to always have one type of response. The only difference is when a assetId is provided the assets array that is returned will only have the values provided in the query param. This choice was made to simplify return values for the end user. Happy to change this or discuss suggestions.

  2. assetId query parameter is treated as an array. It is formatted as such: assetId=id1,id2,id3

Associated Controller: AccountsAssetsController
Associated Service: AccountsAssetsService
Response:

{
    at: { ... },
    assets: [
        {
            assetId: AssetId,
            balance: TAssetBalance,
            isFrozen: bool, 
            isSufficient: bool
        }
        ...
    ]
}

AssetBalance: substrate ref | polkadot-js ref

GET accounts/{accountId}/asset-approvals?assetId=assetId&delegate=accountId

Note: Both a delegate and assetId are required for this endpoint. The owner => accoundId and the delegate can be referred to as the AssetApprovalKey in polkadot-js or ApprovalKey in substrate.

Associated Controller: AccountsAssetsController
Associated Service: AccountsAssetsService
Response:

{
    at: { ... },
    amount: TAssetBalance,
    deposit: TAssetDepositBalance
}

AssetApproval: substrate ref | polkadot-js
AssetApprovalKey: substrate ref | polkadot-js

TODO's

  • Asset Endpoints
  • Unit tests
  • Manually test all cases against statemint
    • assets/:assetId/asset-info
    • accounts/:address/asset-balances
    • accounts/:address/asset-balances?assets[]=10&assets[]=1
    • accounts/:address/asset-approvals (Fails correctly, this is subject to change)(delegate, and assetId might be part of the path)
    • accounts/:address/asset-approvals?assetId=assetId&delegate=delegateId
  • Docs
    • inline docs
    • swagger ui docs

Other

  1. Important to note I added statemint's metadata to the metadata library. It was the only way for me to have access to the assets methods inside of testing.

Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

Looks good! I just have one suggestion for how to change the api for fetchAssetApproval that I think will improve readability of the None response. (And tiny recs in docs)

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm, modulo nitpicks.

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
src/chains-config/kusamaControllers.ts Show resolved Hide resolved
src/controllers/AbstractController.ts Outdated Show resolved Hide resolved
src/services/accounts/AccountsAssetsService.ts Outdated Show resolved Hide resolved
src/services/accounts/AccountsAssetsService.ts Outdated Show resolved Hide resolved
src/services/accounts/AccountsAssetsService.ts Outdated Show resolved Hide resolved
* Note: It borrows some variables used in the parachains constant section
*
* Used in `/assets` and `/accounts` endpoints
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine either way but I don't mind long test files generally. They are typically not read top to bottom anyway?

src/types/responses/AccountAssets.ts Outdated Show resolved Hide resolved
TarikGul and others added 7 commits May 11, 2021 10:39
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
@TarikGul
Copy link
Member Author

TarikGul commented May 11, 2021

This PR is ready to merge. Just manually tested against Statemint today again. In return we found out the version of polkadot needed to be updated in order to resolve a few more types. I also changed all the pattern fields for accountId in the docs to format: SS58 in order to close #545.

@TarikGul TarikGul merged commit e83bc7e into master May 11, 2021
@TarikGul TarikGul deleted the tarik-assets-endpoint branch May 11, 2021 21:57
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.

Remove ineffective regexes from docs Assets API for Statemint
4 participants