rust-esplora-client icon indicating copy to clipboard operation
rust-esplora-client copied to clipboard

Replacing reqwest crate with async_minreq crate

Open psg-19 opened this issue 7 months ago • 4 comments

Addresses #121

Overview

This PR replaces our reqwest-based async client with a lightweight async-minreq fork. All existing unit tests pass, but because async-minreq pulls in Tokio 1.44.0 and Tokio-rustls 0.26.2, the project’s MSRV must be bumped to 1.71.0 in CI.

Changes

  • Dependency swap

    • Removed reqwest (and its feature flags)
    • Added
      async_minreq = { git = "https://github.com/psg-19/async-minreq" }
  • Async client rewrite

    • Updated async.rs to use the async-minreq API for GET/POST, timeouts, retries, and HTTPS.
  • Tests & MSRV

    • Verified unit tests pass on Rust 1.71.0.
    • CI workflows now declare rust-version = "1.71.0" to match Tokio’s MSRV.

Looking forward to feedback on these changes and any suggestions for improvement!

Binary Size And Dependency tree

Feature Command Used Async Minreq Reqwest
Binary Size Dependency Tree Binary Size Dependency Tree
build (debug) cargo build 6409 KB 298 6514 KB 434
build (release) cargo build -r 1627 KB 298 1616 KB 434
async cargo build --no-default-features --features async 1709 KB 290 1914 KB 396
async-https cargo build --no-default-features --features async-https 2034 KB 295 1915 KB 431
async-https-native cargo build --no-default-features --features async-https-native 1710 KB 314 1915 KB 431
async-https-rustls cargo build --no-default-features --features async-https-rustls 2034 KB 295 1876 KB 423
async-https-rustls-manual-roots cargo build --no-default-features --features async-https-rustls-manual-roots NA NA 1876 KB 420

Command used for calculating dependency tree -> cargo tree --no-default-features --features xyz | wc -l

My Machine specs -> intel i7 12th gen, rtx 3050ti

OS -> windows subsystem for linux

Checklists

All Submissions:

  • [x] I’ve signed all my commits
  • [x] I followed the contribution guidelines
  • [x] I ran cargo fmt and cargo clippy before committing

New Features:

  • [x] I’ve added tests for the new feature
  • [x] I’ve added docs for the new feature

psg-19 avatar Jun 08 '25 14:06 psg-19

@tnull thanks for taking a look, @psg-19 is my summer of bitcoin mentee. I agree this is a big change but as you pointed out the goal is to reduce the dependency tree. I and @psg-19 will work with @BEULAHEVANJALIN on async_minreq to upstream changes we need and do any other work to get that crate ready for production use.

notmandatory avatar Jun 12 '25 15:06 notmandatory

Pull Request Test Coverage Report for Build 16983255418

Details

  • 21 of 26 (80.77%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 86.919%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/async.rs 21 26 80.77%
<!-- Total: 21 26
Files with Coverage Reduction New Missed Lines %
src/async.rs 1 80.0%
<!-- Total: 1
Totals Coverage Status
Change from base Build 16626978737: 0.0%
Covered Lines: 1010
Relevant Lines: 1162

💛 - Coveralls

coveralls avatar Jun 12 '25 18:06 coveralls

Another option we should consider for this PR is making async_minreq an optional "experimental" features so projects can help test it out while the async_minreq crate matures. The down-side is this doesn't follow cargo convention about not having conflicting features, will blow-up the number of feature flags and make the code a bit harder to read/reason about. But still worth a look, could rename features like this:

async-minreq = ["async_minreq", "async_minreq/proxy", "tokio?/time"]
async-minreq-https = ["async-minreq", "async_minreq/https"]
async-minreq-https-native = ["async-minreq", "async_minreq/https-native"]
async-minreq-https-rustls = ["async-minreq", "async_minreq/https-rustls"]
async-minreq-https-rustls-manual-roots = ["async-minreq"]

notmandatory avatar Jul 30 '25 15:07 notmandatory

The latest commit to feature flag the async-minreq code looks good! I have a few suggestions for finishing this off:

  1. put the current async code blocks before the async-minreq blocks so that if somehow both features are enabled the current reqwest based code will return first, also the diff will be easier to review against the current code.
  2. I think I saw a couple places where the final return statements for the async feature aren't feature gated.
  3. The README should be updated to state the new MSRV is updated to 1.71 and what the new pinned dependencies are, or similar to how it's done in the bdk repo you can put all the pinning in a pin-msrv.sh script and just reference that in both the README and use it in the CI workflows.
  4. Also in the README you should describe the available feature flags and say that the async-minreq one is currently "experimental" but may become the only async client In the future once it's more mature.
  5. Cleanup commit history and follow conventional commits style commit messages (I can show you how to do this with 'git rebase').

notmandatory avatar Aug 01 '25 21:08 notmandatory