search

bitcoindevkit / rust-electrum-client

Bitcoin Electrum client library. Supports plaintext, TLS and Onion servers.
MIT License
80 stars 62 forks source link

Clean up dependencies and features #146

Open LLFourn opened 2 months ago

LLFourn commented 2 months ago

There is way too much feature gating logic to me.

  1. We shouldn't have a method like new_proxy_ssl, just have a single method that starts an electrum TLS connection given a TcpStream. Of course then we have a convenience method to do the initial connection to a url string if they don't want proxies. But if they do then it's fine to have them set up the proxy connection themselves.
  2. Do we even need openssl as a dependency at all? If someone wants to use socks5 then can depend on openssl themelves or use one of the many pure rust socks5 client libraries like socks5-client. We can have an example showing how to do tor. IMO it'd be better to try and use socks5-client rather than openssl. For TLS we can just be opinionated and use rustls.

This seems like something you would want to do anyway @oleonardolima for arti?

I'll turn this into an issue that I think we should tackle first.

_Originally posted by @LLFourn in https://github.com/bitcoindevkit/rust-electrum-client/pull/138#discussion_r1757704931_

oleonardolima commented 2 months ago

Yes, I do agree with both (1) and (2), there are also issues with other features such s proxy previously mentioned by tnull, refer to https://github.com/bitcoindevkit/rust-electrum-client/issues/91, which is basically mandatory and we can't use the client with --no-default-features.

Indeed, with arti-client I do need the client to receive a data stream (e.g. TcpStream, arti-client::DataStream), and was planning on such refactoring. It also requires being async, which detangling all existing features will make it easy to have an async version.

Thanks for opening the issue, I moved the #138 back to draft for now.