Add a transports crate & initial Network abstraction #2

prestwich · 2023-07-10T21:53:09Z

Transports crate handles JSON-RPC semantics and is based on past work in ethers-rs

gakonst

good start - light pass

crates/transports/src/call.rs

crates/transports/src/error.rs

crates/transports/src/transports/http.rs

crates/transports/src/call.rs

crates/transports/src/transport.rs

prestwich · 2023-07-12T18:54:09Z

making a few trait changes here

crates/network/src/lib.rs

gakonst · 2023-08-18T22:06:23Z

crates/middleware/src/lib.rs

+    fn send_transaction<'s: 'fut, 'a: 'fut, 'fut>(
+        &'s self,
+        tx: &'a N::TransactionRequest,
+    ) -> MwareFut<'fut, N::Receipt, TransportError> {


love the param over the network

gakonst · 2023-08-18T22:07:10Z

crates/networks/src/lib.rs

+{
+    fn set_gas(&mut self, gas: alloy_primitives::U256);


This seems to make sense to me - wdyt @mattsse

gakonst · 2023-08-18T22:08:09Z

crates/middleware/src/lib.rs

+/// Middleware use a tower-like Layer abstraction
+pub trait MwareLayer<N: Network> {
+    type Middleware<T: Transport>: Middleware<N, T>;
+
+    fn layer<M, T>(&self, inner: M) -> Self::Middleware<T>
+    where
+        M: Middleware<N, T>,
+        T: Transport;
+}


seems reasonable, curious to see how this ends up looking for various stacks

gakonst · 2023-08-18T22:08:55Z

crates/middleware/src/lib.rs

+
+/// Middleware is parameterized with a network and a transport. The default
+/// transport is type-erased, but you can do `Middleware<N, Http>`.
+pub trait Middleware<N: Network, T: Transport = BoxTransport>: Send + Sync {


it might be worth doing a couple test-drives of this abstraction with an Ethereum mainnet and Celo transaction and having both providers side-by-side

you mean ethereum and optimism?

Yes or that, just recall the Celo txs having more differences, but actually we have the Optimism Deposit Tx which should be a good test too

gakonst · 2023-08-18T22:11:38Z

crates/middleware/src/lib.rs

+        &'s self,
+        tx: &'a N::TransactionRequest,


ok so we de-asynctraited the trait, i guess it makes some things easier, but havent fully realized it yet

I'll re-async-trait it. just wanted to make sure i understood the inner workings of async-trait so i could make better decisions about it

I was optimistic that I would be able to introduce a non-boxpinned future type that would cover mware usecases, but that results in some type complexity explosion and extra indirection for to erase types of boxed fns 😮‍💨

gakonst · 2023-08-18T22:13:53Z

crates/transports/src/transports/http.rs

+
+    #[inline]
+    fn call(&mut self, req: Box<RawValue>) -> Self::Future {
+        let replacement = self.client.clone();


guessing this is ok to do?

this is the tower-recommended pattern. It's permissible for the clone to not return ready on poll_ready, so you replace to ensure the original is the one filling the service

gakonst · 2023-08-18T22:14:11Z

crates/transports/src/transports/http.rs

+            let json = resp.text().await?;
+
+            RawValue::from_string(json).map_err(|err| TransportError::deser_err(err, ""))


Maybe bytes faster here?

RawValue is a wrapper for str (not &str), and should re-use the String's allocation. so it ought to be equivalent

gakonst · 2023-08-18T22:15:42Z

crates/transports/src/transports/json_service.rs

+        match to_json_raw_value(&req) {
+            Ok(raw) => JsonRpcFuture {
+                state: States::Pending {
+                    fut: client.call(raw),
+                },
+                _resp: std::marker::PhantomData,
+            },


really hoping for some giga-unsafe way to get us around these manual conversions

crates/transports/src/transports/mod.rs

gakonst · 2023-08-18T22:20:07Z

crates/transports/src/transports/mod.rs

+impl Clone for BoxTransport {
+    fn clone(&self) -> Self {
+        Self {
+            inner: self.inner.clone_box(),
+        }
+    }
+}
+
+trait CloneTransport: Transport {
+    fn clone_box(&self) -> Box<dyn CloneTransport + Send + Sync>;
+}
+
+impl<T> CloneTransport for T
+where
+    T: Transport + Clone + Send + Sync,
+{
+    fn clone_box(&self) -> Box<dyn CloneTransport + Send + Sync> {
+        Box::new(self.clone())
+    }
+}


Can be simplified by doing trait CloneTransport: Transport + Clone + Send + Sync {} and using that inside of BoxTransport? Or do we need all intermediate traits/should we seal them

traits with a Clone supertrait are never object safe as Sized is a supertrait of Clone. The CloneTransport trait here is private, and only used by the Clone impl on BoxTransport, so we don't need to seal it. it's just invisible to lib consumers

this code is basically a copy of the structure of tower's BoxCloneService setup. which I wanted to use, but doesn't have + Send + Sync

gakonst

We should also check to see that error handling is "nice" even when stacking many layers.

As-is right now I mainly see that we have removed generics from the providers, but the middleware abstraction looks somewhat similar with before eg fn inner(), beyond using the more Tower-esque functions for stacking?

gakonst · 2023-08-23T17:59:10Z

crates/transports/src/client.rs

+    /// `true` if the transport is local.
+    pub(crate) is_local: bool,


What's the difference between a local and non-local transport?

we might write code to slam local nodes and not slam remote nodes

prestwich · 2023-08-23T21:29:18Z

We should also check to see that error handling is "nice" even when stacking many layers.
As-is right now I mainly see that we have removed generics from the providers, but the middleware abstraction looks somewhat similar with before eg fn inner(), beyond using the more Tower-esque functions for stacking?

Basically the only way to achieve nice error types AND stacking AND useful object-safety is to have a single error type and force all middleware to use it. so that's the current plan

// no generic
pub enum MiddlewareError {
    TransportError(TransportError),
    Custom(Box<...>)
}

We can't have an associated type or a generic type on the middleware trait, as that would break Box<dyn ...> across Mware with different error types. E.g. Box<dyn Middleware<N, Err1>> is different from Box<dyn Middleware<N, Err2>>. Same for Box<dyn Middleware<N, Error=Err1>> vs <N, Error = Err2> This ruins user abstractions unless they have the exact same middleware stack for each instance

feature: transports crate

83a5c8a

prestwich added the enhancement New feature or request label Jul 10, 2023

prestwich requested a review from DaniPopes July 10, 2023 21:53

prestwich self-assigned this Jul 10, 2023

gakonst reviewed Jul 12, 2023

View reviewed changes

prestwich added 2 commits July 12, 2023 08:56

review: address comments

0203bad

refactor: RpcObject trait

7528c5e

gakonst reviewed Jul 12, 2023

View reviewed changes

crates/transports/src/call.rs Outdated Show resolved Hide resolved

crates/transports/src/transport.rs Outdated Show resolved Hide resolved

prestwich added 15 commits July 12, 2023 14:07

refactor: RpcResult type

fe6142d

refactor: small code quality

b2e3705

deps: bump Cargo.toml

f1010c6

chore: delete unused src

04f3c49

refactor: more stuff

366a5ac

refactor: remove Params type from RpcCall

4de4a8e

fix: remove extra to_json_raw_value

27a6c0f

refactor: minor legibility

e2703df

feat: unwrap variants

53128fd

fix: add debug bounds

76bef7f

feature: some cool combinators on rpccall

d654407

fix: hide __ENFORCE_ZST

1d691ec

feature: DummyNetwork compile check

0073a12

test: dummynet compile checks

9d736b5

doc: fix comment

268412b

prestwich changed the title ~~Add a transports crate~~ Add a transports crate & initial Network abstraction Jul 13, 2023

prestwich commented Jul 13, 2023

View reviewed changes

crates/network/src/lib.rs Outdated Show resolved Hide resolved

prestwich added 4 commits July 13, 2023 18:05

wip: mware and combinator stuff

3cf3f2d

fuck jsonrpsee

5c53aa8

feature: blanket

efb5cdc

test: http impls transport

cf00429

prestwich added 16 commits August 1, 2023 15:12

feature: manual future for json rpc to avoid higher-ranked lifetime

dcdb4cf

cleanup: some clippy and stuff

d45a19d

feature: client builder

b8dbb9a

refactor: move is_local to transport

763a5c6

chore: clippy

9d1f491

feature: generic request

0f3fb8f

feature: allow type-erased rpc client

843009b

chore: cleanup in transports mod

2b83171

chore: misc cleanup

562e539

refactor: more crate

ceda9f7

chore: clippy cleanup

b7a8d1e

fix: lifetimes for rpc calls

36513b2

feature: lifetime on rpccall

e204269

feature: BoxTransport

7a9726e

chore: clippy

ba8fd02

refactor: cow for jsonrpc params

5cd4826

gakonst reviewed Aug 18, 2023

View reviewed changes

prestwich added 8 commits August 19, 2023 10:05

fix: lint

7f8923b

refactor: rename to boxed

047ba1d

docs and misc convenience

c5ce15b

refactor: docs and cleanup

b7fbff4

docs: more of em

7173fcc

refactor: seal transport

ee69008

feature: seal transport

5770e67

rename middleware to provider

0c6b2c9

gakonst reviewed Aug 23, 2023

View reviewed changes

chore: remove dead code

8a6a838

prestwich merged commit 611a057 into main Aug 24, 2023
12 checks passed

gakonst deleted the prestwich/transports branch August 24, 2023 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a transports crate & initial Network abstraction #2

Add a transports crate & initial Network abstraction #2

prestwich commented Jul 10, 2023

gakonst left a comment

prestwich commented Jul 12, 2023

gakonst Aug 18, 2023

gakonst Aug 18, 2023

gakonst Aug 18, 2023

gakonst Aug 18, 2023

prestwich Aug 19, 2023

gakonst Aug 20, 2023

gakonst Aug 18, 2023

prestwich Aug 19, 2023

prestwich Aug 19, 2023

gakonst Aug 18, 2023

prestwich Aug 19, 2023

gakonst Aug 18, 2023

prestwich Aug 19, 2023

gakonst Aug 18, 2023

gakonst Aug 18, 2023

prestwich Aug 19, 2023 •

edited

Loading

prestwich Aug 19, 2023

gakonst left a comment

gakonst Aug 23, 2023

prestwich Aug 23, 2023

prestwich commented Aug 23, 2023

		let json = resp.text().await?;

		RawValue::from_string(json).map_err(\|err\| TransportError::deser_err(err, ""))

		/// `true` if the transport is local.
		pub(crate) is_local: bool,

Add a transports crate & initial Network abstraction #2

Add a transports crate & initial Network abstraction #2

Conversation

prestwich commented Jul 10, 2023

gakonst left a comment

Choose a reason for hiding this comment

prestwich commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestwich Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestwich commented Aug 23, 2023

prestwich Aug 19, 2023 •

edited

Loading