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: Exclude field::type_name from metadata validation #595

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Jul 6, 2022

The #591 presented a metadata validation issue detected on the Snowbridge parachain.

The metadata validation would reject extrinsics due to the following difference in metadata:

Source Metadata
snowbridge "typeName": "/«/ u32 /»/"
polkadot "typeName": "u32"

This PR removes the hashing of the typeName as the unique deterministic representation of the field is deduced from its type and name.

Testing

Utilizing the metadata provided in the issue:

subxt-cli -- codegen --file ours.metadata.scale > ours.rs
subxt-cli -- codegen --file theirs.metadata.scale > theirs.rs

diff ours.rs theirs.rs
#ok - identical hashes for validation

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv self-assigned this Jul 6, 2022
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Interesting, I never really thought about it but omitting the check for type_name makes sense since you can do validation through type, and name itself as @lexnv mentioned.

Nice job 👍

@lexnv lexnv merged commit b3f6ff1 into master Jul 7, 2022
@lexnv lexnv deleted the 591_md_validation_improvement branch July 7, 2022 09:23
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