Skip to content

Add proxy for Esplora & Electrum#711

Merged
reez merged 1 commit intobitcoindevkit:masterfrom
reez:esplora-proxy
Mar 27, 2025
Merged

Add proxy for Esplora & Electrum#711
reez merged 1 commit intobitcoindevkit:masterfrom
reez:esplora-proxy

Conversation

@reez
Copy link
Copy Markdown
Collaborator

@reez reez commented Mar 26, 2025

Description

Add proxy to Esplora and Electrum

Resolves comment #662 (comment)

Notes to the reviewers

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

@reez reez changed the title Add proxy for Esplora & Electrum (Draft) Add proxy for Esplora & Electrum Mar 26, 2025
@reez reez changed the title (Draft) Add proxy for Esplora & Electrum Add proxy for Esplora & Electrum Mar 26, 2025
@thunderbiscuit
Copy link
Copy Markdown
Member

One comment I have is that this works fairly well with the addition of the new constructor, but if we wanted to expose more of the builder options we'd be stuck with a crazy amount of constructors (one for each combination of arguments the users might need):

  • new_with_proxy
  • new_with_proxy_and_timeout
  • new_with_timeout_and_retry
  • new_with_retry_and_validate_domain
  • new_with_validate_domain_and_proxy

ect.

An alternative approach would be to make use of default arguments in the main constructor. The new parameters can then be added without creating breaking changes.

@reez
Copy link
Copy Markdown
Collaborator Author

reez commented Mar 26, 2025

One comment I have is that this works fairly well with the addition of the new constructor, but if we wanted to expose more of the builder options we'd be stuck with a crazy amount of constructors (one for each combination of arguments the users might need):

  • new_with_proxy
  • new_with_proxy_and_timeout
  • new_with_timeout_and_retry
  • new_with_retry_and_validate_domain
  • new_with_validate_domain_and_proxy

ect.

An alternative approach would be to make use of default arguments in the main constructor. The new parameters can then be added without creating breaking changes.

Absolutely! Updated

@reez reez requested a review from thunderbiscuit March 26, 2025 20:50
@thunderbiscuit
Copy link
Copy Markdown
Member

thunderbiscuit commented Mar 27, 2025

I don't personally like the Rust naming much but I see that the electrum client calls its method on the builder socks5 (whereas the Esplora calls it the more intuitive name proxy).

I'm sort of open to keeping the name proxy here for the Electrum client but it is a slight rename of the true Rust method.

@thunderbiscuit
Copy link
Copy Markdown
Member

Ok scratch my previous comment. After asking chatGPT I get this interesting answer:

ChatGPT Question and Answer

The rust electrum-client library has a builder with a socks5 method. Is this socks5 method always used for proxies? Is it used for anything else?

Great question.

In the context of the electrum-client Rust library, the .socks5() method is specifically and only used to configure a SOCKS5 proxy. It’s not used for anything else — not for authentication, TLS settings, or other types of proxy protocols like HTTP proxies.

TL;DR

✅ Used for:
• Connecting to an Electrum server via a SOCKS5 proxy

❌ Not used for:
• HTTP proxies
• Authentication (username/password for the SOCKS5 proxy — not supported out of the box)
• TLS/SSL settings
• General connection options

My thoughts

I think that in that way since proxy5 seems to be a subset of what a proxy means, it's probably more accurate to keep the name socks5 (even though I didn't really know what it meant the first time I saw it).

@reez
Copy link
Copy Markdown
Collaborator Author

reez commented Mar 27, 2025

@thunderbiscuit yeah I thought the naming was tricky so I went for proxy on both, but totally open to renaming. To make sure I'm on the same page, are you thinking:

constructor(string url, optional string? socks5 = null);

instead of

constructor(string url, optional string? proxy = null);

or something different on the Electrum specifically?

@thunderbiscuit
Copy link
Copy Markdown
Member

Yeah that's what I had in mind.

// Electrum
constructor(string url, optional string? socks5 = null);

// Esplora
constructor(string url, optional string? proxy = null);

The argument on the socks5 method on the Electrum builder is actually socks5_config, but in this case because we are transforming the builder pattern into a constructor, I think keeping the name of the method is clearer.

@reez
Copy link
Copy Markdown
Collaborator Author

reez commented Mar 27, 2025

Yeah that's what I had in mind.

// Electrum
constructor(string url, optional string? socks5 = null);

// Esplora
constructor(string url, optional string? proxy = null);

The argument on the socks5 method on the Electrum builder is actually socks5_config, but in this case because we are transforming the builder pattern into a constructor, I think keeping the name of the method is clearer.

Updated 👍

Copy link
Copy Markdown
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK a3dce7a.

@reez
Copy link
Copy Markdown
Collaborator Author

reez commented Mar 27, 2025

rebased, will merge after tests pass

@reez reez merged commit 78cf4fd into bitcoindevkit:master Mar 27, 2025
26 checks passed
@reez reez deleted the esplora-proxy branch March 27, 2025 16:48
@reez reez mentioned this pull request Mar 27, 2025
8 tasks
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