-
Notifications
You must be signed in to change notification settings - Fork 59
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
convert create2 test #497
base: main
Are you sure you want to change the base?
convert create2 test #497
Conversation
@marioevz do you want the test coverage to be mandatory checkpoint as well? or perhaps we can setup a cli job to do it using evmone script that I was developing. hm.. |
I think we could start with a CLI entry point to check coverage, if you have an idea of implementation we could open another PR and follow it up there. After that PR is merged we can do a follow up PR to try to embed it into the CI to automatically check it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall! Just some suggestions.
+ Op.SSTORE(slot_returndatasize_before_create, Op.RETURNDATASIZE()) | ||
+ make_create() | ||
+ Op.SSTORE(slot_returndatasize_after_create, Op.RETURNDATASIZE()) | ||
+ Op.SSTORE(slot_code_worked, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ Op.SSTORE(slot_code_worked, 1) | |
+ Op.SSTORE(slot_code_worked, 1) | |
+ Op.RETURNDATACOPY(0, 0, Op.RETURNDATASIZE()) | |
+ Op.SSTORE(slot_return_data_sha3, Op.SHA3(0, Op.RETURNDATASIZE())) |
What do you think about also SHA3-comparing the return data just to test the integrity of the return data after the create opcode has been called?
We can get the expected hash as follows:
from ethereum.crypto.hash import keccak256
...
call_return_data = int.to_bytes(0x0000111122223333444455556666777788889999AAAABBBBCCCCDDDDEEEEFFFF, 32, byteorder='big').ljust(call_return_size, b'\0')[0:call_return_size]
...
storage={
slot_code_worked: 1,
slot_returndatasize_before_create: 32,
slot_returndatasize_after_create: 0,
slot_return_data_sha3: keccak256(call_return_data),
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, the values does not match when returnsize is not 32. need to debug
ah I see. so we can ask the call to return into 0..x but the return datasize will always sys what was the actual return (0, y) inside call. this is another test need to see that return(0,y) inside the call does only write to allowed (0,x) when we called the contract
569916e
to
d3b25c0
Compare
.github/workflows/coverage.yaml
Outdated
@@ -0,0 +1,74 @@ | |||
name: Evmone Coverage Report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not reviewed this file yet, but I think we need to split this file into its own PR to have it properly reviewed, since it's completely orthogonal to test implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but on this pr, I can test directly that it works with the given test changes.
It's not ready yet.
d50fb32
to
06e4ed2
Compare
4909a21
to
e1427fc
Compare
🗒️ Description
Port
🔗 Related Issues
none
✅ Checklist
NOT READY FOR MERGE (NEED coverage)
mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.