Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(exceptions,specs): class to verify exception strings #795

Merged
merged 9 commits into from
Oct 23, 2024

Conversation

winsvega
Copy link
Collaborator

@winsvega winsvega commented Sep 12, 2024

🗒️ Description

Extent exception mapper with base class so that any t8n can define exception string maps for any exception types

I put transaction exception validation into exception mapper for convinience atm.
Example exception message:

src/ethereum_test_exceptions/exception_mapper.py:84: in check_transaction
    raise Exception(
E   Exception: TransactionException (pos=1, nonce=0x1)
E   Error: exception mismatch:
E    want = 'Undefined' (TransactionException.TYPE_3_TX_INVALID_BLOB_VERSIONED_HASH),
E    got  = 'blob 0 has invalid hash version' (UndefinedException.UNDEFINED_EXCEPTION)
E    No message defined for TransactionException.TYPE_3_TX_INVALID_BLOB_VERSIONED_HASH, please add it to GethExceptionMapper
E    No exception defined for error message got, please add it to GethExceptionMapper

🔗 Related Issues

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
  • Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

I like the ExceptionMapper idea, I think it might be too t8n-focused but it could be used in many other places, e.g. exceptions from the Engine API when we run the tests in hive.

I think this PR can start with TransitionToolExceptionMapper defined in src/evm_transition_tool to make ExceptionMapper a bit more generic to open the door to future PRs to implement other subclasses.

src/ethereum_test_exceptions/exception_mapper.py Outdated Show resolved Hide resolved
src/ethereum_test_specs/base.py Outdated Show resolved Hide resolved
src/ethereum_test_specs/base.py Outdated Show resolved Hide resolved
src/ethereum_test_exceptions/geth_exceptions.py Outdated Show resolved Hide resolved
@winsvega winsvega force-pushed the exception_mapper branch 3 times, most recently from db0b38b to 28598f3 Compare October 8, 2024 10:15
@winsvega winsvega requested a review from marioevz October 8, 2024 10:34
@winsvega
Copy link
Collaborator Author

winsvega commented Oct 8, 2024

still need to setup other clients for this. but I refactored the suggestions. can check the code.
now I need to fill the exception strings for other tools.

Ah, and need to enable it for state tests

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Hey it's looking great overall, but I think we need to address the comments, mainly because the default t8n to fill tests has become the execution_specs t8n, so I think the exception mapper should be complete from the start.

src/ethereum_test_specs/helpers.py Outdated Show resolved Hide resolved
src/ethereum_test_specs/helpers.py Outdated Show resolved Hide resolved
src/ethereum_test_specs/helpers.py Outdated Show resolved Hide resolved
src/ethereum_test_specs/helpers.py Outdated Show resolved Hide resolved
src/ethereum_test_exceptions/exception_mapper.py Outdated Show resolved Hide resolved
src/evm_transition_tool/execution_specs.py Outdated Show resolved Hide resolved
@winsvega winsvega force-pushed the exception_mapper branch 2 times, most recently from 8d04c8b to 3a5677b Compare October 15, 2024 10:08
Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

This looks fantastic, looking forward to using these exceptions in consume in due course!

A few small comments below.

src/ethereum_test_specs/blockchain.py Show resolved Hide resolved
src/ethereum_clis/clis/execution_specs.py Show resolved Hide resolved
src/ethereum_test_specs/helpers.py Outdated Show resolved Hide resolved
src/ethereum_test_specs/helpers.py Outdated Show resolved Hide resolved
@danceratopz danceratopz added scope:evm Scope: evm_transition_tool package type:feat type: Feature scope:fw Scope: Framework (evm|tools|forks|pytest) labels Oct 16, 2024
@marioevz
Copy link
Member

Fixed tox. It turns out there was an invalid transaction in the framework tests.

Copy link
Member

@marioevz marioevz 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!

@marioevz marioevz changed the title feat(exception mapper): class to verify exception strings feat(exceptions,specs): class to verify exception strings Oct 23, 2024
@marioevz marioevz merged commit 247c312 into main Oct 23, 2024
5 checks passed
@marioevz marioevz deleted the exception_mapper branch October 23, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:evm Scope: evm_transition_tool package scope:fw Scope: Framework (evm|tools|forks|pytest) type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants