-
Notifications
You must be signed in to change notification settings - Fork 19
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 Missing runtime APIs for Metadata v15 #2204
Conversation
js/api-augment/types.d.ts
Outdated
// Unknown is causing these to be left out of the generated types. | ||
// Adding them here for now. |
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 believe this is a type generation issue. I see a few other places that have to do this. As these are all simple, for now, I think it is best to add them here and move on.
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.
Hmm, they implemented TypeInfo
macro . This is weird!
@@ -1294,6 +1309,348 @@ cumulus_pallet_parachain_system::register_validate_block! { | |||
BlockExecutor = cumulus_pallet_aura_ext::BlockExecutor::<Runtime, Executive>, | |||
} | |||
|
|||
// The implementation has to be here due to the linking in the macro. | |||
// It CANNOT be extracted into a separate file | |||
sp_api::impl_runtime_apis! { |
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.
These are just copied back from the runtime/frequency/src/apis.rs
file. No additional changes.
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
|
||
#[test] | ||
fn runtime_apis_are_populated() { | ||
assert!(RUNTIME_API_VERSIONS.len() > 0); |
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.
👍
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.
👍 fabuloso!
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 read the code and it looked good afaik.
js/api-augment/types.d.ts
Outdated
// Unknown is causing these to be left out of the generated types. | ||
// Adding them here for now. |
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.
Hmm, they implemented TypeInfo
macro . This is weird!
@@ -28,19 +28,8 @@ export default { | |||
event: 'u8', | |||
data: 'Vec<u8>', | |||
}, | |||
}, | |||
runtime: { |
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.
Runtime types are part of the generation now
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 reviewed the code changes. All looks good.
Goal
The goal of this PR is to make sure that Metadata v15 has the correct Runtime API call data
Closes #2202
Discussion
Why? I cannot be 100% sure. See the linked issues in #2202 for more details.
From what I can tell, the way rust macros work with context means that this macro is stuck being in the same file as the const that uses it is
pub const VERSION
Specifics
e2e/capacity/capacity_rpc.test.ts
. Issue noted in code commentTesting
make start
&&subxt metadata --version 15 -f json | jq .[1].V15.apis
(Before this result would have been empty, now you have the runtime data!)