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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EOF code validation draft #3670

Merged
merged 3 commits into from
Jul 20, 2021
Merged

Add EOF code validation draft #3670

merged 3 commits into from
Jul 20, 2021

Conversation

axic
Copy link
Member

@axic axic commented Jul 20, 2021

No description provided.

@axic axic force-pushed the eof-validation branch from 97b9d2a to 68ed1f5 Compare July 20, 2021 08:47
@axic axic force-pushed the eof-validation branch from a3f1e22 to 2262742 Compare July 20, 2021 09:07
@axic axic marked this pull request as ready for review July 20, 2021 09:22
@axic axic force-pushed the eof-validation branch from 2262742 to 984cac6 Compare July 20, 2021 09:25
@eth-bot eth-bot enabled auto-merge (squash) July 20, 2021 09:25
@axic axic changed the title Add EOF validation draft Add EOF code validation draft Jul 20, 2021
@eth-bot eth-bot merged commit a380e73 into ethereum:master Jul 20, 2021
@axic
Copy link
Member Author

axic commented Jul 20, 2021

Why was this auto merged without review from any other editor?

@MicahZoltu
Copy link
Contributor

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.

@alita-moore
Copy link
Contributor

Got it, will address this

@axic axic deleted the eof-validation branch July 20, 2021 22:00
@axic
Copy link
Member Author

axic commented Jul 20, 2021

@MicahZoltu should I ask you for a review nonetheless? 😉

Copy link
Contributor

@MicahZoltu MicahZoltu left a 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*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).

Copy link
Contributor

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.

Copy link
Member Author

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.

Comment on lines +78 to +81
## Test Cases

TBA

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Test Cases
TBA

Consider just leaving this section out until you have something for it, since this is an optional section anyway.

Copy link
Member Author

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.

@alita-moore
Copy link
Contributor

@MicahZoltu this should be addressed now

PhABC pushed a commit to PhABC/EIPs that referenced this pull request Jan 25, 2022
* Add EOF validation draft

* Rename to EIP-3670

* Use new discussion url
Copy link

@amirkhan7javi amirkhan7javi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

الله یحفظکم

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants