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(test-convert) : Fillerconvert WIP parse .json test vectors #1294

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

winsvega
Copy link
Contributor

@winsvega winsvega commented Mar 11, 2025

🗒️ Description

WIP prototype of .json/yml test fillers

I added pydantic models (WIP) of state test fillers.
and a function that will build pyspec transition test out of it.

lost of logic to port and reimplement in python. here is just an example of how it will look like.

General Idea:
-take .json test filler -> get list of individual test vectors (id, env, pre, tx, post) -> run state_test() normally

the complexity raises because original state test filler hash vector of [tx] and [post] that need to be parsed back to single test again. which I think is in the end better idea. due to a complexity of having many test vectors in one file, we will have 1 vector 1 file. even though env,pre is copied

🔗 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.

@winsvega winsvega self-assigned this Mar 11, 2025
@winsvega winsvega marked this pull request as draft March 11, 2025 10:13
@winsvega winsvega requested a review from marioevz March 11, 2025 10:13
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.

Looks good overall, just a couple of comments.

file.txt Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Unintentionally added file perhaps?

import tempfile

from pydantic.functional_validators import BeforeValidator
from typing_extensions import Annotated
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
from typing_extensions import Annotated
from typing import Annotated

return "0x" + b.hex()


AddressInFiller = Annotated[str, BeforeValidator(parse_address)]
Copy link
Member

Choose a reason for hiding this comment

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

Seems to me that Address should work here, but is there any reason why it doesn't?



AddressInFiller = Annotated[str, BeforeValidator(parse_address)]
ValueInFiller = Annotated[str, BeforeValidator(parse_hex_number)]
Copy link
Member

Choose a reason for hiding this comment

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

Similar to previous comment, would HexNumber work here?


AddressInFiller = Annotated[str, BeforeValidator(parse_address)]
ValueInFiller = Annotated[str, BeforeValidator(parse_hex_number)]
CodeInFiller = Annotated[str, BeforeValidator(parse_code)]
Copy link
Member

Choose a reason for hiding this comment

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

I agree that the BeforeValidator is definitely needed here, but I think it would be better to have:

Suggested change
CodeInFiller = Annotated[str, BeforeValidator(parse_code)]
CodeInFiller = Annotated[Bytes, BeforeValidator(parse_code)]

Just to not have to work with strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't serialize if its Bytes

for key, value in account.storage.items():
storage[key] = value

# TODO looks like pyspec post state verification will not work for default values?
Copy link
Member

Choose a reason for hiding this comment

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

We can use model_fields_set of the account to see if a value was explicitly set.

For example, if "balance" in account.model_fields_set, it means the value was explicitly set to zero instead the value being zero because if fell back to the default.

AddressInFiller = Annotated[str, BeforeValidator(parse_address)]
ValueInFiller = Annotated[str, BeforeValidator(parse_hex_number)]
CodeInFiller = Annotated[str, BeforeValidator(parse_code)]
Hash32InFiller = Annotated[str, BeforeValidator(parse_hash32)]
Copy link
Member

Choose a reason for hiding this comment

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

It seems like Hash should work here without issues.

Copy link
Contributor Author

@winsvega winsvega Mar 15, 2025

Choose a reason for hiding this comment

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

I thought it was better to separate pydantic for the testFiller scope. Otherwise, schemas get mixed here and there.
So this part of the code would not depend on existing pyspec pydantic schemas. we won't need to maintain it and once the tests are converted it can be archived. and if any logic will be required during pydantic implementation of this schemas I won't need to interfere into existing pyspecs.

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.

2 participants