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

Update EIP-4824: Move to Review #7137

Closed
wants to merge 3 commits into from

Conversation

thelastjosh
Copy link
Contributor

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.

@thelastjosh thelastjosh requested a review from eth-bot as a code owner June 4, 2023 05:07
@github-actions github-actions bot added c-status Changes a proposal's status s-review This EIP is in Review t-erc labels Jun 4, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jun 4, 2023

File EIPS/eip-4824.md

Requires 1 more reviewers from @axic, @Pandapip1, @SamWilsn, @xinbenlv

@eth-bot eth-bot changed the title Move to Review status Update EIP-4824: Move to Review Jun 4, 2023
@eth-bot eth-bot added the e-review Waiting on editor to review label Jun 4, 2023
Copy link
Contributor

@xinbenlv xinbenlv left a comment

Choose a reason for hiding this comment

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

I couldn't comment on exact lines, hence the overall comment: can you move anything that are deemed reference implementations into its section rather than in the specification section?

@thelastjosh
Copy link
Contributor Author

Hi @xinbenlv there are no reference implementations in the current specification; the solidity code gives the precise specification for the expected registration & indexing contracts on all EVM chains.

Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Couple things I can't comment directly on:

  • You have IERC-4824 in your solidity block. I bet this was a find-and-replace error.
  • I'd recommend removing the ERC-4824Registration contract because it contains implementation details. EIPs shouldn't place any restrictions on the internals of implementations.
  • In fact, I'd suggest removing all implementations outside the reference implementation for the same reason.

Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Still a few weirdly named identifiers (eg. ERC-4824RegistrationSummoner).

It looks like there's still a bunch of implementation code in the specification section (like ERC-4824Registration). If this isn't implementation, but something else, the preceding text needs to do a better job of introducing it as an example. If they are examples, they should be moved to the reference implementation section.

@SamWilsn
Copy link
Contributor

I am closing this pull request because we are in the process of separating EIPs and ERCs into distinct repositories. Unfortunately, as far as we are aware, GitHub does not provide any tools to ease this migration, so every pull request will need to be re-opened manually.

As this is a PR to create / modify an ERC, I will kindly ask you to redirect this to the new repository at ethereum/ERCs. We have prepared a guide to help with the process.

If there is relevant history here, please link to this PR from the new pull request.

On behalf of the EIP Editors, I apologize for this inconvenience.

@SamWilsn SamWilsn closed this Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-status Changes a proposal's status e-review Waiting on editor to review s-review This EIP is in Review t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants