-
Notifications
You must be signed in to change notification settings - Fork 254
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
Add runtime_metadata_url to pull metadata directly from a node #689
Conversation
/// | ||
/// **Note:** This is a wrapper over [RuntimeGenerator] for static metadata use-cases. | ||
pub fn generate_runtime_api<P>( | ||
pub fn generate_runtime_api_from_path<P>( |
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.
Nit: we could also update the RuntimeGenerator
doc to reflect this change
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.
Ooh good point; our docs CI thing caught that also :)
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.
this is a nice feature to have, LGTM
I have a question why https
is not supported?
codegen/src/utils/fetch_metadata.rs
Outdated
|
||
/// Returns the metadata bytes from the provided URL, blocking the current thread. | ||
pub fn fetch_metadata_bytes_blocking(url: &Uri) -> Result<Vec<u8>, FetchMetadataError> { | ||
tokio::runtime::Builder::new_multi_thread() |
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.
right, tokio is not initialized here because it's a compile time however you could have another helper function to avoid repeating the tokio runtime init:
fn tokio_block_on<T, Fut: Future<Output = T>>(fut: Fut) -> Result<T, FetchMetadataError> {
tokio::runtime::Builder::new_multi_thread()
.enable_all()
.build()
.unwrap()
.block_on(fut)
}
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.
Good idea!
I'd have to check; for some reason it wasn't supported by the looks of it in the previous iteration, and so I didn't think too much about it. I'll check and see whether that restriction can be removed (I mean, if WSS is supported I don't see why not!) |
Yeah, HTTPS does indeed work fine, I guess there was no reason it wasn't always supported! Hrm perhaps it was a hangover from before we used jsonrpsee.. |
And reuse this fetch functionality in the CLI crate to avoid duplication.
Closes #640