-
Notifications
You must be signed in to change notification settings - Fork 974
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: implement generalizable rpc middleware #9429
Conversation
924a271
to
50848b3
Compare
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.
looking good so far, removing the intermediary type definitely helped a lot
crates/rpc/rpc-builder/src/lib.rs
Outdated
where | ||
RpcMiddleware: for<'a> Layer<RpcService, Service: RpcServiceT<'a>> + Clone + Send + 'static, | ||
<RpcMiddleware as Layer<RpcService>>::Service: Send + std::marker::Sync, | ||
{ |
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 need several implant blocks for this type, and we want to be as unrestrictive as possible, so the setters are easy to call
we only need to enforce these bounds when we start the server
@@ -105,7 +117,7 @@ async fn test_launch_same_port_different_cors() { | |||
EthApiBuild::build, | |||
); | |||
let addr = test_address(); | |||
let res = RpcServerConfig::ws(Default::default()) | |||
let res = RpcServerConfig::<Identity>::ws(Default::default()) |
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 shouldn't be necessary if we put the init functions in another impl block that doesn't require this generic
884d8fc
to
bc09a85
Compare
738dae4
to
0be45b9
Compare
thank you for this feedback! i'm learning a lot about idiomatic rust from you, and i really appreciate that in this updated code created an impl block to handle initialization and moved the trait bounds from the config type to the start 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.
great!
the pr does change additional metric setup internals, I don't want to do this in this pr, because this is out of scope.
we can limit this pr to the introduction of the additional middleware value and then try to integrate it in a followup then it's easier.
I'm fine with leaving the additional rpc_middleware unused at first
/// convenience. To set a custom [`IdProvider`], please use [`Self::with_id_provider`]. | ||
pub fn with_ws(mut self, config: ServerBuilder<Identity, Identity>) -> 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.
these accept self and return Self, and don't care about the current generic, so these should be moved to the
`impl RpcServerConfig
scope
crates/rpc/rpc-builder/src/lib.rs
Outdated
@@ -1591,7 +1623,7 @@ pub struct TransportRpcModules<Context = ()> { | |||
/// The original config | |||
config: TransportRpcModuleConfig, | |||
/// rpcs module for http | |||
http: Option<RpcModule<Context>>, | |||
pub http: Option<RpcModule<Context>>, |
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 wanna keep this private
crates/rpc/rpc-builder/src/lib.rs
Outdated
RpcServiceBuilder::new().layer( | ||
modules | ||
.http | ||
.as_ref() | ||
.or(modules.ws.as_ref()) | ||
.map(RpcRequestMetrics::same_port) | ||
.unwrap_or_default(), | ||
), | ||
) | ||
.set_rpc_middleware(self.rpc_middleware) |
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 now puts the metrics setup on the caller, we don't want this.
can we please keep the rpc_middleware setup as it was before and then integrate the custom middleware in a next step
there's a lot that can go wrong 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.
this pr should not change the any metrics setup
0ca715c
to
47f51b7
Compare
47f51b7
to
2717dba
Compare
sounds good. i've updated the branch accordingly |
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.
lgtm!
next we can try merging the existing metrics layer setup with the additional rpc middleware field
Thank you for the review. This was my first pr with the prefix "feat" 🙌 That sounds good to me, I'll give it a shot! |
towards more flexibility of rpc middleware in the style of
Server
fromjsonrpsee
as part of this feature
RpcRequestMetrics
was made public, with the idea that it (or whatever other middleware the operator wants to use) be added when the config/server is started