-
Notifications
You must be signed in to change notification settings - Fork 248
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
move fetch metadata
to a separate crate subxt_utils_fetchmetadata
#1829
Conversation
@@ -6,7 +6,7 @@ | |||
//! This is used by the `#[subxt]` macro and `subxt codegen` CLI command, but can also | |||
//! be used directly if preferable. | |||
|
|||
#![deny(unused_crate_dependencies, missing_docs)] |
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.
we are using cargo-machete anyway for unused deps and I don't want to add
#[cfg(all(feature = "jsonrpsee", not(feature = "fetch-metadata))] use {tokio as _, url as _}
codegen/Cargo.toml
Outdated
@@ -25,11 +26,12 @@ quote = { workspace = true } | |||
syn = { workspace = true } | |||
scale-info = { workspace = true } | |||
subxt-metadata = { workspace = true } | |||
jsonrpsee = { workspace = true, features = ["async-client", "client-ws-transport-tls", "http-client"], optional = true } | |||
jsonrpsee = { workspace = true, features = ["ws-client", "http-client"], optional = true } |
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 guess this means that, without importing jsonrpsee
myself to enable TLS features, that we won't be able to use fetch-metadata with httpS urls?
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.
yes without jsonrpsee it will only work to fetch metadata from a file or runtime path...
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.
Sorry, I meant that based on these feature changes, jsonrpsee won't work with TLS urls now unless I import it in my top level package to enable them or something like that!
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.
ooooh, tls enabled by default by the ws-client and http-client :P
macro/Cargo.toml
Outdated
@@ -16,6 +16,7 @@ description = "Generate types and helpers for interacting with Substrate runtime | |||
[features] | |||
web = ["subxt-codegen/web"] | |||
runtime-path = ["polkadot-sdk"] | |||
runtime-metadata-insecure-url = ["subxt-utils/fetch-metadata-url"] |
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: Just to align with the above I'd call it runtime-url
orrr, rename both to match the attrs so we have:
runtime-metadata-path
runtime-metadata-insecure-url
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.
Looks good; A step in the right direction I think!
I'm still on the fence over having one utils crate versus several (because feature flags make everything messier) but this is a step up from the current state :)
… into na-feature-gate-jsonrpsee-macro
fetch metadata
to a separate crate subxt_utils_fetchmetadata
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.
Nice! Had a brief view over the PR and looks solid 👍
Close #1812
This PR moves the
fetch metadata
to a separate crate and also makes it optional for thesubxt macro/codegen
use it behind a feature flag.So, if one disables jsonrpsee in subxt then the following would end-up in a compile error: