-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Revise EIP-2535 #5120
Revise EIP-2535 #5120
Conversation
Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s): (fail) eip-2535.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.
We should get this to align with latest template before merging. Remove Simple Summary, add description
header field, correct sections to match EIP sections, remove external links, etc.
At the moment, this doesn't look much like an EIP at all, and I actually recommend moving it back to Review as it will need a lot of work to bring it in line with our repository standards.
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 not sure why I approved this. 😬 This still doesn't follow the template at all. Lots of sections that aren't part of the templated sections for example. Also the comment above about order of metadata fields needs to be fixed.
I am not done editing this, but I'll commit the changes 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.
Looks pretty good overall, just a few nits.
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):
|
Is @eip-automerger the new @eth-bot? |
@mudgen Please note that I have removed a lot of content from this. This is primarily so that this PR can be merged, but feel free to re-add things like the spec for Diamond Storage & App Storage. I might also consider adding myself as an author. LMK if that's okay! Finally, I also feel like the spec could be tighter overall. I would recommend bundling up many of the conventions and making them extensions. Notably, I would also like if the names were more strict, to aid with interfacing. This is outside of the scope of this PR, though. |
@Pandapip1 I appreciate your help but I don't want to make a lot of the changes in this pull request. I do not want to rewrite or remove what is already written. But I do want changed some of the header names and other smaller changes to make it more compliant with the EIP template. |
Depends on what you mean by "new". 😀 It is the auto-merge bot letting you know what needs to change in order to merge this PR. |
I'll let @mudgen do this. |
CC: @mudgen