Introduce Metadata type #974

jsdw · 2023-05-22T12:33:24Z

To now, we've been internally using frame_metadata::RuntimeMetadataV15 (or V14 or whatever in the past). We have various internal functionality that uses this, and then in Subxt we wrap it with some caching and other bits and pieces to expose things in a way Subxt needs.

This PR instead transforms the wire format frame_metadata::RuntimeMetadataV14/frame_metadata::RuntimeMetadataV15 into a Metadata struct. Why?

To take control of the metadata API we expose; validation/retain stuff is all now provided on this Metadata struct, and we expose an interface that gives access to all aspects of the metadata but allows it to internally change over time.
Changes to frame_metadata going forwards (eg new metadata versions) do not force breaking changes in Subxt; we can expose functionality to decode metadata into our Metadata type and continue to operate and expose this same type as it is extended with new functionality etc.
Rely more on scale-info types; rather than wrapping the RuntimeMetadata and duplicating PalletMetadata, ConstantMetadata etc, we just provide direct access to the variants already in the type registry. Practically, this means that we expose everything that we know about more directly, and so don't need to replicate APIs to do the same.
As a result of the above, the subxt_metadata crate becomes much more of a complete crate on its own, rather than a collection of functions that operate on some version of frame_metadata.

For reviewing, I'd suggest that you focus most of your efforts on the metadata folder and subxt_metadata crate (and then see how it's made available also in subxt/src/metadata/metadata_type.rs. Mostly everything else is just fallout from these changes to shift everything to using this new metadata type. I'll add comments for other changes that might be worth more attention.

jsdw · 2023-05-24T10:48:39Z

codegen/src/api/runtime_apis.rs

+                    #trait_name_str,
+                    #method_name_str,


I moved Runtime APIs to be split like calls etc, so that you provide separately the trait name and then method name. One reason for doing this was simply allow consistency in the metadata between how Runtime APIs and Pallet based things are accessed (ie you ask for trait name and then can ask for a specific method or just get docs for the trait ot whatever.

Issue opened for this: #976

jsdw · 2023-05-24T10:50:12Z

codegen/src/api/mod.rs

@@ -632,9 +621,9 @@ impl RuntimeGenerator {

                /// check whether the Client you are using is aligned with the statically generated codegen.
                pub fn validate_codegen<T: #crate_path::Config, C: #crate_path::client::OfflineClientT<T>>(client: &C) -> Result<(), #crate_path::error::MetadataError> {
-                    let runtime_metadata_hash = client.metadata().metadata_hash(&PALLETS);
+                    let runtime_metadata_hash = client.metadata().hasher().only_these_pallets(&PALLETS).hash();


Something I bumped into here; we validate codegen based on only the generated pallets, but we don't take into account if runtime APIs have also been stripped. We should open an issue to ensure that this validation properly takes into account stripped runtime APIs too.

jsdw · 2023-05-24T10:51:21Z

subxt/src/metadata/metadata_type.rs

The metadata exposed from subxt is now just a simple wrapper around subxt_metadata::Metadata so that it can be cloned cheaply (it didn't feel right to wrap the subxt_metadata::Metadata itself in an Arc since we might want to modify it etc at this point (eg .retain() or whatever), and only when it is used by Subxt we are happy with it being read only.

jsdw · 2023-05-24T10:54:08Z

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

+            let mut tmp_dir = std::env::temp_dir();
+            tmp_dir.push(format!("{TEST_DIR_PREFIX}{idx}"));
+            let _ = std::fs::remove_dir_all(tmp_dir);


Simplified because we only care about index and not whether we are iterating all or specific pallets.

lexnv · 2023-05-24T13:29:37Z

subxt/src/client/online_client.rs

@@ -19,8 +19,7 @@ use crate::{
 };
 use derivative::Derivative;
 use futures::future;
-use parking_lot::RwLock;
-use std::sync::Arc;
+use std::sync::{Arc, RwLock};


lexnv · 2023-05-24T13:32:15Z

testing/integration-tests/src/codegen/codegen_tests.rs

@@ -11,7 +11,7 @@ use subxt_codegen::{CratePath, DerivesRegistry, RuntimeGenerator, TypeSubstitute

 fn generate_runtime_interface_from_metadata(metadata: RuntimeMetadataPrefixed) -> String {
    // Generate a runtime interface from the provided metadata.
-    let generator = RuntimeGenerator::new(metadata);
+    let generator = RuntimeGenerator::new(metadata.try_into().unwrap());


nit: Maybe we can provide an expect message to align with the below generator?

MAkes sense; I'll add one :)

lexnv

LGTM! The new metadata API looks solid! 👍

niklasad1 · 2023-05-24T14:09:18Z

metadata/src/utils/ordered_map.rs

+/// A minimal ordered map to let one search for
+/// things by key or get the values in insert order.
+#[derive(Debug, Clone)]
+pub struct OrderedMap<K, V> {


I'm not super happy about this because it's quite hard to understand such as push_insert which inserts one item at the end of the Vec and one item at key in the Map.

but fine :P

I'm happy to hear any suggestions here! My goal is just to preserve the order of items as seen in metadata, but also allow access by some other key (name, mostly). It just exists so that there's no need to manually keep a vec and hashmap in sync for doing this, but perhaps there's a better approach or maybe some api changes that would make it easier to understand? :)

it's fine I tried to think about a better name for this because OrderedMap then it becomes BTreeMap in my head directly then.

VecAndMap or something similar makes more sense to me but OrderedMap is more elegant...

(as a side note, I avoided exposing this at all in the interface so it's super easy to change without breaking anything :))

ok, I didn't look at the code where it's used and yes I see why you added it

niklasad1 · 2023-05-24T14:17:38Z

subxt/src/constants/constants_client.rs

@@ -39,13 +39,14 @@ impl<T: Config, Client: OfflineClientT<T>> ConstantsClient<T, Client> {
            let expected_hash = self
                .client
                .metadata()
-                .constant_hash(address.pallet_name(), address.constant_name())?;
+                .pallet_by_name(address.pallet_name())


dq: is this to ensure that the pallet name was stripped?

I'm not sure I follow; perhaps you could elaborate?:)

I was referring to changed API:

previous version:

client.metadata().constant_hash(address.pallet_name(), address.constant_name())?;

new code:

client .metadata() .pallet_by_name(address.pallet_name()) .ok_or_else(|| MetadataError::PalletNameNotFound(address.pallet_name().to_owned()))? .constant_hash(address.constant_name()) .ok_or_else(|| MetadataError::ConstantNameNotFound(address.constant_name().to_owned()))?;

Aah, I had a couple of reasons for breaking the API into two here:

By getting the pallet first, we then have the information needed to find the relevant constant/storage/etc rather than needing hashmaps with both pallet and constant/sotrage/whatever keys in.

Instead of needing to provide Results back to say whether it was the pallet or constant that couldn't be found, the metadata can just return Options in a lot of cases, and leave it up to eg Subxt how to present the error, which felt cleaner design wise.

There may be other reasons to want to access a pallet by its name or whatever, and in some cases (eg in this PR) it's useful to have both the pallet and the specific event/whatever details to hand back anyway :)

subxt/src/events/events_type.rs

…s_empty()

niklasad1 · 2023-05-25T08:00:52Z

subxt/src/storage/storage_address.rs

-        let storage = pallet.storage(&self.entry_name)?;
+        let pallet = metadata
+            .pallet_by_name(self.pallet_name())
+            .ok_or_else(|| MetadataError::PalletNameNotFound(self.pallet_name().to_owned()))?;


this ok_or_else is repeated quite often, so I guess it could make sense to return Error from these methods i.e, pallet_by_name(), storage(), entry_by_name() but doesn't have to be addressed in this PR

From the PoV of the Metadata API itself, I think Options are cleaner (we either get a pallet or we don't, and we can either find the constant or we can't etc). I think it's up to Subxt to decide how to map those all into errors. I'm sure we can improve this though and eg write some wrapper/helper functions around the subxt::Metadata to handle this better; let me have a look :)

niklasad1 · 2023-05-25T08:02:08Z

subxt/src/runtime_api/runtime_payload.rs

+            .method_by_name(&self.method_name)
+            .ok_or_else(|| MetadataError::RuntimeMethodNotFound((*self.method_name).to_owned()))?;
+
+        // TODO [jsdw]: This isn't good. The options are:


perhaps open an issue for it :)

Good reminder, thank you :)

niklasad1 · 2023-05-25T08:08:54Z

metadata/src/lib.rs

+    }
+
+    /// Access a pallet given its name.
+    pub fn pallet_by_name(&self, pallet_name: &str) -> Option<PalletMetadata<'_>> {


As my previous comment, it would nice to return Result<PalletMetadata<'_>>, PalletNameNotFound> here to avoid doing it when it's called.

I think Option's are cleaner here, and then the thing using the metadata can decide how it should map those into errors (if it wishes), but yeah that bit we can make nicer :)

niklasad1

good stuff, I like the new metadata API

* WIP new Metadata type * Finish basic Metadata impl inc hashing and validation * remove caching from metadata; can add that higher up * remove caches * update retain to use Metadata * clippy fixes * update codegen to use Metadata * clippy * WIP fixing subxt lib * WIP fixing tests, rebuild artifacts, fix OrderedMap::retain * get --all-targets compiling * move DispatchError type lookup back to being optional * cargo clippy * fix docs * re-use VariantIndex to get variants * add docs and enforce docs on metadata crate * fix docs * add test and fix docs * cargo fmt * address review comments * update lockfiles * ExactSizeIter so we can ask for len() of things (and hopefully soon is_empty()

jsdw added 21 commits May 18, 2023 14:46

WIP new Metadata type

7df775b

Finish basic Metadata impl inc hashing and validation

9f97f71

remove caching from metadata; can add that higher up

cfc819b

Merge branch 'master' into jsdw-metadata

fbec048

remove caches

b779437

update retain to use Metadata

c657e1e

clippy fixes

5c53c28

update codegen to use Metadata

fbafbdf

clippy

e3e4c0e

WIP fixing subxt lib

cad8e14

Merge branch 'master' into jsdw-metadata

552d0ed

WIP fixing tests, rebuild artifacts, fix OrderedMap::retain

937d86a

get --all-targets compiling

d90f996

move DispatchError type lookup back to being optional

7ee67dd

cargo clippy

fd17ed9

fix docs

406d7d1

re-use VariantIndex to get variants

93d43ec

add docs and enforce docs on metadata crate

0c89336

fix docs

c4d4cf5

add test and fix docs

061d51b

cargo fmt

48bbf14

jsdw commented May 24, 2023

View reviewed changes

jsdw marked this pull request as ready for review May 24, 2023 10:55

jsdw requested a review from a team as a code owner May 24, 2023 10:55

jsdw mentioned this pull request May 24, 2023

Fix codegen validation when Runtime APIs are stripped #976

Closed

lexnv reviewed May 24, 2023

View reviewed changes

lexnv approved these changes May 24, 2023

View reviewed changes

niklasad1 reviewed May 24, 2023

View reviewed changes

subxt/src/events/events_type.rs Outdated Show resolved Hide resolved

jsdw added 3 commits May 24, 2023 15:31

address review comments

8853a15

update lockfiles

1006f38

ExactSizeIter so we can ask for len() of things (and hopefully soon i…

f0e4893

…s_empty()

niklasad1 reviewed May 25, 2023

View reviewed changes

niklasad1 approved these changes May 25, 2023

View reviewed changes

jsdw merged commit b9f5419 into master May 25, 2023
7 checks passed

jsdw deleted the jsdw-metadata branch May 25, 2023 09:35

jsdw mentioned this pull request Jun 1, 2023

Prep for 0.29.0 release #992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Metadata type #974

Introduce Metadata type #974

jsdw commented May 22, 2023 •

edited

jsdw May 24, 2023

jsdw May 24, 2023

jsdw May 24, 2023

jsdw May 24, 2023 •

edited

jsdw May 24, 2023

lexnv May 24, 2023

lexnv May 24, 2023

jsdw May 24, 2023

lexnv left a comment

niklasad1 May 24, 2023

jsdw May 24, 2023

niklasad1 May 24, 2023

jsdw May 24, 2023

niklasad1 May 25, 2023

niklasad1 May 24, 2023

jsdw May 24, 2023

niklasad1 May 25, 2023

jsdw May 25, 2023

niklasad1 May 25, 2023

jsdw May 25, 2023 •

edited

niklasad1 May 25, 2023

jsdw May 25, 2023

niklasad1 May 25, 2023

jsdw May 25, 2023

niklasad1 left a comment

Introduce Metadata type #974

Introduce Metadata type #974

Conversation

jsdw commented May 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexnv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw May 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

jsdw commented May 22, 2023 •

edited

jsdw May 24, 2023 •

edited

jsdw May 25, 2023 •

edited