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

Export int as hex for t8n env #1027

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Export int as hex for t8n env #1027

wants to merge 2 commits into from

Conversation

winsvega
Copy link
Contributor

@winsvega winsvega commented Dec 17, 2024

πŸ—’οΈ Description

Export values in it's hex representation when interacting with t8n

πŸ”— Related Issues

#992

βœ… 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 added type:refactor Type: Refactor scope:fill Scope: fill command labels Dec 17, 2024
@winsvega winsvega requested a review from danceratopz December 17, 2024 13:20
@winsvega
Copy link
Contributor Author

But this affects all exports. Do we have issues with that?

@winsvega winsvega self-assigned this Dec 17, 2024
@marioevz
Copy link
Member

I think the solution to this is not changing the Number to be HexNumber but to use HexNumber where we want to see hexadecimals.

@winsvega
Copy link
Contributor Author

winsvega commented Dec 17, 2024

XNumber type is to be exported as hex not Number in EnvironmentGeneric[XNumber]

@winsvega winsvega changed the title Export int as hex for t8n and ?everywhere? Export int as hex for t8n env Dec 19, 2024
@winsvega winsvega requested a review from marioevz December 19, 2024 10:59
@marioevz
Copy link
Member

I just checked the failing unit test and it seems to be an error in EELS, so the change is breaking the interface.

I think that we need to implement some changes in EELS before this change goes in, otherwise we might break test generation.

Another thing is that I compared the generated fixtures for Cancun before and after this PR and there are some differences, so the change is somehow ending up in the fixtures too.

@marioevz
Copy link
Member

Pushed a fix for an evm-tool unit test that was breaking on collection.

Also the files in folder src/ethereum_clis/tests/fixtures might need an update but not entirely sure.

@winsvega
Copy link
Contributor Author

winsvega commented Jan 8, 2025

the fixtures were being generated with error in evmone because it treats all input as hex.
so we provide 131072 dec input meaning value of (0x020000) but evmone reads it as 0x131072 hex and that will affect the fixtures.

to avoid potential issue in other t8n's this PR forcing the export in 0x format of geth

@gurukamath
Copy link
Contributor

I think that we need to implement some changes in EELS before this change goes in, otherwise we might break test generation.

Could you point me to a particular json fixture where I can replicate the issue?

@winsvega
Copy link
Contributor Author

I think that we need to implement some changes in EELS before this change goes in, otherwise we might break test generation.

Could you point me to a particular json fixture where I can replicate the issue?

@gurukamath try this command described in here
ethereum/execution-specs#1078

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:fill Scope: fill command type:refactor Type: Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants