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

Add new two-week review process to EIPs #1100

Merged
merged 34 commits into from
Jun 4, 2018
Merged

Conversation

fulldecent
Copy link
Contributor

@fulldecent fulldecent commented May 18, 2018

This PR adds a two-week "Last Call" process for every EIP. You can subscribe to these EIPs via RSS so you can have a last say before something becomes final. There have been 400 new pull requests since ERC-721, I mostly only want to review the serious ones that make it to Last Call.

ERC-721 (currently stuck in Draft status) will be the first ERC to go to Last Call. See #1101.

Because some of the current EIP-1 wording is inaccurate, confusing or contradictory, it was necessary for me to fully rewrite the workflow section and document each EIP status and transition of statuses. I hope this makes the text more approachable for all readers.

Pasting my proposed update below for convenience. / Link to live updated version / Link to current text to be replaced


Following is the process that a successful EIP will move along:

[ WIP ] -> [ DRAFT ] -> [ LAST CALL ] -> [ ACCEPTED ] -> [ FINAL ]

Each status change is requested by the EIP author by @-mentioning or emailing the EIP editors. The EIP editors will process these requests as per the conditions below.

  • Work in progress (WIP) -- Once the champion has asked the Ethereum community whether an idea has any chance of support, they will write a draft EIP as a [pull request]. Consider including an implementation if this will aid people in studying the EIP.
    • ➡️ Draft -- If agreeable, EIP editor will assign the EIP a number (generally the issue or PR number related to the EIP) and merge your pull request. The EIP editor will not unreasonably deny an EIP.
    • ❌ Draft -- Reasons for denying draft status include being too unfocused, too broad, duplication of effort, being technically unsound, not providing proper motivation or addressing backwards compatibility, or not in keeping with the Ethereum philosophy.
  • Draft -- Once the first draft has been merged, you may submit follow-up pull requests with further changes to your draft until such point as you believe the EIP to be mature and ready to proceed to the next status. An EIP in draft status must be implemented to be considered for promotion to the next status (ignore this requirement for core EIPs).
    • ➡️ Last Call -- If agreeable, the EIP editor will assign Last Call status and set a review end date, normally 14 days later.
    • ❌ Last Call -- A request for Last Call status will be denied if material changes are still expected to be made to the draft. We hope that EIPs only enter Last Call once, so as to avoid unnecessary noise on the RSS feed. Last Call will be denied if the implementation is not complete and supported by the community.
  • Last Call -- This EIP will listed prominently on the http://eips.ethereum.org/ website (subscribe via RSS at last-call.xml).
    • ❌ -- A Last Call which results in material changes or substantial unaddressed complaints will cause the EIP to revert to Draft.
    • ➡️ Accepted (Core EIPs only) -- After the review end date, the Ethereum Core Developers will vote on whether to accept this change. If yes, the status will upgrade to Accepted.
    • ➡️ Final (Not core EIPs) -- A successful Last Call without material changes or unaddressed complaints will become Final.
  • Accepted (Core EIPs only) -- This is being implemented by Ethereum Core Developers.
    • ➡️ Final -- Standards Track Core EIPs must be implemented in at least three viable Ethereum clients before it can be considered Final. When the implementation is complete and supported by the community, the status will be changed to “Final”.
  • Final -- This EIP represents the current state-of-the-art. A Final EIP should only be updated to correct errata.

Other exceptional statuses include:

  • Deferred -- This is for core EIPs that have been put off for a future hard fork.
  • Rejected -- An EIP that is fundamentally broken and will not be implemented.
  • Active -- This is similar to Final, but denotes an EIP which which may be updated without changing its EIP number.
  • Superseded -- An EIP which was previously final but is no longer considered state-of-the-art. Another EIP will be in Final status and reference the Superseded EIP.


Follow on work after merge

@fulldecent
Copy link
Contributor Author

Ping
@sfultong sfultong
@chfast chfast
@elenadimitrova elenadimitrova
@MicahZoltu MicahZoltu
@Souptacular Souptacular
@tjayrush tjayrush
@nicksavers nicksavers
@gcolvin gcolvin
@jamesray1 jamesray1

@fulldecent
Copy link
Contributor Author

fulldecent commented May 20, 2018

xxx

At present, we have on the table: #956 which adds a two-week review process to finalize an EIP. There is consensus on this issue. Unfortunately it also includes other contentious items, such as allowing something to become final if only one implementation exists. (DISCUSSION OF ONE-IMPLEMENTATION RULE IS OFF TOPIC IN THIS THREAD.)

The one-implementation rule is a great debate and we should have it. But right now the more important issue is that PRESENTLY THE EIP PROCESS IS BROKEN AND WE HAVE NO WAY TO ACCEPT EIPS. Specifically, the current process requires Core Dev approval and the Core Dev just updated their process to clarify they will not adjudicate ERCs.

Presently, ERC-721 is widely recognized as a standard that is ready to move forward. And it is widely recognized that the TWO WEEK REVIEW is the next step.

I have distilled /#956 for only the two-week review part. This is at #1100

Please, let’s merge #1100 quickly, enter #721 into that process (PR is ready, #1101), and then continue to debate remaining improvements.

xxxx

https://ethereum-magicians.org/t/add-two-week-review-to-eip-process-mergeable/428

@elenadimitrova
Copy link

I'd like to hear what @Arachnid thinks but otherwise I am ok to process #721 without the need for procedure. This EIP shouldn't be necessary for #721 or indeed other EIPs while discussions in #956 continue. We have over 100 EIPs in some intermediate state while new process is being instated so this is a good case to decide what we do. My take is to expedite conversations and finalise #956 as it has been open for 2 months and discussions on the last update (post EIP0 in Toronto) have been open for 2 weeks.

EIPS/eip-1.md Outdated
@@ -3,121 +3,108 @@ eip: 1
title: EIP Purpose and Guidelines
status: Active
type: Meta
author: Martin Becze <mb@ethereum.org>, Hudson Jameson <hudson@ethereum.org>
author: Martin Becze <mb@ethereum.org>, Hudson Jameson <hudson@ethereum.org>, William Entriken <github.com@phor.net>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the process for adding an author to an EIP? In particular, it feels like we should be conservative with authors on EIP-1, especially with the auto-accept bot.

Copy link
Contributor

Choose a reason for hiding this comment

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

EIP-1 isn't draft, so the auto-accept bot won't automerge any changes to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MicahZoltu My name is no longer there, now it is a link to all authors.

Does this resolve your concern? If yes, will you be able to turn your comment into an "Approve" vote?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not an editor, my approval holds to value. 😄

Copy link
Contributor

@Arachnid Arachnid left a comment

Choose a reason for hiding this comment

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

This PR makes a lot of formatting and content changes to EIP-1. If your goal is to just introduce a last call status, why include so many other changes?

Unfortunately it also includes other contentious items, such as allowing something to become final if only one implementation exists. (DISCUSSION OF ONE-IMPLEMENTATION RULE IS OFF TOPIC IN THIS THREAD.)

The previous wording of EIP 1 makes no mention of a number of required implementations, so this isn't a loosening of standards.

EIPS/eip-1.md Outdated
created: 2015-10-27, 2017-02-01
---

What is an EIP?
--------------
## What is an EIP?
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning behind changing all the header styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use Typora editor, this is how the editor is saving the file.

I have reviewed every FINAL status EIP and they are all using the format that this PR uses.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also easier to prepend a heading with a couple of hashes than it is to use dashes on the next line (and line them up for presentation, and have to update them each time the heading is changed).

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to reformat EIP 1, please submit a separate PR for it. It's very hard to find the content changes amongst all the formatting changes here.

EIPS/eip-1.md Outdated

The EIP repository editors change the EIPs status. Please send all EIP-related email to the EIP editors or preferably (e.g. for public transparency) you can ping one of the Editors that is involved with your pull request (PR); they are listed under [EIP editors below](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md#eip-editors). Also see [EIP Editor Responsibilities & Workflow](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md#eip-editor-responsibilities-and-workflow).
```
[ A pull request ] -> [ DRAFT ] -> [ TWO WEEK REVIEW ] -> [ FINAL ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "Two Week Review", I think "Last Call" would be a better description.

This workflow also doesn't reflect the process for core EIPs. I think updating the diagram would be a better idea than including this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the accepted status here fulldecent@c84ee64

Last Call is good wording, implemented here fulldecent@d6bed1b

Swift Evolution uses Active Review, that would be fine too https://apple.github.io/swift-evolution/

The diagram is a nice to have. And the current options on the table are unnecessarily complicated. Please do not condition the acceptance of this PR on a diagram because currently the EIP has a huge problem, we can finalize any EIPS, and this PR is the fastest way to fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not saying you must update the diagram - just that the ascii diagram you add here is overly simplified and thus likely to mislead.

@fulldecent
Copy link
Contributor Author

@MicahZoltu @Arachnid Thank you for your comments. Do the comments and commits above address your concerns? Are you able to support this PR now and would you please merge it?

This PR is in response to our current broken EIP process as a result of the core dev meeting. Surely we can have additional progress, I am happy to argue on the finer points of #956, rebase that, and stay involved in EIP-1 improvements. But for today, we have a serious problem and this PR solves it with minimal controversy.

To be clear, I strongly recognize the value of a LAST CALL. And 721 needs that. We have started a de facto last-call already. I am excited to get this PR done and get 721 on LAST CALL de jure. I am sure the 721 case study will be studied and affect policy going forward.

@Arachnid thanks for all your help. AFTER if this PR is merged, I will please ask your assistance to update the EIP website for LAST CALLS. You are welcome to call/text/email me and I can help or get help for this ASAP.

Thanks everyone, I hope I am being a PITA in a good way here.

EIPS/eip-1.md Outdated
- **Interface** — includes improvements around client [API/RPC] specifications and standards, and also certain language-level standards like method names ([EIP59], [EIP6]) and [contract ABIs]. The label “interface” aligns with the [interfaces repo] and discussion should primarily occur in that repository before an EIP is submitted to the EIPs repository.
- **ERC** — application-level standards and conventions, including contract standards such as token standards ([ERC20]), name registries ([ERC26], [ERC137]), URI schemes ([ERC67]), library/package formats ([EIP82]), and wallet formats ([EIP75], [EIP85]).
- An **Informational EIP** describes an Ethereum design issue, or provides general guidelines or information to the Ethereum community, but does not propose a new feature. Informational EIPs do not necessarily represent Ethereum community consensus or a recommendation, so users and implementers are free to ignore Informational EIPs or follow their advice.
- A **Meta EIP** describes a process surrounding Ethereum or proposes a change to (or an event in) a process. Process EIPs are like Standards Track EIPs but apply to areas other than the Ethereum protocol itself. They may propose an implementation, but not to Ethereum's codebase; they often require community consensus; unlike Informational EIPs, they are more than recommendations, and users are typically not free to ignore them. Examples include procedures, guidelines, changes to the decision-making process, and changes to the tools or environment used in Ethereum development. Any meta-EIP is also considered a Process EIP.
Copy link
Contributor

Choose a reason for hiding this comment

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

A note for other reviewers: the only difference is the reduction of spaces at the start of each bullet point.

EIPS/eip-1.md Outdated
[ A pull request ] -> [ DRAFT ] -> [ TWO WEEK REVIEW ] -> [ FINAL ]
```

The [EIP editors](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md#eip-editors) update the status of EIPs at the request of EIP authors. Please ping the editors by mentioning them in your pull request or send them an email. See alse [EIP Editor Responsibilities & Workflow](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md#eip-editor-responsibilities-and-workflow).
Copy link
Contributor

Choose a reason for hiding this comment

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

alse -> also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected in 03aab50

EIPS/eip-1.md Outdated

The EIP repository editors change the EIPs status. Please send all EIP-related email to the EIP editors or preferably (e.g. for public transparency) you can ping one of the Editors that is involved with your pull request (PR); they are listed under [EIP editors below](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md#eip-editors). Also see [EIP Editor Responsibilities & Workflow](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md#eip-editor-responsibilities-and-workflow).
```
[ A pull request ] -> [ DRAFT ] -> [ TWO WEEK REVIEW ] -> [ FINAL ]
Copy link
Contributor

Choose a reason for hiding this comment

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

AIUI this is for non-standard EIPs. @elenadimitrova's diagram should also be included here for Standard Track EIPS.

EIPS/eip-1.md Outdated

Once the champion has asked the Ethereum community whether an idea has any chance of acceptance a draft EIP should be presented as a [pull request].

If the EIP editors approve the EIP ([see the EIP editors workflow section below for details](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md#eip-editor-responsibilities-and-workflow)) and the authors are happy for it to be merged as a draft, the EIP editor will assign the EIP a number (generally the issue or PR number related to the EIP) and merge your pull request. The EIP editor will not unreasonably deny an EIP. (EIP authors can request for it to be merged after they have finished editing it, and editors can ask if they have finished editing it and would like it to be merged. This prevents merging an EIP when more edits are intended to be made by the author, although it can always be edited after merging.) Reasons for denying EIP status include duplication of effort, being technically unsound, not providing proper motivation or addressing backwards compatibility, or not in keeping with the [Ethereum philosophy](https://github.com/ethereum/wiki/wiki/White-Paper#philosophy).
Copy link
Contributor

Choose a reason for hiding this comment

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

(EIP authors can request for it to be merged after they have finished editing it, and editors can ask if they have finished editing it and would like it to be merged. This prevents merging an EIP when more edits are intended to be made by the author, although it can always be edited after merging.)

Not sure why this is removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously draft process was more difficult, now we have automerge bot. This means that waiting to merge to draft is no longer a reason to delay drafts.

Copy link
Contributor

Choose a reason for hiding this comment

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

The automerge bot is used for editing existing draft. But to merge a PR as a new draft, an editor still needs to assign the EIP number before it can be automerged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for clarifying. I have added this back in with d09dd90. This commit clarifies that the author requests merging. But I have reworded slightly to remove "after they have finished editing it" because they are not finished editing it.

@jamesray1 If you are more comfortable with another wording, please feel free to directly add a commit into this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK that will do.

EIPS/eip-1.md Outdated

Once the first draft has been merged, you may submit follow-up pull requests with further changes to your draft until such point as you believe the EIP to be mature and ready to proceed to the next phase.

Standards Track EIPs consist of three parts, a design document, implementation, and finally if warranted an update to the [formal specification]. The EIP should be reviewed and accepted before an implementation is begun, unless an implementation will aid people in studying the EIP. Standards Track Core EIPs must be implemented in at least three viable Ethereum clients before it can be considered Final.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I don't see why this should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have re-added this sentence with 58d22b4. I have changed to "accepted as draft" to clarify which status this is.

EIPS/eip-1.md Outdated
The possible paths of the status of EIPs are as follows:

![EIP Process](../assets/eip-1/process.png)
An EIP in draft status must have implementations to be considered for promotion to the next status. When no further changes are planned and adove rules are met, the champion will request, and the editor will approve a change to "two week review" status. This EIP will listed prominently on the http://eips.ethereum.org/ website (subscribe via RSS here [TODO LATER, ADD A LINK HERE]). After the review period, if all issues are resolved, then the editor will change to final status. Or if issues remain then it may be returned to draft.
Copy link
Contributor

Choose a reason for hiding this comment

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

An EIP is listed on the website as soon as it is merged as a draft, after being accepted as final or accepted it will move to this category.

EIPS/eip-1.md Outdated
@@ -3,121 +3,108 @@ eip: 1
title: EIP Purpose and Guidelines
status: Active
type: Meta
author: Martin Becze <mb@ethereum.org>, Hudson Jameson <hudson@ethereum.org>
author: Martin Becze <mb@ethereum.org>, Hudson Jameson <hudson@ethereum.org>, William Entriken <github.com@phor.net>
Copy link
Contributor

Choose a reason for hiding this comment

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

There are actually 11 contributors to this; but listing all of them here would be verbose and take more time to update—better to let GitHub do that. You could just append [and others](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 0b4245e

EIPS/eip-1.md Outdated
[the Issues section of this repository]: https://github.com/ethereum/EIPs/issues
[markdown]: https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet
[Bitcoin's BIP-0001]: https://github.com/bitcoin/bips
[Python's PEP-0001]: https://www.python.org/dev/peps/
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see an advantage in removing the bullet points.

EIPS/eip-1.md Outdated
created: 2015-10-27, 2017-02-01
---

What is an EIP?
--------------
## What is an EIP?
Copy link
Contributor

Choose a reason for hiding this comment

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

It's also easier to prepend a heading with a couple of hashes than it is to use dashes on the next line (and line them up for presentation, and have to update them each time the heading is changed).

@fulldecent
Copy link
Contributor Author

Here is a formatting fix PR

#1112.

I can quickly rebase this PR if that is merged.

@fulldecent
Copy link
Contributor Author

Hello @jamesray1 @Arachnid and other interested reviewers. May I please have your feedback here. My goal is for this commit to simply add the two-week review ("Last Call") concept. There are a couple extra wording changes to correct mistakes in how EIP1 describes the current process.

I still expect much more discussion and improvements on the other items. But I hope this PR properly codifies the change that we have consensus on.

Please merge, and please let's keep this moving. If I've made a mistake, don't be shy, you are welcome to commit in my branch if that will help get it done.


I wont be a stranger and I will stick around for discussing the other stuff, the flow chart, and more. But right now I need your help to get this through, and mark ERC721 as Last Call. And once this is done I can get back to helping with Solidity 0.5.0! Thank you for you help.

EIPS/eip-1.md Outdated
@@ -3,7 +3,7 @@ eip: 1
title: EIP Purpose and Guidelines
status: Active
type: Meta
author: Martin Becze <mb@ethereum.org>, Hudson Jameson <hudson@ethereum.org>
author: Martin Becze <mb@ethereum.org>, Hudson Jameson <hudson@ethereum.org>, [and others](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I think this will break jekyll's postprocessing. If you want to keep it, can you check by running a local copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't render nicely. But it is not broken. Respectfully requesting to merge as is and improve this rendering later.

screen shot 2018-05-25 at 1 40 23 pm

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this is out of scope for this PR, but it would be nice to fix this rendering. Here is one option: https://stackoverflow.com/questions/28819413/add-link-to-yaml-in-multimarkdown-metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the link. P.S. presently your review status on this PR is denied. Is anything else outstanding to earn your approved vote?

Copy link
Contributor

@jamesray1 jamesray1 Jun 1, 2018

Choose a reason for hiding this comment

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

As a quick temporary fix, perhaps we could change this to:
..., and others:
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md

This would at least make it easier to copy and paste the link. I'm not sure how this renders, e.g. if newlines will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamesray1 Does this resolve all your issues? Will you please mark PR as approved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, but the change isn't showing above and that commit hash isn't in the history.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah the extra colon would have stuffed it up. If you put the whole value in quotes that should work, let me try that locally.

https://stackoverflow.com/questions/8783705/how-could-should-i-state-colon-punctuation-in-a-yaml-file

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having an issue with jekyll, which is unrelated to the change in the author filed, so I can't test this.

Copy link
Contributor Author

@fulldecent fulldecent Jun 2, 2018

Choose a reason for hiding this comment

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

Sorry about that, this time the push went through. Please repull.

EIPS/eip-1.md Outdated
The normal process is:

```
[ A pull request ] -> [ DRAFT ] -> [ LAST CALL ] -> [APPROVED] -> [ FINAL ]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still concerned this oversimplifies matters, especially since this only describes the process for non-core EIPs - the process for core EIPs is different.

Also, 'approved' isn't part of the non-core process; it goes straight from last call to final.

It's more like:

Core EIPs:

Draft -> Editor last call -> Approved(?) -> Accepted -> Final

Non-core EIPs:

Draft -> Last call -> Final

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that Last Call will cause a core EIP to go from this EIP process to the core dev meeting, where upon the best outcome is that it is approved. Then at some point it is deployed to a majority of clients.

The fact that EF deploys it to EF-supported clients seems immaterial from our EIP perspective. The time when 51% of clients are running it seems relevant from our perspective.

I am not trying to change things here or rock the boat. I am honestly trying to get Last Call in, which of course touches into these other processes.

Please help me understand how to fix this and get it done. Also, please give me a call so we can figure this out together.


I am not proposing this now, but another option is:

Non-core: [ DRAFT ] -> [ LAST CALL ] -> [ FINAL ]
Core:     [ DRAFT ] -> [ LAST CALL ] -> [ FINAL ] -> [ DEPLOYED ]

Where non-core has EIP editors making "final" decision. And Core has Core Devs making "final" decision. Then "deployed" is merely a review of 51% deployment (adjudicated by EIP editors). This is how I understand the process as actually happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that Last Call will cause a core EIP to go from this EIP process to the core dev meeting, where upon the best outcome is that it is approved. Then at some point it is deployed to a majority of clients.

The process for Core EIPs has always involved back-and-forward between the EIP process and core devs. Progressing to the 'Accepted' and 'Final' states require approval from All Core Devs, with 'Final' requiring that it's been implemented for a future hard fork.

I don't see anything in this PR that changes that, so it's important that any proposed diagrams adequately describe the actual process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using "accepted as draft", clearly that wording doesn't make sense if "accepted" is a status too.

I thought the core devs only made the decision to implement. I thought the users decided to upgrade and the EIP editors to recognize when that upgrade was deployed.

It is not clear to me how to update my wording to fix it and get this accepted. It is also not clear to me how the process actually works or how it is written on paper.

This is why I asked for a phone call. I am documenting these things and I can make it great I just need to understand it first for myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One issue is your note "closing PRs as soon as possible by either merging or rejecting them" -- this does not match the diagram.

I want to understand this fully but all the questions and answers here are piecemeal. If I understand it completely I can write it well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am using "accepted as draft", clearly that wording doesn't make sense if "accepted" is a status too.

The status you want is just 'Draft', not 'Accepted as Draft'.

I thought the core devs only made the decision to implement. I thought the users decided to upgrade and the EIP editors to recognize when that upgrade was deployed.

For Core EIPs, the devs decide to implement, and when to configure a hard fork including the changes. If that hard fork is successful, then the state changes to Final.

It is not clear to me how to update my wording to fix it and get this accepted. It is also not clear to me how the process actually works or how it is written on paper.

I already provided a suggestion for an alternate wording; what was wrong with that?

One issue is your note "closing PRs as soon as possible by either merging or rejecting them" -- this does not match the diagram.

The diagram shows the states that EIPs can be in, not the process for updating them. A PR is how you update the state or contents of an EIP.

EIPS/eip-1.md Outdated

Once the champion has asked the Ethereum community whether an idea has any chance of acceptance a draft EIP should be presented as a [pull request].

If the EIP editors approve the EIP ([see the EIP editors workflow section below for details](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md#eip-editor-responsibilities-and-workflow)) and the authors are happy for it to be merged as a draft, the EIP editor will assign the EIP a number (generally the issue or PR number related to the EIP) and merge your pull request. The EIP editor will not unreasonably deny an EIP. (EIP authors can request for it to be merged after they have finished editing it, and editors can ask if they have finished editing it and would like it to be merged. This prevents merging an EIP when more edits are intended to be made by the author, although it can always be edited after merging.) Reasons for denying EIP status include duplication of effort, being technically unsound, not providing proper motivation or addressing backwards compatibility, or not in keeping with the [Ethereum philosophy](https://github.com/ethereum/wiki/wiki/White-Paper#philosophy).
EIP authors can request for the pull request to be merged as Draft when significant progress is made. Remember, Drafts can always be edited after merging. If the EIP editors accept the EIP as a draft ([see the EIP editors workflow section below for details](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md#eip-editor-responsibilities-and-workflow)), the EIP editor will assign the EIP a number (generally the issue or PR number related to the EIP) and merge your pull request. The EIP editor will not unreasonably deny an EIP. Reasons for denying EIP status include duplication of effort, being technically unsound, not providing proper motivation or addressing backwards compatibility, or not in keeping with the [Ethereum philosophy](https://github.com/ethereum/wiki/wiki/White-Paper#philosophy).
Copy link
Contributor

Choose a reason for hiding this comment

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

The intended workflow is for PRs to be promptly either accepted, rejected, or have alterations requested. I'd rather not normalise a process where people open 'WIP' PRs and leave them that way indefinitely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently is seems that the default action is getting to draft and keeping it open. We should change that. But I argue that this text above is accurate. Please let's merge this and then correct the process with a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are changing that, and I've already moved to closing PRs as soon as possible by either merging or rejecting them. Enshrining the previous process in EIP 1 would be a step backwards.

EIPS/eip-1.md Outdated
An EIP can also be assigned status “Deferred”. The EIP author or editor can assign the EIP this status when no progress is being made on the EIP. Once an EIP is deferred, the EIP editor can re-assign it to draft status.

An EIP can also be “Rejected”. Perhaps after all is said and done it was not a good idea. It is still important to have a record of this fact.
An EIP in draft status must have implementations to be considered for promotion to the next status. When no further changes are planned and adove rules are met, the champion will request, and the editor will approve a change to "Last Call" status. This EIP will listed prominently on the http://eips.ethereum.org/ website (subscribe via RSS here [TODO LATER, ADD A LINK HERE]). After the review period, if all issues are resolved, then the editor will proceed to the next status; if issues remain then it may be returned to draft.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put any TODOs in comments so they won't show up for readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RSS feed coming in a sec...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RSS feed added with d0735df

EIPS/eip-1.md Outdated
![EIP Process](../assets/eip-1/process.png)

Some Informational and Process EIPs may also have a status of “Active” if they are never meant to be completed, e.g. EIP 1 (this EIP).
Other exceptional outcomes for EIPS include "deferred", a temporary status for drafts which have no recent progress, and "rejected" which records an EIP that is fundamentally broken and will not be implemented. Also, "active" denotes an EIP that is in effect but which may be updated without changing its EIP number. A "final" EIP should be updated on to correct errata, although another EIP may wish to "supersede" it — in that case you would have an original and succesor EIP both in final status.
Copy link
Contributor

Choose a reason for hiding this comment

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

'Deferred' is for core EIPs that have been put off for a future hard fork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, updated in 50ae6ac

@fulldecent
Copy link
Contributor Author

@Arachnid Added the RSS link, this is critical for transparency.

screen shot 2018-05-25 at 2 14 55 pm
screen shot 2018-05-25 at 2 15 14 pm
screen shot 2018-05-25 at 2 20 02 pm

Updating #721 to be Last Call status is out of scope of this PR. That is addressed in #1101. But I am showing in this screenshot so you can see it works.

@fulldecent
Copy link
Contributor Author

Hello @jamesray1 @Arachnid and other interested reviewers. Thank you for your latest feedback. I believe I have addressed all open issues. One exception may be a wording/status flow for Accepted/Approved from @Arachnid. Again, please call me on the phone -- I think we all want to get this done. And of course I will ask about #1101 right after if you can merge this.

Also in the past few hours:

  • Added an RSS feed for Last Call. This is the golden nugget and how we avoid low quality standards.

Thank you.

@fulldecent
Copy link
Contributor Author

Hello editors:

Nick Johnson (@Arachnid)
Casey Detrio (@cdetrio)
Hudson Jameson (@Souptacular)
Vitalik Buterin (@vbuterin)
Nick Savers (@nicksavers)
Martin Becze (@wanderer)


I believe all discussion has been had and all defects are addressed in this pull request. I am asking for an approver and a committer to put this PR through.

This PR adds a two-week review step ("Last Call") to the EIP process. Anybody can subscribe to the last call RSS feed (implemented in this PR) so they can comment before an EIP becomes "final". This is a minor change which I believe has consensus. There are other things which we can fix in EIP and the discussion will be hearty. But specifically, this is the least contentious part, so let's get this done first.

Please merge. And then next, please put ERC-721 through this two-week process. The PR to do that is #1101. We will learn, increment, and keep breaking and improving things.

Thanks for your help. And again, if you have questions CALL ME, you should have/can get my number.

EIPS/eip-1.md Outdated
The normal process is:

```
[ A pull request ] -> [ DRAFT ] -> [ LAST CALL ] -> [APPROVED] -> [ FINAL ]
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that Last Call will cause a core EIP to go from this EIP process to the core dev meeting, where upon the best outcome is that it is approved. Then at some point it is deployed to a majority of clients.

The process for Core EIPs has always involved back-and-forward between the EIP process and core devs. Progressing to the 'Accepted' and 'Final' states require approval from All Core Devs, with 'Final' requiring that it's been implemented for a future hard fork.

I don't see anything in this PR that changes that, so it's important that any proposed diagrams adequately describe the actual process.

index.html Outdated
@@ -3,7 +3,10 @@
title: Home
---

<h1 class="page-heading">EIPs <a href="https://gitter.im/ethereum/EIPs?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge"><img src="https://badges.gitter.im/Join%20Chat.svg" alt="Gitter"></a></h1>
<h1 class="page-heading">EIPs
<a href="https://gitter.im/ethereum/EIPs?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge"><img src="https://badges.gitter.im/Join%20Chat.svg" alt="Gitter"></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're editing this, would you mind replacing the & with &amp; here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected in f6a321c

last-call.xml Outdated
<rss version="2.0" xmlns:atom="http://www.w3.org/2005/Atom">
<channel>
<title>Ethereum EIPs Last Call Review</title>
<description>All EIPs which are in the two-week "last call" status, please help review these and provid your feedback!</description>
Copy link
Contributor

Choose a reason for hiding this comment

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

s/provid/provide/

last-call.xml Outdated
<description>All EIPs which are in the two-week "last call" status, please help review these and provid your feedback!</description>
<link>{{ site.url }}</link>
<atom:link href="{{ site.url }}/last-call.xml" rel="self" type="application/rss+xml" />
{% assign eips = site.pages|where:"status","Last Call"|sort:"eip" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This sort order may result in RSS/Atom readers missing any old EIPs that have just entered last call. I'm not sure if there's a way around it, though, unless Jekyll supports reading modification times of files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The correct solution is to sort based on Last Call review date, which is in the front matter.

Some solution might involve changing the way we put the dates into the front matter.

But I hope we can address that after #1100 is merged.

@@ -0,0 +1,22 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this!

@fulldecent
Copy link
Contributor Author

@Arachnid Per your request, PR is now the preferred mechanism to request status changes, see 98cb10d

Copy link
Contributor

@Arachnid Arachnid left a comment

Choose a reason for hiding this comment

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

I'm happy with this. What do other editors think? @Souptacular? @nicksavers?

Copy link
Contributor

@Souptacular Souptacular left a comment

Choose a reason for hiding this comment

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

lgtm

@fulldecent
Copy link
Contributor Author

Right now we have + @Arachnid, and + @Souptacular, and also all @jamesray1 issues are addressed.

Is this sufficient to merge? If yes, please do. If no, please explain the remaining endorsements required.

Thank you.

@fulldecent
Copy link
Contributor Author

@Souptacular Thank you for your endorsement. Will you please merge?

Copy link
Contributor

@nicksavers nicksavers left a comment

Choose a reason for hiding this comment

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

Clarifies a lot. The descriptions are good enough for me. And it provides a clearer, hopefully less ambiguous path towards Final.

@@ -3,7 +3,8 @@ eip: 1
title: EIP Purpose and Guidelines
status: Active
type: Meta
author: Martin Becze <mb@ethereum.org>, Hudson Jameson <hudson@ethereum.org>
author: Martin Becze <mb@ethereum.org>, Hudson Jameson <hudson@ethereum.org>, and others
Copy link
Contributor

@jamesray1 jamesray1 Jun 2, 2018

Choose a reason for hiding this comment

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

Please change this to:

author: "Martin Becze <mb@ethereum.org>, Hudson Jameson <hudson@ethereum.org>, and others:  
        https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md"

Testing that locally, it doesn't render the newline since they are stripped, but it does show the colon. We could fix up the rendering of the link and newline later after this is merged. Although I noticed that you managed to get the newline to render, so if you know how to fix that, please do so.

Copy link
Contributor

@jamesray1 jamesray1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Arachnid Arachnid merged commit e8b64f6 into ethereum:master Jun 4, 2018
@fulldecent
Copy link
Contributor Author

Awesome, thanks Nick!

@shrugs
Copy link

shrugs commented Jun 4, 2018

@fulldecent should the status variable of ERC721's metadata be Two week review or Last Call? As it is, it doesn't show up in https://eips.ethereum.org/last-call.xml because of https://github.com/ethereum/EIPs/pull/1100/files#diff-ad82c3bf887504d13862da41609f9632R11

@fulldecent
Copy link
Contributor Author

@shrugs It should be Last Call. I am seeing it in the XML. I am checking with

curl https://eips.ethereum.org/last-call.xml

@shrugs
Copy link

shrugs commented Jun 5, 2018

Ah, so sorry, my software was folding that whole item into a single line and I didn't see it at all. Cheers

@jamesray1
Copy link
Contributor

jamesray1 commented Jun 5, 2018

@fulldecent, I would say that @Arachnid did not merge this too hastily! :Chuckles:

@jamesray1
Copy link
Contributor

Although, @elenadimitrova's flowchart wasn't included, could someone make another PR for that?

Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Jul 19, 2018
* Add new two-week review process to EIPs

* Add ACCEPTED status, thanks @Arachnid

* Use last call, thanks @Arachnid

* Add other authors

* Re-add "request to merge"

* Add accepted as draft

* Match statuses to words used in text

* Match whitespace

* Add last call RSS

* add RSS link to EIP1

* Update deferred wording

* Provide

* "EIP authors can request"

* Correct HTML error

* review-period last date only

* Briefer review end date name

* alse

* Fully document statuses and transitions

* One implementation for draft

* Focus on the goal

* Use prior definition of final

* Use Accepted

* Use Accepted

* PR is the preferred mechanism to request status changes

* hide markdown formatting
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.

9 participants