-
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(tests): extend Op with Op.OOG macro opcode #457
Conversation
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 really like the idea! Just a couple of suggestions so let me know if it sounds ok to you and if it still achieves the original purpose you were thinking of.
what about this part?
I see that if you use SENDALL it will still be trated as SELFDESTRUCT ah this part is actually required if Macro commands live in the same enum because they are based on the same opcode |
ecfd90a
to
530f6cc
Compare
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.
Still not completely on-board with the idea that the opcodes and the macros should live in the same Enum
, but definitely open to discuss it!
What do you guys think? @spencer-tb @danceratopz
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.
Some small comments.
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 really like this, but I'd rather see Macro
defined as its own, independent type.
Why Macro can't be inherited from Opcode? I mean, then I will have to implement () logic for it that returns bytes. |
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 good to me! I like the new separation between Macro and Opcode.
@winsvega Could you just check the suggested docstring change before we merge?
Co-authored-by: Mario Vega <[email protected]>
Co-authored-by: Mario Vega <[email protected]>
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.
The suggested change has nothing to do with the addition of the Macro
class, but came up whilst reviewing #440,
🗒️ Description
I think it would be handy to have test purpose Op that will trigger OOG if called
This will simplify parametrisations and code in tests when paramtrising over subcall return types
Such as RETURN, REVERT
Op.OOG(x, y) is replaced with Op..SHA3(0, 100000000000)
alowing to use
Example:
#440
🔗 Related Issues
#455
✅ Checklist
mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.