Skip to content

Conversation

mpaulucci
Copy link
Collaborator

Motivation
Current error message was confusing.

@Copilot Copilot AI review requested due to automatic review settings August 12, 2025 14:45
@mpaulucci mpaulucci requested a review from a team as a code owner August 12, 2025 14:45
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves error handling by replacing a panic with a proper error return when the Store is initialized with a genesis configuration that differs from what's already stored in the database. The change provides a clearer, more actionable error message for users encountering configuration mismatches.

  • Replaces panic behavior with proper error handling using StoreError::Custom
  • Updates the error message to be more user-friendly and informative
  • Adjusts corresponding test to verify error handling instead of panic catching

@github-actions github-actions bot added the L1 Ethereum client label Aug 12, 2025
Copy link

github-actions bot commented Aug 12, 2025

Lines of code report

Total lines added: 2
Total lines removed: 2
Total lines changed: 4

Detailed view
+--------------------------------+-------+------+
| File                           | Lines | Diff |
+--------------------------------+-------+------+
| ethrex/crates/storage/error.rs | 55    | +2   |
+--------------------------------+-------+------+
| ethrex/crates/storage/store.rs | 1470  | -2   |
+--------------------------------+-------+------+

Copy link
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

LGTM. Left some comments about style and further simplifications

@github-project-automation github-project-automation bot moved this to In Review in ethrex_l1 Aug 12, 2025
@mpaulucci mpaulucci added this pull request to the merge queue Aug 12, 2025
Some(_) => panic!("{GENESIS_DIFF_PANIC_MESSAGE}"),
Some(_) => {
error!(
"The chain configuration stored in the database is incompatible with the provided configuration. If you intended to switch networks, clear the database (e.g., run `ethrex removedb`) and try again."
Copy link
Contributor

Choose a reason for hiding this comment

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

The user could also specify another datadir without needing to remove the db, right? Do you think we should specify this here so that they know they have another alternative if they don't want to remove the current database?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea!

@mpaulucci mpaulucci removed this pull request from the merge queue due to a manual request Aug 12, 2025
@mpaulucci mpaulucci added this pull request to the merge queue Aug 14, 2025
Merged via the queue into main with commit 58a6347 Aug 14, 2025
32 checks passed
@mpaulucci mpaulucci deleted the improve-chain-incompatible-message branch August 14, 2025 16:35
@github-project-automation github-project-automation bot moved this from In Review to Done in ethrex_l1 Aug 14, 2025
pedrobergamini pushed a commit to pedrobergamini/ethrex that referenced this pull request Aug 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L1 Ethereum client
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants