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 blocking-https-rustls feature on esplora client #1408

Merged

Conversation

thunderbiscuit
Copy link
Contributor

@thunderbiscuit thunderbiscuit commented Apr 18, 2024

The blocking feature on the rust-esplora-client library changed from ureq to minreq, which does not come with https enabled by default, breaking previously working code that simply enabled the blocking feature on the bdk_esplora crate.

This change will enable what is currently the "default https" flag for the minreq library when using the blocking feature on bdk_esplora, reverting that breaking change.

Notes to the reviewers

Another way we could do this (let me know if this is preferable) is to add a new feature called blocking-https-rustls:

blocking = ["esplora-client/blocking"]
blocking-https-rustls = ["esplora-client/blocking-https-rustls"]

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory
Copy link
Member

notmandatory commented Apr 19, 2024

🔧 I'd prefer you add the blocking-https-rustls feature which should be used in the default feature since I expect it to be the most common case. But I'd like to keep the plain blocking feature for those rare cases when someone doesn't want the extra https dependencies (ie. they're using a TOR or some other proxy).

@thunderbiscuit
Copy link
Contributor Author

Sounds good. I fixed the features as per recommendation.

Copy link
Contributor

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK d3a14d4

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK d3a14d4

@evanlinjin evanlinjin merged commit 9800f8d into bitcoindevkit:master Apr 20, 2024
12 checks passed
@danielabrozzoni danielabrozzoni mentioned this pull request May 2, 2024
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants