enhance(ci): Add EIP Tracker Issue Template #1847
Conversation
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/osaka #1847 +/- ##
============================================
Coverage 87.31% 87.31%
============================================
Files 541 541
Lines 32832 32832
Branches 3015 3015
============================================
Hits 28668 28668
Misses 3557 3557
Partials 607 607
Flags with carried forward coverage won't be shown. Click here to find out more. β View full report in Codecov by Sentry. π New features to boost your workflow:
|
Carsons-Eels
left a comment
Carsons-Eels
left a comment
There was a problem hiding this comment.
Some initial thoughts/suggestions
| - [ ] 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. |
There was a problem hiding this comment.
I think it's important to document when we, either on our own or as a team, have had to make decisions about the architecture of an implementation which are not obvious/straightforward and explain why those choices were made. I'm thinking of cases where we have structurally altered the implementation due to code review comments or there were multiple good options for implementation.
| - [ ] 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.
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.
| - [ ] Testing checklist complete | ||
| (https://github.com/ethereum/execution-specs/blob/HEAD/docs/writing_tests/checklist_templates/eip_testing_checklist_template.md). |
There was a problem hiding this comment.
| - [ ] 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 | |
| . |
spencer-tb
left a comment
spencer-tb
left a comment
There was a problem hiding this comment.
I think adding a phase 1 (CFI'd) & phase 2 (SFI'd) to the Process Status section could be nice! Phase 1 would be for initial tests, and included in the devnet. Phase 2 would be for. spec/test coverage reviewed by 2 EELS / 2 EEST / 1 EIP author, extra tests added and passing, EIP successful in first testnet.
We could add a regressions section to. For significant changes, whether there are issues that come up during a devnet, missed test coverage or even whether the EIP is pulled out of the fork (and the reasons why).
Let me know what you think! Happy to merge now and get the process going nonetheless.
|
|
||
| ## Process Status | ||
|
|
||
| - [ ] Hive tests passing on all implementations. |
There was a problem hiding this comment.
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.
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.
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.
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.
Co-authored-by: Carson <[email protected]>
Co-authored-by: spencer <[email protected]>
Co-authored-by: Carson <[email protected]>
Co-authored-by: spencer <[email protected]>
| > [!IMPORTANT] | ||
| > A specifications specialist and a testing specialist should ideally share ownership of the EIP. | ||
|
|
||
| - [ ] Add the issue to the target fork milestone if applicable (i.e., the EIP is at least in the (CFI stage)[https://eips.ethereum.org/EIPS/eip-7723#considered-for-inclusion]). |
There was a problem hiding this comment.
broken link styling I think
| - [ ] Add the issue to the target fork milestone if applicable (i.e., the EIP is at least in the (CFI stage)[https://eips.ethereum.org/EIPS/eip-7723#considered-for-inclusion]). | |
| - [ ] Add the issue to the target fork milestone if applicable (i.e., the EIP is at least in the [CFI stage](https://eips.ethereum.org/EIPS/eip-7723#considered-for-inclusion)). |
|
|
||
| ## Process Status | ||
|
|
||
| - [ ] Hive tests passing on all implementations. |
There was a problem hiding this comment.
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.
4 participants
ποΈ Description
Adds a tracker issue template for EIPs to help better monitor the specification and testing progress.
π Related Issues or PRs
N/A.
β Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture