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

v16: Expose the unstable metadata v16 #5732

Open
wants to merge 45 commits into
base: master
Choose a base branch
from

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Sep 16, 2024

Builds on top of: #5274

cc @paritytech/subxt-team

lexnv added 30 commits August 7, 2024 18:05
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv marked this pull request as ready for review October 25, 2024 10:13
@lexnv lexnv requested review from a team and koute as code owners October 25, 2024 10:13

crates:
- name: sp-metadata-ir
bump: patch
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be minor i.e, you add new functionality in backward compatible manner?

@@ -151,7 +151,7 @@ impl From<ExtrinsicMetadataIR> for ExtrinsicMetadata {
fn from(ir: ExtrinsicMetadataIR) -> Self {
ExtrinsicMetadata {
ty: ir.ty,
version: ir.version,
version: *ir.versions.get(0).expect("Metadata V14 supports only one version"),
Copy link
Member

Choose a reason for hiding this comment

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

Is there any plan to support several versions here and how to to find the latest version? I guess it will be last item in the Vec?

So my question is really if you need to do a linear search to find the specific version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we want to return the lowest version. Because tools are currently using v4, and it is better not to break them by showing version 5 while 4 is still supported.

Then if the tool want to update they will use metadata v16.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that V14 should continue to return the lowest version supported.

@@ -117,7 +117,7 @@ pub fn expand_runtime_metadata(
pallets: #scrate::__private::vec![ #(#pallets),* ],
extrinsic: #scrate::__private::metadata_ir::ExtrinsicMetadataIR {
ty,
version: <#extrinsic as #scrate::sp_runtime::traits::ExtrinsicMetadata>::VERSION,
versions: #scrate::__private::vec![ <#extrinsic as #scrate::sp_runtime::traits::ExtrinsicMetadata>::VERSION ],
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe similar to what Niklas asked but, I guess this should now contain 2 versions, 4 and 5, as of #3685 merging?

Copy link
Contributor

Choose a reason for hiding this comment

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

ExtrinsicMetadata::VERSION should be a vec itself (or static slice).

address_ty: ir.address_ty,
call_ty: ir.call_ty,
signature_ty: ir.signature_ty,
extra_ty: ir.extra_ty,
Copy link
Contributor

@jsdw jsdw Oct 31, 2024

Choose a reason for hiding this comment

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

Just a reminder that we should remove this type from V16 (there will be multiple versions of tx extensions, so this type which only points at one version will not be helpful); can't remember if there is an issue for this already :)

ExtrinsicMetadata {
versions: ir.versions.into_iter().map(Into::into).collect(),
address_ty: ir.address_ty,
call_ty: ir.call_ty,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the same as OuterEnums.call_enum_ty? if so, can remove this also!

@@ -170,8 +170,8 @@ pub struct ExtrinsicMetadataIR<T: Form = MetaForm> {
///
/// Note: Field used for metadata V14 only.
pub ty: T::Type,
/// Extrinsic version.
pub version: u8,
Copy link
Contributor

@jsdw jsdw Oct 31, 2024

Choose a reason for hiding this comment

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

Reminder that extensions below will also need to have a shape more like Vec<(extensions_version, Vec<TransactionExtensionMetadataIR>)> or something, but as with the other things I assume this will be a separate PR :)

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.

LGTM!

I saw a couple of things which I assume will be followup PRs, and I suppose this is mainly just about making V16 exposed, but added notes anyways just in case :)

ir.apis.into_iter().map(Into::into).collect(),
ir.outer_enums.into(),
// Substrate does not collect yet the custom metadata fields.
// This allows us to extend the V15 easily.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This allows us to extend the V15 easily.
// This allows us to extend the V16 easily.

am I wrong?

TransactionExtensionMetadata {
identifier: ir.identifier,
ty: ir.ty,
additional_signed: ir.implicit,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe metadata v16 should also rename it to implicit?

// Presume all extensions work for all versions.
// This is true for now.
let transaction_extensions_by_version = ir
.versions
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a different version, isn't it? ir.versions is the transaction version (4 or 5 nowadays), but transaction)extensions_by_version maps the transaction extension version to the list of extensions that apply.

Thus, at the moment we'll have just one version, 0u8, which maps to the list of transaction extensions we have. I guess the ExtrinsicMetadataIR should be tweaked to record this information though, so that it's just mapped more 1-to-1 into this ExtrinsicMetadata?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the extension_version is a different version. Here we should for now just assume 0.

@@ -1409,10 +1409,10 @@ impl SignaturePayload for () {

/// Implementor is an [`Extrinsic`] and provides metadata about this extrinsic.
pub trait ExtrinsicMetadata {
/// The format version of the `Extrinsic`.
/// The format versions of the `Extrinsic`.
///
/// By format is meant the encoded representation of the `Extrinsic`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// By format is meant the encoded representation of the `Extrinsic`.
/// By format we mean the encoded representation of the `Extrinsic`.

@@ -846,7 +846,7 @@ macro_magic = { version = "0.5.1" }
maplit = { version = "1.0.2" }
memmap2 = { version = "0.9.3" }
memory-db = { version = "0.32.0", default-features = false }
merkleized-metadata = { version = "0.1.0" }
merkleized-metadata = { git = "https://github.com/lexnv/merkleized-metadata.git", branch = "lexnv/update-frame-md" }
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
merkleized-metadata = { git = "https://github.com/lexnv/merkleized-metadata.git", branch = "lexnv/update-frame-md" }
merkleized-metadata = "0.1.1"

@@ -100,7 +100,7 @@ impl From<TransactionExtensionMetadataIR> for SignedExtensionMetadata {
impl From<ExtrinsicMetadataIR> for ExtrinsicMetadata {
fn from(ir: ExtrinsicMetadataIR) -> Self {
ExtrinsicMetadata {
version: ir.version,
version: *ir.versions.get(0).expect("Metadata V15 supports only one version"),
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
version: *ir.versions.get(0).expect("Metadata V15 supports only one version"),
version: *ir.versions.iter().min().expect("Metadata V15 supports only one version"),

// Presume all extensions work for all versions.
// This is true for now.
let transaction_extensions_by_version = ir
.versions
Copy link
Member

Choose a reason for hiding this comment

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

Yeah the extension_version is a different version. Here we should for now just assume 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants