diff --git a/prdoc/pr_6337.prdoc b/prdoc/pr_6337.prdoc new file mode 100644 index 000000000000..aeab61cdf933 --- /dev/null +++ b/prdoc/pr_6337.prdoc @@ -0,0 +1,17 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Don't expose metadata for Runtime APIs that haven't been implemented + +doc: + - audience: Runtime User + description: | + Prior to this PR, the metadata for runtime APIs would contain all methods for the + latest version of each API, regardless of which version a runtime implements. This + PR fixes that, so that the runtime API metadata reflects what is actually implemented. + +crates: + - name: sp-api-proc-macro + bump: major + - name: sp-consensus-babe + bump: patch \ No newline at end of file diff --git a/substrate/frame/support/test/tests/runtime_metadata.rs b/substrate/frame/support/test/tests/runtime_metadata.rs index 81377210eb43..7523a415d458 100644 --- a/substrate/frame/support/test/tests/runtime_metadata.rs +++ b/substrate/frame/support/test/tests/runtime_metadata.rs @@ -178,7 +178,7 @@ fn runtime_metadata() { RuntimeApiMethodMetadataIR { name: "wild_card", inputs: vec![RuntimeApiMethodParamMetadataIR:: { - name: "_", + name: "__runtime_api_generated_name_0__", ty: meta_type::(), }], output: meta_type::<()>(), diff --git a/substrate/primitives/api/proc-macro/src/decl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/decl_runtime_apis.rs index cb213f2fd627..ddca1095a192 100644 --- a/substrate/primitives/api/proc-macro/src/decl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/decl_runtime_apis.rs @@ -32,6 +32,7 @@ use proc_macro2::{Span, TokenStream}; use quote::quote; +use std::collections::{BTreeMap, HashMap}; use syn::{ fold::{self, Fold}, parse::{Error, Parse, ParseStream, Result}, @@ -43,8 +44,6 @@ use syn::{ TraitItem, TraitItemFn, }; -use std::collections::{BTreeMap, HashMap}; - /// The structure used for parsing the runtime api declarations. struct RuntimeApiDecls { decls: Vec, @@ -133,7 +132,7 @@ fn remove_supported_attributes(attrs: &mut Vec) -> HashMap<&'static s /// ``` fn generate_versioned_api_traits( api: ItemTrait, - methods: BTreeMap>, + methods: BTreeMap>, ) -> Vec { let mut result = Vec::::new(); for (version, _) in &methods { @@ -189,15 +188,12 @@ fn generate_runtime_decls(decls: &[ItemTrait]) -> Result { extend_generics_with_block(&mut decl.generics); let mod_name = generate_runtime_mod_name_for_trait(&decl.ident); let found_attributes = remove_supported_attributes(&mut decl.attrs); - let api_version = - get_api_version(&found_attributes).map(|v| generate_runtime_api_version(v as u32))?; + let api_version = get_api_version(&found_attributes).map(generate_runtime_api_version)?; let id = generate_runtime_api_id(&decl.ident.to_string()); - let metadata = crate::runtime_metadata::generate_decl_runtime_metadata(&decl); - let trait_api_version = get_api_version(&found_attributes)?; - let mut methods_by_version: BTreeMap> = BTreeMap::new(); + let mut methods_by_version: BTreeMap> = BTreeMap::new(); // Process the items in the declaration. The filter_map function below does a lot of stuff // because the method attributes are stripped at this point @@ -255,6 +251,12 @@ fn generate_runtime_decls(decls: &[ItemTrait]) -> Result { _ => (), }); + let versioned_methods_iter = methods_by_version + .iter() + .flat_map(|(&version, methods)| methods.iter().map(move |method| (method, version))); + let metadata = + crate::runtime_metadata::generate_decl_runtime_metadata(&decl, versioned_methods_iter); + let versioned_api_traits = generate_versioned_api_traits(decl.clone(), methods_by_version); let main_api_ident = decl.ident.clone(); @@ -505,7 +507,7 @@ fn generate_runtime_api_version(version: u32) -> TokenStream { } /// Generates the implementation of `RuntimeApiInfo` for the given trait. -fn generate_runtime_info_impl(trait_: &ItemTrait, version: u64) -> TokenStream { +fn generate_runtime_info_impl(trait_: &ItemTrait, version: u32) -> TokenStream { let trait_name = &trait_.ident; let crate_ = generate_crate_access(); let id = generate_runtime_api_id(&trait_name.to_string()); @@ -537,7 +539,7 @@ fn generate_runtime_info_impl(trait_: &ItemTrait, version: u64) -> TokenStream { } /// Get changed in version from the user given attribute or `Ok(None)`, if no attribute was given. -fn get_changed_in(found_attributes: &HashMap<&'static str, Attribute>) -> Result> { +fn get_changed_in(found_attributes: &HashMap<&'static str, Attribute>) -> Result> { found_attributes .get(&CHANGED_IN_ATTRIBUTE) .map(|v| parse_runtime_api_version(v).map(Some)) @@ -545,7 +547,7 @@ fn get_changed_in(found_attributes: &HashMap<&'static str, Attribute>) -> Result } /// Get the api version from the user given attribute or `Ok(1)`, if no attribute was given. -fn get_api_version(found_attributes: &HashMap<&'static str, Attribute>) -> Result { +fn get_api_version(found_attributes: &HashMap<&'static str, Attribute>) -> Result { found_attributes .get(&API_VERSION_ATTRIBUTE) .map(parse_runtime_api_version) @@ -610,7 +612,7 @@ impl CheckTraitDecl { /// /// Any error is stored in `self.errors`. fn check_method_declarations<'a>(&mut self, methods: impl Iterator) { - let mut method_to_signature_changed = HashMap::>>::new(); + let mut method_to_signature_changed = HashMap::>>::new(); methods.into_iter().for_each(|method| { let attributes = remove_supported_attributes(&mut method.attrs.clone()); diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index de922e3253e4..89d0665f28cf 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -15,14 +15,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -use crate::{ - common::API_VERSION_ATTRIBUTE, - utils::{ - extract_block_type_from_trait_path, extract_impl_trait, - extract_parameter_names_types_and_borrows, generate_crate_access, - generate_runtime_mod_name_for_trait, parse_runtime_api_version, prefix_function_with_trait, - versioned_trait_name, AllowSelfRefInParameters, RequireQualifiedTraitPath, - }, +use crate::utils::{ + extract_api_version, extract_block_type_from_trait_path, extract_impl_trait, + extract_parameter_names_types_and_borrows, generate_crate_access, + generate_runtime_mod_name_for_trait, prefix_function_with_trait, versioned_trait_name, + AllowSelfRefInParameters, ApiVersion, RequireQualifiedTraitPath, }; use proc_macro2::{Span, TokenStream}; @@ -31,11 +28,10 @@ use quote::quote; use syn::{ fold::{self, Fold}, - parenthesized, parse::{Error, Parse, ParseStream, Result}, parse_macro_input, parse_quote, spanned::Spanned, - Attribute, Ident, ImplItem, ItemImpl, LitInt, LitStr, Path, Signature, Type, TypePath, + Attribute, Ident, ImplItem, ItemImpl, Path, Signature, Type, TypePath, }; use std::collections::HashMap; @@ -466,7 +462,7 @@ fn extend_with_runtime_decl_path(mut trait_: Path) -> Path { trait_ } -fn extend_with_api_version(mut trait_: Path, version: Option) -> Path { +fn extend_with_api_version(mut trait_: Path, version: Option) -> Path { let version = if let Some(v) = version { v } else { @@ -841,88 +837,6 @@ fn filter_cfg_attrs(attrs: &[Attribute]) -> Vec { attrs.iter().filter(|a| a.path().is_ident("cfg")).cloned().collect() } -/// Parse feature flagged api_version. -/// E.g. `#[cfg_attr(feature = "enable-staging-api", api_version(99))]` -fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result> { - let cfg_attrs = attrs.iter().filter(|a| a.path().is_ident("cfg_attr")).collect::>(); - - let mut cfg_api_version_attr = Vec::new(); - for cfg_attr in cfg_attrs { - let mut feature_name = None; - let mut api_version = None; - cfg_attr.parse_nested_meta(|m| { - if m.path.is_ident("feature") { - let a = m.value()?; - let b: LitStr = a.parse()?; - feature_name = Some(b.value()); - } else if m.path.is_ident(API_VERSION_ATTRIBUTE) { - let content; - parenthesized!(content in m.input); - let ver: LitInt = content.parse()?; - api_version = Some(ver.base10_parse::()?); - } - Ok(()) - })?; - - // If there is a cfg attribute containing api_version - save if for processing - if let (Some(feature_name), Some(api_version)) = (feature_name, api_version) { - cfg_api_version_attr.push((feature_name, api_version, cfg_attr.span())); - } - } - - if cfg_api_version_attr.len() > 1 { - let mut err = Error::new(span, format!("Found multiple feature gated api versions (cfg attribute with nested `{}` attribute). This is not supported.", API_VERSION_ATTRIBUTE)); - for (_, _, attr_span) in cfg_api_version_attr { - err.combine(Error::new(attr_span, format!("`{}` found here", API_VERSION_ATTRIBUTE))); - } - - return Err(err); - } - - Ok(cfg_api_version_attr - .into_iter() - .next() - .map(|(feature, name, _)| (feature, name))) -} - -/// Represents an API version. -struct ApiVersion { - /// Corresponds to `#[api_version(X)]` attribute. - pub custom: Option, - /// Corresponds to `#[cfg_attr(feature = "enable-staging-api", api_version(99))]` - /// attribute. `String` is the feature name, `u64` the staging api version. - pub feature_gated: Option<(String, u64)>, -} - -// Extracts the value of `API_VERSION_ATTRIBUTE` and handles errors. -// Returns: -// - Err if the version is malformed -// - `ApiVersion` on success. If a version is set or not is determined by the fields of `ApiVersion` -fn extract_api_version(attrs: &Vec, span: Span) -> Result { - // First fetch all `API_VERSION_ATTRIBUTE` values (should be only one) - let api_ver = attrs - .iter() - .filter(|a| a.path().is_ident(API_VERSION_ATTRIBUTE)) - .collect::>(); - - if api_ver.len() > 1 { - return Err(Error::new( - span, - format!( - "Found multiple #[{}] attributes for an API implementation. \ - Each runtime API can have only one version.", - API_VERSION_ATTRIBUTE - ), - )); - } - - // Parse the runtime version if there exists one. - Ok(ApiVersion { - custom: api_ver.first().map(|v| parse_runtime_api_version(v)).transpose()?, - feature_gated: extract_cfg_api_version(attrs, span)?, - }) -} - #[cfg(test)] mod tests { use super::*; diff --git a/substrate/primitives/api/proc-macro/src/runtime_metadata.rs b/substrate/primitives/api/proc-macro/src/runtime_metadata.rs index 4cba524dbe25..6be396339259 100644 --- a/substrate/primitives/api/proc-macro/src/runtime_metadata.rs +++ b/substrate/primitives/api/proc-macro/src/runtime_metadata.rs @@ -17,14 +17,11 @@ use proc_macro2::TokenStream as TokenStream2; use quote::quote; -use syn::{parse_quote, ItemImpl, ItemTrait, Result}; - -use crate::{ - common::CHANGED_IN_ATTRIBUTE, - utils::{ - extract_impl_trait, filter_cfg_attributes, generate_crate_access, - generate_runtime_mod_name_for_trait, get_doc_literals, RequireQualifiedTraitPath, - }, +use syn::{parse_quote, spanned::Spanned, ItemImpl, ItemTrait, Result}; + +use crate::utils::{ + extract_api_version, extract_impl_trait, filter_cfg_attributes, generate_crate_access, + generate_runtime_mod_name_for_trait, get_doc_literals, RequireQualifiedTraitPath, }; /// Get the type parameter argument without lifetime or mutability @@ -72,7 +69,10 @@ fn collect_docs(attrs: &[syn::Attribute], crate_: &TokenStream2) -> TokenStream2 /// /// The metadata is exposed as a generic function on the hidden module /// of the trait generated by the `decl_runtime_apis`. -pub fn generate_decl_runtime_metadata(decl: &ItemTrait) -> TokenStream2 { +pub fn generate_decl_runtime_metadata<'a>( + decl: &ItemTrait, + versioned_methods_iter: impl Iterator, +) -> TokenStream2 { let crate_ = generate_crate_access(); let mut methods = Vec::new(); @@ -86,17 +86,7 @@ pub fn generate_decl_runtime_metadata(decl: &ItemTrait) -> TokenStream2 { // This restricts the bounds at the metadata level, without needing to modify the `BlockT` // itself, since the concrete implementations are already satisfying `TypeInfo`. let mut where_clause = Vec::new(); - for item in &decl.items { - // Collect metadata for methods only. - let syn::TraitItem::Fn(method) = item else { continue }; - - // Collect metadata only for the latest methods. - let is_changed_in = - method.attrs.iter().any(|attr| attr.path().is_ident(CHANGED_IN_ATTRIBUTE)); - if is_changed_in { - continue; - } - + for (method, version) in versioned_methods_iter { let mut inputs = Vec::new(); let signature = &method.sig; for input in &signature.inputs { @@ -135,14 +125,21 @@ pub fn generate_decl_runtime_metadata(decl: &ItemTrait) -> TokenStream2 { Ok(deprecation) => deprecation, Err(e) => return e.into_compile_error(), }; + + // Methods are filtered so that only those whose version is <= the `impl_version` passed to + // `runtime_metadata` are kept in the metadata we hand back. methods.push(quote!( #( #attrs )* - #crate_::metadata_ir::RuntimeApiMethodMetadataIR { - name: #method_name, - inputs: #crate_::vec![ #( #inputs, )* ], - output: #output, - docs: #docs, - deprecation_info: #deprecation, + if #version <= impl_version { + Some(#crate_::metadata_ir::RuntimeApiMethodMetadataIR { + name: #method_name, + inputs: #crate_::vec![ #( #inputs, )* ], + output: #output, + docs: #docs, + deprecation_info: #deprecation, + }) + } else { + None } )); } @@ -176,12 +173,15 @@ pub fn generate_decl_runtime_metadata(decl: &ItemTrait) -> TokenStream2 { #crate_::frame_metadata_enabled! { #( #attrs )* #[inline(always)] - pub fn runtime_metadata #impl_generics () -> #crate_::metadata_ir::RuntimeApiMetadataIR + pub fn runtime_metadata #impl_generics (impl_version: u32) -> #crate_::metadata_ir::RuntimeApiMetadataIR #where_clause { #crate_::metadata_ir::RuntimeApiMetadataIR { name: #trait_name, - methods: #crate_::vec![ #( #methods, )* ], + methods: [ #( #methods, )* ] + .into_iter() + .filter_map(|maybe_m| maybe_m) + .collect(), docs: #docs, deprecation_info: #deprecation, } @@ -242,10 +242,43 @@ pub fn generate_impl_runtime_metadata(impls: &[ItemImpl]) -> Result Result { +pub fn parse_runtime_api_version(version: &Attribute) -> Result { let version = version.parse_args::().map_err(|_| { Error::new( version.span(), @@ -231,7 +231,7 @@ pub fn parse_runtime_api_version(version: &Attribute) -> Result { } /// Each versioned trait is named 'ApiNameVN' where N is the specific version. E.g. ParachainHostV2 -pub fn versioned_trait_name(trait_ident: &Ident, version: u64) -> Ident { +pub fn versioned_trait_name(trait_ident: &Ident, version: u32) -> Ident { format_ident!("{}V{}", trait_ident, version) } @@ -334,6 +334,89 @@ pub fn get_deprecation(crate_: &TokenStream, attrs: &[syn::Attribute]) -> Result .unwrap_or_else(|| Ok(quote! {#crate_::metadata_ir::DeprecationStatusIR::NotDeprecated})) } +/// Represents an API version. +pub struct ApiVersion { + /// Corresponds to `#[api_version(X)]` attribute. + pub custom: Option, + /// Corresponds to `#[cfg_attr(feature = "enable-staging-api", api_version(99))]` + /// attribute. `String` is the feature name, `u32` the staging api version. + pub feature_gated: Option<(String, u32)>, +} + +/// Extracts the value of `API_VERSION_ATTRIBUTE` and handles errors. +/// Returns: +/// - Err if the version is malformed +/// - `ApiVersion` on success. If a version is set or not is determined by the fields of +/// `ApiVersion` +pub fn extract_api_version(attrs: &[Attribute], span: Span) -> Result { + // First fetch all `API_VERSION_ATTRIBUTE` values (should be only one) + let api_ver = attrs + .iter() + .filter(|a| a.path().is_ident(API_VERSION_ATTRIBUTE)) + .collect::>(); + + if api_ver.len() > 1 { + return Err(Error::new( + span, + format!( + "Found multiple #[{}] attributes for an API implementation. \ + Each runtime API can have only one version.", + API_VERSION_ATTRIBUTE + ), + )); + } + + // Parse the runtime version if there exists one. + Ok(ApiVersion { + custom: api_ver.first().map(|v| parse_runtime_api_version(v)).transpose()?, + feature_gated: extract_cfg_api_version(attrs, span)?, + }) +} + +/// Parse feature flagged api_version. +/// E.g. `#[cfg_attr(feature = "enable-staging-api", api_version(99))]` +fn extract_cfg_api_version(attrs: &[Attribute], span: Span) -> Result> { + let cfg_attrs = attrs.iter().filter(|a| a.path().is_ident("cfg_attr")).collect::>(); + + let mut cfg_api_version_attr = Vec::new(); + for cfg_attr in cfg_attrs { + let mut feature_name = None; + let mut api_version = None; + cfg_attr.parse_nested_meta(|m| { + if m.path.is_ident("feature") { + let a = m.value()?; + let b: LitStr = a.parse()?; + feature_name = Some(b.value()); + } else if m.path.is_ident(API_VERSION_ATTRIBUTE) { + let content; + parenthesized!(content in m.input); + let ver: LitInt = content.parse()?; + api_version = Some(ver.base10_parse::()?); + } + Ok(()) + })?; + + // If there is a cfg attribute containing api_version - save if for processing + if let (Some(feature_name), Some(api_version)) = (feature_name, api_version) { + cfg_api_version_attr.push((feature_name, api_version, cfg_attr.span())); + } + } + + if cfg_api_version_attr.len() > 1 { + let mut err = Error::new(span, format!("Found multiple feature gated api versions (cfg attribute with nested `{}` attribute). This is not supported.", API_VERSION_ATTRIBUTE)); + for (_, _, attr_span) in cfg_api_version_attr { + err.combine(Error::new(attr_span, format!("`{}` found here", API_VERSION_ATTRIBUTE))); + } + + return Err(err); + } + + Ok(cfg_api_version_attr + .into_iter() + .next() + .map(|(feature, name, _)| (feature, name))) +} + #[cfg(test)] mod tests { use assert_matches::assert_matches; diff --git a/substrate/primitives/api/test/tests/decl_and_impl.rs b/substrate/primitives/api/test/tests/decl_and_impl.rs index 211a08561fd4..890cf6eccdbc 100644 --- a/substrate/primitives/api/test/tests/decl_and_impl.rs +++ b/substrate/primitives/api/test/tests/decl_and_impl.rs @@ -24,7 +24,7 @@ use substrate_test_runtime_client::runtime::{Block, Hash}; /// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real /// runtime. -pub enum Runtime {} +pub struct Runtime {} decl_runtime_apis! { pub trait Api { @@ -306,3 +306,62 @@ fn mock_runtime_api_works_with_advanced() { mock.wild_card(Hash::repeat_byte(0x01), 1).unwrap_err().to_string(), ); } + +#[test] +fn runtime_api_metadata_matches_version_implemented() { + let rt = Runtime {}; + let runtime_metadata = rt.runtime_metadata(); + + // Check that the metadata for some runtime API matches expectation. + let assert_has_api_with_methods = |api_name: &str, api_methods: &[&str]| { + let Some(api) = runtime_metadata.iter().find(|api| api.name == api_name) else { + panic!("Can't find runtime API '{api_name}'"); + }; + if api.methods.len() != api_methods.len() { + panic!( + "Wrong number of methods in '{api_name}'; expected {} methods but got {}: {:?}", + api_methods.len(), + api.methods.len(), + api.methods + ); + } + for expected_name in api_methods { + if !api.methods.iter().any(|method| &method.name == expected_name) { + panic!("Can't find API method '{expected_name}' in '{api_name}'"); + } + } + }; + + assert_has_api_with_methods("ApiWithCustomVersion", &["same_name"]); + + assert_has_api_with_methods("ApiWithMultipleVersions", &["stable_one", "new_one"]); + + assert_has_api_with_methods( + "ApiWithStagingMethod", + &[ + "stable_one", + #[cfg(feature = "enable-staging-api")] + "staging_one", + ], + ); + + assert_has_api_with_methods( + "ApiWithStagingAndVersionedMethods", + &[ + "stable_one", + "new_one", + #[cfg(feature = "enable-staging-api")] + "staging_one", + ], + ); + + assert_has_api_with_methods( + "ApiWithStagingAndChangedBase", + &[ + "stable_one", + "new_one", + #[cfg(feature = "enable-staging-api")] + "staging_one", + ], + ); +} diff --git a/substrate/primitives/consensus/babe/src/lib.rs b/substrate/primitives/consensus/babe/src/lib.rs index ee07da6829f5..163fbafa8dd4 100644 --- a/substrate/primitives/consensus/babe/src/lib.rs +++ b/substrate/primitives/consensus/babe/src/lib.rs @@ -134,7 +134,7 @@ pub enum ConsensusLog { } /// Configuration data used by the BABE consensus engine. -#[derive(Clone, PartialEq, Eq, Encode, Decode, RuntimeDebug)] +#[derive(Clone, PartialEq, Eq, Encode, Decode, RuntimeDebug, TypeInfo)] pub struct BabeConfigurationV1 { /// The slot duration in milliseconds for BABE. Currently, only /// the value provided by this type at genesis will be used.