feat: refactor request builder workflow #431

leruaa · 2024-03-30T21:05:31Z

Closes #423

prestwich

good direction 🦺

crates/network/src/ethereum/builder.rs

prestwich

feature req:

/// Returns the transaction type that this builder will attempt to build. This does not imply 
/// that the builder is ready to build.
pub fn output_tx_type(&self) -> TxType

/// Returns the transaction type that this builder will build. `Err` if the builder
/// is not ready to build, containing the attempt type and missing/conflicted information
pub fn output_tx_type_checked(&self) -> Result<TxType, Vec<TransactionRequestError>>;

crates/network/src/ethereum/builder.rs

crates/network/src/transaction/builder.rs

prestwich · 2024-04-01T15:16:27Z

crates/network/src/transaction/builder.rs

@@ -38,6 +39,44 @@ impl TransactionBuilderError {
    }
 }

+/// Wrapper for [`InvalidTransactionRequestError`]s.
+#[derive(Debug)]
+pub struct InvalidTransactionRequestErrors(pub Vec<InvalidTransactionRequestError>);


it seems like this newtype is not adding any functionality, or preventing any common mistakes. It can be omitted, with the buildererror variant containing an unwrapped Vec

Mmm It's for the Display impl, used on:

#[error("{0:?} transaction can't be built: {1}")] InvalidTransactionRequest(TxType, InvalidTransactionRequestErrors),

let's manually impl that display?

prestwich · 2024-04-01T15:17:02Z

crates/network/src/ethereum/builder.rs

+    }
+}
+
+fn will_build_2930(request: &TransactionRequest) -> Result<bool, TransactionBuilderError> {


these can be pub

Let's also add some docs

prestwich · 2024-04-03T17:56:56Z

the more i look at this the more complexity I see. e.g. a TransactionRequest may be valid for submission via send_transaction but not sufficiently filled to allow for signing via build

I may do some work on this 🤔

leruaa · 2024-04-11T13:15:19Z

@prestwich Did you have time to have a look at this? Or maybe you can give pointers on the remaining work needed?

prestwich · 2024-04-12T13:03:44Z

i'm still traveling for the next few days, so I don't have time to properly review :( I will be working on this and may take it over somewhat. sorry for taking so long and then stepping on your toes

mattsse

this flow makes sense, but I worry the preferred type detection has unexpected results

how to enforce build will actually result in specific type?

I think form a builder pov, I'd also like to have something like

 builder().
   ....
   .ensure_type(ty: impl AsRef<u8>)?
   .build_preferred(, ty: impl AsRef<u8>)

mattsse · 2024-04-12T15:12:14Z

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

+        } else if self.access_list.is_some() {
+            TxType::Eip2930
+        } else if self.gas_price.is_some() {
+            TxType::Legacy
+        } else {
+            TxType::Eip1559
+        }


should 2930 only be if gas price is also set?

otherwise all txs with 1559 fee fields and an accesslist will be 2930

my basic design approach here is to prefer the MOST specific (LEAST common) thing set, to try to conform to user intent in most cases. So preference order is basically as follows:

Blobs are specific and critical to the tx, and have highest priority

Access lists are specific and (almost always) not critical

legacy gas is specific and never critical

eip1559 is the default case if no more-specific intention has been specified

otherwise all txs with 1559 fee fields and an accesslist will be 2930

this seems like correct behavior to me, because if the user specified an access list it is a more-specific intention than specifying 1559 gas

mattsse · 2024-04-12T15:20:08Z

crates/network/src/transaction/builder.rs

@@ -227,8 +230,24 @@ pub trait TransactionBuilder<N: Network>: Default + Sized + Send + Sync + 'stati
    /// a valid transaction.
    fn can_build(&self) -> bool;

+    /// Returns the transaction type that this builder will attempt to build.
+    /// This does not imply that the builder is ready to build.
+    fn output_tx_type(&self) -> TxType;


hmm, can we use the txtype in the trait here?

because txtypes are network specific

could operate on u8 or make txtype part of the network trait

good catch, we cannot.

could operate on u8 or make txtype part of the network trait

I am partial to the idea of using TxType on the network trait, but at that point, i would also want to incorporate into En/Decodable2718, and make something like trait En/Decodable2718<T: Into<u8> + TryFrom<u8, Error = _>> or similar so that we can enforce that the 2718 implementation uses the network TxType via a type TxEnvelope: Encodable2718<Self::TxType> bound

let me think about it a bit. most likely outcome is that this PR will operate on u8, and i will reserve network-assoc txtype for a later pr

okay i made a network specific tx type because it felt right >.>

incorporating it into encodable/decodable can be later

prestwich · 2024-04-15T15:19:02Z

this flow makes sense, but I worry the preferred type detection has unexpected results

how to enforce build will actually result in specific type?

I think form a builder pov, I'd also like to have something like
 builder().
   ....
   .ensure_type(ty: impl AsRef<u8>)?
   .build_preferred(, ty: impl AsRef<u8>)

@mattsse i've added the following. I copied the pattern from other builders, but i couldn't tell you which ones because it's beena minute since I saw them

 fn assert_preferred(&self, ty: N::TxType) -> Self {
    dbg_assert_eq!(self.preferred_type(), ty);
}

fn assert_preferred_chained(self, ty: N::TxType) -> Self { 
    self.assert_preferred(ty);
    self
}

prestwich · 2024-04-15T16:18:52Z

~~@mattsse i failed to consider AnyTxType in the abstraction. will need to do some work there~~

ok done

* feat: refactor request builder workflow * feat: return all tx builder errors in one invocation * fix: avoid nesting * feat: move get_invalid_*_fields() to TransactionRequest * feat: Impl Display for TxType * feat: (wip) get info about transaction type currently building * wip: refactor refactor refactor * feat: tx type in trait * fix: core fmt * fix: tests passing * fix: no_std in consensus * feature: assert_preferred * fix: anytxtype --------- Co-authored-by: James <james@prestwi.ch>

leruaa requested review from DaniPopes, gakonst, mattsse, onbjerg and prestwich as code owners March 30, 2024 21:05

prestwich requested changes Mar 30, 2024

View reviewed changes

crates/network/src/ethereum/builder.rs Outdated Show resolved Hide resolved

crates/network/src/ethereum/builder.rs Outdated Show resolved Hide resolved

crates/network/src/ethereum/builder.rs Outdated Show resolved Hide resolved

prestwich requested changes Apr 1, 2024

View reviewed changes

onbjerg added the debt Tech debt which needs to be addressed label Apr 1, 2024

leruaa requested a review from Evalir as a code owner April 1, 2024 20:25

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 force-pushed the Improve_tx_request_builder branch from 23ead62 to 5b5e7c9 Compare April 12, 2024 13:20

mattsse requested changes Apr 12, 2024

View reviewed changes

prestwich mentioned this pull request Apr 15, 2024

fix: build_4844 is missing the transaction sidecar #403 #536

Closed

3 tasks

leruaa and others added 10 commits April 15, 2024 12:27

feat: refactor request builder workflow

23923ae

feat: return all tx builder errors in one invocation

0e19433

fix: avoid nesting

9483cb8

feat: move get_invalid_*_fields() to TransactionRequest

7fb4121

feat: Impl Display for TxType

9df4c34

feat: (wip) get info about transaction type currently building

b3a5d13

wip: refactor refactor refactor

5fde07b

feat: tx type in trait

8b57a23

fix: core fmt

b332720

fix: tests passing

b06cd85

prestwich force-pushed the Improve_tx_request_builder branch from f53e649 to b06cd85 Compare April 15, 2024 11:27

prestwich mentioned this pull request Apr 15, 2024

[Refactor] Make En/Decodable2718 generic over a tx type #540

Open

fix: no_std in consensus

f571d5a

feature: assert_preferred

89a0652

fix: anytxtype

384cd46

This was linked to issues Apr 15, 2024

[Feature] Blob fee estimation in Provider::estimate_eip1559_fees #376

Closed

[Bug] 4844 transaction gas estimation does not attach blobs #403

Closed

prestwich merged commit cb95183 into alloy-rs:main Apr 16, 2024
18 checks passed

mattsse mentioned this pull request Apr 16, 2024

fix: add back transaction type #552

Merged

prestwich mentioned this pull request Apr 25, 2024

fix: signer filler now propagates missing keys from builder #637

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor request builder workflow #431

feat: refactor request builder workflow #431

leruaa commented Mar 30, 2024

prestwich left a comment

prestwich left a comment •

edited

Loading

prestwich Apr 1, 2024

leruaa Apr 1, 2024 •

edited

Loading

prestwich Apr 2, 2024

prestwich Apr 1, 2024

Evalir Apr 2, 2024

prestwich commented Apr 3, 2024

leruaa commented Apr 11, 2024

prestwich commented Apr 12, 2024

mattsse left a comment

mattsse Apr 12, 2024

prestwich Apr 15, 2024

mattsse Apr 12, 2024

prestwich Apr 12, 2024

prestwich Apr 15, 2024 •

edited

Loading

prestwich commented Apr 15, 2024

prestwich commented Apr 15, 2024 •

edited

Loading

feat: refactor request builder workflow #431

feat: refactor request builder workflow #431

Conversation

leruaa commented Mar 30, 2024

prestwich left a comment

Choose a reason for hiding this comment

prestwich left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leruaa Apr 1, 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

prestwich commented Apr 3, 2024

leruaa commented Apr 11, 2024

prestwich commented Apr 12, 2024

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

prestwich Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

prestwich commented Apr 15, 2024

prestwich commented Apr 15, 2024 • edited Loading

prestwich left a comment •

edited

Loading

leruaa Apr 1, 2024 •

edited

Loading

prestwich Apr 15, 2024 •

edited

Loading

prestwich commented Apr 15, 2024 •

edited

Loading