-
Notifications
You must be signed in to change notification settings - Fork 415
feat(spec,test): Update EIP-7708 coinbase expectation based on latest spec #2106
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
feat(spec,test): Update EIP-7708 coinbase expectation based on latest spec #2106
Conversation
- Add CREATE2 support via with_all_create_opcodes marker - Add tests for SELFDESTRUCT to coinbase - revealed a change needed since the last update was made to the EIP that should be included in the currect refspec (miner fees paid before finalization LOG2).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## eips/amsterdam/eip-7708 #2106 +/- ##
===========================================================
- Coverage 86.14% 84.14% -2.01%
===========================================================
Files 599 642 +43
Lines 39491 42267 +2776
Branches 3782 4066 +284
===========================================================
+ Hits 34021 35566 +1545
- Misses 4848 5971 +1123
- Partials 622 730 +108
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
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 think this looks great, my only suggestion would be to add a test which validates that the amount in the finalization log includes the priority fee calculation. I don't see that as a reason to block this merge though
| ) | ||
|
|
||
| # EIP-7708: Emit burn logs for balances held by accounts marked for | ||
| # deletion AFTER miner fee transfer. |
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.
| # deletion AFTER miner fee transfer. | |
| # deletion AFTER miner fee transfer. (EIPs#11190 clarification) |
The finalization balance is included in the log that's checked in the expected receipt. Both tests have this check, one for priority fee by itself (not funded after ETH burn) and one for priority fee + funded amount 👌🏼. |
3eb3161
into
ethereum:eips/amsterdam/eip-7708
🗒️ Description
Recent spec changes were updated to emit finalization LOG2 after EIP-1559 fees are paid to coinbase.
This PR corrects the spec and adds a correctly failing test before this spec change and passes with the spec change. Tests that when the self-destructing (in same tx) address is also the
coinbase, it receives the fees from thesenderbefore the log and thus the log amount contains these fees. We have two test cases:coinbaseis created, funded (LOG3), and destroyed in same tx (LOG2 shows all eth is burned)coinbasereceives miner fees fromsendercoinbaseis created, funded (LOG3) and destroyed in same tx (LOG2 shows all eth is burned)coinbaseis funded by an outside account self-destructing to it (LOG3)coinbasereceives miner fees fromsender🔗 Related Issues or PRs
✅ 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.Cute Animal Picture