Don't expose metadata for Runtime APIs that haven't been implemented #6337
Conversation
| @@ -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)] | |||
There was a problem hiding this comment.
Just some random error I encountered: this type is used in a Runtime API and so must impl TypeInfo.
| @@ -334,6 +334,88 @@ pub fn get_deprecation(crate_: &TokenStream, attrs: &[syn::Attribute]) -> Result | |||
| .unwrap_or_else(|| Ok(quote! {#crate_::metadata_ir::DeprecationStatusIR::NotDeprecated})) | |||
| } | |||
|
|
|||
| /// Represents an API version. | |||
There was a problem hiding this comment.
I didn't change any of this code; just moved it to the utils module so that I could use it in runtime_metadata too.
| @@ -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 {} | |||
There was a problem hiding this comment.
I needed to instantiate this to call runtime_metadata() on it in the test, hence changing to struct.
jsdw
added
I2-bug
polkadot/Cargo.lock
Outdated
|
I've taken a look, but I haven't run the macro expansion, it's remaining the same, minus the unimplemented methods, right? Also can you update frame-support tests? |
| @@ -48,6 +48,8 @@ pub struct MetadataIR<T: Form = MetaForm> { | |||
| pub struct RuntimeApiMetadataIR<T: Form = MetaForm> { | |||
| /// Trait name. | |||
| pub name: T::String, | |||
| /// The base api_version declared on the trait. | |||
There was a problem hiding this comment.
We don't need any of these changes, if you do the filtering directly in runtime_metadata by forwarding there the impl api version.
There was a problem hiding this comment.
Makes sense; I can remove the base_version from that and rely on mod_name::VERSION instead!
One small thing this bumps into is that VERSION is a u32 whereas we use u64s to track method versions (which causes a type error when comparing one with the other). Any objection to me making method_versions be treated as u32s too (or alternately, make VERSION a u64)?
…tytech/polkadot-sdk into jsdw-versioned-runtime-api-metadata
| impl<T: Form> RuntimeApiMetadataIR<T> { | ||
| /// This returns a version of `Self` keeping only the | ||
| /// methods for which the provided function returns true. | ||
| pub fn keeping_methods<F>(self, f: F) -> Self |
There was a problem hiding this comment.
What I mean is that you can also remove this, just forward the api_version to runtime_metadata.
There was a problem hiding this comment.
But different methods can be implemented at different versions, so my understanding is that I need to look at the version that was implemented and then filter out all those methods that have a version greater than this?
There was a problem hiding this comment.
ie if we have:
decl_runtime_apis! {
#[api_version(2)]
pub trait ApiWithMultipleVersions {
fn stable_one(data: u64);
#[api_version(3)]
fn new_one();
#[api_version(4)]
fn glory_one();
}
}
impl_runtime_apis! {
#[api_version(3)]
impl self::ApiWithMultipleVersions<Block> for Runtime {
fn stable_one(_: u64) {}
fn new_one() {}
}
}Then I see that we are implementing version 3 of the APIs, and so I filter out methods declared higher than that (above, glory_one) using keeping_methods since they haven't been implemented
There was a problem hiding this comment.
fn runtime_metadata(impl_version: u32);
runtime_metadata(version_from_the_attribute);
What I propose is to move the filtering to the runtime_metadata function. We are calling this function any way and then filter the functions. Instead we could filter directly in runtime_metadata.
There was a problem hiding this comment.
Ooh I see, gotcha! I considered that early on but probably opted not to change the interface :) Lemme try this and see what it looks like!
There was a problem hiding this comment.
Done! This means sp-metadata-ir is untouched now which is nice, though I guess means a major bump on sp-proc-macro since the interface changes a little. The code is a bit simpler I think now though which I like!
…tytech/polkadot-sdk into jsdw-versioned-runtime-api-metadata
|
/cmd --help |
Command help: |
|
/cmd fmt |
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
…tytech/polkadot-sdk into jsdw-versioned-runtime-api-metadata
…tytech/polkadot-sdk into jsdw-versioned-runtime-api-metadata
|
/cmd fmt |
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
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 thatimpl_runtime_apismight 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
u32s (rather than VERSION beingu32andapi_versions beingu64) for consistency / ease of comparison.A test is added which works with both the
enable-staging-apifeature in api/tests enabled or disabled, to check all of this.