Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Metadata V15: Enrich extrinsic type info for decoding #14123

Merged
merged 29 commits into from
Jun 28, 2023

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented May 11, 2023

Extrinsic Metadata fields

Metadata is enriched to provide users with additional types for decoding extrinsics:

  • Address type
  • Call type
  • Signature type
  • Extra type

Although already available on the extrinsic's type generic parameters, having the fields
readily available provides enough information for users to decode extrinsics without
making assumptions about the metadata shape (e.g., this eliminates the need of subxt
to parse those fields manually here, feature also requested by the linked issue).

Note: the Extra type similar to others can be used to decode the extrinsics, as well as to
skip decoding over those fields (see subxt's decoding).

Incomplete UncheckedExtrisnic type

It may be possible for certain substrate-based chains, or even substrate tests, to construct a
runtime with different types, or missing types. For those edge cases, the type reported back in
the metadata is the type of the tuple meta_type::<()>().

Depends on frame-metadata release.
Closes #12929.

Subxt Testing

  • Fetched the unstable metadata
  • Type IDs match the extrinsic type params
# This is the extrinsic type with enriched info
"extrinsic": {
    "ty": 786,
    "version": 4,
    "address_ty": 161,
    "call_ty": 135,
    "signature_ty": 348,
    "extra_ty": 787,

# This is the extrinsic.ty which has the type params exactly the
# same as what is reported in the extrinsic for address, call, signature, extra
    "id": 786,
    "type": {
      "path": [
        "sp_runtime",
        "generic",
        "unchecked_extrinsic",
        "UncheckedExtrinsic"
      ],
      "params": [
        {
          "name": "Address",
          "type": 161
        },
        {
          "name": "Call",
          "type": 135
        },
        {
          "name": "Signature",
          "type": 348
        },
        {
          "name": "Extra",
          "type": 787
        }
      ],

// @paritytech/subxt-team @jsdw @bkchr

lexnv added 6 commits May 11, 2023 11:42
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>
@lexnv lexnv requested review from a team, jsdw and bkchr May 11, 2023 11:29
@lexnv lexnv self-assigned this May 11, 2023
@lexnv lexnv added A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit C1-low PR touches the given topic and has a low impact on builders. labels May 11, 2023
lexnv added 5 commits May 15, 2023 23:06
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>
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks overall good, just one suggestion about using a type alias.

@@ -86,7 +86,11 @@ where
/// Submit transaction onchain by providing the call and an optional signature
pub fn submit_transaction(
call: <T as SendTransactionTypes<LocalCall>>::OverarchingCall,
signature: Option<<T::Extrinsic as ExtrinsicT>::SignaturePayload>,
signature: Option<(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why we have to break down SignaturePayload? can't this have TypeInfo itself?

And if it is being broken down and not used, I rather actually delete SignaturePayload if it is not being used anymore.

@@ -106,13 +106,20 @@ impl<Address, Call, Signature, Extra: SignedExtension> Extrinsic
{
type Call = Call;

type SignaturePayload = (Address, Signature, Extra);
Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, it is being done already 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, a type SignaturePayloadOf<E> = (<E as Extrinsic>::X, <E as Extrinsic>::Y, <E as Extrinsic>::Z) will help prevent potential error where the order of these 3 might be mistaken.

@paritytech-ci paritytech-ci requested a review from a team May 26, 2023 12:43
@jsdw
Copy link
Contributor

jsdw commented May 26, 2023

Does extrinsic.ty have any use I wonder with this change? In Subxt at least, the only reason to ever inspect the type was to find out the type IDs of the Signature/Extra/etc bits, which are now handily provided by this change, so perhaps it can be removed from the metadata.

@ascjones might be able to provide some insight into the motivation for including that type in the first place (or maybe it's been around much longer than V14?)

/// Usually it will contain a `Signature` and
/// may include some additional data that are specific to signed
/// extrinsics.
type SignaturePayload;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of breaking most of the code we could just do

type SignaturePayload: SignaturePayload;
trait SignaturePayload {
      type Address: TypeInfo;
      type Signature: TypeInfo;
      type Extra: TypeInfo;
}

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, I like this approach more! Thanks for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bkchr What's your opinion on having the extrinsic.ty in the metadata after this change (the message James mentioned above)?

We should have enough type info by relying on:

  • address_ty
  • call_ty
  • signature_ty
  • extra_ty

The extrinsic.ty has a manual implementation of TypeInfo that adds the above ty as generic types, but since we explicitly expose that info with this PR, I believe we should remove the extrinsic.ty.

That is unless we'll break other things

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea if someone is using this. However, frankly speaking by only having the types you listed you don't know the encoding of the Extrinsic type. While currently it may also already not being correct as it probably doesn't include the Compact<u32> length in from of the extrinsinc encoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I'll add the TypeDefCompact<u32> type to the extrinsic metadata to provide even better decoding abilities to end users. Considering this, I'll remove the extrinsic_ty from the metadata, unless encountering some edge-cases from UX teams.

The type only contained the address, call, signature and extra on its type info definition, and users should be able to obtain those details (and more) directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

iirc the extrinsic type was quite useless anyway before; it mostly just said "hey we want to encode the extrinsic to some vec of bytes". It doesn't provide any useful information on how those bytes need to be formed etc.

There are already a bunch of things we must just assume already about encoding extrinsics, ie the order of the signature stuff, how the not-really-scale Option for the signature is encoded (ie it has a version number &&'d to it) etc. I've always assumed this format information to be encoded in the tx version (4 atm); if a transaction has this version, it must be encoded in this way.

The metadata, in my mind, only needs to provide the information that might vary from chain to chain (eg the actual additional params etc). This tx version can encode the actual format that this stuff is turned into an extrinsic.

All of this to say, what would adding a TypeDefCompact<u32> to the metadata help with? I'd assume it to be part of the V4 extrinsic format I guess, but perhaps I'm wrong on this :)

Copy link
Member

Choose a reason for hiding this comment

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

There are already a bunch of things we must just assume already about encoding extrinsics, ie the order of the signature stuff, how the not-really-scale Option for the signature is encoded (ie it has a version number &&'d to it) etc. I've always assumed this format information to be encoded in the tx version (4 atm); if a transaction has this version, it must be encoded in this way.

Good point.

The metadata, in my mind, only needs to provide the information that might vary from chain to chain (eg the actual additional params etc). This tx version can encode the actual format that this stuff is turned into an extrinsic.

Just because every body is currently using our UncheckedExtrinsic doesn't mean that this will always be the case. However, it makes sense that this is a type that we can not depict in as type metadata.

lexnv added 3 commits May 29, 2023 13:28
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv added 3 commits May 29, 2023 15:49
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>
@lexnv lexnv requested a review from a team May 29, 2023 13:40
lexnv added 3 commits May 29, 2023 17:06
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>
primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved
lexnv and others added 5 commits June 2, 2023 14:46
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>
@lexnv
Copy link
Contributor Author

lexnv commented Jun 5, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

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.

Looks great; well done!

Remember to release frame-metadata etc before merging :)

…ode_info

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

lexnv commented Jun 28, 2023

Polkadot companion is expected to fail since the latest companion conflicts, but will be solved by paritytech/polkadot#7401 (comment)

@lexnv
Copy link
Contributor Author

lexnv commented Jun 28, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit e00031e into master Jun 28, 2023
@paritytech-processbot paritytech-processbot bot deleted the lexnv/extrinsic_decode_info branch June 28, 2023 12:44
@lexnv lexnv mentioned this pull request Jun 29, 2023
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* metadata-ir: Add extrinsic type info to decode address, call, sig

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

* frame-metadata: Point to unreleased branch

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

* metadata-ir: Include addrees, call, signature in V15 conversion

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

* metadata-ir: Include extra ty

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

* construct_runtime: Extract address,call,sig,extra ty from tx type

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

* frame/tests: Check metadata populates xt types correctly

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

* metadata-ir/tests: Add extra fields on ExtrinsicMetadataIR

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

* primitives/traits: Expand the `Extrinsic::SignaturePayload`

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

* primitives: Adjust to new `Extrinsic` associated types

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

* frame/metadata: Simplify metadata generation

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

* frame/example: Adjust to new interface

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

* frame/tests: Adjust `extrinsic_metadata_ir_types`

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

* Revert the additional Extrinsic' associated types

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

* primitives: Add `SignaturePayload` marker trait

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

* primitives: Implement SignaturePayload for empty tuple

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

* Adjust to new SignaturePayload trait

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

* tests: Adjust `extrinsic_metadata_ir_types` to new interface

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

* frame/support: Adjust pallet test

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

* frame: Add Extrinsic length prefix to the metadata

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

* primitives: Populate `ExtrinsicMetadataIR` with `len_ty`

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

* Update primitives/runtime/src/traits.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

* Apply cargo fmt

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

* v15: Remove len type of the extrinsic

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

* cargo: Update frame-metadata

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

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V14 metadata doesn't provide enough information to decode extrinsic
5 participants