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

new(tests): add tests for DATALOADN validation and execution #1162

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chfast
Copy link
Member

@chfast chfast commented Jan 30, 2025

πŸ—’οΈ Description

πŸ”— 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. delete converted DATALOADN testsΒ tests#1439
  • 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.

@chfast chfast added the scope:tests Scope: Changes EL client test cases in `./tests` label Jan 30, 2025
@chfast chfast requested review from marioevz and shemnon January 30, 2025 19:29
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.

Some minor comments

Section.Code(
Op.DATALOADN[index] + Op.SSTORE(0, unchecked=True) + Op.STOP,
),
Section.Data(index * b"\xbe" + sentinel.to_bytes(32) + suffix_len * b"\xaf"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Section.Data(index * b"\xbe" + sentinel.to_bytes(32) + suffix_len * b"\xaf"),
Section.Data(
index * b"\xbe" + sentinel.to_bytes(32, byteorder="big") + suffix_len * b"\xaf"
),

Which python version are you currently using? 3.10 fails if byteorder is not specified.

@pytest.mark.parametrize("suffix_len", [0, 1, 31, 32, 24000])
def test_dataloadn(eof_state_test: EOFStateTestFiller, index: int, suffix_len: int):
"""Basic tests for DATALOADN execution."""
sentinel = 0x8000000000000000000000000000000000000000000000000000000000000001
Copy link
Member

Choose a reason for hiding this comment

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

This is a really nice idea to keep the expected outcome simple πŸ‘

container=Container(
sections=[
Section.Code(
Op.DATALOADN[index] + Op.SSTORE(0, unchecked=True) + Op.STOP,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Op.DATALOADN[index] + Op.SSTORE(0, unchecked=True) + Op.STOP,
Op.SSTORE(0, Op.DATALOADN[index]) + Op.STOP,

Or is the current arrangement used to keep the usage of Op.DATALOADN at the start of the line?

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`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants