Feature: Joinable transaction fillers #426

prestwich · 2024-03-29T21:59:03Z

WIP PR for discussion

Motivation

Closes [Bug] Ordering problem in interaction between NonceManagerLayer and SignerLayer #406
Supersedes feat(layers): FillTxLayer #374
Resolves ordering issues with tx fillers by creating signaling for ready and finished and calling in a loop until all are either finished or unready

Solution

create a TxFiller<N> trait
create a JoinFill<L, R, N> to compose TxFiller<N>
Create a FillProvider<F, P, N, T> so that TxFiller<N> all share a common provider layer

Remaining:

See CONSIDER comments for discussion
Convert gas estimation and nonce and chain id to use this pattern
Modify ProviderBuilder to add .with_filler() for fillers
Add send_transaction_internal to abstract over builder and envelope
Change behavior of <RootProvider as Provider>::send_transaction to always prefer raw transaction
finish porting signer
tests passing

PR Checklist

Added Tests
Added Documentation
Breaking changes

crates/provider/src/layers/join_fill.rs

gakonst · 2024-04-05T19:30:22Z

crates/contract/README.md

-let provider = ProviderBuilder::new().with_recommended_layers().on_builtin("http://localhost:8545").await?;
+let provider = ProviderBuilder::new().with_recommended_fillers().on_builtin("http://localhost:8545").await?;


nit, both layers + fillers in example?

and still thinking about the on_builtin naming separately, did you not like connect_?

there are currently no layers because they were all converted to fillers 😅

gakonst · 2024-04-05T19:42:39Z

crates/network/src/transaction/builder.rs

+    /// True if the builder contains all necessary information to be submitted
+    /// to the `eth_sendTransaction` endpoint.
+    fn can_submit(&self) -> bool;
+
+    /// True if the builder contains all necessary information to be built into
+    /// a valid transaction.
+    fn can_build(&self) -> bool;


gakonst · 2024-04-05T19:45:15Z

crates/provider/Cargo.toml

 alloy-json-rpc.workspace = true
 alloy-network.workspace = true
+alloy-node-bindings = { workspace = true, optional = true }
+alloy-signer-wallet = { workspace = true, optional = true }


i am guessing this is fine / no risk of circular dep in the future

it'd be a major break from the abstraction stack if there were 😅

gakonst · 2024-04-05T19:46:03Z

crates/provider/src/builder.rs

@@ -22,6 +29,33 @@ pub trait ProviderLayer<P: Provider<T, N>, T: Transport + Clone, N: Network = Et
 #[derive(Debug, Clone, Copy)]
 pub struct Identity;

+impl<N> TxFiller<N> for Identity


gakonst

Great stuff. Mainly nits and questions.

gakonst · 2024-04-05T20:06:36Z

crates/provider/src/builder.rs

+    /// Add a chain ID filler to the stack being built. The filler will attempt
+    /// to fetch the chain ID from the provider using
+    /// [`Provider::get_chain_id`]. the first time a transaction is prepared,
+    /// and will cache it for future transactions.
+    pub fn fetch_chain_id(self) -> ProviderBuilder<L, JoinFill<Identity, ChainIdFiller>, N> {
+        self.filler(ChainIdFiller::default())
+    }
+
+    /// Add a specific chain ID to the stack being built. The filler will
+    /// fill transactions with the provided chain ID, regardless of the chain ID
+    /// that the provider reports via [`Provider::get_chain_id`].
+    pub fn with_chain_id(
+        self,
+        chain_id: u64,
+    ) -> ProviderBuilder<L, JoinFill<Identity, ChainIdFiller>, N> {
+        self.filler(ChainIdFiller::new(Some(chain_id)))
    }


this is cool

crates/provider/src/builder.rs

gakonst · 2024-04-05T20:07:26Z

crates/provider/src/builder.rs

+    /// Add preconfigured set of layers handling gas estimation and nonce management
+    pub fn with_recommended_fillers(self) -> ProviderBuilder<L, RecommendFiller, N> {
+        self.filler(GasFiller).filler(NonceFiller::default()).filler(ChainIdFiller::default())
+    }


Am I allowed to do with_recommended_fillers().with_chain_id(1)? I guess yes?

the answer is no*. no because with_chain_id is implemented only when the builder has the Identity nop filler.

you could do with_recommended_fillers().filler(ChainIdFiller::new(Some(1))) but then you would have multiple ChainId fillers

crates/provider/src/builder.rs

gakonst · 2024-04-05T20:17:33Z

crates/provider/src/builder.rs

+        L: ProviderLayer<
+            crate::ReqwestProvider<Ethereum>,
+            alloy_transport_http::Http<reqwest::Client>,
+            Ethereum,


Maybe AnyNetwork? Believe we have some non-Ethereum code in Anvil for RPC responses in some places, or when forking?

kinda reluctant here, as it adds complexity to writing tests to cover behavior that the tests don't really need

gakonst · 2024-04-05T20:54:01Z

crates/provider/src/provider.rs

+        match tx {
+            SendableTx::Builder(tx) => {
+                let tx_hash = self.client().request("eth_sendTransaction", (tx,)).await?;
+                Ok(PendingTransactionBuilder::new(self.root(), tx_hash))
+            }
+            SendableTx::Envelope(tx) => {
+                let mut encoded_tx = vec![];
+                tx.encode_2718(&mut encoded_tx);
+                self.send_raw_transaction(&encoded_tx).await
+            }
+        }


found this enum idea + internal very interesting

originally i was going to push this enum into the network crate, but it turned out to be a lot easier and cleaner to do it here. additional followup work would be send_envelope

gakonst · 2024-04-05T20:54:44Z

crates/provider/src/provider.rs

+/// This type is used to allow for fillers to convert a builder into an envelope
+/// without changing the user-facing API.
+///
+/// Users should NOT use this type directly. It should only be used as an
+/// implementation detail of [`Provider::send_transaction_internal`].


gakonst · 2024-04-05T20:57:17Z

crates/provider/src/fillers/signer.rs

+    async fn fill(
+        &self,
+        _fillable: Self::Fillable,
+        tx: SendableTx<N>,
+    ) -> TransportResult<SendableTx<N>> {
+        let builder = match tx {
+            SendableTx::Builder(builder) => builder,
+            _ => return Ok(tx),
+        };
+
+        let envelope = builder.build(&self.signer).await.map_err(RpcError::local_usage)?;
+
+        Ok(SendableTx::Envelope(envelope))
+    }


OK I see the need for the enum now.

Are there other cases beyond signers where we expect the builder -> envelope conversion?

technically signing is the only way to make this conversion, as the builder will never have a signature, and the envelope must have a signature

gakonst · 2024-04-05T20:58:31Z

crates/provider/src/fillers/signer.rs

+    N: Network,
+    S: NetworkSigner<N> + Clone,
+{
+    type Fillable = ();


Is this the signature type of the NetworkSigner? But I guess is unused.

NetworkSigner never outputs a signature type, to allow for abstraction over future signature types. This is the only implementation reason why fill is async. I would have liked to keep it sync, but that's incompatible with signature abstraction unless we introduce an additional abstraction like SignatureFor<N> which I think we don't want to do 😮‍💨

gakonst · 2024-04-05T20:58:53Z

crates/provider/src/fillers/signer.rs

+impl<S, N> TxFiller<N> for SignerFiller<S>
+where
+    N: Network,
+    S: NetworkSigner<N> + Clone,


Given this, how should users think about NetworkSigner vs Signer vs ...?

The average user should:

Always use NetworkSigner

Never use Signer

Never use TxSigner

mattsse

I like that we now have a clear and configurable processing for:
a) filling transactions
b) building them

for most use cases the builtin fillers are sufficient but they can still be customized, only then the user has to deal with the filler trait
and the behaviour is now properly documented and should be easy to follow

crates/json-rpc/src/error.rs

mattsse · 2024-04-06T05:08:04Z

crates/network/src/any/builder.rs

    }

-    fn get_blob_sidecar(&self) -> Option<&BlobTransactionSidecar> {
-        self.deref().get_blob_sidecar()
+    fn blob_sidecar(&self) -> Option<&BlobTransactionSidecar> {


dropping the get_ prefix is good

matches the style of the rest of all the other methods

mattsse · 2024-04-06T05:09:46Z

crates/provider/src/builder.rs

+        <JoinFill<F, SignerFiller<alloy_network::EthereumSigner>> as ProviderLayer<
+            L::Provider,
+            alloy_transport_http::Http<reqwest::Client>,
+        >>::Provider,
+        alloy_node_bindings::AnvilInstance,
+    )
+    where
+        L: ProviderLayer<
+            crate::ReqwestProvider<Ethereum>,
+            alloy_transport_http::Http<reqwest::Client>,
+            Ethereum,
+        >,
+        F: TxFiller<Ethereum>
+            + ProviderLayer<L::Provider, alloy_transport_http::Http<reqwest::Client>, Ethereum>,


hehe, fine with this since all of this is internal complexity

just don't look at this impl block lol

onbjerg

smol nit, t first pass this seems ok

crates/provider/src/fillers/mod.rs

Co-authored-by: Oliver Nordbjerg <[email protected]>

@DaniPopes

* feature: TxFiller * lint: clippy * doc: CONSIDER * doc: more notes * fix: get rid of ugly lifetimes * fix: docs and lints * fix: remove populate functions * nit: remove commented code * feature: FillerControlFlow * doc: lifecycle notes * refactor: request -> prepare * lint: clippy * fix: missing block in absorb * fix: absorb preserves association * refactor: additional info in missing * fix: impl_future macro * fix: resolve considers * refactor: gas layer to gas filler * refactor: improve provider builder * refactor: filler is outmost layer * refactor: protect against double-fill and add anvil shortcut * doc: improve docstrings for noncemanager and gas filler * fix: delete unused types * refactor: layers to fillers * feature: chain id filler * refactor: send_transaction_inner and SendableTx * wip: sig filler * refactor: SignerFiller * fix: remove clone * docs: fix some * fix: complete todo * feature: anvil feature for alloy-provider * wip: tests * fix: apply changes from other PRs * chore: fmt * feature: on_anvil * fix: workaround anvil gas est issue * fix: doctests * fix: anvil => request * fix: test * chore: note about blocking on TODO * feature: local usage error * fix: review nits * Update crates/provider/src/fillers/mod.rs Co-authored-by: Oliver Nordbjerg <[email protected]> * fix: capitalization so @DaniPopes doesn't hurt me --------- Co-authored-by: Oliver Nordbjerg <[email protected]>

prestwich added enhancement New feature or request debt Tech debt which needs to be addressed labels Mar 29, 2024

prestwich self-assigned this Mar 29, 2024

prestwich changed the title ~~Feature: Joinable transaction fille~~ Feature: Joinable transaction fillers Mar 29, 2024

prestwich commented Mar 29, 2024

View reviewed changes

crates/provider/src/layers/join_fill.rs Outdated Show resolved Hide resolved

mattsse reviewed Mar 29, 2024

View reviewed changes

crates/provider/src/layers/join_fill.rs Outdated Show resolved Hide resolved

crates/provider/src/layers/join_fill.rs Outdated Show resolved Hide resolved

prestwich force-pushed the prestwich/join-fill branch 2 times, most recently from 1aebabf to dd3da7a Compare March 30, 2024 16:32

prestwich mentioned this pull request Apr 1, 2024

feat(layers): FillTxLayer #374

Closed

3 tasks

prestwich force-pushed the prestwich/join-fill branch 2 times, most recently from 5be7b58 to 0d29fb8 Compare April 2, 2024 22:52

prestwich mentioned this pull request Apr 3, 2024

[Bug] Delete internal-test-utils #452

Closed

prestwich force-pushed the prestwich/join-fill branch from 243d7e4 to 7ea802d Compare April 3, 2024 16:32

prestwich mentioned this pull request Apr 5, 2024

[Bug] Transport(Custom(MissingKey("gas_price"))) in TransactionRequest #464

Closed

prestwich linked an issue Apr 5, 2024 that may be closed by this pull request

[Bug] Transport(Custom(MissingKey("gas_price"))) in TransactionRequest #464

Closed

prestwich added 15 commits April 5, 2024 07:53

feature: TxFiller

adaef96

lint: clippy

ec38e5d

doc: CONSIDER

247599c

doc: more notes

1621849

fix: get rid of ugly lifetimes

9e009ca

fix: docs and lints

d1a19c5

fix: remove populate functions

55ef5f3

nit: remove commented code

a64b1c4

feature: FillerControlFlow

a76ecaf

doc: lifecycle notes

d272290

refactor: request -> prepare

935fd9b

lint: clippy

021c1bd

fix: missing block in absorb

c19aaf4

fix: absorb preserves association

5f59ec0

refactor: additional info in missing

49cf6a8

prestwich added 4 commits April 5, 2024 07:58

fix: complete todo

ca3197d

feature: anvil feature for alloy-provider

bc3bc80

wip: tests

8022cc8

fix: apply changes from other PRs

7353f79

prestwich force-pushed the prestwich/join-fill branch from 0d1f468 to 7353f79 Compare April 5, 2024 15:02

prestwich added 6 commits April 5, 2024 08:17

chore: fmt

1c376d7

feature: on_anvil

9401eac

fix: workaround anvil gas est issue

0846652

fix: doctests

a7295e5

fix: anvil => request

90ba04c

fix: test

fe0aa67

prestwich marked this pull request as ready for review April 5, 2024 18:26

prestwich requested review from DaniPopes, gakonst, onbjerg and Evalir as code owners April 5, 2024 18:26

chore: note about blocking on TODO

4ced66c

gakonst reviewed Apr 5, 2024

View reviewed changes

feature: local usage error

e21f8fe

prestwich removed a link to an issue Apr 5, 2024

[Bug] Transport(Custom(MissingKey("gas_price"))) in TransactionRequest #464

Closed

gakonst reviewed Apr 5, 2024

View reviewed changes

fix: review nits

fef56d4

mattsse reviewed Apr 6, 2024

View reviewed changes

onbjerg reviewed Apr 6, 2024

View reviewed changes

crates/provider/src/fillers/mod.rs Outdated Show resolved Hide resolved

prestwich and others added 2 commits April 6, 2024 11:40

Update crates/provider/src/fillers/mod.rs

52ce1dc

Co-authored-by: Oliver Nordbjerg <[email protected]>

fix: capitalization so @DaniPopes doesn't hurt me

b48ff24

prestwich merged commit da41ddb into main Apr 8, 2024
18 checks passed

prestwich deleted the prestwich/join-fill branch April 8, 2024 16:32

zerosnacks mentioned this pull request Apr 9, 2024

[Feature] Add GasFiller example to contracts and as gas_filler example in layers alloy-rs/examples#12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Joinable transaction fillers #426

Feature: Joinable transaction fillers #426

prestwich commented Mar 29, 2024 •

edited

Loading

gakonst Apr 5, 2024

prestwich Apr 5, 2024

gakonst Apr 5, 2024

gakonst Apr 5, 2024

prestwich Apr 6, 2024

gakonst Apr 5, 2024

gakonst left a comment

gakonst Apr 5, 2024

gakonst Apr 5, 2024

prestwich Apr 5, 2024

gakonst Apr 5, 2024

prestwich Apr 5, 2024

gakonst Apr 5, 2024

prestwich Apr 5, 2024

gakonst Apr 5, 2024

gakonst Apr 5, 2024

prestwich Apr 5, 2024

gakonst Apr 5, 2024

prestwich Apr 5, 2024

gakonst Apr 5, 2024

prestwich Apr 5, 2024 •

edited

Loading

mattsse left a comment

mattsse Apr 6, 2024

prestwich Apr 6, 2024

mattsse Apr 6, 2024

prestwich Apr 6, 2024

onbjerg left a comment

		let provider = ProviderBuilder::new().with_recommended_layers().on_builtin("http://localhost:8545").await?;
		let provider = ProviderBuilder::new().with_recommended_fillers().on_builtin("http://localhost:8545").await?;

Feature: Joinable transaction fillers #426

Feature: Joinable transaction fillers #426

Conversation

prestwich commented Mar 29, 2024 • edited Loading

Motivation

Solution

Remaining:

PR Checklist

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

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

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 Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

mattsse left a 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

onbjerg left a comment

Choose a reason for hiding this comment

prestwich commented Mar 29, 2024 •

edited

Loading

prestwich Apr 5, 2024 •

edited

Loading