Skip to content

Conversation

raxhvl
Copy link
Member

@raxhvl raxhvl commented Sep 9, 2025

πŸ—’οΈ Description

πŸ”— Related Issues or PRs

N/A.

βœ… Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • 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.

@raxhvl raxhvl changed the title ✨ feat(EIP-7928): OOG: Intrinsic gas ✨ feat(test): EIP-7928 OOG test cases Sep 9, 2025
@raxhvl raxhvl self-assigned this Sep 9, 2025
@raxhvl raxhvl added scope:tests Scope: Changes EL client test cases in `./tests` type:test Type: Add/refactor fw unit tests; no fw or el client test case changes labels Sep 9, 2025
@raxhvl raxhvl changed the title ✨ feat(test): EIP-7928 OOG test cases ✨ feat(tests): EIP-7928 OOG test cases Sep 9, 2025
@raxhvl raxhvl force-pushed the feat/eip-7928/tests branch from 780a23e to c89d947 Compare September 15, 2025 14:41
@fselmo fselmo force-pushed the feat/eip-7928/tests branch from c89d947 to 4f79419 Compare October 1, 2025 22:00
@fselmo
Copy link
Collaborator

fselmo commented Oct 1, 2025

@raxhvl, as you know, we're pushing toward The Weldβ„’ at the moment (eels + eest merging) and so we are trying to get PRs close to mergeable. I'm happy to go a different direction here but this needed a rebase, lint fixing, and all the tests needed the expectations updated so I made some decisions and pushed them that we can then work on top of. Let's discuss the approach here if this is not what we want but my changes here, other than rebasing and lint fix, were:

  • Change all test cases to readable in the sense that:
    1. We calculate the exact gas and check the positive expectation
    2. We then subtract a single gas from this so a reader knows for sure that it is failing at that Opcode, and we check the appropriate expectation (note: not None since the target is still recorded, but empty expectation for the address instead).
  • Rename test file to *_opcodes.py. This feels quite broad but the original intention is to test all sorts of single opcode edge cases. This could use more thought.
  • Remove the exception markers as these are not exception tests, just runs out of gas.
  • Removed the two basic test cases from the valid tests file as reads and writes are now tested positively with the positive SLOAD and SSTORE tests.

I think this makes the cases more obvious to a reader (subtracting exactly 1 gas for OOG) and allows us to test both positive and negative cases in the same test. That said, I am quite open to another organization for these so please let me know your thoughts now that this is stable and let's change these here accordingly!

@fselmo fselmo marked this pull request as ready for review October 1, 2025 22:09
@fselmo fselmo force-pushed the feat/eip-7928/tests branch from 4f79419 to 4453022 Compare October 1, 2025 22:29
@raxhvl
Copy link
Member Author

raxhvl commented Oct 2, 2025

Thanks @fselmo, I agree with all your decisions.

I think between this and the positive we cover a lot of ground. @marioevz does it cover everything you had in mind?

There are some more OOG test cases in this PR: #2116, lets add them later and keep this PR minimal.

I made a small change - added ids to test parameters to make results more readable. 6d0de55

@raxhvl raxhvl force-pushed the feat/eip-7928/tests branch from 4453022 to e8b5ca4 Compare October 2, 2025 06:43
Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

lgtm πŸ‘πŸΌ Thanks!

@fselmo fselmo changed the title ✨ feat(tests): EIP-7928 OOG test cases ✨ feat(tests): EIP-7928 Single-Opcode Success and OOG test cases Oct 2, 2025
@fselmo fselmo merged commit ae9b597 into main Oct 2, 2025
16 checks passed
@fselmo fselmo deleted the feat/eip-7928/tests branch October 2, 2025 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:tests Scope: Changes EL client test cases in `./tests` type:test Type: Add/refactor fw unit tests; no fw or el client test case changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants