-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Add EOF code validation draft #3670
Conversation
Why was this auto merged without review from any other editor? |
Hmm, good question. cc @alita-moore If an editor is an author, the PR should wait for another editor to approve before merging. It is probably simpler to just require an approval from any editor (like all PRs) and ignore that the author happens to be an editor. |
Got it, will address this |
@MicahZoltu should I ask you for a review nonetheless? 😉 |
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.
No blockers for moving to draft, left a couple very minor suggestions. I won't rehash our previous debate about including a Simple Summary (even if it feels redundant with the Abstract), but I still encourage it.
|
||
## Specification | ||
|
||
*Remark:* We rely on the notation of *initcode*, *code* and *creation* as defined by EIP-3540. |
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.
*Remark:* We rely on the notation of *initcode*, *code* and *creation* as defined by EIP-3540. | |
*Remark:* We rely on the notation of *initcode*, *code* and *creation* as defined by [EIP-3540](./eip-3540.md). |
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.
While technically you linked in the abstract, I'm a fan of linking everywhere, or at least linking to the first use in the Specification section. Not a blocker.
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.
Thanks, added into our WIP update.
## Test Cases | ||
|
||
TBA | ||
|
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.
## Test Cases | |
TBA |
Consider just leaving this section out until you have something for it, since this is an optional section anyway.
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.
Noted, though in the next update we have test cases listed.
@MicahZoltu this should be addressed now |
* Add EOF validation draft * Rename to EIP-3670 * Use new discussion url
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.
الله یحفظکم
No description provided.