-
Notifications
You must be signed in to change notification settings - Fork 262
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
Support V16 metadata and refactor metadata code #1967
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors metadata handling by removing the bidirectional conversion from subxt::Metadata to frame_metadata and by streamlining metadata stripping while adding support for v16 metadata features such as pallet view functions and pallet associated types. Key changes include:
- Removing and refactoring outer enum hash logic and the corresponding parameters across validation functions.
- Updating conversion modules (v14, v15, v16) with new implementations, including support for v16 metadata.
- Adjustments in various components (CLI, runtime API, codegen, tx, config) to use new transaction extension and metadata stripping methods.
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
metadata/src/utils/validation/outer_enum_hashes.rs | Entire removal of the outer enum hash module, reflecting that outer enum hashing is now handled differently. |
metadata/src/utils/validation.rs | Removal of the outer_enum_hashes parameter from multiple functions and corresponding updates on hash computation. |
metadata/src/utils/outer_enums.rs | Introduction of a new module to locate outer enums in the type registry for v14 metadata. |
metadata/src/lib.rs | Updates to metadata struct with new methods for associated types and pallet view functions as well as adjustments to hash and retain functions. |
metadata/src/from/* | New and updated conversion implementations for v14, v15, and v16 metadata. |
Others (examples, core, codegen, cli) | Various adjustments to use the updated metadata and transaction extension APIs to support the refactoring and v16 metadata. |
Comments suppressed due to low confidence (3)
metadata/src/utils/validation.rs:88
- The outer_enum_hashes parameter has been removed from hash computations. Please verify that this change does not alter the intended hash semantics and that all existing tests cover these scenarios.
concat_and_hash2(&field_name_bytes, &get_type_hash_recurse(registry, field.ty.id, cache))
cli/src/commands/metadata.rs:63
- New support for v16 metadata stripping has been added; please ensure that sufficient tests are implemented to validate that the stripping logic preserves necessary properties across V14, V15 and V16 metadata.
RuntimeMetadata::V16(md) => md.strip_metadata(keep_pallets_fn, keep_runtime_apis_fn),
codegen/src/api/runtime_apis.rs:129
- The method hash is now retrieved using the new 'hash()' function; please confirm that its output is fully compatible with previous implementations to avoid any API mismatches during runtime calls.
let call_hash = method.hash();
testing/integration-tests/src/full_client/metadata_validation.rs
Outdated
Show resolved
Hide resolved
testing/integration-tests/src/full_client/metadata_validation.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work.
LGTM, I think this PR is improvement when it comes readability of the code.
Would be awesome with empirical metrics how much the codesize would increase w.r.t trimming.
} | ||
} | ||
|
||
fn from_transaction_extension_metadata( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same dq about From
instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! I'd write these as From
impls if I wanted it a part of a public interface, but I suppose I didn't want that sort of interface so it was easier to just write freestanding functions to do what was needed, and then it's obvious that nothing will leak into the public interface that isn't needed / might change :)
utils/strip-metadata/src/lib.rs
Outdated
let new_ids = m.get_types_mut().retain(|id| keep_these_ids.contains(&id)); | ||
|
||
// Map IDs found in the metadata to new ones as needed after the retaining: | ||
m.iter_type_ids_mut().for_each(|id| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe filter_map().for_each()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went the other way and simplified to a normal for loop:
for id in m.iter_type_ids_mut() {
if let Some(new_id) = new_ids.get(id) {
*id = *new_id;
};
}
filter_map.for_each would have felt a bit ugly I think ie something like:
m.iter_type_ids_mut()
.filter_map(|id| new_ids.get(id).map(|new_id| (id, new_id))
.for_each(|(id, new_id)| *id = new_id)
utils/strip-metadata/src/lib.rs
Outdated
} | ||
} | ||
|
||
either!(Either: A B); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe more conventional Left
and Right
, also why not use either
that is already a dependency? is it because of finnicky Iter
trait defs on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! This is a bit of a vestige from when I had a couple of Either defs (which is also why not Left
and Right
, because I had an Either6
with 6 branches, so the Either
aligned with using letters instead!)
I deleted this and justu sed the either
crate!
This PR:
Adds support for some v16 metadata features and converting from v16 metadata (currently unstable); notably pallet view functions and pallet associated types (Future PRs will make use of this new information).
Removes the ability to convert back from
subxt::Metadata
toframe_metadata
. Our internalsubxt::Metadata
doesn't care about everything in the metadata, and not mandating that it be convertible back intoframe_metadata
makes it a non-issue to throw away any information that we don't care about.Moves metadata stripping off
Metadata
(since you can't encode it any more) and onto the individualframe_metadata
versions that we've implemented it for. This was necessary given the above, and means that when we strip metadata for a particular version, we won't change anything else about it.Nerfs metadata stripping in order to avoid potential issues and reduce code complexity. Metadata stripping would achieve the biggest size reduction by also stripping variants from the "outer enums" (
RuntimeCall
/RuntimeError
/RuntimeEvent
), but it's hard to figure out:Our current implementation had some potantial issues around (2), and a bunch of complexity to address (1), so we no longer strip variants. The downside is that metadata stripping won't reduce the size of the metadata much now (the outer enums contain a lot of type information which we keep regardless), but it can still help to exclude APIs you don't intend to work with.
Moves metadata stripping to a new utils crate, reworks it to (hopefully) be much easier to follow and adds some (more) tests to check that it is working as intended.
To illustrate the nerfing of size reduction:
subxt metadata
: 681Ksubxt metadata --runtime-apis ''
: 651Ksubxt metadata --pallets System,Balances,Contracts,Council,Democracy --runtime-apis ''
: 518Ksubxt metadata --pallets Balances --runtime-apis ''
: 501Ksubxt metadata --pallets '' --runtime-apis ''
: 495Kmaster
:subxt metadata
: 681Ksubxt metadata --runtime-apis ''
: 651Ksubxt metadata --pallets System,Balances,Contracts,Council,Democracy --runtime-apis ''
: 459Ksubxt metadata --pallets Balances --runtime-apis ''
: 334Ksubxt metadata --pallets '' --runtime-apis ''
: 324KYMMV depending on the node configuration. I thought I'd seen much smaller sizes on
master
but perhaps that was on a different chain.