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

Metadata V15: Expose types for the overarching Call, Event, Error enums #14143

Merged
merged 57 commits into from
Jun 28, 2023

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented May 15, 2023

This PR exposes the overarching types for RuntimeEvent andRuntimeCall and RuntimeError for the V15 metadata.

The RuntimeEvent andRuntimeCall are represented as is.

The code that previously generate the RuntimeEvent is extended to now generate the RuntimeError enum since they share almost 95% of the functionality.

Construct Runtime Changes

Runtimes can be constructed implicitly or explicitly.
To ensure that the Error enum is propagated to the metadata for each pallet that declares one with #[pallet::error] macro the construction of the runtime is changed:

State Machine

The construct_runtime transitioned from an implicit to an explicit state.
For explicit declarations, we are now including the Error<T> part if defined.

# Old declaration
implicit -> explicit

# New declaration
implicit -> explicit -> explicit expanded

This introduces a new state explicit expanded which transforms the construct_runtime in a similar manner.
For optimization purposes, any implicit declaration is transformed into an explicit expanded form (ie if all pallets are declared implicit, then we'll expand the macro only once as before).

The tt_extra_parts is added to each pallet definition and is responsible for creating a token stream of expanded pallet parts:

expanded :: { Error }

This effectively transforms:

# implicit declaration
Balances: frame_balances

# Transformed to explicit declaration
Balances: frame_balances expanded::{} ::{Pallet, Call, ...}

For explicit declarations:

# Explict declaration
Balances: frame_balances ::{Pallet, Call}

# Transformed to explicit expanded declaration
# This exposes by default the `Error` pallet part if defined.
# Pallet parts are collected into the same vector: `Pallet`, `Call`, and `Error`.
Balances: frame_balances expanded::{ Error } ::{Pallet, Call}

polkadot companion: paritytech/polkadot#7401

Depends on: paritytech/frame-metadata#57
Closes paritytech/frame-metadata#43.

// @paritytech/subxt-team

@lexnv lexnv requested a review from a team May 15, 2023 11:40
@lexnv lexnv self-assigned this May 15, 2023
@lexnv lexnv added A0-please_review Pull request needs code review. 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”. labels May 15, 2023
@lexnv lexnv added the B1-note_worthy Changes should be noted in the release notes label May 15, 2023
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

seems good to me, apart from comments below

frame/support/Cargo.toml Outdated Show resolved Hide resolved
primitives/metadata-ir/Cargo.toml Outdated Show resolved Hide resolved
primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved
frame/support/src/lib.rs Outdated Show resolved Hide resolved
@sam0x17 sam0x17 self-requested a review May 23, 2023 14:47
@kianenigma kianenigma requested a review from a team May 25, 2023 14:43
@lexnv lexnv requested review from bkchr and jsdw June 21, 2023 10:24
@lexnv
Copy link
Contributor Author

lexnv commented Jun 21, 2023

After having a discussion with @bkchr, it is possible to have the RuntimeError without any breaking changes to users that explicitly declare the runtime.

That being said, this PR changes a bit the construct_runtime! logic to add an extra expansion of the pallet parts in case the construct_runtime! is not fully expanded.

The implicit state of pallets: System: frame_system will transition directly to the end state System: expanded::{Pallet, Call...}. The keyword expanded is added to mark the end of the transitions, and users should not bother with this.

The explicit part was previously the final state. Now, explicit parts System: frame_system::{Pallet, Call} will transition similarly to implicit parts to the end state: System: frame_system ::expanded::{Error} :: {Pallet, Call}.
Note that additional parts are now added to the expanded section. We forward currently just the Error iff defined, but could change this in the future to include more parts.

Please let me know what you think of it considering it is touching a bit more places of code now @bkchr, @jsdw , @sam0x17 🙏

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Reviewed most of it, will hopefully do the rest this evening!

@@ -52,8 +52,8 @@ construct_runtime! {
NodeBlock = Block,
UncheckedExtrinsic = UncheckedExtrinsic
{
System: frame_system::{Pallet, Call, Storage, Config, Event<T>},
Pallet: pallet::{Pallet, ValidateUnsigned},
System: frame_system expanded::{}::{Pallet, Call, Storage, Config, Event<T>},
Copy link
Member

Choose a reason for hiding this comment

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

Why do you provide here expanded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be the "final state" of the construct_runtime!, without this the macro will try to get expanded one more time. And the expansions require the pallets to expose tt_extra_parts macro, and for dummy pallets that's not the case (not sure here if this is constructed with only valid pallets 🤔 )

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a bug?

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've reverted this to remove the expanded word, the output looks similar to before.
Some small differences in how the compiler reports the error, most probably related to the extra expansion

Example

missing_event_generic_on_module_with_instance.rs (this is the entire file)

use frame_support::construct_runtime;

construct_runtime! {
	pub struct Runtime where
		Block = Block,
		NodeBlock = Block,
		UncheckedExtrinsic = UncheckedExtrinsic
	{
		System: system::{Pallet},
		Balance: balances::<Instance1>::{Event},
	}
}

fn main() {}

If we don't add the expanded keyword this would produce:

error[E0433]: failed to resolve: use of undeclared crate or module `balances`
  --> tests/construct_runtime_ui/missing_event_generic_on_module_with_instance.rs:10:12
   |
10 |         Balance: balances::<Instance1> ::{Event},
   |                  ^^^^^^^^ use of undeclared crate or module `balances`

When we would actually expect:

error: Instantiable pallet with no generic `Event` cannot be constructed: pallet `Balance` must have generic `Event`
  --> $DIR/missing_event_generic_on_module_with_instance.rs:10:3
   |
10 |         Balance: balances::<Instance1> ::{Event},
   |         ^^^^^^^

I'm presuming that because the construct_runtime! is not in it's final expression (does not contain the expanded), the macro tries to expand the pallet one more time. In doing so it calls balances::tt_extra_parts!, which does not exist (ie theres no balances module).

The "Instantiable pallet with no generic Event cannot be constructed: pallet Balance must have generic" compile error should be coming from the final step of the construct_runtime! macro. And I think it's not outputted/detected because the missing module error has priority for the compiler 🤔

Let me know if this sounds right :D

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 would love to get your feedback on the above 🙏

frame/support/procedural/src/construct_runtime/parse.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/construct_runtime/parse.rs Outdated Show resolved Hide resolved
@kianenigma
Copy link
Contributor

kianenigma commented Jun 27, 2023

@gupnik I suggest you taking a look at the construct_runtime changes. This might also be relevant to paritytech/polkadot-sdk#232.

@lexnv
Copy link
Contributor Author

lexnv commented Jun 28, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 9f6fecf into master Jun 28, 2023
@paritytech-processbot paritytech-processbot bot deleted the lexnv/metadata_outer_enums branch June 28, 2023 09:44
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…ms (paritytech#14143)

* frame-metadata: Point to unreleased branch

Signed-off-by: Alexandru Vasile <[email protected]>

* frame: Generalize outer enum generation for events and errors

Signed-off-by: Alexandru Vasile <[email protected]>

* frame: Remove individual generation of outer enum events

Signed-off-by: Alexandru Vasile <[email protected]>

* primitives/traits: Add marker trait for outer runtime enums

Signed-off-by: Alexandru Vasile <[email protected]>

* frame: Derive Clone, PartialEq, Eq for RuntimeEvents only

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/pallet: Include `#[pallet::error]` enum into pallet parts

Signed-off-by: Alexandru Vasile <[email protected]>

* metadata-ir: Include call, event, error types

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/metadata: Include outer enum types in V15 metadata

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/tests: Ensure `RuntimeError` includes `#[pallet::error]` parts

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/support: Document the reserved name for `RuntimeError`

Signed-off-by: Alexandru Vasile <[email protected]>

* frame: Use self-generated `RuntimeEvent` for `GetRuntimeOuterEnumTypes`

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/ui: Fix UI tests

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/support: Remove unused system path

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/ui: Unexpected field and reintroduce frame_system::Config for RuntimeCall

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/support: Remove `GetRuntimeOuterEnumTypes` marker trait

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/support: Remove `;` from macro

Signed-off-by: Alexandru Vasile <[email protected]>

* Update frame-metadata to point to unreleased branch

Signed-off-by: Alexandru Vasile <[email protected]>

* Rename error_enum_ty to module_error_enum_ty

Signed-off-by: Alexandru Vasile <[email protected]>

* Update module_error_ty documentation

Signed-off-by: Alexandru Vasile <[email protected]>

* frame: Implement from_dispatch_error

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/support: Adjust test to ModuleErrorType

Signed-off-by: Alexandru Vasile <[email protected]>

* Fix clippy

Signed-off-by: Alexandru Vasile <[email protected]>

* Improve documentation

Signed-off-by: Alexandru Vasile <[email protected]>

* frame/tests: Check `from_dispatch_error` impl

Signed-off-by: Alexandru Vasile <[email protected]>

* Update frame-metadata

Signed-off-by: Alexandru Vasile <[email protected]>

* Remove the module_error_ty

Signed-off-by: Alexandru Vasile <[email protected]>

* Apply fmt

Signed-off-by: Alexandru Vasile <[email protected]>

* Revert unneeded parts

Signed-off-by: Alexandru Vasile <[email protected]>

* Revert "Revert unneeded parts"

This reverts commit b94bbd1.

Revert "Apply fmt"

This reverts commit 9b1c3e7.

Revert "Remove the module_error_ty"

This reverts commit 98de5b2.

* Update frame-metadata to origin/master

Signed-off-by: Alexandru Vasile <[email protected]>

* Add outerEnums to the metadata

Signed-off-by: Alexandru Vasile <[email protected]>

* Add tests

Signed-off-by: Alexandru Vasile <[email protected]>

* Keep backwards compatibility for explicit pallet parts

Signed-off-by: Alexandru Vasile <[email protected]>

* Rename tt_error_part to be more generic

Signed-off-by: Alexandru Vasile <[email protected]>

* Increase recursion_limit to 1k

Signed-off-by: Alexandru Vasile <[email protected]>

* Rename `fully_expanded` to `expanded`

Signed-off-by: Alexandru Vasile <[email protected]>

* Improve documentation

Signed-off-by: Alexandru Vasile <[email protected]>

* Adjust UI tests

Signed-off-by: Alexandru Vasile <[email protected]>

* Update UI tests

Signed-off-by: Alexandru Vasile <[email protected]>

* Update undefined_validate_unsigned_part.stderr UI test

Signed-off-by: Alexandru Vasile <[email protected]>

* Adjust yet again

Signed-off-by: Alexandru Vasile <[email protected]>

* Optimise macro expansions

Signed-off-by: Alexandru Vasile <[email protected]>

* Use latest frame-metadata and rename `moduleErrorType` to `RuntimeError`

Signed-off-by: Alexandru Vasile <[email protected]>

* Fix comment

Signed-off-by: Alexandru Vasile <[email protected]>

* Apply fmt

Signed-off-by: Alexandru Vasile <[email protected]>

* Update frame/support/procedural/src/construct_runtime/parse.rs

Co-authored-by: Bastian Köcher <[email protected]>

* Update frame/support/procedural/src/construct_runtime/parse.rs

Co-authored-by: Bastian Köcher <[email protected]>

* Update frame-metadata PR

Signed-off-by: Alexandru Vasile <[email protected]>

* Remove `expanded` from error messages and fix typo

Signed-off-by: Alexandru Vasile <[email protected]>

* Move docs to the function

Signed-off-by: Alexandru Vasile <[email protected]>

* ui: Use the intermed syntax for pallet parts

Signed-off-by: Alexandru Vasile <[email protected]>

* Update frame-metadata with latest release

Signed-off-by: Alexandru Vasile <[email protected]>

* frame: Address feedback for `from_dispatch_error`

Signed-off-by: Alexandru Vasile <[email protected]>

---------

Signed-off-by: Alexandru Vasile <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
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
Status: Done
Development

Successfully merging this pull request may close these issues.

Expose types for the overarching Call, Error, Event types
7 participants