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-1363: Update erc1363 to modern recommendations #7877

Conversation

vittominacori
Copy link
Contributor

@vittominacori vittominacori commented Oct 19, 2023

This PR wants to only update EIP content in order to follow the most updated recommendation from EIP-1.

  • Some texts have been updated to better explain the ERC-1363 behavior, after years of discussion. No changes in behavior.
  • Some texts have been moved from abstract or motivation to the dedicated rationale section.
  • References to EIPs have been moved to the EIP-N format.
  • Code specification have been indented and commented.
  • The creation date have been updated with the date of the first issue opening and the first commit into the reference implementation (erroneously it was the date of PR instead).
  • References to test cases and reference implementation have been aded as it was not permitted before (but it seems that many EIPs have them).

@github-actions github-actions bot added c-update Modifies an existing proposal s-final This EIP is Final t-erc labels Oct 19, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Oct 19, 2023

File EIPS/eip-1363.md

Requires 2 more reviewers from @axic, @gcolvin, @lightclient, @Pandapip1, @SamWilsn

@eth-bot eth-bot changed the title Update erc1363 to modern recommendations Update EIP-1363: Update erc1363 to modern recommendations Oct 19, 2023
@eth-bot eth-bot added the e-consensus Waiting on editor consensus label Oct 19, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Oct 19, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Oct 19, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Oct 19, 2023
@vittominacori
Copy link
Contributor Author

vittominacori commented Oct 23, 2023

@axic, @gcolvin, @lightclient, @Pandapip1, @SamWilsn

Is this is PR waiting for ci to pass before discussing?

These are the reasons because ci failed:

Error: error[markdown-order-section]: body has extra section(s)
  --> EIPS/eip-13[6](https://github.com/ethereum/EIPs/actions/runs/6586057763/job/17893640657?pr=7877#step:3:7)3.md
   |
13 | ## Simple Summary

Simple Summary has been added in the first draft of this EIP and it is still available on eip-20 or eip-721. Should this be removed?

Error: error[preamble-re-discussions-to]: preamble header `discussions-to` should point to a thread on ethereum-magicians.org

There is not link to magicians forum as it was not discussed there in 2018. It points to the issue like the eip-721

Error: error[preamble-req]: preamble is missing header(s): `description`

There is no description in original eip as it was not required. It can be simply added if required.

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.

This does not make the minimum changes necessary, and does not fully satisfy EIP-1.

@vittominacori
Copy link
Contributor Author

This does not make the minimum changes necessary, and does not fully satisfy EIP-1.

@Pandapip1 do you mean that I must add description in preamble and remove simple summary to fully comply with eip-1?

Also the original discussion link points to issue. There are lot of discussions on this repo issues or on openzeppelin issues and PR or on magicians forum, but not official eip discussion on magicians. I can't edit that link.

Is there something else to address?

@Pandapip1
Copy link
Member

Pandapip1 commented Oct 23, 2023

Whenever you change something, it has to comply with EIP-1. So, for example, the Simple Summary should be moved to the description preamble entry. Self-references should not be by number, etc...

If you don't change something, like the discussion link, you don't need to bring it up to date.

Additionally, avoid completely rephrasing anything ('rephrasing' code / interfaces also counts).

@github-actions
Copy link

The commit d32a328 (as a parent of 1f4f56a) contains errors.
Please inspect the Run Summary for details.

@vittominacori
Copy link
Contributor Author

I've submitted the suggested changes to totally apply with EIP-1.

I've move some text to the right section. I've also updated some text just to clarify the meaning as it was written in 2018 and it sound like obsolete wording.

I've not changed any code/interface (just indentation and some comments).

@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Oct 23, 2023
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal e-consensus Waiting on editor consensus s-final This EIP is Final t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants