-
Notifications
You must be signed in to change notification settings - Fork 83
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: add tests for JUMP instruction #291
Conversation
(test failure due unrelated to PR) |
There is a third case but that can't be covered here for now, which is: Example in the EVM spec: |
haha yes! just realized that. |
Will start advertising it but the best source of truth we got is: |
(added the third case as test, and marked it ignored for now) |
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.
after that lgtm
Snapshot Comparison Report: No changes in gas consumption. |
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'm fine with the logic of the tests, but prefer to have a named counter
variable to clearly states that we push the input of the JUMP
opcode
crates/evm/src/tests/test_instructions/test_memory_operations.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_memory_operations.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_memory_operations.cairo
Outdated
Show resolved
Hide resolved
(rebased to main, incase it fixes snapshot CI) |
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.
lgtm!
Approved, just rebase for conflicts |
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Resolves: #251
What is the new behavior?
JUMPDEST
(in that case its valid) else its invalid0x5B
is part of data segment and is not an opcode (in an PUSHN) context.Does this introduce a breaking change?