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

Remove proxy as optional feature #143

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thunderbiscuit
Copy link
Member

Potentially fixes #91.

I wanted to take a stab at this but I'm not sure I really understood why proxy was a feature in the first place. Was there a reason to have it turned off that we want to preserve, in which case we might need to address this by changing the feature-gating done on the Client type?

Anyway just opening this to see if it passes the whole CI workflow.

@tnull
Copy link
Contributor

tnull commented Aug 13, 2024

Potentially fixes #91.

I wanted to take a stab at this but I'm not sure I really understood why proxy was a feature in the first place. Was there a reason to have it turned off that we want to preserve, in which case we might need to address this by changing the feature-gating done on the Client type?

Anyway just opening this to see if it passes the whole CI workflow.

FWIW, it would be great to be able to disable the optional dependencies. However, as it stands, the proxy feature doesn't really seem to be fully optional. The only difference it makes is that users that only want to use api::ElectrumApi, but not the actual client would be able to disable the additional proxy dependencies. Then again, if that's the only use-case, it should probably be called client, not proxy, as it's mandatory for client use currently.

Comment on lines +439 to 443
#[cfg(feature = "default")]
/// Transport type used to establish a connection to a server through a socks proxy
pub type ElectrumProxyStream = Socks5Stream;
#[cfg(any(feature = "default", feature = "proxy"))]
#[cfg(feature = "default")]
impl RawClient<ElectrumProxyStream> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC this is the only place it's required to have the proxy feature, on both ElectrumProxyStream type and the RawClient<ElectrumProxyStream> implementation.

I think the default should the ElectrumSSlStream equivalent, as the use-rustls is a default feature.

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.

Client is only exported when proxy feature is enabled.
3 participants