Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Metadata type #974

Merged
merged 24 commits into from
May 25, 2023
Merged

Introduce Metadata type #974

merged 24 commits into from
May 25, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented May 22, 2023

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?

  1. 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.
  2. 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.
  3. 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.
  4. 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.

Comment on lines +76 to +77
#trait_name_str,
#method_name_str,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue opened for this: #976

@@ -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();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@jsdw jsdw May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +95 to +97
let mut tmp_dir = std::env::temp_dir();
tmp_dir.push(format!("{TEST_DIR_PREFIX}{idx}"));
let _ = std::fs::remove_dir_all(tmp_dir);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jsdw jsdw marked this pull request as ready for review May 24, 2023 10:55
@jsdw jsdw requested a review from a team as a code owner May 24, 2023 10:55
@@ -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};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@@ -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());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAkes sense; I'll add one :)

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The new metadata API looks solid! 👍

/// 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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()))?;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. 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.
  3. 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 :)

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()))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@jsdw jsdw May 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

.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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps open an issue for it :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good reminder, thank you :)

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good stuff, I like the new metadata API

@jsdw jsdw merged commit b9f5419 into master May 25, 2023
@jsdw jsdw deleted the jsdw-metadata branch May 25, 2023 09:35
@jsdw jsdw mentioned this pull request Jun 1, 2023
tadeohepperle pushed a commit that referenced this pull request Jun 1, 2023
* 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()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants