Skip to content

chore(consensus): Add trait object error variant to ConsensusError#492

Merged
emhane merged 12 commits intoop-rs:unstablefrom
LeeliRen6:error
Dec 17, 2025
Merged

chore(consensus): Add trait object error variant to ConsensusError#492
emhane merged 12 commits intoop-rs:unstablefrom
LeeliRen6:error

Conversation

@LeeliRen6
Copy link

Close #481

Adds a Custom variant to ConsensusError that wraps an error trait object, following the same pattern as DatabaseError in #388. This enables type-safe error handling between ConsensusError and custom L2 error types without matching on error strings.

Also removes PartialEq and Eq derives since Arc<dyn Error> can't implement them, and fixes affected tests in downloaders.

cc @emhane

Copy link

@emhane emhane left a comment

Choose a reason for hiding this comment

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

nice!

@emhane
Copy link

emhane commented Dec 15, 2025

asides the smol comments about importing types in import prelude, as you're probably aware the remaining tests using equality of ConsensusError need fixing. use matches macro rule for this, see how it's done in #388. for example

assert_eq!(
             result.unwrap_err(),
            ConsensusError::BlobGasUsedDiff(GotExpected { got: DA_FOOTPRINT, expected: 0 })
);

becomes

assert!(
    matches!(
             result.unwrap_err(),
            ConsensusError::BlobGasUsedDiff(GotExpected { got, expected } if got == DA_FOOTPRINT && expected == 0)
    )
);

@codecov
Copy link

codecov bot commented Dec 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 38.34%. Comparing base (a6a7ec5) to head (e1ee9c3).
⚠️ Report is 1 commits behind head on unstable.

Additional details and impacted files

Impacted file tree graph

Files with missing lines Coverage Δ
crates/consensus/common/src/validation.rs 69.89% <ø> (ø)
crates/consensus/consensus/src/lib.rs 0.00% <ø> (ø)
crates/net/downloaders/src/file_client.rs 0.00% <ø> (ø)
...tes/net/downloaders/src/headers/reverse_headers.rs 13.90% <ø> (ø)
crates/net/downloaders/src/headers/task.rs 34.42% <ø> (ø)
crates/optimism/consensus/src/lib.rs 57.14% <ø> (ø)
crates/optimism/consensus/src/validation/mod.rs 43.08% <ø> (ø)

... and 7 files with indirect coverage changes

Flag Coverage Δ
e2e 35.53% <ø> (+0.13%) ⬆️
unit 31.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 54.44% <ø> (ø)
op historical proof 90.13% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@LeeliRen6
Copy link
Author

Hi @emhane should be good this time

@LeeliRen6 LeeliRen6 requested a review from emhane December 15, 2025 15:53
@emhane
Copy link

emhane commented Dec 16, 2025

we don't want to change the logic, just the syntax. we still need to verify the error data in tests. see how it's done in 246a9c5

@emhane emhane added K-debt Kind: debt M-good-first-issue Meta: good issue for first time contributors A-consensus Area: consensus engine labels Dec 16, 2025
@LeeliRen6
Copy link
Author

we don't want to change the logic, just the syntax. we still need to verify the error data in tests. see how it's done in 246a9c5

Thanks! Please have a look again.

Copy link

@emhane emhane left a comment

Choose a reason for hiding this comment

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

nice!

@LeeliRen6 LeeliRen6 requested a review from emhane December 16, 2025 19:54
@emhane emhane enabled auto-merge December 17, 2025 12:36
@emhane emhane disabled auto-merge December 17, 2025 14:11
@emhane emhane merged commit 02701b0 into op-rs:unstable Dec 17, 2025
50 checks passed
emhane added a commit that referenced this pull request Dec 18, 2025
)

Close #481

Adds a `Custom` variant to `ConsensusError` that wraps an error trait
object, following the same pattern as `DatabaseError` in #388. This
enables type-safe error handling between `ConsensusError` and custom L2
error types without matching on error strings.

Also removes `PartialEq` and `Eq` derives since `Arc<dyn Error>` can't
implement them, and fixes affected tests in downloaders.

cc @emhane

---------

Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
emhane added a commit that referenced this pull request Dec 18, 2025
)

Close #481

Adds a `Custom` variant to `ConsensusError` that wraps an error trait
object, following the same pattern as `DatabaseError` in #388. This
enables type-safe error handling between `ConsensusError` and custom L2
error types without matching on error strings.

Also removes `PartialEq` and `Eq` derives since `Arc<dyn Error>` can't
implement them, and fixes affected tests in downloaders.

cc @emhane

---------

Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
emhane added a commit that referenced this pull request Jan 5, 2026
)

Close #481

Adds a `Custom` variant to `ConsensusError` that wraps an error trait
object, following the same pattern as `DatabaseError` in #388. This
enables type-safe error handling between `ConsensusError` and custom L2
error types without matching on error strings.

Also removes `PartialEq` and `Eq` derives since `Arc<dyn Error>` can't
implement them, and fixes affected tests in downloaders.

cc @emhane

---------

Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-consensus Area: consensus engine K-debt Kind: debt M-good-first-issue Meta: good issue for first time contributors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ConsensusError variant wrapping trait object error

2 participants