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

Metadata difference command #1015

Merged
merged 20 commits into from
Jun 21, 2023
Merged

Conversation

tadeohepperle
Copy link
Contributor

@tadeohepperle tadeohepperle commented Jun 14, 2023

Explore the differences between two nodes

Example

subxt diff ./artifacts/polkadot_metadata_small.scale ./artifacts/polkadot_metadata_tiny.scale
subxt diff ./artifacts/polkadot_metadata_small.scale wss://rpc.polkadot.io:443

Example output (colored in cli):

Pallets:
    ~ Balances
        Calls:
            - transfer_allow_death
            - force_set_balance
            + set_balance
            - upgrade_accounts
            - set_balance_deprecated
        Constants:
            - MaxHolds
            - MaxFreezes
        Storage Entries:
            - Freezes
            ~ Account (Changed: value type, default value)
            - Holds
    + Scheduler
    + Session
    + Democracy
    + Claims
    + Crowdloan
    + Registrar
    + FastUnstake
    + Initializer
    + Offences
    + Bounties
    + Treasury
    ~ System
        Storage Entries:
            ~ Events (Changed: value type)
            ~ Account (Changed: value type, default value)
    + ChildBounties
    + Babe
    + ElectionProviderMultiPhase
    + Configuration
    + ParaInherent
    + VoterList
    + Hrmp
    + Ump
    + ParasShared
    ~ Multisig
        Calls:
            ~ as_multi
            ~ as_multi_threshold_1
    + PhragmenElection
    + Utility
    + Proxy
    + ParachainsOrigin
    + Paras
    + NominationPools
    + Auctions
    + Timestamp
    + Indices
    + Grandpa
    + AuthorityDiscovery
    + Slots
    + TransactionPayment
    + Historical
    + TechnicalMembership
    + ImOnline
    + ParaSessionInfo
    + Authorship
    + Dmp
    + ParasDisputes
    + Preimage
    + ParaScheduler
    + TechnicalCommittee
    + Vesting
    + Identity
    + XcmPallet
    + Council
    + Tips
    + ParaInclusion
Runtime APIs:
    - TransactionPaymentCallApi
    - Core
    - MmrApi
    - BeefyApi
    - GrandpaApi
    - AccountNonceApi
    - BlockBuilder
    - TaggedTransactionQueue
    - StakingApi
    - AuthorityDiscoveryApi
    - SessionKeys
    - BabeApi
    - Metadata
    - NominationPoolsApi
    - ParachainHost
    - OffchainWorkerApi
    - TransactionPaymentApi

@tadeohepperle tadeohepperle marked this pull request as ready for review June 14, 2023 12:21
@tadeohepperle tadeohepperle requested a review from a team as a code owner June 14, 2023 12:21
cli/src/commands/diff.rs Outdated Show resolved Hide resolved
cli/src/commands/diff.rs Outdated Show resolved Hide resolved
node1: String,
node2: String,
#[clap(long, short)]
version: Option<MetadataVersion>,
Copy link
Member

Choose a reason for hiding this comment

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

I don't get why version is a CLI parameter, how would one insert --version here and why?

Better to read that from the metadata itself?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The version is which version of metadata is being requested when a url is given (eg are we downloading V14 metadatas or unstable-v15 metadatas or "whatever is latest stable"; something like that)

#[clap(long, short)]
version: Option<MetadataVersion>,
#[clap(long, short)]
pallet: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

This isn't used AFAIU, what's intention behind it? Just compare a single pallet in the metadata?

If we want this, it would be better with Vec to compare a one or more pallets

@xlc
Copy link

xlc commented Jun 15, 2023

doesn't subwasm already have something like this?

cli/src/commands/diff.rs Outdated Show resolved Hide resolved
cli/src/commands/diff.rs Outdated Show resolved Hide resolved
cli/src/commands/diff.rs Outdated Show resolved Hide resolved
@niklasad1
Copy link
Member

niklasad1 commented Jun 16, 2023

doesn't subwasm already have something like this?

Yes, subwasm has something similar but it compares wasm files and this/subxt operates on metadata either by passing an URL or a path to the metadata. Perhaps the subxt output might be a little easier to read because of the scale encode/decode stuff better to try it out :)

Both are useful but if you are already a user of subxt the diff command is more ergonomic to use, most likely one has already embedded a static metadata file in the binary.

@jsdw
Copy link
Collaborator

jsdw commented Jun 16, 2023

Some small feedback bits re the output:

  • wonder whether <ENTRY1> and <ENTRY2> could be followed by help like "metadata file or node URL" in the following?:
error: the following required arguments were not provided:
  <ENTRY1>
  <ENTRY2>

Usage: subxt diff <ENTRY1> <ENTRY2>

For more information, try '--help'.
  • It appears (and please correct me if I'm wrong) that subxt diff A B will show changes relative to "A", ie which pallets were added or removed to get to A. I'd normally expect a diff command to show changes with respect to "B". Could this be swapped around?

Otherwise, the output looks fantastic and I love the use of colours :)

Comment on lines 347 to 372
let mut calls: CallsHashmap = HashMap::new();
if let Some(call_variants) = pallet_metadata_1.call_variants() {
for call_variant in call_variants {
let (e1, _) = calls.entry(&call_variant.name).or_default();
*e1 = Some(call_variant);
}
}
if let Some(call_variants) = pallet_metadata_2.call_variants() {
for call_variant in call_variants {
let (e1, e2) = calls.entry(&call_variant.name).or_default();
// skip all entries with the same hash:
if let Some(e1_inner) = e1 {
let e1_hash = pallet_metadata_1
.call_hash(&e1_inner.name)
.expect("call should be present");
let e2_hash = pallet_metadata_2
.call_hash(&call_variant.name)
.expect("call should be present");
if e1_hash == e2_hash {
calls.remove(call_variant.name.as_str());
continue;
}
}
*e2 = Some(call_variant);
}
}
Copy link
Collaborator

@jsdw jsdw Jun 16, 2023

Choose a reason for hiding this comment

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

The general pattern of "give me a list of the differences between two things" feels like something that could be abstracted here since it's repeated several times; the only logic that changes each time is "how do I turn a thing into something comparable".

I'd propose it'd be a fun challenge to have a go at doing this abstracting; I could imagine ultimately ending with a signature something like this:

fn diff<T, C: PartialEq>(
    items_a: impl IntoIterator<Item = T>,
    comparable_a: impl Fn(&T) -> C
    items_b: impl IntoIterator<Item = T>,
    comparable_b: impl Fn(&T) -> C
) -> Vec<Diff<T>>

Doing this might also negate the need for a slightly arbitrary TryFrom<(a,b)> type thing from tuples to Diff, since it would all be done in this one place, and would make it easier to check the diffing logic is all good in one place rather than several :)

@@ -509,6 +519,11 @@ impl<'a> RuntimeApiMetadata<'a> {
pub fn method_hash(&self, method_name: &str) -> Option<[u8; 32]> {
crate::utils::validation::get_runtime_api_hash(self, method_name)
}

/// Return a hash for the entire pallet.
Copy link
Collaborator

@jsdw jsdw Jun 16, 2023

Choose a reason for hiding this comment

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

Suggested change
/// Return a hash for the entire pallet.
/// Return a hash for the entire runtime API trait.

The comment on 519 is also wrong, whoops! It should be "Return a hash for the method, or None if it was not found"

Comment on lines 27 to 28
entry1: FileOrUrl,
entry2: FileOrUrl,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps a slightly more descriptive name for these like metadata_or_url1 and metadata_or_url2 would make the output a little clearer.

I wonder whether adding comments above these would lead to nicer help being shown in the output too :)

@jsdw
Copy link
Collaborator

jsdw commented Jun 16, 2023

Looks great; just a few suggestions :)

@tadeohepperle
Copy link
Contributor Author

it appears (and please correct me if I'm wrong) that subxt diff A B will show changes relative to "A", ie which pallets were added or removed to get to A.

@jsdw I think maybe you understood it wrong. It shows things with a + that are present in B but not in A, so things that were added to get from A to B. And vice versa. For example consider this:
subxt diff ./artifacts/polkadot_metadata_small.scale ./artifacts/polkadot_metadata_tiny.scale
gives as output

Pallets:
    - Staking
    - Balances
    - Multisig
    - System
Runtime APIs:
    ~ TransactionPaymentCallApi

indicating that some pallets are removed going from small to tiny.
Or maybe I am understanding it wrong?

@jsdw
Copy link
Collaborator

jsdw commented Jun 19, 2023

Oh interesting; hopefully I just got it wrong then, lemme check! I compared kuama to polkadot iirc and it seemed as if it was the otherway round but I'll try again to confirm :)

Edit: yup my mistake; I played around some more and must have just confused myself!

@@ -303,8 +313,8 @@ impl StorageMetadata {
}

/// An iterator over the storage entries.
pub fn entries(&self) -> impl ExactSizeIterator<Item = &StorageEntryMetadata> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really a big deal, but I was wodnering whether there was a reason for this change?

IIRC the reason I tended to iterate over other metadata structs exposed here rather than give back slices was just to give a little more wiggle room in case we wanted to change the internals a bit in the future. There are a couple of other places in this API that return the ExactSizeIterator even though they could return slices too for this reason :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it because I needed to do an unwrap_or_default() and I could not get a default impl ExactSizeIterator working properly that is the exact same type as the self.entries.values().iter().
Hmm, do you have an idea how I could solve this?

pallet_metadata_1
            .storage()
            .map(|s| s.entries())
            .unwrap_or_default()

Removed(T),
}

fn diff<T, C: PartialEq, I: Hash + PartialEq + Eq>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome! Much prefer this :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(and with this logic now separated, you could also have a quick unit test or two that diffing things using this function works as expected :))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a unit test. This also made me realize that the output order was random because of the random nature of hash maps. So I added alphabetical ordering as well.

@jsdw
Copy link
Collaborator

jsdw commented Jun 21, 2023

Awesome stuff; good job @tadeohepperle!

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.

Lovely PR! Amazing job here 🙏

@tadeohepperle tadeohepperle merged commit 2a990ed into master Jun 21, 2023
@tadeohepperle tadeohepperle deleted the tadeo-hepperle-metadata-difference branch June 21, 2023 12:33
@jsdw jsdw mentioned this pull request Jul 24, 2023
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.

5 participants