Skip to content

feat(tools): use Hardfork for fork module loading#2778

Merged
LouisTsai-Csie merged 2 commits into
ethereum:forks/amsterdamfrom
JackCC703:issue-1426-hardfork-paths
May 6, 2026
Merged

feat(tools): use Hardfork for fork module loading#2778
LouisTsai-Csie merged 2 commits into
ethereum:forks/amsterdamfrom
JackCC703:issue-1426-hardfork-paths

Conversation

@JackCC703
Copy link
Copy Markdown
Contributor

🗒️ Description

Adds Hardfork.by_short_name() and uses Hardfork.module() in tooling paths that previously loaded fork modules through hardcoded ethereum.forks.<fork> paths.

This centralizes short-name fork lookup and removes duplicated fork discovery logic from optimized patches, fixture loading, JSON VM loading, and t8n fork-specific helpers.

🔗 Related Issues or PRs

Fixes #1426.

✅ Checklist

  • All: Ran fast static checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    just static
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
  • Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

Cute Animal Picture

Cute animal picture

Copy link
Copy Markdown
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Just the one potential improvement.

Comment thread src/ethereum_optimized/__init__.py Outdated

def monkey_patch_optimized_state_db(
fork_name: str, state_path: Optional[str]
fork_name: Hardfork | str, state_path: Optional[str]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there anywhere that uses fork_name: str left? Would be nice to just avoid the Union here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed in ee910c4. The optimized helpers now take Hardfork directly, and the internal str compatibility path is gone. I checked the callers and they already pass Hardfork instances.

I also moved the helper test so it does not get collected by the filler/docs jobs.

Remove the Hardfork | str union type from optimized monkey-patching
functions, accepting only Hardfork directly per reviewer feedback.
Move test_forks.py into tests/json_loader/ so fill/docs collection
does not pick it up.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.62%. Comparing base (6333758) to head (ee910c4).
⚠️ Report is 5 commits behind head on forks/amsterdam.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           forks/amsterdam    #2778      +/-   ##
===================================================
+ Coverage            88.17%   88.62%   +0.45%     
===================================================
  Files                  577      577              
  Lines                35659    35659              
  Branches              3490     3490              
===================================================
+ Hits                 31442    31604     +162     
+ Misses                3654     3492     -162     
  Partials               563      563              
Flag Coverage Δ
unittests 88.62% <ø> (+0.45%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@LouisTsai-Csie LouisTsai-Csie merged commit 2e71a20 into ethereum:forks/amsterdam May 6, 2026
27 checks passed
@JackCC703 JackCC703 deleted the issue-1426-hardfork-paths branch May 6, 2026 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace hardcoded fork paths with Hardfork class

3 participants