Track additional composite enum metadata

[KiChjang]

In paritytech/substrate#13722, we've added support to allow pallets to optionally specify and expose an enum which gets aggregated into a bigger enum with other pallets containing the same enum with the same name in construct_runtime. We'd like to expose this information as metadata for the pallet and the runtime.

This is the same feature as requested in #48 (comment).

[jsdw]

To make sure that I understand the linked issue, I think the idea is that pallets can use some attribute like #[hold_reason(Foo)], #[hold_reason(Bar)], #[freeze_reason(Wibble)], and the result is that we have enums generated like:

enum HoldReason { Foo, Bar }
enum FreezeReason { Wibble }

Is that correct?

Metadata represents similar things (eg calls) using a variant type, so I'd likely suggest that we add something like a fund_ty property that pointed to a vec of variant types in the type registry which mirror all of the known enums. This would allow for custom things beyond HoldReason or whatever as well if that feature is implemented, without breaking metadata.

Does that make sense? I might have misunderstood something fundamental :)

[KiChjang]

Yes, this is similar to how events and errors get aggregated into RuntimeEvent and RuntimeError in construct_runtime, except that the syntax to do so isn't #[hold_reason] but simply #[pallet::composite_enum]. The name of the enum will then be used to determine the name of the aggregated enum in construct_runtime.

[jsdw]

Actually maybe I misunderstood a little; the thing you want in the metadata is the concrete values picked for those enums rather than the description of the enums themselves (ie the TypeInfo stuff)? I think you mentioned a HashMap at some point, so perhaps we need a BTreeMap (so that it can be scale encoded) of enum name to enum value in the metadata for each pallet?

(I guess I want to check; what will you be doing with this metadata?)

[bkchr]

Maybe we should start having the possibility to add random types to the metadata.

[jsdw]

Random values (I think that's what you mean, but maybe there's value in allowing random types to be added to the registry without associated values?) that were interesting could exist in a shape like:

BTreeMap<String, { type_id: u32, value: Vec<u8> }>

And that could exist both in individual pallets and at the global level in the metadata, where the type_id points into the type registry so we know how to decode it and can generate the type or whatever.

(We could also use the scale-value::Value enum, but I think the above would be more general/useful offhand)

Does this make sense? If so, we could add something like this to accomodate this hold_reason etc stuff and then we'd have the flexibility to support other semi-arbitrary values that people may want access to.

[KiChjang]

It should either be a HashMap or a BTreeMap, because not every pallet is going to contain a FreezeReason, HoldReason, LockId or a SlashReason, and so you can't have a field that is named similar to any of the previously mentioned enums.

Moreover, and this is probably a bit too premature to say so, what we wanted to do eventually is to expand the functionality of the #[pallet::composite_enum] attribute so that it supports any enum with custom naming, so having the proper infrastructure now to support that use case would also be nice, hence the HashMap or BTreeMap approach.

[jsdw]

(We could also use the scale-value::Value enum, but I think the above would be more general/useful offhand)

Ah to be clear, I didn't mean use this instead of a BTreeMap, but just instead of the value/tpye_id bit (but I prefer the value/type_id approach).

what we wanted to do eventually is to expand the functionality of the #[pallet::composite_enum] attribute so that it supports any enum with custom naming

I'm not very familiar with these things; could you give an example of how that'd be used and what the outputs would be that we'd want to be saving to metadata?

(does the BTreeMap<String, { type_id: u32, value: Vec<u8> }> approach cover this?)

[KiChjang]

Yes, the BTreeMap approach covers it. All that we need is to not have the metadata contain a field that's named after any of the composite_enum names, as they may not exist on each and every pallet.

[KiChjang]

Let me try to elaborate in more detail:

The #[pallet::composite_enum] is used like the following:

#[pallet::composite_enum]
pub enum HoldReason {
    Staking,
    Governance,
}

Where HoldReason is called the enum name and can contain any arbitrary number of variants.

During construct_runtime, if the pallet containing the HoldReason is included in the runtime, an enum like the following will be generated:

pub enum RuntimeHoldReason {
    PalletName(pallet::HoldReason),
}

This entire mechanism here is similar to how RuntimeOrigin, RuntimeError, RuntimeEvent and RuntimeCall is generated. The only difference is that every runtime metadata always contains an origin, error, event or call field, even though the runtime doesn't contain pallets that have a custom origin, error, event or call.

However, all of HoldReason, LockId, FreezeReason and SlashReason are completely optional, so we cannot have a runtime metadata field that is named similar to the aforementioned 4 pallet parts. Thus, the BTreeMap approach here is the correct way to handle it.

[jsdw]

Ooh I see, so it _is _ about arbitrary types and not values then?

So the RuntimeCall etc types are basically just referenced a little like this in metadata (actually call is a bad example but whatever):

call_ty: u32

and then in the type registry we have some type info which is sortof like Variant{ name: "RuntimeCall", variants: vec![...] }.

And what you're saying is that In this case, we can't have concrete fields like call_ty etc because the enums are entirely optional might not exist.

So I guess we could have something in metadata like this (at the top level):

BTreeMap<String, { type_id: u32 }>

Where the string is eg "HoldReason" and the type_id points to the type registry which would have the actual variant info like Variant { name: "HoldReason", variants: vec![{ variant: "Staking", type_id: 123 }, { variant: "Governance", type_id: 456 }] }.

Then we can also add arbitrary custom types behind some key if there is a need to expose other types in the metadata that may be useful.

A question I still have about all of this is: assuming these are now in the metadata; how would people use these types? What value do they have to users?

I mean, basically every other type we store in the type registry is either an argument to some call or storage entry or whatever (so we need it to encode data to make that call/query storage), or it's the result of some call or whatever (so we know how to decode the stuff we get back from the node).

Where would this HoldReason enum for instance fit into all of that? what would users be needing to do with this enum type info?

From the original issue:

This is primarily used in pallets that wish to provide a reason for freezing funds, holding funds, locking funds and/or slashing funds in them.

So there must be a value corresponding to this enum somewhere? How does the user get hold of it? Is it stored behind a runtime API or some constant or something? If so then the type would end up being in the metadata anyway because we store info about all constants and runtime APIs and storage entries. Or is the idea that we do need some arbitrary value storage in the metadata that isn't any of those, to also store a value for HoldReason or whatever?

(sorry for the longer than expected reply; just typing as I ponder this really)

[KiChjang]

One example of where HoldReason is used is in the NIS pallet config:

https://github.com/paritytech/substrate/blob/58be496f61f98b8e04fbf11a46e56bb9c2b3d9c7/frame/nis/src/lib.rs#L222-L225

You can imagine that it will be very useful for the front-end UI to display just which pallet is putting the funds on hold, for what reason, and most importantly, the amount of funds that the pallet is holding.

As an aside, do note that the concept of "holds" and "freezes" are new concepts -- they are designed to replace the concept of "reserves". I don't exactly recall the details, but this issue in Substrate explains it comprehensively.

[jsdw]

One example of where HoldReason is used is in the NIS pallet config:

https://github.com/paritytech/substrate/blob/58be496f61f98b8e04fbf11a46e56bb9c2b3d9c7/frame/nis/src/lib.rs#L222-L225

Isn't that just a constant though, which would be stored in the metadata with a corresponding type ID already?

[KiChjang]

Is HoldReason already stored in the metadata? If so, then this issue would be about making the metadata extensible, per above.

[jsdw]

Is HoldReason already stored in the metadata? If so, then this issue would be about making the metadata extensible, per above.

I started a substrate --dev node and pointed Polkadot.js at it and there does seem to be an nis.holdReason():

Looking at the metadata it appears to be an enum with one variant:

{
  "id": 459,
  "type": {
    "path": [
      "kitchensink_runtime",
      "HoldReason"
    ],
    "def": {
      "variant": {
        "variants": [
          {
            "name": "Nis",
            "index": 0
          }
        ]
      }
    }
  }
},

I can't say whether that's expected offhand, but it exists already :)

If so, then this issue would be about making the metadata extensible, per above.

Is there still some need to store types or values that aren't constants/storage types/tx types etc then?

[KiChjang]

Wait, this is simply because the NIS pallet contains a config item called HoldReason and we display that in the metadata. What I want instead is for the pallet to also expose the HoldReason enum annotated with #[pallet::composite_enum].

[KiChjang]

Following a private conversation with @jsdw, it's now clear to me that:

1. Pallet metadata need not contain each and every pallet part; case in point would be #[pallet::origin]
2. The purpose of putting things into metadata is purely for encoding/decoding, and not for displaying all relevant type info of a pallet
3. Types that appear in extrinsic arguments are automatically put into the type registry

Given the above findings, it becomes apparent that there's no further need to create an extra field in PalletMetadata to house any composite_enum. Closing as resolved.

[jsdw]

Perfect!

The purpose of putting things into metadata is purely for encoding/decoding, and not for displaying all relevant type info of a pallet

I guess I'd expand this slightly to "the purpose of putting things into metadata is to provide all necessary information for things to make use of our APIs" (quite general I know), which is encoding/decoding but also eg providing documentation, allowing for codegen, or whatever. So I'd def understand if a future need to expose pallet config that is useful for UIs to present in some way (but not useful for encoding/decoding etc) comes up :)