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

Add metadata V15 with Runtime API support #48

Merged
merged 19 commits into from
Mar 30, 2023
Merged

Add metadata V15 with Runtime API support #48

merged 19 commits into from
Mar 30, 2023

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Jan 23, 2023

The v15 metadata extends the v14 with the runtime field. This field (similarly to the pallets field) contains all the information needed to generate an API in higher-level projects (subxt, capi etc).

The runtime field is a collection of TraitMetadata objects that contain:

  • trait name
  • optionally a trait version
  • methods available:
    • method name
    • method inputs (name and meta::Type - ie bytes: &[u8])
    • method output (meta::Type)
    • documentation
  • documentation (that is only visible at decl_runtime_apis and valuable for UX)

While the metadata v15 might not yet be in its final form, this PR focus on adding support for the Runtime while allowing users (Substrate) to still utilize the v14.

The metadata v15 also includes the documentation of pallets needed by #47 and introduced in substrate via paritytech/substrate#13452.

More details: paritytech/substrate#12939 (comment).

Part of paritytech/substrate#12939

cc @paritytech/tools-team

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@@ -172,6 +176,12 @@ pub enum RuntimeMetadata {
/// Version 14 for runtime metadata, as raw encoded bytes.
#[cfg(not(feature = "v14"))]
V14(OpaqueMetadata),
/// Version 15 for runtime metadata.
#[cfg(feature = "v15-unstable")]
V15(v15::RuntimeMetadataV15),
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't notice, but how do you ask for V15 metadata in Substrate; will you ask for metadata_at_version(u32::MAX) or something? Or do you ask for 15? Just wondeirng whether we do anything to highlight the instability of it in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, in substrate we'll have to ask for u32::MAX until we can call the v15 stable

frame-metadata/src/v15.rs Outdated Show resolved Hide resolved
frame-metadata/src/v15.rs Outdated Show resolved Hide resolved
frame-metadata/src/v15.rs Outdated Show resolved Hide resolved
@jsdw
Copy link
Contributor

jsdw commented Mar 15, 2023

Other than a couple of naming things this looks good to me!

It takes a slightly different approach than eg PalletCallMetadata for exposing the type information for runtime calls (PalletCallMetadata points to some variant type to encode the possible options), but I think it's a good approach, and necessary if you want to have a return type anyway (unlike calls).

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Contributor

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Awesome!

@KiChjang
Copy link

Is it still ok if we add hold_reason as a new metadata field on pallets? paritytech/substrate#13722
More context here: paritytech/polkadot-sdk#236, we'd probably also need lock_id, slash_reason and freeze_reason as well.

@lexnv
Copy link
Contributor Author

lexnv commented Mar 28, 2023

Of course, I'll try to add those fields after I have a proper look at those PRs.

We can still add things to the metadata V15 as we'll start with a v15-unstable, which gives us enough flexibility until we call it stable.

There is also this counterpart in substrate: paritytech/substrate#13302.

@bkchr What do you think?

@jsdw
Copy link
Contributor

jsdw commented Mar 28, 2023

We can still add things to the metadata V15 as we'll start with a v15-unstable, which gives us enough flexibility until we call it stable.

This is an important point; others can continue to add to the V15 metadata while it's "unstable" (it doesn't need to rely on us; each addition can be considered/merged separately in their own PRs) and with the new runtime APIs it should be easier to introduce V16 etc as needed too (and support the last N versions to be returned in a Substrate node to avoid breaking things for people).

Comment on lines 31 to 32
/// Current prefix of metadata
pub const META_RESERVED: u32 = 0x6174656d; // 'meta' warn endianness
Copy link
Member

Choose a reason for hiding this comment

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

Do we always copy this into every file? Should not be required?

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 have moved the META_RESERVED one level above in the lib.rs and used that for the V15 file. I've left the other metadata versions as they are to avoid any breaking changes for this release

frame-metadata/src/v15.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Mar 29, 2023

@bkchr What do you think?

Same as what @jsdw said!

lexnv and others added 2 commits March 29, 2023 18:51
Co-authored-by: Bastian Köcher <git@kchr.de>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
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.

5 participants