-
Notifications
You must be signed in to change notification settings - Fork 245
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
feat(providers): connect_boxed api #342
Conversation
crates/rpc-client/tests/it/ws.rs
Outdated
@@ -1,7 +1,7 @@ | |||
use alloy_node_bindings::Anvil; | |||
use alloy_primitives::U64; | |||
use alloy_rpc_client::{ClientBuilder, RpcCall}; | |||
use alloy_transport_ws::WsConnect; | |||
use alloy_transport::WsConnect; |
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.
Failing in CI
Need some help with fixing ci. Not sure why it's failing.
|
crates/provider/Cargo.toml
Outdated
@@ -14,12 +14,13 @@ exclude.workspace = true | |||
[dependencies] | |||
alloy-json-rpc.workspace = true | |||
alloy-network.workspace = true | |||
alloy-rpc-client = { workspace = true, features = ["reqwest"] } | |||
alloy-rpc-client = { workspace = true, features = ["reqwest", "pubsub"] } |
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.
Need some help with fixing ci. Not sure why it's failing.
WsConnect
is to be imported fromalloy_transport_ws
. That's how it is onmain
use alloy_transport_ws::WsConnect;
dep:alloy-transport-ws
is under the ws
feature, but the test is cfg(feature = "pubsub")
so when the pubsub feature is active, but the ws
feature is not, there's an error. This is triggered by this feature line here, I believe
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.
basically, this feature activation causes the cargo resolver to activate ONLY pubsub
on alloy-rpc-client, and not ws
, which causes the alloy-transport-ws to be missing when the test runs. This feature should not be activated here, it should instead be activated under the pubsub
feature n this crate
we should also change the cfg in rpc-client/tests/it/main.rs
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.
summary:
- remove this
pubsub
feature activation - update cfgs in
rpc-client/tests/it/main.rs
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.
cc @mattsse @DaniPopes should ws
and ipc
be default features for the provider
crate?
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.
summary:
- remove this
pubsub
feature activation- update cfgs in
rpc-client/tests/it/main.rs
We need the pubsub
feature enabled on alloy-rpc-client as it is required for connect_pubsub
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.
that implies that connect_pubsub
should be cfg gated behind the alloy-provider pubsub
feature
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.
and the WS/IPC components also need cfg gating
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.
that implies that
connect_pubsub
should be cfg gated behind the alloy-providerpubsub
feature
If I'm not mistaken, this means disabling the connect_boxed
method as it uses connect_pubsub
for ws and ipc.
Not seeing a way to gate connect_pubsub using the pubsub feature without sacrificing connect_boxed
.
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.
it means changing the behavior to produce ONLY enabled trandport types. eg. only http when pubsub is disabled
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.
1 more feature:
The connect_boxed
shortcut should also exist in the ProviderBuilder
as fn connect_boxed(self, s: &str) -> Result<L::Provider>
did a significant cleanup here:
|
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.
smol suggestion
927b25b
to
f424a22
Compare
|
||
# rpc | ||
alloy-json-rpc = { workspace = true, default-features = false, optional = true } | ||
alloy-rpc-client = { workspace = true, default-features = false, optional = true } | ||
alloy-rpc-client = { workspace = true, 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.
this two lines now cause warnings as the root cargo toml doesn't set default-features
Motivation
Ref: #318
Solution
Implement a
connect_boxed(&str) -> RootProvider<N, BoxTransport>
method forRootProvider<N, T>
.PR Checklist