feat: implement generalizable rpc middleware #9429
Conversation
smatthewenglish
requested review from
mattsse,
Rjected,
emhane and
gakonst
as code owners
smatthewenglish
force-pushed
the
middleware
branch
5 times, most recently
from
924a271 to
50848b3
Compare
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.
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.
this shouldn't be necessary if we put the init functions in another impl block that doesn't require this generic
smatthewenglish
force-pushed
the
middleware
branch
3 times, most recently
from
884d8fc to
bc09a85
Compare
smatthewenglish
force-pushed
the
middleware
branch
4 times, most recently
from
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.
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.
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.
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.
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.
this pr should not change the any metrics setup
smatthewenglish
force-pushed
the
middleware
branch
3 times, most recently
from
0ca715c to
47f51b7
Compare
smatthewenglish
force-pushed
the
middleware
branch
from
47f51b7 to
2717dba
Compare
sounds good. i've updated the branch accordingly |
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
Serverfromjsonrpseeas part of this feature
RpcRequestMetricswas made public, with the idea that it (or whatever other middleware the operator wants to use) be added when the config/server is started