Support V16 metadata and refactor metadata code (#1967)

* WIP integrate unstable v16 metadata into Subxt

* first pass moving retain to the CLI tool

* Remove otuer enum variant stripping and move now simpler strip_metadata to new crate. test it

* tidyup to use stripmetadata package etc

* Fix / comment out tests

* fmt

* clippy

* Fix wasm example

* wasm-example fix

* wasm-example fix

* Maske sure to move IDs around after types.retain()

* fmt

* Tweak comment

* Find dispatch error separately to avoid issues during mapping

* Expose associated type information in pallet metadata

* Hopefully fix flaky archive RPC

* remove unwanted temp file

* Address nits

* Add back commented-otu tests and address review comments

* use either, and simplify for_each

testing/integration-tests/src/full_client/client/archive_rpcs.rs

@@ -10,8 +10,7 @@ use crate::{
     utils::{node_runtime, TestNodeProcess},
 };
 use codec::Encode;
-use futures::{stream, Stream, StreamExt};
-use std::task::Poll;
+use futures::{Stream, StreamExt};
 use subxt::{
     blocks::Block,
     client::OnlineClient,
@@ -98,15 +97,31 @@ async fn archive_unstable_call() {
 async fn archive_unstable_finalized_height() {
     let ctx = test_context().await;
     let rpc = ctx.chainhead_rpc_methods().await;
-    let mut blocks = fetch_finalized_blocks(&ctx, 3).await;
 
-    while let Some(block) = blocks.next().await {
-        let subxt_block_height = block.number() as usize;
+    // This test is quite ugly. Originally, we asked for finalized blocks from subxt and
+    // asserted that the archive height we then get back matches, but that is subject to
+    // races between subxt's stream and reality (and failed surprisingly often). To try
+    // to avoid this, we weaken the test to just check that the height increments over time.
+    let mut last_block_height = None;
+    loop {
+        // Fetch archive block height.
         let archive_block_height = rpc.archive_unstable_finalized_height().await.unwrap();
 
-        // Note: may be prone to race if call is super slow for some reason, since a new
-        // block may have been finalized since subxt reported it.
-        assert_eq!(subxt_block_height, archive_block_height);
+        // On a dev node we expect blocks to be finalized 1 by 1, so panic
+        // if the height we fetch has grown by more than 1.
+        if let Some(last) = last_block_height {
+            if archive_block_height != last && archive_block_height != last + 1 {
+                panic!("Archive block height should increase 1 at a time, but jumped from {last} to {archive_block_height}");
+            }
+        }
+
+        last_block_height = Some(archive_block_height);
+        if archive_block_height > 5 {
+            break;
+        }
+
+        // Wait a little before looping
+        tokio::time::sleep(tokio::time::Duration::from_secs(1)).await;
     }
 }
 

testing/integration-tests/src/full_client/metadata_validation.rs

@@ -3,16 +3,41 @@
 // see LICENSE for license details.
 
 use crate::{node_runtime, subxt_test, test_context, TestContext};
-use frame_metadata::v15::{
-    CustomMetadata, ExtrinsicMetadata, OuterEnums, PalletCallMetadata, PalletMetadata,
-    PalletStorageMetadata, RuntimeMetadataV15, StorageEntryMetadata, StorageEntryModifier,
-    StorageEntryType,
+use codec::Decode;
+use frame_metadata::{
+    v15::{
+        CustomMetadata, ExtrinsicMetadata, OuterEnums, PalletCallMetadata, PalletMetadata,
+        PalletStorageMetadata, RuntimeMetadataV15, StorageEntryMetadata, StorageEntryModifier,
+        StorageEntryType,
+    },
+    RuntimeMetadata, RuntimeMetadataPrefixed,
 };
 use scale_info::{
     build::{Fields, Variants},
     meta_type, Path, Type, TypeInfo,
 };
-use subxt::{Metadata, OfflineClient, SubstrateConfig};
+use subxt::{Metadata, OfflineClient, OnlineClient, SubstrateConfig};
+
+async fn fetch_v15_metadata(client: &OnlineClient<SubstrateConfig>) -> RuntimeMetadataV15 {
+    let payload = node_runtime::apis().metadata().metadata_at_version(15);
+    let runtime_metadata_bytes = client
+        .runtime_api()
+        .at_latest()
+        .await
+        .unwrap()
+        .call(payload)
+        .await
+        .unwrap()
+        .unwrap()
+        .0;
+    let runtime_metadata = RuntimeMetadataPrefixed::decode(&mut &*runtime_metadata_bytes)
+        .unwrap()
+        .1;
+    let RuntimeMetadata::V15(v15_metadata) = runtime_metadata else {
+        panic!("Metadata is not v15")
+    };
+    v15_metadata
+}
 
 async fn metadata_to_api(metadata: Metadata, ctx: &TestContext) -> OfflineClient<SubstrateConfig> {
     OfflineClient::new(
@@ -27,15 +52,6 @@ fn v15_to_metadata(v15: RuntimeMetadataV15) -> Metadata {
     subxt_md.into()
 }
 
-fn modified_metadata<F>(metadata: Metadata, f: F) -> Metadata
-where
-    F: FnOnce(&mut RuntimeMetadataV15),
-{
-    let mut metadata = RuntimeMetadataV15::from((*metadata).clone());
-    f(&mut metadata);
-    v15_to_metadata(metadata)
-}
-
 fn default_pallet() -> PalletMetadata {
     PalletMetadata {
         name: "Test",
@@ -97,72 +113,72 @@ fn pallets_to_metadata(pallets: Vec<PalletMetadata>) -> Metadata {
     ))
 }
 
-#[subxt_test]
-async fn metadata_converting_works_ok() {
-    let ctx = test_context().await;
-    let api = ctx.client();
-
-    assert!(
-        node_runtime::is_codegen_valid_for(&api.metadata()),
-        "Should be valid initially"
-    );
-
-    let metadata = RuntimeMetadataV15::from((*api.metadata()).clone());
-    let metadata = v15_to_metadata(metadata);
-
-    assert!(
-        node_runtime::is_codegen_valid_for(&metadata),
-        "Should still be valid after conversion back and forth"
-    );
-}
-
 #[subxt_test]
 async fn full_metadata_check() {
     let ctx = test_context().await;
     let api = ctx.client();
+    let mut v15_metadata = fetch_v15_metadata(&api).await;
 
-    // Runtime metadata is identical to the metadata used during API generation.
-    assert!(node_runtime::is_codegen_valid_for(&api.metadata()));
+    // Runtime metadata is identical to the metadata we just downloaded
+    let metadata_before = v15_to_metadata(v15_metadata.clone());
+    assert!(node_runtime::is_codegen_valid_for(&metadata_before));
 
     // Modify the metadata.
-    let metadata = modified_metadata(api.metadata(), |md| {
-        md.pallets[0].name = "NewPallet".to_string();
-    });
+    v15_metadata.pallets[0].name = "NewPallet".to_string();
 
     // It should now be invalid:
-    assert!(!node_runtime::is_codegen_valid_for(&metadata));
+    let metadata_after = v15_to_metadata(v15_metadata);
+    assert!(!node_runtime::is_codegen_valid_for(&metadata_after));
 }
 
 #[subxt_test]
 async fn constant_values_are_not_validated() {
     let ctx = test_context().await;
     let api = ctx.client();
+    let mut v15_metadata = fetch_v15_metadata(&api).await;
+
+    // Build an api from our v15 metadata to confirm that it's good, just like
+    // the metadata downloaded by the API itself.
+    let api_from_original_metadata = {
+        let metadata_before = v15_to_metadata(v15_metadata.clone());
+        metadata_to_api(metadata_before, &ctx).await
+    };
 
     let deposit_addr = node_runtime::constants().balances().existential_deposit();
 
     // Retrieve existential deposit to validate it and confirm that it's OK.
-    assert!(api.constants().at(&deposit_addr).is_ok());
+    assert!(api_from_original_metadata
+        .constants()
+        .at(&deposit_addr)
+        .is_ok());
 
     // Modify the metadata.
-    let metadata = modified_metadata(api.metadata(), |md| {
-        let existential = md
-            .pallets
-            .iter_mut()
-            .find(|pallet| pallet.name == "Balances")
-            .expect("Metadata must contain Balances pallet")
-            .constants
-            .iter_mut()
-            .find(|constant| constant.name == "ExistentialDeposit")
-            .expect("ExistentialDeposit constant must be present");
+    let existential = v15_metadata
+        .pallets
+        .iter_mut()
+        .find(|pallet| pallet.name == "Balances")
+        .expect("Metadata must contain Balances pallet")
+        .constants
+        .iter_mut()
+        .find(|constant| constant.name == "ExistentialDeposit")
+        .expect("ExistentialDeposit constant must be present");
 
-        // Modifying a constant value should not lead to an error:
-        existential.value = vec![0u8; 32];
-    });
+    // Modifying a constant value should not lead to an error:
+    existential.value = vec![0u8; 32];
 
-    let api = metadata_to_api(metadata, &ctx).await;
+    // Build our API again, this time form the metadata we've tweaked.
+    let api_from_modified_metadata = {
+        let metadata_before = v15_to_metadata(v15_metadata);
+        metadata_to_api(metadata_before, &ctx).await
+    };
 
-    assert!(node_runtime::is_codegen_valid_for(&api.metadata()));
-    assert!(api.constants().at(&deposit_addr).is_ok());
+    assert!(node_runtime::is_codegen_valid_for(
+        &api_from_modified_metadata.metadata()
+    ));
+    assert!(api_from_modified_metadata
+        .constants()
+        .at(&deposit_addr)
+        .is_ok());
 }
 
 #[subxt_test]

testing/ui-tests/Cargo.toml

@@ -16,4 +16,5 @@ frame-metadata = { workspace = true }
 codec = { package = "parity-scale-codec", workspace = true, features = ["derive", "bit-vec"] }
 subxt = { workspace = true, features = ["native", "jsonrpsee", "runtime-wasm-path"] }
 subxt-metadata = { workspace = true }
+subxt-utils-stripmetadata = { workspace = true }
 generate-custom-metadata = { path = "../generate-custom-metadata" }

testing/ui-tests/src/lib.rs

@@ -17,11 +17,32 @@ mod storage;
 mod utils;
 
 use crate::utils::MetadataTestRunner;
+use frame_metadata::{RuntimeMetadata, RuntimeMetadataPrefixed};
+use subxt_utils_stripmetadata::StripMetadata;
 
 // Each of these tests leads to some rust code being compiled and
 // executed to test that compilation is successful (or errors in the
 // way that we'd expect).
 
+fn strip_metadata<Pallets, Apis>(
+    metadata: &mut RuntimeMetadataPrefixed,
+    pallets: Pallets,
+    apis: Apis,
+) where
+    Pallets: Fn(&str) -> bool,
+    Apis: Fn(&str) -> bool,
+{
+    match &mut metadata.1 {
+        RuntimeMetadata::V14(m) => m.strip_metadata(pallets, apis),
+        RuntimeMetadata::V15(m) => m.strip_metadata(pallets, apis),
+        RuntimeMetadata::V16(m) => m.strip_metadata(pallets, apis),
+        m => panic!(
+            "Metadata should be V14, V15 or V16, but is V{}",
+            m.version()
+        ),
+    }
+}
+
 #[test]
 fn ui_tests() {
     let mut m = MetadataTestRunner::default();
@@ -62,7 +83,7 @@ fn ui_tests() {
     // Test retaining only specific pallets and ensure that works.
     for pallet in ["Babe", "Claims", "Grandpa", "Balances"] {
         let mut metadata = MetadataTestRunner::load_metadata();
-        metadata.retain(|p| p == pallet, |_| true);
+        strip_metadata(&mut metadata, |p| p == pallet, |_| true);
 
         t.pass(
             m.new_test_case()
@@ -74,7 +95,7 @@ fn ui_tests() {
     // Test retaining only specific runtime APIs to ensure that works.
     for runtime_api in ["Core", "Metadata"] {
         let mut metadata = MetadataTestRunner::load_metadata();
-        metadata.retain(|_| true, |r| r == runtime_api);
+        strip_metadata(&mut metadata, |_| true, |r| r == runtime_api);
 
         t.pass(
             m.new_test_case()
@@ -87,7 +108,8 @@ fn ui_tests() {
     // client state is full:
     {
         let mut metadata = MetadataTestRunner::load_metadata();
-        metadata.retain(
+        strip_metadata(
+            &mut metadata,
             |p| ["Babe", "Claims"].contains(&p),
             |r| ["Core", "Metadata"].contains(&r),
         );
@@ -104,12 +126,17 @@ fn ui_tests() {
     // _not_ compare valid against client with differently stripped metadata.
     {
         let mut codegen_metadata = MetadataTestRunner::load_metadata();
-        codegen_metadata.retain(
+        strip_metadata(
+            &mut codegen_metadata,
             |p| ["Babe", "Claims"].contains(&p),
             |r| ["Core", "Metadata"].contains(&r),
         );
         let mut validation_metadata = MetadataTestRunner::load_metadata();
-        validation_metadata.retain(|p| p != "Claims", |r| r != "Metadata");
+        strip_metadata(
+            &mut validation_metadata,
+            |p| p != "Claims",
+            |r| r != "Metadata",
+        );
 
         t.pass(
             m.new_test_case()

testing/ui-tests/src/utils/metadata_test_runner.rs

@@ -3,8 +3,8 @@
 // see LICENSE for license details.
 
 use codec::{Decode, Encode};
+use frame_metadata::RuntimeMetadataPrefixed;
 use std::io::Read;
-use subxt_metadata::Metadata;
 
 static TEST_DIR_PREFIX: &str = "subxt_generated_ui_tests_";
 static METADATA_FILE: &str = "../../artifacts/polkadot_metadata_full.scale";
@@ -17,7 +17,7 @@ pub struct MetadataTestRunner {
 impl MetadataTestRunner {
     /// Loads metadata that we can use in our tests. Panics if
     /// there is some issue decoding the metadata.
-    pub fn load_metadata() -> Metadata {
+    pub fn load_metadata() -> RuntimeMetadataPrefixed {
         let mut file =
             std::fs::File::open(METADATA_FILE).expect("Cannot open metadata.scale artifact");
 
@@ -25,7 +25,7 @@ impl MetadataTestRunner {
         file.read_to_end(&mut bytes)
             .expect("Failed to read metadata.scale file");
 
-        Metadata::decode(&mut &*bytes).expect("Cannot decode metadata bytes")
+        RuntimeMetadataPrefixed::decode(&mut &*bytes).expect("Cannot decode metadata bytes")
     }
 
     /// Create a new test case.
@@ -54,7 +54,7 @@ impl Drop for MetadataTestRunner {
 pub struct MetadataTestRunnerCaseBuilder {
     index: usize,
     name: String,
-    validation_metadata: Option<Metadata>,
+    validation_metadata: Option<RuntimeMetadataPrefixed>,
     should_be_valid: bool,
 }
 
@@ -76,7 +76,7 @@ impl MetadataTestRunnerCaseBuilder {
 
     /// Set metadata to be validated against the generated code.
     /// By default, we'll validate the same metadata used to generate the code.
-    pub fn validation_metadata(mut self, md: impl Into<Metadata>) -> Self {
+    pub fn validation_metadata(mut self, md: impl Into<RuntimeMetadataPrefixed>) -> Self {
         self.validation_metadata = Some(md.into());
         self
     }
@@ -100,15 +100,12 @@ impl MetadataTestRunnerCaseBuilder {
     ///
     /// The generated code will be tidied up when the `MetadataTestRunner` that
     /// this was handed out from is dropped.
-    pub fn build<M>(self, macro_metadata: M) -> String
-    where
-        M: TryInto<Metadata>,
-        M::Error: std::fmt::Debug,
-    {
-        let macro_metadata = macro_metadata.try_into().expect("can into Metadata");
-        let validation_metadata = self
-            .validation_metadata
-            .unwrap_or_else(|| macro_metadata.clone());
+    pub fn build(self, macro_metadata: frame_metadata::RuntimeMetadataPrefixed) -> String {
+        let validation_metadata = self.validation_metadata.unwrap_or_else(|| {
+            // RuntimeMetadataPrefixed doesn't implement Clone for some reason (we should prob fix that).
+            // until then, this hack clones it by encoding and then decoding it again from bytes..
+            clone_via_encode(&macro_metadata)
+        });
 
         let index = self.index;
         let mut tmp_dir = std::env::temp_dir();
@@ -178,3 +175,8 @@ impl MetadataTestRunnerCaseBuilder {
         tmp_rust_path
     }
 }
+
+fn clone_via_encode<T: codec::Encode + codec::Decode>(item: &T) -> T {
+    let bytes = item.encode();
+    T::decode(&mut &*bytes).unwrap()
+}