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

Address some copy editing issues #5890

Closed
wants to merge 4 commits into from

Conversation

fulldecent
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.

@github-actions github-actions bot added c-update Modifies an existing proposal t-erc labels Nov 7, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Nov 7, 2022

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):


(fail) eip-5732.md

classification
updateEIP

mattstam
mattstam previously approved these changes Nov 8, 2022
Copy link
Contributor

@mattstam mattstam left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for this!

@eth-bot eth-bot enabled auto-merge (squash) November 8, 2022 00:31
eth-bot
eth-bot previously approved these changes Nov 8, 2022
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.

Seems EIPW is complaining... Let me see if I could quickly fix it

EIPS/eip-5732.md Outdated Show resolved Hide resolved
EIPS/eip-5732.md Outdated Show resolved Hide resolved
EIPS/eip-5732.md Outdated
@@ -10,44 +10,37 @@ type: Standards Track
category: ERC
created: 2022-09-29
requires: 165

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

EIPS/eip-5732.md Outdated Show resolved Hide resolved
EIPS/eip-5732.md Outdated Show resolved Hide resolved
EIPS/eip-5732.md Outdated Show resolved Hide resolved
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.

Thank you @fulldecent for helping editing. I also fixes some copy editing pieces. and also addressed the question you brought up in the ethereum-magicians.org

@xinbenlv
Copy link
Contributor

xinbenlv commented Nov 9, 2022

@fulldecent thanks for the PR, could you go ahead apply the suggested change and you shall be good to merge this PR. Or I am also happy to carry it out if you are bounded by bandwidth

@xinbenlv
Copy link
Contributor

xinbenlv commented Nov 10, 2022

Hi @fulldecent thank you for the help. Since you didn't respond, I created a PR using your commits, incorporating my suggested edits. and becomes PR 5910 and PR 5909.

  1. The editorial suggestions are accepted and incorporated.
  2. The specification ambiguity about "STRICTLY INCREASING" is clarified.
  3. The question that "secret_salt is unspecified" is clarified
  4. The demo reference implementations are removed, the only one left is ENS's actual implementation.

By now I believe I've addressed all your feedbacks. Thank you very much!

For you and everyone else, please let me know if there are any other suggestions / feedback for this EIP before we finalize it

@xinbenlv
Copy link
Contributor

With the above incorporations, feel free to close this PR.

auto-merge was automatically disabled November 12, 2022 17:16

Head branch was pushed to by a user without write access

@fulldecent fulldecent dismissed stale reviews from eth-bot and mattstam via bc5add6 November 12, 2022 17:16
@fulldecent
Copy link
Contributor Author

@xinbenlv Thank you, all updated

@fulldecent fulldecent closed this Nov 12, 2022
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 t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants