TxRequest into EIP-4844 without sidecar #1093

Dinonard · 2024-07-23T14:52:35Z

Motivation

The TransactionArguments struct's build_typed_tx method will, in case of EIP-4844 transaction, always try to build a TypedTransaction using sidecar.

This isn't necessarily correct if sidecar is None but blob_versioned_hashes are set to Some(...).
In this case, it should be fine to build a transaction without sidecar.

This PR allows that to happen.

Solution

implement build_consensus_tx for TransactionRequest for conversion into TypedTransaction
modifies some private legacy functions to return Result<...> instead of panicking
leaves the other legacy function(s) functionally the same

PR Checklist

Added Tests

mattsse

what's the usecase for this?
new 4844 txs are useless without the sidecar

Dinonard · 2024-07-23T17:38:29Z

what's the usecase for this? new 4844 txs are useless without the sidecar

This part: https://github.com/ethereum-optimism/optimism/blob/develop/op-service/signer/transaction_args.go#L20

op-signer relies on some external service/proxy to handle signing the Ethereum transaction.
To generate signature, entire sidecar isn't needed, only versioned blob hashes, and that's what they send to the service/proxy.

I'm building this particular service, and would like to be able to construct an TxEip4844 without the sidecar.
Since it already exists as struct & enum value here, and minimum changes are required to make it work with existing functions, I thought I'd propose the change. 🙂

mattsse · 2024-07-23T18:25:59Z

To generate signature, entire sidecar

ah gotcha, valid.

will check in a bit

mattsse

I don't want to break the existing build_4844 function, so this feature should be a new function entirely

mattsse · 2024-07-25T09:41:22Z

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

@@ -247,7 +247,51 @@ impl TransactionRequest {
    ///
    /// If required fields are missing. Use `complete_4844` to check if the
    /// request can be built.
-    fn build_4844(mut self) -> TxEip4844WithSidecar {
+    fn build_4844(self) -> TxEip4844Variant {


I'd like to keep build_4844 -> TxEip4844WithSidecar

Done, kept the same function name & footprint.

mattsse · 2024-07-25T09:42:35Z

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

+    ///
+    /// If required fields are missing. Use `complete_4844` to check if the
+    /// request can be built.
+    fn build_4844_without_sidecar(self) -> TxEip4844 {


this seems appropriate as an additional function

Dinonard · 2024-07-25T13:00:27Z

Hope I understood your intention correctly 🙂

build_4848_with_sidecar has been renamed back to build_4844 to keep it same as before
build_4844 that returns the 4848 Variant has been renamed to build_4844_variant

mattsse

this usecase isn't for building a transaction that can be sent to the network but is for building a raw 4844 txs. I'd prefer having this as a separate call and only allow withsidecar when building the envelope

I'd also like to see a test for this.

mattsse · 2024-07-25T17:33:21Z

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

+        // Either `sidecar` or `blob_versioned_hashes` must be set.
+        if self.sidecar.is_none() && self.blob_versioned_hashes.is_none() {
            missing.push("sidecar");
+            missing.push("blob_versioned_hashes");


this is a bit of a footgun, because this now also allows without sidecar which was not the original intention of this because this is supposed to be a transactionbuilder and 4844 without sidecar can't be sent.

I'd like to keep it that way but rather expose another transactionbuilder function explicitly for this usecase

I've reverted changes to the legacy, now it should be the same as before (instead of the panics).
I've left a TODO/question for you in the code, please let me know what you think.

Introduced a dedicated TryFrom to handle the conversion I need.

mattsse

please undo changes that are not relevant to this feature, this changes a lot of existing functions that are unrelated.

this pr should only introduce new functionality not change existing ones

Dinonard · 2024-07-29T05:49:29Z

please undo changes that are not relevant to this feature, this changes a lot of existing functions that are unrelated.

this pr should only introduce new functionality not change existing ones

Hey @mattsse, can you please point out to what exactly are you referring to?

I believe I only modified what I had to to get the required functionality. 🤔

legacy public interface has been kept the same as you requested
- the public method build_typed_tx still panics in case a param is missing (will remove TODO to change that)
The proposed conversion has been achieved via TryFrom impl
- To achieve this I reused existing private methods named build_
  - the change I made was that these methods now return Result instead of panicking
  - I also broke down the existing Eip4844 method to allow building both variants
- legacy behavior was preserved
tests have been added for everything

I re-checked again, and I cannot see where I modified anything that wasn't needed.
Existing code has been reused without impacting the actual functionality provided by the public interface.

mattsse · 2024-07-29T19:45:38Z

this changes fn build_{}(self) functions and I don't see why this would be necessary

none of this should be necessary to add a function that builds a raw 4844 tx object

I'd like to start over here with a single function that does this because we're including this in the existing build_tx calls

mattsse · 2024-07-29T19:51:07Z

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

    /// request can be built.
-    fn build_4844(mut self) -> TxEip4844WithSidecar {
-        self.populate_blob_hashes();
+    fn build_4844_without_sidecar(self) -> Result<TxEip4844, &'static str> {


this is the new function we need, everything elese is unrelated, should not be part of this pr

Dinonard · 2024-07-29T21:15:16Z

this changes fn build_{}(self) functions and I don't see why this would be necessary

none of this should be necessary to add a function that builds a raw 4844 tx object

I'd like to start over here with a single function that does this because we're including this in the existing build_tx calls

I don't want to add a function that builds a raw 4844 tx object, but a function that will provide best possible representation of TypedTransaction from the provided TransactionRequest.

And I don't want the call that builds it to panic - instead, I'd like it to return an error, hence the approach I took.
Reuse of existing code without changing anything except replacing panics with Result<...>, but keeping the behavior of the all previous public methods the same (with panics) as you requested.

mattsse

can we please split this feature into two things then

a) a function that builds a raw 4844 tx

b) and then we solve the conversion to typed transaction separately

Dinonard · 2024-07-30T06:33:37Z

can we please split this feature into two things then

a) a function that builds a raw 4844 tx

b) and then we solve the conversion to typed transaction separately

Do you mean to create two separate PRs?
I will do that if you want.

However, I'm not sure what exactly the function that builds the raw 4844 tx would look like or how it would be used.
All functions that build specific types of tx are private, e.g. fn build_1559(self).
Adding a new private, dead-code, function would be weird.

To take a step back, I have to say that I appreciate your comments - you've been very responsive and that's not always the case with first-time PRs 😅. I'm very confused about what the actual high-level requirement is here though. I thought I understood you before - keep legacy behavior the same - which I respected. But I'm no longer sure I do 😅.

Are you worried that my changes would break the legacy?
If yes, would you be more comfortable if I first make a PR where I add tests for the legacy code, and then re-visit this PR?

I'm just trying to make the way forward clear, to save you review time & to ensure we keep the code quality high.

mattsse · 2024-07-30T13:24:38Z

sorry, I realize now I haven't fully understood the feature request at first and overlooked that the usecase here is to create a typedtransaction of which raw 4844 is one variant, hence I got confused about the other changes.

I need to reevaluate in a bit, ty

Dinonard · 2024-07-31T15:18:57Z

create a typedtransaction of which raw 4844 is one variant,

Yes, that's exactly what I need 🙂.
E.g. if in the future, OP team decides to send the sidecar as well, the code should just continue to work.

Will await your reply, appreciate your time and effort! 🙇

Dinonard · 2024-08-06T06:28:19Z

Hey @mattsse, did you maybe have time to take another look?

Would be great if we could get this finished by the end of week 🙏

mattsse · 2024-08-06T13:20:34Z

okay, I looked at this again.

I'd like to do your changes slightly differently

we introduce build_488 without sidecar

then we introduce another pub fn build_consensus_tx(self) -> Result<TypedTransaction, Self> that does not enforce sidecar for 4844 and document this properly

bonus points if we make the fn build_ functions return a result, like you did here but with a new error type that wraps the request

struct BuildTransactionErr {tx: Self, error: String}

Dinonard · 2024-08-07T05:44:53Z

Thanks!

I've modified the PR per your suggestions (and my understanding).

One deviation is that I've kept the build_ functions return Err as &'static string because this helps me avoid cloning inside the build_ functions themselves. I can easily change this but the code will end up being more bulky with boilerplatish error conversions.

The new public build_consensus_tx function returns BuildTransactionErr as you suggested.

mattsse

cool, thanks for the tests and docs

Dinonard · 2024-08-07T11:40:56Z

Thank you! 😄

Dinonard added 3 commits July 23, 2024 16:46

TxReq into Eip4844 without sidecar

9050dc8

Formatting

1d4878e

Minor comments

9d7b570

Dinonard marked this pull request as ready for review July 23, 2024 14:53

Dinonard requested review from DaniPopes, gakonst, mattsse, onbjerg, prestwich and Evalir as code owners July 23, 2024 14:53

mattsse reviewed Jul 23, 2024

View reviewed changes

mattsse requested changes Jul 25, 2024

View reviewed changes

Address review comments

b8ab4ee

mattsse requested changes Jul 25, 2024

View reviewed changes

Dinonard added 2 commits July 26, 2024 14:53

Address review comments

901e3fb

Remove comment

68f0932

mattsse requested changes Jul 27, 2024

View reviewed changes

mattsse requested changes Jul 29, 2024

View reviewed changes

Review comments

048740f

mattsse approved these changes Aug 7, 2024

View reviewed changes

mattsse merged commit c3ccf7e into alloy-rs:main Aug 7, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TxRequest into EIP-4844 without sidecar #1093

TxRequest into EIP-4844 without sidecar #1093

Dinonard commented Jul 23, 2024 •

edited

Loading

mattsse left a comment

Dinonard commented Jul 23, 2024

mattsse commented Jul 23, 2024

mattsse left a comment

mattsse Jul 25, 2024

Dinonard Jul 25, 2024

mattsse Jul 25, 2024

Dinonard commented Jul 25, 2024

mattsse left a comment

mattsse Jul 25, 2024

Dinonard Jul 26, 2024

mattsse left a comment

Dinonard commented Jul 29, 2024

mattsse commented Jul 29, 2024 •

edited

Loading

mattsse Jul 29, 2024

Dinonard commented Jul 29, 2024

mattsse left a comment

Dinonard commented Jul 30, 2024

mattsse commented Jul 30, 2024

Dinonard commented Jul 31, 2024

Dinonard commented Aug 6, 2024

mattsse commented Aug 6, 2024 •

edited

Loading

Dinonard commented Aug 7, 2024

mattsse left a comment

Dinonard commented Aug 7, 2024

TxRequest into EIP-4844 without sidecar #1093

TxRequest into EIP-4844 without sidecar #1093

Conversation

Dinonard commented Jul 23, 2024 • edited Loading

Motivation

Solution

PR Checklist

mattsse left a comment

Choose a reason for hiding this comment

Dinonard commented Jul 23, 2024

mattsse commented Jul 23, 2024

mattsse left a comment

Choose a reason for hiding this comment

mattsse Jul 25, 2024

Choose a reason for hiding this comment

Dinonard Jul 25, 2024

Choose a reason for hiding this comment

mattsse Jul 25, 2024

Choose a reason for hiding this comment

Dinonard commented Jul 25, 2024

mattsse left a comment

Choose a reason for hiding this comment

mattsse Jul 25, 2024

Choose a reason for hiding this comment

Dinonard Jul 26, 2024

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Dinonard commented Jul 29, 2024

mattsse commented Jul 29, 2024 • edited Loading

mattsse Jul 29, 2024

Choose a reason for hiding this comment

Dinonard commented Jul 29, 2024

mattsse left a comment

Choose a reason for hiding this comment

Dinonard commented Jul 30, 2024

mattsse commented Jul 30, 2024

Dinonard commented Jul 31, 2024

Dinonard commented Aug 6, 2024

mattsse commented Aug 6, 2024 • edited Loading

Dinonard commented Aug 7, 2024

mattsse left a comment

Choose a reason for hiding this comment

Dinonard commented Aug 7, 2024

Dinonard commented Jul 23, 2024 •

edited

Loading

mattsse commented Jul 29, 2024 •

edited

Loading

mattsse commented Aug 6, 2024 •

edited

Loading