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 metadata versions endpoints #1424

Merged
merged 8 commits into from
Apr 4, 2024
Merged

feat: add metadata versions endpoints #1424

merged 8 commits into from
Apr 4, 2024

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Apr 2, 2024

Closes #1410

Description

The current implementation differs from the initial idea outlined in the #1410 issue. We did not add a query param in the existing endpoints in order to maintain the current functionality unchanged. Instead, the following new endpoints were added:

  • /runtime/metadata/{metadataVersion}
    • The output will be the metadata in json format.
  • /runtime/metadata/versions
    • The output will be an array of available metadata versions, e.g. ["14", "15"] when connected to Kusama/Polkadot chain.
  • transaction/material/{metadataVersion}
    • If query param metadata is defined then the output will be in the requested format (scale or json).
    • If it is not defined, the output will in the default json format.

The input for path parameter metadataVersion is expected in a vX format, where X represents the version number of the metadata. Examples: v14, v15.

Notes-Thoughts on Code Implementation

  • The validation part of the metadataVersion path parameter is repeated in both getMetadataVersioned function and getTransactionMaterialwithVersionedMetadata. Maybe I could add these validation checks in a middleware and call it from there 🤔
  • In transaction/material/{metadataVersion} when metadata is set to scale, I return metadata.toString() (before I was returning metadata.toHex()). I replaced that so that the output is the same as in p-js apps when calling metadata.metadataAtVersion() for a specific version).

Tests

  • Tested that the correct versions of metadata are returned in Kusama and Polkadot chains.
  • Tested in Kusama and Polkadot for error cases like providing for the metadataVersion param:
    • a non existing metadata version, e.g. : v25, v11
    • a non number, e.g. : vJDHS or abcf
    • badly formatted query param, e.g. : 14 or 15. the input should be of format v14/v15 or V14/V15.
  • Tested in Kusama for older blocks, e.g. :
    • runtime/metadata/v14?at=225911
    • transaction/material/v14?metadata=scale&at=225911
      both these cases will return an error message Function 'api.call.metadata.metadataVersions()' is not available at this block height. since for these new endpoints we make use of the new call metadataVersions() which not available in older blocks.
  • Tested that the correct format is returned when the query param is set to scale or json format in transaction/material endpoint.

Todos

  • Fix docs so it does not mention v14 or v15 but the generic path param metadataVersion
  • Replace the check for metadata versions 14 and 15 with a dynamic check based on the versions detected in the connected chain.

@Imod7 Imod7 requested a review from a team as a code owner April 2, 2024 16:46
… for versions 14 and 15

- transaction material with metadata returns metadata regardless of the 'metadata' query param
- updated docs
Copy link
Contributor

@bee344 bee344 left a comment

Choose a reason for hiding this comment

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

Looks amazing

- transaction
summary: Get all the network information needed to construct a transaction offline and
the version of metadata specified in `metadataVersion`.
description: Returns the material that is universal to constructing any
Copy link
Member

Choose a reason for hiding this comment

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

"Returns all the materials necessary to constructing any signed transactions offline."

I don't think we need any other details, and we can also get rid of the mention to /tx/artifacts.

description: The version of metadata. The input is expected in a `vX` format, where `X`
represents the version number (e.g. `v14`, `v15`). By default, metadata is outputted
in 'JSON' format, unless the `metadata` query parameter is provided, in which case it can be
either in 'JSON' or 'SCALE' format.
Copy link
Member

Choose a reason for hiding this comment

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

I think JSON, and SCALE should be lower case here to be consistent

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

let api;
Copy link
Member

Choose a reason for hiding this comment

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

const api = at
    ? await this.api.at(hash)
    : this.api

);
}

const availableVersions = (await api.call.metadata.metadataVersions()).toHuman() as string[];
Copy link
Member

Choose a reason for hiding this comment

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

I think toJSON() might be more fitting here. Generally speaking I think toHuman is good for user facing data that is meant to be displayed, such as UI's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's awesome! Thank you so much for this one!

Changed it and added the as unknown as Vec<u32> since this reflects the actual type which is returned from await api.call.metadata.metadataVersions(). If inspected with .toRawType(), it is a Vec<u32>.

}

const availableVersions = (await api.call.metadata.metadataVersions()).toHuman() as string[];
if (version && availableVersions?.includes(version.toString()) === false) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (version && availableVersions?.includes(version.toString()) === false) {
if (version && !availableVersions?.includes(version.toString())) {

throw new Error(`Version ${version} of Metadata is not available.`);
}

const registry = api ? api.registry : this.api.registry;
Copy link
Member

Choose a reason for hiding this comment

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

api will always be true here.

Copy link
Contributor Author

@Imod7 Imod7 Apr 4, 2024

Choose a reason for hiding this comment

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

Ofc! 😱 Changing this!

}

const availableVersions = (await api.call.metadata.metadataVersions()).toHuman() as string[];
if (version && availableVersions?.includes(version.toString()) === false) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (version && availableVersions?.includes(version.toString()) === false) {
if (version && !availableVersions?.includes(version.toString())) {

*/
async fetchMetadataVersioned(hash: BlockHash, metadataVersion: number): Promise<Metadata> {
const { api } = this;
const apiAt = await api.at(hash);
Copy link
Member

Choose a reason for hiding this comment

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

Were potentially calling api.at twice, I would just pass the api from the controller down to the service and call it apiAt or something.

*/
async fetchMetadataVersions(hash: BlockHash): Promise<string[]> {
const { api } = this;
const apiAt = await api.at(hash);
Copy link
Member

Choose a reason for hiding this comment

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

Same with this api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the previous one but for this one, in the RuntimeMetadataController > getMetadataAvailableVersions I don't call api.at. I just get the hash.

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.

Really amazing job! Just a few talking points and nits.

Imod7 and others added 4 commits April 4, 2024 14:48
- throw an error if newer call is not available for older blocks
- removed some comments left in node transaction spec file
Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>
@Imod7 Imod7 merged commit beb02ba into master Apr 4, 2024
15 checks passed
@Imod7 Imod7 deleted the domi-metadata-version branch April 4, 2024 14:32
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.

feat: add version query param in transaction/material endpoint
4 participants