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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

py-evm-issue-1466: Added test cases for EIP170 #2002

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

Conversation

junning-tong
Copy link
Contributor

What was wrong?

Check issue-1466 for reference

Double checking this is the intended functionality.

EIP170 states that the contract size limit was changed to 2**14 + 2**13 which is 24,576 bytes. The following line implementations the constant for EIP170, but it is off by one.

EIP170_CODE_SIZE_LIMIT = 24577

How was it fixed?

  • Change EIP170_CODE_SIZE_LIMIT from 24577 to 24576
  • Add test cases

Cute Animal Picture

馃悥馃悥馃悥馃悥馃悥馃悥馃悥馃悥

@junning-tong junning-tong force-pushed the py-evm-issue-1466 branch 3 times, most recently from ad5f40e to 94c781b Compare April 18, 2021 22:34
@pipermerriam
Copy link
Member

Before we merge this, I'd like to understand why this wasn't caught using consensus tests because this seems like the type of thing that 1) I would be surprised if it wasn't in the tests and 2) if it isn't in the tests it should be added upstream

@junning-tong
Copy link
Contributor Author

junning-tong commented Apr 19, 2021

@pipermerriam the newly added test file is in directory tests/core/vm/.
Should I move it to tests/core/consensus/?

@pipermerriam
Copy link
Member

No, I'm referring to these tests: https://github.com/ethereum/tests/

They exist as a submodule in the repository.

@junning-tong
Copy link
Contributor Author

Hi @pipermerriam I see a commit which added test cases in fixtures submodules. https://github.com/ethereum/py-evm/pull/1998/files

Should I move newly added .json to the submodule as well?
tests/core/vm/fixtures/spurious_dragon_computation_test_code_size_limitation.json

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.

None yet

2 participants