Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

feat(derive): Pipeline Builder#127

Merged
refcell merged 15 commits intomainfrom
refcell/pipeline-builder
Apr 27, 2024
Merged

feat(derive): Pipeline Builder#127
refcell merged 15 commits intomainfrom
refcell/pipeline-builder

Conversation

@refcell
Copy link
Contributor

@refcell refcell commented Apr 19, 2024

Description

Implements the pipeline builder.

Metadata

Fixes #110

@refcell refcell self-assigned this Apr 19, 2024
@refcell refcell added K-feature Kind: feature A-proof Area: proof crates labels Apr 19, 2024
@refcell refcell added this to the Table Stakes milestone Apr 19, 2024
@refcell refcell requested a review from clabby April 19, 2024 20:24
Comment on lines +138 to +139
let tx = TxDeposit::decode(&mut execution_payload.transactions[0][1..].as_ref())
.map_err(|e| anyhow::anyhow!(e))?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like that I had to do this to strip the 0x7e deposit prefix @clabby, I think there's an easier way to do this, but the issue is we check if the bytes are deposit transactions based on their first byte and then I think we should be decoding the deposit tx here. Maybe this should just be refactored into a TxDeposit::decode_with_prefix() method that gracefully strips the prefix? Curious as to your thoughts here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use OpTxEnvelope::decode_2718 (I think OpTxEnvelope::decode does the same), which will decode a OpTxEnvelope::TxDeposit(TxDeposit). Just need some extra matching.

We should do this - need to ensure that the first tx is always a deposit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So unfortunately TxDeposit::decode does not support this, it reads the header first in it's decoding and that's why the unit tests strip the 0x7e type prefix. e.g. https://github.com/clabby/op-alloy/blob/refcell/consensus-port/crates/op-consensus/src/transaction/optimism.rs#L177

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I've added a manual deposit tx type check. Will write up a ticket to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a ticket here #147

Copy link
Contributor

@clabby clabby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. The ol' Box<&dyn Trait> recursion isn't too bad.

Comment on lines +138 to +139
let tx = TxDeposit::decode(&mut execution_payload.transactions[0][1..].as_ref())
.map_err(|e| anyhow::anyhow!(e))?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use OpTxEnvelope::decode_2718 (I think OpTxEnvelope::decode does the same), which will decode a OpTxEnvelope::TxDeposit(TxDeposit). Just need some extra matching.

We should do this - need to ensure that the first tx is always a deposit.

@refcell refcell added this pull request to the merge queue Apr 27, 2024
Merged via the queue into main with commit 52036db Apr 27, 2024
@github-actions github-actions bot mentioned this pull request Apr 27, 2024
This was referenced May 29, 2024
This was referenced Jun 6, 2024
This was referenced Jun 16, 2024
@clabby clabby deleted the refcell/pipeline-builder branch July 2, 2024 03:39
theochap pushed a commit to ethereum-optimism/optimism that referenced this pull request Dec 10, 2025
* feat(derive): span batch validation

* feat(derive): span batch validity unit tests

* feat(derive): span batch unit tests with acceptance test

* fix(derive): unit tests

* fix(derive): add more unit tests

* feat(derive): span batch validity unit tests for txs

* feat(derive): pipeline builder

* fix(derive): so close :sadge

* fix(derive): ugly refactor

* fix(derive): pipeline construction and trait abstractions

* fix(derive): nit fixes

* fix(derive): temp manual deposit type check
theochap pushed a commit to ethereum-optimism/optimism that referenced this pull request Jan 14, 2026
* feat(derive): span batch validation

* feat(derive): span batch validity unit tests

* feat(derive): span batch unit tests with acceptance test

* fix(derive): unit tests

* fix(derive): add more unit tests

* feat(derive): span batch validity unit tests for txs

* feat(derive): pipeline builder

* fix(derive): so close :sadge

* fix(derive): ugly refactor

* fix(derive): pipeline construction and trait abstractions

* fix(derive): nit fixes

* fix(derive): temp manual deposit type check
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A-proof Area: proof crates K-feature Kind: feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(derive): Pipeline Builder

2 participants