feat: network abstraction and transaction builder #190

prestwich · 2024-02-06T21:13:58Z

Motivation

We want a transaction builder that can build one of several strongly-typed consensus transactions for signing to implement send_transaction. The intended flow is:

User specifies fields they want to add to their transaction
They pass it off to a signer, where the concrete transaction type is resolved from the builder (e.g. legacy, EIP-1559, ..)
The transaction is signed and sent

This is made complicated due to our network abstraction:

Different networks have a different set of transactions
Different networks can have different signature schemes, and some are valid for the same transaction types, i.e. our current associated type Signature for transactions is not a good abstraction

This flow should also work server side, i.e. if I get a TransactionRequest over the wire from a client, I should be able to resolve it to a consensus type transaction. I should also be able to take an already-signed RLP-encoded transaction and decode it into one of the valid transaction types.

This PR intends to:

Add a builder
Complete the request → signer flow, including adding send_transaction in the provider and the alloy-contract call builder
Remove the temporary provider (and related code) and fully transition to the network-parameterized provider type

Solution

Adds a TransactionBuilder trait and implements this for TransactionRequest
Adds an Ethereum network with all types defined
Removes the temporary provider, replacing it with a new, network-parameterized provider
Adjusts alloy-contract to work with the new provider
Moves a lot of types around, because a lot of the abstractions we made had not been put together, and could not without circular dependencies
Splits Transaction into Transaction and SignableTransaction, since not all transactions are signable (e.g. Optimism deposit tx's)
Moves the associated Signature type in Transaction into a parameter instead
Adds 3 new signer traits: TxSigner, TxSignerSync and NetworkSigner
- TxSigner is essentially ripped out of the Signer trait and should be implemented for all signers, even if the signer does not care about transactions.
- It is needed for e.g. Ledger/Trezor where tx info is needed. A blanket implementation is not provided because it isn't allowed.
- These two traits essentially state that a signer can sign any transaction with the given Signature. Signer implementors need to implement this and Signer.
- NetworkSigner instead is implemented by network crate authors. This trait is implemented for a wrapper type and essentially states that the signer can sign any transaction for the given network. (See EthereumSigner)
Finish up the ProviderBuilder abstraction and add a SignerLayer to add signing on top of an existing provider

Companion PRs:

Things left:

Possibly improve the ergonomics of ProviderBuilder a bit

Follow ups

Block responses in the provider are not network parameterized yet, so the transactions and header are the same for all networks. This should be simple to adjust later. ([Feature] Add network-parameterized block responses #267)
Add an AnyNetwork that is essentially Ethereum + OtherFields on all types [Feature] Add AnyNetwork #272
Maybe move Wallet etc. out of alloy-signers to make alloy-network wasm compatible again, see feat: network abstraction and transaction builder #190 (comment) ([Feature] Move Wallet/LocalWallet etc to its own crate #262)
Conversions/impls making it easier to instantiate a RootProvider ([Feature] Make it easier to instantiate a RootProvider #266)

Breaking changes

Anything that uses alloy-contract, alloy-provider, or alloy-consensus likely broke with this PR as the API changed.

PR Checklist

Added Tests
Added Documentation
Breaking changes

crates/consensus/src/lib.rs

crates/consensus/src/transaction/builder.rs

crates/signer/src/signer.rs

gakonst · 2024-02-09T02:21:16Z

crates/consensus/src/transaction/typed.rs

+/// The TypedTransaction enum represents all Ethereum transaction request types.
+///
+/// Its variants correspond to specific allowed transactions:
+/// 1. Legacy (pre-EIP2718) [`TxLegacy`]
+/// 2. EIP2930 (state access lists) [`TxEip2930`]
+/// 3. EIP1559 [`TxEip1559`]
+/// 4. EIP4844 [`TxEip4844`]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub enum TypedTransaction {


Should this impl RLP?

unsure, do we need it? cc @mattsse perhaps

crates/consensus/src/transaction/builder.rs

crates/consensus/src/transaction/eip1559.rs

crates/consensus/src/transaction/eip4844.rs

crates/consensus/src/transaction/legacy.rs

gakonst · 2024-02-15T22:00:32Z

crates/network/src/lib.rs

+// todo: block responses are ethereum only, so we need to include this in here too, or make `Block`
+// generic over tx/header type


let's do as follow-up IMO when we're adding the 2nd network?

crates/network/src/transaction/builder.rs

gakonst · 2024-02-15T22:02:40Z

crates/network/src/transaction/signer.rs

+// todo: move
+/// A signer capable of signing any transaction for the given network.
+#[async_trait]
+pub trait NetworkSigner<N: Network> {
+    /// Asynchronously sign an unsigned transaction.
+    async fn sign(&self, tx: N::UnsignedTx) -> alloy_signer::Result<N::TxEnvelope>;
+}
+
+// todo: move
+/// An async signer capable of signing any [SignableTransaction] for the given [Signature] type.
+#[async_trait]
+pub trait TxSigner<Signature> {
+    /// Asynchronously sign an unsigned transaction.
+    async fn sign_transaction(
+        &self,
+        tx: &mut dyn SignableTransaction<Signature>,
+    ) -> alloy_signer::Result<Signature>;


what's the difference between these 2?

NetworkSigner is a signer capable of signing any transaction with any signature type for a given network, TxSigner is a signer capable of signing any transaction regardless of network for a given signature type.

The idea is to have fewer generics for signers

crates/providers/src/lib.rs

gakonst · 2024-02-15T22:09:45Z

crates/providers/src/lib.rs

+        key: U256,
+        tag: Option<BlockId>,
+    ) -> TransportResult<StorageValue> {
+        self.client.prepare("eth_getStorageAt", (address, key, tag.unwrap_or_default())).await


generally find it weird we're calling this self.client.prepare when it's sending the request, shouldnt it be self.client.prepare(...).send().await or self.client.request(...)?

I think it's called prepare because it doesn't do any serialization or really anything at all until awaited. The send method in your example is equiv to awaiting it

crates/providers/src/lib.rs

crates/contract/src/call.rs

onbjerg · 2024-03-07T19:55:13Z

crates/signer/src/wallet/private_key.rs

@@ -258,81 +257,6 @@ mod tests {
        assert_eq!(recovered2, address);
    }

-    #[tokio::test]


This test was moved to the network crate

crates/signer/src/signer.rs

crates/signer-trezor/src/signer.rs

crates/signer-ledger/src/signer.rs

onbjerg · 2024-03-07T19:58:06Z

crates/rpc-types/src/eth/transaction/request.rs

-    ///
-    /// Returns `None` if the transaction is not a contract creation (the `to` field is set), or if
-    /// the `from` or `nonce` fields are not set.
-    pub fn calculate_create_address(&self) -> Option<Address> {


this is now part of the Builder trait, so removed it here. can add it back if we want

I added it for convenience, it's not strictly necessary but it's nice to have

builder trait should cover us here

crates/network/src/ethereum/mod.rs

onbjerg · 2024-03-07T20:03:36Z

crates/network/src/ethereum/mod.rs

+}
+
+/// Build a legacy transaction.
+fn build_legacy(request: TransactionRequest) -> Result<TxLegacy, TransactionBuilderError> {


Ideally I'd have these on TransactionRequest, but that would cause a circular dep (alloy-network depends on rpc-types -> rpc-types would need to depend on alloy-network for TransactionBuilderError)

onbjerg · 2024-03-07T20:04:43Z

crates/providers/src/chain.rs

                    Err(RpcError::Transport(err)) if retries > 0 && err.recoverable() => {
                        debug!(number, %err, "failed to fetch block, retrying");
                        retries -= 1;
                        continue;
                    }
+                    Ok(None) if retries > 0 => {


the rpc can respond with the block not existing, which can be the case if we are querying the latest block but the node doesn't have it yet. in that case i think we retry imo

crates/providers/src/new.rs

it would be nicer if we had the error, but that would cause cyclical dep

onbjerg · 2024-03-11T11:36:33Z

rebased ptal @gakonst @DaniPopes, after merge I will create all the follow up issues

DaniPopes · 2024-03-11T13:37:14Z

I realized we're not running doctests in CI because when I ran cargo t --all-features it failed on a doctest, @onbjerg can you please look into it? I couldn't figure it out

onbjerg · 2024-03-11T13:51:02Z

created all the follow up issues, will look into doctest

onbjerg · 2024-03-11T14:05:37Z

@DaniPopes we are, it just showed up later. i ran cargo t --all-features and got the same error as ci (which is fixed now)

DaniPopes · 2024-03-11T15:05:01Z

Yes, because I added it in 81c98a5 😄

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

prestwich self-assigned this Feb 6, 2024

This was referenced Feb 7, 2024

feat(consensus): TypedTransaction #183

Closed

debt(anvil): remove transaction_request_to_typed conversion foundry-rs/foundry#7048

Open

gakonst reviewed Feb 9, 2024

View reviewed changes

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

gakonst reviewed Feb 9, 2024

View reviewed changes

onbjerg mentioned this pull request Feb 9, 2024

Deduplicate TypedTransaction paradigmxyz/reth#6508

Closed

Evalir mentioned this pull request Feb 9, 2024

[wip] feat(anvil): migrate tests to alloy foundry-rs/foundry#6960

Closed

onbjerg force-pushed the prestwich/tx-builder branch 2 times, most recently from d0a8838 to 3300b0c Compare February 13, 2024 14:12

This was referenced Feb 14, 2024

feat(foundry-common): NameOrAddress ENS util foundry-rs/foundry#7122

Merged

debt(foundry-common): Refactor ens utility to alloy foundry-rs/foundry#7127

Open

onbjerg changed the title ~~Wip: Tx Builder and signing refactor~~ Wip: Tx Builder, provider and signing refactor Feb 15, 2024

gakonst reviewed Feb 15, 2024

View reviewed changes

Evalir mentioned this pull request Feb 16, 2024

feat(cast): Move non tx methods to alloy foundry-rs/foundry#7129

Merged

gakonst mentioned this pull request Feb 16, 2024

ClientRefs, Poller, and Streams #179

Merged

3 tasks

Evalir mentioned this pull request Feb 23, 2024

fix(consensus): correct TxEip1559.encode_with_signature #232

Closed

3 tasks

onbjerg mentioned this pull request Feb 27, 2024

feat: add TxKind alloy-rs/core#542

Merged

3 tasks

onbjerg force-pushed the prestwich/tx-builder branch 6 times, most recently from 7ff8f67 to 0ac977d Compare March 7, 2024 17:58

DaniPopes reviewed Mar 7, 2024

View reviewed changes

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

onbjerg force-pushed the prestwich/tx-builder branch from e454e82 to f23137f Compare March 7, 2024 19:29

onbjerg reviewed Mar 7, 2024

View reviewed changes

onbjerg mentioned this pull request Mar 7, 2024

feat: add network generic to sol-macro alloy-rs/core#557

Merged

3 tasks

onbjerg changed the title ~~Wip: Tx Builder, provider and signing refactor~~ Network abstraction and transaction builder Mar 8, 2024

onbjerg added 10 commits March 11, 2024 12:36

chore: rm outdated todo

8afa99c

feat: EthereumSigner::new

6f9b180

chore: code nit

a7be381

docs: note about network zst

17eb93a

chore: nit

bf86056

refactor: rename sign to sign_transaction

5210141

feat: ProviderBuilder::signer

69bb2e1

docs: more network signer docs

9324861

refactor: remove unneeded 'static

2425bc9

refactor: set_chain_id_checked sort of

95f22db

it would be nicer if we had the error, but that would cause cyclical dep

DaniPopes added 5 commits March 11, 2024 14:07

chore: use LocalWallet

5a41fbb

fix: docs

6e03867

chore: dedup signing code

ceb865a

chore: more dedup

ad9c520

ci: run doctests

81c98a5

This was referenced Mar 11, 2024

[Feature] Move Wallet/LocalWallet etc to its own crate #262

Closed

[Feature] Make it easier to instantiate a RootProvider #266

Closed

[Feature] Add network-parameterized block responses #267

Closed

[Feature] Rename RpcClientInner::prepare #270

Closed

test: fix doctest

d2a349e

onbjerg merged commit 05d0d0e into main Mar 11, 2024
15 checks passed

onbjerg deleted the prestwich/tx-builder branch March 11, 2024 15:05

This was referenced Mar 11, 2024

feat: Rename alloy-providers to alloy-provider #205

Closed

[Feature] Initial Network abstraction spec #80

Closed

eyusufatik mentioned this pull request Oct 1, 2024

Improve TransactionRequest to TypedTransactionRequest conversion chainwayxyz/citrea#199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: network abstraction and transaction builder #190

feat: network abstraction and transaction builder #190

prestwich commented Feb 6, 2024 •

edited by onbjerg

Loading

gakonst Feb 9, 2024

onbjerg Mar 8, 2024 •

edited

Loading

gakonst Feb 15, 2024

onbjerg Mar 11, 2024

gakonst Feb 15, 2024

onbjerg Mar 7, 2024

gakonst Feb 15, 2024

onbjerg Mar 8, 2024 •

edited

Loading

onbjerg Mar 7, 2024

onbjerg Mar 7, 2024

DaniPopes Mar 8, 2024

gakonst Mar 8, 2024

onbjerg Mar 7, 2024

onbjerg Mar 7, 2024

gakonst Mar 8, 2024

onbjerg commented Mar 11, 2024

DaniPopes commented Mar 11, 2024

onbjerg commented Mar 11, 2024

onbjerg commented Mar 11, 2024

DaniPopes commented Mar 11, 2024

		// todo: block responses are ethereum only, so we need to include this in here too, or make `Block`
		// generic over tx/header type

feat: network abstraction and transaction builder #190

feat: network abstraction and transaction builder #190

Conversation

prestwich commented Feb 6, 2024 • edited by onbjerg Loading

Motivation

Solution

Follow ups

Breaking changes

PR Checklist

Choose a reason for hiding this comment

onbjerg Mar 8, 2024 • edited Loading

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

onbjerg Mar 8, 2024 • edited Loading

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

onbjerg commented Mar 11, 2024

DaniPopes commented Mar 11, 2024

onbjerg commented Mar 11, 2024

onbjerg commented Mar 11, 2024

DaniPopes commented Mar 11, 2024

prestwich commented Feb 6, 2024 •

edited by onbjerg

Loading

onbjerg Mar 8, 2024 •

edited

Loading

onbjerg Mar 8, 2024 •

edited

Loading