Skip to content
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(transport-http): layer client #1227

Merged
merged 33 commits into from
Sep 18, 2024
Merged

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Sep 2, 2024

Motivation

Currently, there is no way to modify the absolute low-level request such as http::Request.

This is due to the limitation that transport layers work over alloy_json_rpc::RequestPacket and not http::Request.

Solution

Making the hyper transport HTTP client generic over an underlying service.

By default, it serves as a vanilla hyper client like before (use HyperTransport::new()), with the added benefit of adding tower layers on top of the hyper::Client and using that in HyperTransport::with_service(service).

This new set of layers will work over http::Request which is used by hyper allowing us to modify request properties such as headers.

A HyperTransport with layers can be initialized like so:

let hyper_client = Client::builder(TokioExecutor::new()).build_http::<Full<HyperBytes>>();

let service = tower::ServiceBuilder::new().layer(SetRequestHeaderLayer::if_not_present(header::USER_AGENT, HeaderValue::from_static("alloy app"))).service(hyper_client);

let layer_transport = alloy_transport_http::HyperTransport::with_service(service);

let rpc_client = alloy_rpc_client::RpcClient::new(layer_transport, true);

This will help implement a JWT auth layer #1161 that refreshes auth tokens.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@yash-atreya
Copy link
Member Author

The LoggingLayer is just for testing and demo, I'll be removing it

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, this should work.

I'd like to see some more advanced tests/exampls here that make use of some tower-http components

for example auth

see also https://github.com/tower-rs/tower-http/blob/3f98dc19bc6c70aa6e4912b63538d89443f03b19/examples/axum-key-value-store/src/main.rs

crates/transport-http/src/layers/logging.rs Outdated Show resolved Hide resolved
@yash-atreya yash-atreya changed the title [wip] feat(transport-http): reqwest layer client [wip] feat(transport-http): layer client Sep 7, 2024
@yash-atreya yash-atreya marked this pull request as ready for review September 9, 2024 10:46
@yash-atreya
Copy link
Member Author

What we currently have working is the following:

  • A new HTTP transport client based on hyper that lets users create and layer services on top of each other that modify the underlying http::Request or http::Response types.
  • This is enabled through the use of tower::ServiceBuilder
  • One can now write a layer with a service that works over http::Request. Examples include [Feature] Add JWT layer for transport #1161 that modifies the auth header, and/or append a X-Flashbots-Signature https://docs.flashbots.net/flashbots-auction/advanced/rpc-endpoint#authentication
  • tower-http layers that do not change the response type or error type work as well, this is due to the restrictive trait bounds we've used.
  • All of this is very useful but not ideal, we want to work with all tower-http layers

TODO

  • tower-http layers like TimeoutLayer or TraceLayer don't work because they modify the response type to something like http::Response<TimeoutBody<Incoming>> which is incompatible with expected response type i.e type HyperResponse = http::Response<Incoming> type. Thinking of ways to make this more flexible.

cc @mattsse @klkvr

@yash-atreya yash-atreya changed the title [wip] feat(transport-http): layer client feat(transport-http): layer client Sep 10, 2024
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits

overall this looks good

crates/transport-http/Cargo.toml Outdated Show resolved Hide resolved
crates/transport-http/src/hyper_layer_transport.rs Outdated Show resolved Hide resolved
crates/transport-http/src/hyper_layer_transport.rs Outdated Show resolved Hide resolved
@klkvr
Copy link
Member

klkvr commented Sep 11, 2024

  • tower-http layers like TimeoutLayer or TraceLayer don't work because they modify the response type to something like http::Response<TimeoutBody<Incoming>> which is incompatible with expected response type i.e type HyperResponse = http::Response<Incoming> type. Thinking of ways to make this more flexible.

I think we could just use Body trait to restrict response body? https://docs.rs/http-body/1.0.0/http_body/trait.Body.html

something like Response<T> where T: Body<Data = Bytes>

crates/transport-http/src/hyper_transport.rs Outdated Show resolved Hide resolved
crates/transport-http/src/hyper_transport.rs Outdated Show resolved Hide resolved
crates/transport-http/src/lib.rs Outdated Show resolved Hide resolved
@yash-atreya
Copy link
Member Author

I am not sure we should have that. I believe that point of TransportConnect is in knowing how to instantiate transport. If we require user to provide it with a ready transport instance, then user can just directly configure this instance instead of relying on TransportConnect

we can only have TransportConnect implemented for default hyper transport I think

This makes sense!
Removed the transport from HttpConnect, and impl TransportConnect for default HyperTransport

Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits

crates/transport-http/src/hyper_transport.rs Outdated Show resolved Hide resolved
crates/transport-http/src/hyper_transport.rs Outdated Show resolved Hide resolved
Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mattsse mattsse merged commit 68115f5 into main Sep 18, 2024
26 checks passed
@mattsse mattsse deleted the yash/reqwest-tower-integration branch September 18, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants