refactor(ws-client+ws-transport): make WsTransportClientBuilder generic over a data stream

[oleonardolima]

TL;DR

Refactoring the APIs on WsTranportClientBuilder to generic ones over any data stream that implements the required traits described below would make it easy to use for anyone to use the jsonrpsee websocket client over its desired transport layer and data stream. And it wouldn't lead to maintaining a forked crate or the jsonrpsee crate to absorb and maintain Tor, and others, support.

What

I'm working with @elsirion on adding connection over Tor support to Fedimint client, as part of a SoB'23 project.

In order to do that we are using arti and arti-client, a Rust implementation of Tor, that makes it simpler on adding Tor support for applications written in Rust.

On our research, discussions, and PoC, it makes it really simple to use and build it with TorClient, and mainly the DataStream, an anonymized TCP stream type. The Tor team has an example of its usage with hyper if you got curious.

That said, Fedimint both client and server, uses the JSON-RPC through jsonrpsee crate, in a pretty straightforward manner on the client side, by using the WsClientBuilder.

In order to not have to fork and maintain another jsonrpsee-client-transport crate to have Tor support on it, and just on the ws-transport module, and to not have a whole Tor support implementation for ws-transport on jsonrpsee crate itself, I would like to propose a change on the API for the WsTranportClientBuilder and also gather your opinions on the proposed approach and our idea in general.

Why

I think it's not sustainable to have a forked version of jsonrpsee-client-transport just for adding Tor support on it, currently, only Fedimint would use it, and would still need to keep up with changes on other crates that build on top on jsonrpsee-client-transport and Fedimint uses, like thejsonrpsee-ws-client itself.

And I also think it does not make sense and it's not sustainable to add support to Tor on jsonrpsee-client-transport, unless it's a planned and needed feature on your roadmap, as it would require to keep-up with improvements, security updates, and API from Arti and TorProject team.

How

Researching and focusing on the WsTranportClientBuilder code, it mainly relies on the EitherStream as the initial TCP stream wrapper, a TLS or non-TLS data stream, and on the Sender and Receiver, that implements TransportSenderT and TransportReceiverT, returned by the soketto::connection::Builder finish function.

The EitherStream, implements both AsyncRead and AsyncWrite, which are required by the TransportSenderT and TransportReceiverT, and the TCP stream used is the default tokio::net::tcp::stream::TcpStream which also implements both AsyncRead and AsyncWrite.

The arti DataStream, an anonymized TCP stream type, also implements both AsyncRead and AsyncWrite, and also the tokio version of the traits, that said it's somewhat easy to implement an ArtiEitherStream type, which has both TLS and non-TLS over Tor, on the client side which would manage the TorClient configuration, network bootstrap and circuit isolation (if needed).

Refactoring the APIs on WsTranportClientBuilder to generic ones over any data_stream that implements the required traits described above would make it easy to use for anyone to use the jsonrpsee websocket client over its desired transport layer and data stream, and wouldn't lead to maintaining a forked crate or the jsonrpsee crate to absorb and maintain Tor, and others, support.

// example

pub struct Sender<T: AsyncRead + AsyncWrite + Unpin> {...}

pub struct Receiver<T: AsyncRead + AsyncWrite + Unpin> {...}

...

impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static> TransportSenderT for Sender<T> {...}

impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static> TransportSenderT for Sender<T> {...}

...

impl WsTransportClientBuilder {
    // NOTE: maintaining a default TCP one
    pub async fn build(self, uri: Uri) -> Result<(Sender<EitherStream>, Receiver<EitherStream>), WsHandshakeError> {
            let target: Target = uri.try_into()?;
            self.try_connect_over_tcp(target).await
        }

    pub async fn build_with_stream<T>(
            &self,
            uri: Uri,
            connector: Option<TlsConnector>,
            data_stream: T,
        ) -> Result<(Sender<T>, Receiver<T>), WsHandshakeError> {...}

    async fn try_connect_over_tcp(
            self,
            mut target: Target,
        ) -> Result<(Sender<EitherStream>, Receiver<EitherStream>), WsHandshakeError> {...}

    async fn try_connect<T>(
            &self,
            mut target: Target,
            mut connector: Option<TlsConnector>,
            data_stream: T,
        ) -> Result<(Sender<T>, Receiver<T>), WsHandshakeError> {...}
}

--

I'm currently working on it, and at the moment the required changes would be close to these here: oleonardolima@592da07 (plus the updated tests), and willing to continue working on it with your suggestions towards a final solution 😀

I'm proposing the usage of generics as I guess it's the crate standard by taking a look at jsonrpsee-core async-client module, and on other crates such as socketto 🤔 , but I'm open to suggestions, discussions , and also if you guys have another idea that would make the usage with other data streams easier.

What do you guys think about the idea? Any other suggestions? Does it make sense for jsonrpsee crate ? 😁

Thanks!

[niklasad1]

Hi @oleonardolima

Thanks for the informative writeup but I'm having troubles to understand whether your idea is to make the underlying stream generic to support different WebSocket streams such as your "anonymized TCP stream" or make it generic to support other transports such as HTTP and remove the TransportSenderT and TransportReceiverT?

Make it generic on only WebSocketClient and to allow overriding the default stream seems like a nice idea.

[oleonardolima]

Hey @niklasad1, thanks for the response, and sorry for the delay.

The idea is to make the WsTransportClientBuilder generic over a stream T to support having WebSocket connections over different streams, e.g. TCP, Tor, or any other connection socket that can be 'wrapped' in a stream.

I think that by this approach, it won't need to remove TransportSenderT/TransportReceiverT as it will be helpful for users to know the required traits for the stream in use, as it would be used on Receiver<T> and Sender<T> 🤔

Cool! That's the idea, the default implementation would still use a TCP stream, and a more advanced client could build a WsClient with a specified WsTransportClientBuilder over its desired data stream 😁

[jsdw]

I've recently also been pondering these traits. Specifically, I wonder whether one should be able to build a jsonrpsee Client from any object (or pair of objects) implementing traits that looked something like this:

/// Something that can be handed bytes to send out.
pub trait SenderT: Send + Sync + 'static {
    /// Send a message out, returning an error if something goes wrong in doing so.
    fn send(&self, data: &[u8]) -> Pin<Box<dyn Future<Output = Result<(), BackendError>> + Send + 'static>>;
}

/// Something that can receive bytes from a source.
pub trait ReceiverT: Send + 'static {
    /// Wait for the next message back. `jsonrpsee` will interpret the bytes as a JSON-RPC Response and
    /// take it from there.
    fn receive(&self) -> Pin<Box<dyn Future<Output = Option<Result<Vec<u8>, BackendError>>> + Send + 'static>>;
}

/// An error that can occur from the backend.
pub type BackendError = Arc<dyn std::error::Error + Send + Sync + 'static>;

These are super general, and implementations (be it WebSocket web, http, whatever) basically just need to be able to describe how they will hand back a response to the client (via receive) or send a message from the client (via send).

My examples currrently use a simple Vec to receive each message under my assumption that it probably wants owned data anyway, but potentially jsonrpsee could provide the buffer or whatever else for performance reasons.

An implementation of these would exist then for Websocket connections (eg something that adapts Soketto bits into the right shape), and at that level, AsyncRead/AsyncWrite things or whatever could be accepted to configure the underlying stream etc.

Not sure if that helps here; just wanted to write my thoughts down!

[flipchan]

Great pr @oleonardolima , I want to add tor and socks5 support for http://github.com/uptest-sc/uptest, which rely on JsonRpsee as its client. Following this thread