Skip to content

Split into esplora-types and esplora-client crates#229

Draft
EliteCoder18 wants to merge 3 commits into
bitcoindevkit:masterfrom
EliteCoder18:feat/esplora-types-workspace
Draft

Split into esplora-types and esplora-client crates#229
EliteCoder18 wants to merge 3 commits into
bitcoindevkit:masterfrom
EliteCoder18:feat/esplora-types-workspace

Conversation

@EliteCoder18

Copy link
Copy Markdown

Description

Addresses #218

Splits this repo into a Cargo workspace with two crates: esplora-client (unchanged behavior --> the blocking/async HTTP clients, apart from one re-export line) and a new esplora-types crate holding just the request/response types, depending only on serde and bitcoin, with no HTTP stack at all. esplora-client depends on esplora-types and re-exports it under both its top level and the existing api path (pub use esplora_types as api;), so no downstream code needs to change imports.

Notes to the reviewers

  • Deliberately not doing no_std for esplora-types here -> it's blocked on one HashMap field in SubmitPackageResult, and I didn't want to unilaterally pick between swapping to hashbrown or feature-gating it. Happy to follow up once there's agreement on direction.
  • cargo-semver-checks flags the api re-export as a breaking removal dug into it, it's a known limitation with how rustdoc JSON represents pub use other_crate as alias re-exports, not a real break. Verified manually by compiling and running code through both the old and new import paths, including calling inherent methods through the old path.
  • Per-crate READMEs are now minimal, matching how bitcoindevkit/bdk does it.
  • Once bitreq work merges I'll rebase this onto latest master.
  • Planning a separate draft PR for mempool-types/mempool-client stacked on top of this one.

Changelog notice

  • feat: split types into a new esplora-types crate, re-exported as esplora_client::api

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the testenv specific to the esplora clients? If it isn't or doesn't have to be can it be shared somehow between the esplora-client and future mempoolspace-client tests? Don't need to change anything right now but something to think about for a future PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed testenv isn't Esplora-specific, and it'd make sense to share it once mempool-client gets its own tests. I'll factor that in when I open that PR.

@notmandatory notmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is very well explained and a good backward compatible design. I made one comment and CI has some failed tests. But otherwise looks good and ready to rebase once bitreq PR is merged.

@EliteCoder18

Copy link
Copy Markdown
Author

This is very well explained and a good backward compatible design. I made one comment and CI has some failed tests. But otherwise looks good and ready to rebase once bitreq PR is merged.

Thanks for the review!

I Dug into the failing Test jobs both panic at the same line (testenv/mod.rs:183,mine_blocks()'s bitcoind RPC call) with an empty JSON-RPC response, It's a connectivity hiccup rather than a logic issue. That file is untouched just relocated in this PR. This exact commit (03dd1c8) also passed clean on my fork's own CI so it looks like runner-level flakiness rather than anything in the diff.

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.

2 participants