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

Metadata V16 (unstable): Enrich metadata with associated types of config traits #5274

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

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Aug 7, 2024

This feature is part of the upcoming metadata V16. The associated types of the Config trait that require the TypeInfo or Parameter bounds are included in the metadata of the pallet. The metadata is not yet exposed to the end-user, however the metadata intermediate representation (IR) contains these types.

Developers can opt out of metadata collection of the associated types by specifying without_metadata optional attribute to the #[pallet::config].

Furthermore, the without_metadata argument can be used in combination with the newly added #[pallet::include_metadata] attribute to selectively include only certain associated types in the metadata collection.

API Design

  • There is nothing to collect from the associated types:
#[pallet::config]
pub trait Config: frame_system::Config {
		// Runtime events already propagated to the metadata.
		type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;

		// Constants are already propagated.
		#[pallet::constant]
		type MyGetParam2: Get<u32>;
	}
  • Default automatic collection of associated types that require TypeInfo or Parameter bounds:
	#[pallet::config]
	pub trait Config: frame_system::Config {
		// Runtime events already propagated to the metadata.
		type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;

		// Constants are already propagated.
		#[pallet::constant]
		type MyGetParam2: Get<u32>;

		// Associated type included by default, because it requires TypeInfo bound.
		/// Nonce doc.
		type Nonce: TypeInfo;

		// Associated type included by default, because it requires
		// Parameter bound (indirect TypeInfo).
		type AccountData: Parameter;

		// Associated type without metadata bounds, not included.
		type NotIncluded: From<u8>;
	}
  • Disable automatic collection
// Associated types are not collected by default.
	#[pallet::config(without_metadata)]
	pub trait Config: frame_system::Config {
		// Runtime events already propagated to the metadata.
		type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;

		// Constants are already propagated.
		#[pallet::constant]
		type MyGetParam2: Get<u32>;

		// Explicitly include associated types.
		#[pallet::include_metadata]
		type Nonce: TypeInfo;

		type AccountData: Parameter;

		type NotIncluded: From<u8>;
	}

Builds on top of the PoC: #4358
Closes: #4519

cc @paritytech/subxt-team

@lexnv lexnv added I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. T11-documentation This PR/Issue is related to documentation. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Aug 8, 2024
@lexnv lexnv marked this pull request as ready for review August 8, 2024 10:36
@lexnv lexnv requested a review from a team as a code owner August 8, 2024 10:36
Signed-off-by: Alexandru Vasile <[email protected]>
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/metadata-v16-updates/9557/1

@@ -320,12 +345,30 @@ pub fn replace_self_by_t(input: proc_macro2::TokenStream) -> proc_macro2::TokenS
.collect()
}

/// Check that the trait item requires the `TypeInfo` bound (or similar).
fn contains_type_info_bound(ty: &TraitItemType) -> bool {
const KNOWN_TYPE_INFO_BOUNDS: &[&str] = &[
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Why not a Set?

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 think we'd need some feature similar to rust-lang/rust#93140 to make the hashSet const here

impl ConfigDef {
pub fn try_from(
frame_system: &syn::Path,
index: usize,
item: &mut syn::Item,
enable_default: bool,
enable_associated_metadata: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe disable_associated_metadata as by default it should be enabled(?)

if config_set.len() != config_values.len() {
return Err(syn::Error::new(
span,
"Invalid duplicated attribute for `#[pallet::config]`. Please remove duplicates.",
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to mention the actual duplicates plus maybe the spans.


impl syn::parse::Parse for ConfigValue {
fn parse(input: syn::parse::ParseStream) -> syn::Result<Self> {
if input.peek(keyword::with_default) {
Copy link
Member

Choose a reason for hiding this comment

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

))
}

if already_constant {
Copy link
Member

Choose a reason for hiding this comment

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

So, if I put include_metadata before constant, this branch will not be taken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this 🙏

Have moved the checks after we iterate over all pallet attributes

substrate/frame/support/src/lib.rs Outdated Show resolved Hide resolved
/// ```
///
/// For more information, see:
/// * [`#[pallet::include_metadata]`](`include_metadata`)
Copy link
Member

Choose a reason for hiding this comment

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

Why are you linking include_metadata when talking about without_metadata?

Copy link
Member

Choose a reason for hiding this comment

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

This other macro does not really provides more information. More like gives you the opportunity to include selected items.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team October 1, 2024 14:54
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Looks nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. T11-documentation This PR/Issue is related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata V16: Enrich metadata with associated types of config traits
5 participants