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

Create EIP-5027 #5027

Merged
merged 20 commits into from
May 22, 2022
Merged

Create EIP-5027 #5027

merged 20 commits into from
May 22, 2022

Conversation

qizhou
Copy link
Contributor

@qizhou qizhou commented Apr 21, 2022

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@qizhou qizhou changed the title Create EIP Create EIP-5027 Apr 21, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Apr 21, 2022

All tests passed; auto-merging...

(pass) eip-5027.md

classification
updateEIP
  • passed!

(pass) assets/eip-5027/0001-unlimit-code-size.patch

classification
ambiguous
  • file assets/eip-5027/0001-unlimit-code-size.patch is associated with EIP 5027; because there are also changes being made to EIPS/eip-5027.md all changes to corresponding assets are also allowed

Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

There are a few issues with this EIP - I've added comments hopefully explaining them.

EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Show resolved Hide resolved
qizhou and others added 2 commits April 22, 2022 22:53
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
EIPS/eip-5027.md Show resolved Hide resolved
EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Show resolved Hide resolved
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.

A couple of minor things needed to merge as a draft, but overall seems reasonable.

EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Outdated Show resolved Hide resolved
Co-authored-by: Pandapip1 <[email protected]>
EIPS/eip-5027.md Outdated Show resolved Hide resolved
Co-authored-by: Sam Wilson <[email protected]>
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.

Last thing I think. Sorry I missed it the first time through.

EIPS/eip-5027.md Outdated Show resolved Hide resolved
EIPS/eip-5027.md Outdated Show resolved Hide resolved
qizhou and others added 2 commits May 20, 2022 21:52
Co-authored-by: Micah Zoltu <[email protected]>
Add opcode number for first reference
@eth-bot eth-bot enabled auto-merge (squash) May 21, 2022 05:01
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

Travis CI is failing. Those issues need to be fixed, whatever they are.

@MicahZoltu
Copy link
Contributor

./assets/eip-5027/0001-unlimit-code-size.patch:87: erro ==> error
./assets/eip-5027/0001-unlimit-code-size.patch:121: memor ==> memory

@MicahZoltu
Copy link
Contributor

Hmm... CI is unhappy with the diff because it cuts off at a fixed number of characters. I'm not sure how to handle this generally, but for now I'm going to force merge since that is the only CI failure. I'm hoping CI doesn't fail like this every time someone touches the file.

@MicahZoltu MicahZoltu disabled auto-merge May 22, 2022 05:00
@MicahZoltu MicahZoltu merged commit ca98714 into ethereum:master May 22, 2022
@qizhou
Copy link
Contributor Author

qizhou commented May 22, 2022

Hmm... CI is unhappy with the diff because it cuts off at a fixed number of characters. I'm not sure how to handle this generally, but for now I'm going to force merge since that is the only CI failure. I'm hoping CI doesn't fail like this every time someone touches the file.

Many thanks for handling this!


## Reference Implementation

The reference implementation on Geth is available at [0001-unlimit-code-size.patch](../assets/eip-5027/0001-unlimit-code-size.patch).
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand it GPL licensed code should not be in the EIP repository. Can this be changed to pointing to a commit in a pull request in Geth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the external link is not recommended in the EIP repository either (as the previous version of this EIP uses the pointer to the commit).

nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Create eip-tbd

* Update eip-tbd

* Update and rename eip-tbd to eip-5027

* Rename eip-5027 to eip-5027.md

* Update eip-5027.md

* Update eip-5027.md

* Update EIPS/eip-5027.md

Co-authored-by: Pandapip1 <[email protected]>

* Update EIPS/eip-5027.md

Co-authored-by: Pandapip1 <[email protected]>

* Update eip-5027.md

* Update eip-5027.md

* Update EIPS/eip-5027.md

Co-authored-by: Sam Wilson <[email protected]>

* Update eip-5027.md

* Update EIPS/eip-5027.md

Co-authored-by: Micah Zoltu <[email protected]>

* add code path in assets

* Update eip-5027.md

* Update EIPS/eip-5027.md

Co-authored-by: Pandapip1 <[email protected]>

* Update EIPS/eip-5027.md

Co-authored-by: Sam Wilson <[email protected]>

* Update eip-5027.md

* Update EIPS/eip-5027.md

Co-authored-by: Micah Zoltu <[email protected]>

* Update eip-5027.md

Add opcode number for first reference

Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Sam Wilson <[email protected]>
Co-authored-by: Micah Zoltu <[email protected]>
Co-authored-by: Qi Zhou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants