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

fix: enable reqwest default-tls feature in transport-http #248

Merged
merged 2 commits into from
Mar 8, 2024

Conversation

zilayo
Copy link
Contributor

@zilayo zilayo commented Mar 5, 2024

Motivation

Reqwest has default-tls as a default feature (https://docs.rs/reqwest/latest/reqwest/#optional-features), however alloy crates import dependencies with default-features = false.

The alloy metadata crate has default-tls as its default feature, which in turn enables this feature in reqwest, so connections using tls via https work fine when using the alloy crate.

However, alloy-providers, alloy-rpc-client, and alloy-transport-http also depend on reqwest, but provide no option to enable default-tls. So if these crates are used directly (instead of eg alloy::alloy_providers) then it will panic when trying to connect to a https url.

Solution

The simplest fix was to add this to the reqwest feature (default) in the alloy-transport-http cargo toml.
Both alloy-providers and alloy-rpc-client also enable alloy-transport-http/reqwest by default, so this enables the reqwest feature for those crates as well.

To confirm the fix:

// #[tokio::test]
async fn it_makes_a_request() {
let infura = std::env::var("HTTP_PROVIDER_URL").unwrap();
let client = ClientBuilder::default().reqwest_http(infura.parse().unwrap());
let params: Cow<'static, _> = Cow::Owned(());
let req: RpcCall<_, Cow<'static, ()>, U64> = client.prepare("eth_blockNumber", params);
req.await.unwrap();
}

  1. Uncomment #[tokio::test]
  2. Run HTTP_PROVIDER_URL=https://rpc.ankr.com/eth cargo test --package alloy-rpc-client -- http::it_makes_a_request --exact

Test passes successfully vs the previous panic in the linked issue.

Fixes #247

PR Checklist

  • [] Added Tests
  • [] Added Documentation
  • [] Breaking changes

@DaniPopes DaniPopes requested a review from prestwich March 7, 2024 21:27
@prestwich prestwich merged commit b370cf3 into alloy-rs:main Mar 8, 2024
14 checks passed
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.

[Bug] Reqwest default-tls feature only enabled when using the alloy metacrate (disabled for other crates)
3 participants