-
Notifications
You must be signed in to change notification settings - Fork 257
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
Fix codegen validation when Runtime APIs are stripped #1000
Conversation
…-runtime-apis-stripped
…-runtime-apis-stripped
codegen/src/api/mod.rs
Outdated
.metadata() | ||
.hasher() | ||
.only_these_pallets(&PALLETS) | ||
.only_these_runtime_apis(&RUNTIME_APIS).hash(); |
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.
Is it just GitHub or are these lines super indented? :D
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 :) |
metadata/src/utils/validation.rs
Outdated
@@ -424,30 +426,54 @@ impl<'a> MetadataHasher<'a> { | |||
self | |||
} | |||
|
|||
/// only hash the provided runtime APIs instead of hashing every runtime API |
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.
/// only hash the provided runtime APIs instead of hashing every runtime API | |
/// Only hash the provided runtime APIs instead of hashing every runtime API. |
…-runtime-apis-stripped
I had a pass over this to add a few tests for the validation. I also ended up:
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 { |
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 think set_metadata
is better name for this method...
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.
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 :)
fixes #976