-
Notifications
You must be signed in to change notification settings - Fork 392
enhance(ci): Add EIP Tracker Issue Template #1847
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
base: forks/osaka
Are you sure you want to change the base?
Changes from 2 commits
4852cfe
75b7ae0
abf6055
691545c
000279d
1d0d104
7d28961
c19ad02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,79 @@ | ||||||||||||||||||||||||
| name: "EIP Implementation Tracker" | ||||||||||||||||||||||||
| description: "Track specification and testing progress for an EIP" | ||||||||||||||||||||||||
| title: "EIP ${{ form.eip_number }} Progress Tracker" | ||||||||||||||||||||||||
| labels: | ||||||||||||||||||||||||
| - A-spec-specs | ||||||||||||||||||||||||
| - A-spec-tests | ||||||||||||||||||||||||
| - C-eip | ||||||||||||||||||||||||
| - C-test | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| body: | ||||||||||||||||||||||||
| - type: input | ||||||||||||||||||||||||
| id: eip_number | ||||||||||||||||||||||||
| attributes: | ||||||||||||||||||||||||
| label: "EIP Number" | ||||||||||||||||||||||||
| description: "Enter the EIP number (digits only)." | ||||||||||||||||||||||||
| placeholder: "e.g., 8024" | ||||||||||||||||||||||||
| validations: | ||||||||||||||||||||||||
| required: true | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - type: input | ||||||||||||||||||||||||
| id: eip_title | ||||||||||||||||||||||||
| attributes: | ||||||||||||||||||||||||
| label: "EIP Title" | ||||||||||||||||||||||||
| description: "Copy the title from the EIP." | ||||||||||||||||||||||||
| placeholder: "e.g., Backwards compatible SWAPN, DUPN, EXCHANGE" | ||||||||||||||||||||||||
| validations: | ||||||||||||||||||||||||
| required: true | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - type: dropdown | ||||||||||||||||||||||||
| id: fork | ||||||||||||||||||||||||
| attributes: | ||||||||||||||||||||||||
| label: "Fork" | ||||||||||||||||||||||||
| description: | | ||||||||||||||||||||||||
| Specify the target fork **only if the EIP has reached the CFI stage**. | ||||||||||||||||||||||||
| More info: https://eips.ethereum.org/EIPS/eip-7723#considered-for-inclusion | ||||||||||||||||||||||||
| options: | ||||||||||||||||||||||||
| - TBD | ||||||||||||||||||||||||
| - amsterdam | ||||||||||||||||||||||||
| - bogota | ||||||||||||||||||||||||
| validations: | ||||||||||||||||||||||||
| required: true | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - type: markdown | ||||||||||||||||||||||||
| attributes: | ||||||||||||||||||||||||
| value: | | ||||||||||||||||||||||||
| ## EIP ${{ form.eip_number }}: ${{ form.eip_title }} | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| **Link:** https://eips.ethereum.org/EIPS/eip-${{ form.eip_number }} | ||||||||||||||||||||||||
marioevz marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ## Target Fork | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Fork **${{ form.fork }}** | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - [ ] Add issue to fork milestone (if applicable). | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ## Ownership | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Owner(s): **TBD** | ||||||||||||||||||||||||
marioevz marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| **Important note:** A specifications specialist and a testing specialist should ideally share ownership of the EIP. | ||||||||||||||||||||||||
marioevz marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ## Specification + Testing Status | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - [ ] Testing complexity assessed and documented. | ||||||||||||||||||||||||
| - [ ] Specification implementation merged to `eips/${{ form.fork }}/eip-${{ form.eip_number }}`. | ||||||||||||||||||||||||
| - [ ] Specification updates merged to the corresponding `forks/${{ form.fork }}` branch. | ||||||||||||||||||||||||
| - [ ] Required testing framework modifications implemented. | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| - [ ] Specification implementation merged to `eips/${{ form.fork }}/eip-${{ form.eip_number }}`. | |
| - [ ] Specification updates merged to the corresponding `forks/${{ form.fork }}` branch. | |
| - [ ] Required testing framework modifications implemented. | |
| - [ ] Specification implementation merged to `eips/${{ form.fork }}/eip-${{ form.eip_number }}`. | |
| - [ ] Specification updates merged to the corresponding `forks/${{ form.fork }}` branch. | |
| - [ ] Important architectural implementation choices documented | |
| - [ ] Required testing framework modifications implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I paraphrased a bit:
EIP updates proposed in case of architectural choices surfaced during implementation.
I used the word "proposed" here because we cannot guarantee a change goes into the EIP (technically, because most of the times the EIP process welcomes implementation feedback very well), just to not imply that we have to try to force this because it's outside of the EIP process.
marioevz marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - [ ] Testing checklist complete | |
| (https://github.com/ethereum/execution-specs/blob/HEAD/docs/writing_tests/checklist_templates/eip_testing_checklist_template.md). | |
| - [ ] [Testing checklist](https://github.com/ethereum/execution-specs/blob/HEAD/docs/writing_tests/checklist_templates/eip_testing_checklist_template.md) complete | |
| . |
marioevz marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to make this more granular.
| - [ ] Hive tests passing on all implementations. | |
| - [ ] Hive tests passing on all implementations: | |
| | Client | Engine | RLP | Sync | | |
| |-------------|--------|-----|------| | |
| | Geth | [ ] | [ ] | [ ] | | |
| | Nethermind | [ ] | [ ] | [ ] | | |
| | Besu | [ ] | [ ] | [ ] | | |
| | Erigon | [ ] | [ ] | [ ] | | |
| | Reth | [ ] | [ ] | [ ] | | |
| | Nimbus-EL | [ ] | [ ] | [ ] | | |
| | Ethrex | [ ] | [ ] | [ ] | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is too much, we could instead create another template.
But even this single bullet-point I'm not completely convinced we should put it in here because it's such a fluctuating state. E.g. we could introduce new tests that break implementations, or a client might have a regression. Do either of those mean we have to come back here and uncheck this checkbox?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about it, I feel we can make this point a bit more flexible:
Hive tests passing on at least two implementations.
Two implementations feels to me like the right balance to mark this as complete. And reasoning is:
- Removing this requirement altogether would be prone to error in our specs/tests. Simply because sometimes the writing of the EIP can lead to multiple interpretations, and these are only surfaced after clients consume the generated tests.
- Requiring only a single passing implementation is also too few since many times the second implementation brings better confirmation that the specs are correctly implemented.
- While itβs true that a third or fourth implementation can still uncover issues, the problems discovered at that stage are beyond the EIP at that point, rather they are part of the completion of the fork itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with "at least two" I think. Are we talking about a full implementation here where we believe the specs are now correct, we have enough tests for coverage, and there are at least two clients passing all of the tests? Or is this still within the development cycle?
It will be the case where specs are implemented, some clients implement it as well, and you start going back and forth on a consensus of what is implemented correctly and what is not. Then you tweak the specs and tests, tweak the client implementations, etc... so...
- At what point in the process are we here?
- With late additions to the EIP do we then come back here and uncheck this until it's back to being implemented, etc? I imagine this is how it works but just curious.
Also, if we go with this approach, we should have a section that tracks which clients have implemented it so we can signal this and ideally we should link to the branch in the relevant repos.
Uh oh!
There was an error while loading. Please reload this page.