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

move eip-712 to Last Call #4383

Closed
wants to merge 11 commits into from
Closed

Conversation

wschwab
Copy link
Contributor

@wschwab wschwab commented Oct 19, 2021

No description provided.

@eth-bot eth-bot enabled auto-merge (squash) October 19, 2021 16:38
@eth-bot
Copy link
Collaborator

eth-bot commented Oct 19, 2021

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


(fail) eip-712.md

classification
statusChange

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.

I only review Core EIPs, so just a few comments here to help facilitate moving this along:

  1. Simple Summary section has been removed.
  2. A new description header field has been added that should contain a human readable one-line description of the EIP.
  3. Remove external links. Inline things as appropriate or just remove the reference entirely.
  4. Remove the Implementation section. This isn't a valid EIP section (also, the content in it isn't appropriate for an EIP).
  5. Add missing Security Considerations section above Copyright section.
  6. Rationale for domainSeparator needs an extra # leading it.

@wschwab
Copy link
Contributor Author

wschwab commented Oct 26, 2021

Thanks @MicahZoltu ! I forgot to check back for comments until now, I'll get to work on these when I can.

@wschwab
Copy link
Contributor Author

wschwab commented Nov 28, 2021

I'm working with Recmo to try and accommodate Micah's suggestions. Most are simple fixes, others are more complicated, particularly the external links one, which would require a significant amount of editing, and really even changing the way the EIP looks today. (Unless there's something I'm misunderstanding, which is totally possible.)

auto-merge was automatically disabled November 28, 2021 08:38

Head branch was pushed to by a user without write access

@wschwab
Copy link
Contributor Author

wschwab commented Nov 28, 2021

(The most recent push should implement everything other than the external links, let me know if I missed something. Still working on finding a good solution on the links.)

EIPS/eip-712.md Outdated Show resolved Hide resolved
EIPS/eip-712.md Outdated Show resolved Hide resolved
@lightclient
Copy link
Member

Could we also point the link to the JSON-RPC spec to https://github.com/ethereum/execution-apis?

Looks good to me otherwise.

@wschwab
Copy link
Contributor Author

wschwab commented Dec 6, 2021

Could we also point the link to the JSON-RPC spec to https://github.com/ethereum/execution-apis?

Done

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

LGTM - @recmo could you approve to move this forward? Thanks.

@lightclient
Copy link
Member

@recmo bump

@wschwab
Copy link
Contributor Author

wschwab commented Apr 5, 2022

I've incorporated the changes recommended in #4709 and also bumped the proposed Last Call date

plz review @recmo

@github-actions github-actions bot added the stale label Jun 4, 2022
@SamWilsn
Copy link
Contributor

SamWilsn commented Jun 4, 2022

Not really stale, but waiting on authors (@recmo, @LogvinovLeon, @dekz).

@SamWilsn
Copy link
Contributor

SamWilsn commented Jun 4, 2022

Also, if you're interested @wschwab, you should add yourself as an author.

@laurenceday
Copy link

Given the length of time that this EIP has taken - and the lack of response from @recmo - with it languishing for years now, the fact that we can't move the status forward without an author approving, and that this is becoming something of a bureaucratic blocker for other EIPs, might I propose that this is effectively 'forked' into a new EIP with authors more amenable to ticking the boxes required so that it can be finalised at last?

@wschwab can't add himself as an author to this EIP without a PR (whether or not he feels like he deserves this due to the nature of his contribution is somewhat by-the-by): that in and of itself requires the approval of an author.

Copy link
Contributor Author

@wschwab wschwab left a comment

Choose a reason for hiding this comment

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

this is the simplest way for me to bump the date up, sorry for the clutter

EIPS/eip-712.md Outdated Show resolved Hide resolved
@wschwab
Copy link
Contributor Author

wschwab commented Aug 11, 2022

okay, I thought it was the simplest way to bump the last call date, but it seems to have gone completely off the rails, I'm trying to fix it up

@wschwab
Copy link
Contributor Author

wschwab commented Aug 11, 2022

okay, now I'm actually really confused - it looks like someone got 712 to Last Call, trying to figure out when and how it happened

@wschwab wschwab mentioned this pull request Aug 11, 2022
@wschwab wschwab closed this Aug 11, 2022
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.

None yet

8 participants