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

Fix codegen validation when Runtime APIs are stripped #1000

Merged

Conversation

tadeohepperle
Copy link
Contributor

fixes #976

@tadeohepperle tadeohepperle requested a review from a team as a code owner June 5, 2023 10:05
codegen/src/api/mod.rs Outdated Show resolved Hide resolved
Comment on lines 633 to 636
.metadata()
.hasher()
.only_these_pallets(&PALLETS)
.only_these_runtime_apis(&RUNTIME_APIS).hash();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it just GitHub or are these lines super indented? :D

@jsdw
Copy link
Collaborator

jsdw commented Jun 6, 2023

Looks perfect! Do we have any tests to check that the hashes line up when we strip pallets and runtime APIs now from metadata? Let's add one or two just to make sure that the hashes all line up :)

@@ -424,30 +426,54 @@ impl<'a> MetadataHasher<'a> {
self
}

/// only hash the provided runtime APIs instead of hashing every runtime API
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// only hash the provided runtime APIs instead of hashing every runtime API
/// Only hash the provided runtime APIs instead of hashing every runtime API.

@jsdw
Copy link
Collaborator

jsdw commented Jun 8, 2023

I had a pass over this to add a few tests for the validation. I also ended up:

  • renaming validate_codegen(client) -> Result<(), Error> to is_codegen_valid_for(metadata) -> bool (having it take metadata and return a bool made more sense to me, and then I wanted to tweak the name to match).
  • getting rid of the PalletMetadataTestRunner, and upgrading MetadataTestRunner to do validation, too.

So probably somebody that isn't me should review it :)


/// Set metadata to be validated against the generated code.
/// By default, we'll validate the same metadata used to generate the code.
pub fn validation_metadata(mut self, md: impl Into<Metadata>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

I think set_metadata is better name for this method...

Copy link
Collaborator

Choose a reason for hiding this comment

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

The build() call is where you pass the actual metadata that the macro will use; this validation_metadata is just the optional metadata you can use to test the is_codegen_valid_for function against :)

@jsdw jsdw merged commit 992ec9a into master Jun 9, 2023
@jsdw jsdw deleted the tadeo-hepperle-fix-codegen-validation-when-runtime-apis-stripped branch June 9, 2023 09:10
@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.

Fix codegen validation when Runtime APIs are stripped
4 participants