Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## eips/amsterdam/eip-8282 #2990 +/- ##
===========================================================
+ Coverage 81.28% 90.52% +9.24%
===========================================================
Files 620 535 -85
Lines 36643 32907 -3736
Branches 3311 3024 -287
===========================================================
+ Hits 29786 29790 +4
+ Misses 6335 2595 -3740
Partials 522 522
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| type: ClassVar[int] = 3 | ||
| """Placeholder request-type byte pending the EIP-8282 final allocation.""" |
There was a problem hiding this comment.
Should this be Final instead of ClassVar?
There was a problem hiding this comment.
I agree that ClassVar leaves the field vulnerable to modifications, but I found at least one pydantic open issue which mentions that the behavior could be updated to the final field being considered an actual field in the instantiated object.
Reasoning being that they want to match the dataclass behavior https://typing.python.org/en/latest/spec/dataclasses.html#dataclass-semantics.
So I think we should keep using ClassVar for now IMO.
There was a problem hiding this comment.
That's a really weird divergence.
In any case, I think ClassVar[Final[int]] would be the correct annotation (source)?
| interaction_contract_address: ClassVar[Address] = Address( | ||
| Spec.BUILDER_DEPOSIT_CONTRACT_ADDRESS | ||
| ) | ||
| min_fee: ClassVar[int] = Spec.MIN_REQUEST_FEE |
There was a problem hiding this comment.
Recommending Final where possible again.
There was a problem hiding this comment.
See response to previous comment.
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
|
Thanks for the review @SamWilsn! I believe all the comments have been addressed, let me know if the PR is ok. |
There was a problem hiding this comment.
Hey, @marioevz, will you still add oog tests here? I saw in
- refactor(tests): generalize system-contract request test helpers (EIP-6110/7002/7251/7685) #2989
you refactor the 6110 deposits oog tests to their own dedicated module.
There was a problem hiding this comment.
Does anyone review how eip-8038 affects system contract interaction? I remember there were some discussion but could not find where it is.
| """ | ||
| Return the minimum gas limit for the block to be considered valid. | ||
| """ | ||
| minimum_block_gas_limit = 5_000 |
There was a problem hiding this comment.
nit: could we change it to GasCosts.LIMIT_MINIMUM
| call_value = builder_deposit_request.value | ||
| case Address(Spec8282.BUILDER_EXIT_CONTRACT_ADDRESS): | ||
| # Fabricate a valid builder exit request to the set-code account | ||
| builder_deposit_request = BuilderExitRequest.from_index(0) |
There was a problem hiding this comment.
| builder_deposit_request = BuilderExitRequest.from_index(0) | |
| builder_exit_request = BuilderExitRequest.from_index(0) |
34f05ea
into
ethereum:eips/amsterdam/eip-8282
…: ignore, Spec dataclass) (#3018) * refactor(test-forks): Use pkgutil to load contract binaries * refactor(test-forks): Remove pre_allocation_blockchain type: ignore comments * chore(tests): Remove `dataclass` from `Spec` classes
* feat(specs): Implement EIP-8282 * feat(test-forks): Implement EIP-8282 framework changes * feat(tests): Implement EIP-8282 tests * feat(tests): Add EIP-8282 to EIP-7685 tests * fix(test-forks): Update EIP-8282 deposit contract to 537b9c1 * fix(tests): Update EIP-8282 builder deposit contract target/max to 32/256 * fix(tests): Failing tests after update * refactor(test-forks): Introduce `minimum_block_gas_limit` * refactor(test-forks): Update EIP-8282 `empty_block_bal_item_count` * refactor(test-forks): Use pkgutil to load contract binaries * refactor(test-forks): Use pkgutil to load EIP-8282 contract binaries * refactor(test-forks): Remove pre_allocation_blockchain type: ignore comments * refactor(test-forks): Remove EIP-8282 pre_allocation_blockchain type: ignore comments * Update tests/amsterdam/eip8282_builder_execution_requests/__init__.py Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> * Update tests/amsterdam/eip8282_builder_execution_requests/spec.py Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> * chore(tests): Remove `dataclass` from `Spec` class EIP-8282 * chore(tests): Make test behavior more explicit Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> * chore(spec,tests): set EIP-8282 builder addresses for glamsterdam-devnet-6 --------- Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Co-authored-by: spencer-tb <spencer.tb@ethereum.org>
* feat(spec,tests): Implement EIP-8282 (#2990) * feat(specs): Implement EIP-8282 * feat(test-forks): Implement EIP-8282 framework changes * feat(tests): Implement EIP-8282 tests * feat(tests): Add EIP-8282 to EIP-7685 tests * fix(test-forks): Update EIP-8282 deposit contract to 537b9c1 * fix(tests): Update EIP-8282 builder deposit contract target/max to 32/256 * fix(tests): Failing tests after update * refactor(test-forks): Introduce `minimum_block_gas_limit` * refactor(test-forks): Update EIP-8282 `empty_block_bal_item_count` * refactor(test-forks): Use pkgutil to load contract binaries * refactor(test-forks): Use pkgutil to load EIP-8282 contract binaries * refactor(test-forks): Remove pre_allocation_blockchain type: ignore comments * refactor(test-forks): Remove EIP-8282 pre_allocation_blockchain type: ignore comments * Update tests/amsterdam/eip8282_builder_execution_requests/__init__.py Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> * Update tests/amsterdam/eip8282_builder_execution_requests/spec.py Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> * chore(tests): Remove `dataclass` from `Spec` class EIP-8282 * chore(tests): Make test behavior more explicit Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> * chore(spec,tests): set EIP-8282 builder addresses for glamsterdam-devnet-6 --------- Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Co-authored-by: spencer-tb <spencer.tb@ethereum.org> * fix(test-forks): Match EIP contract names * fix(test-plugins): Add eth_config unit test * feat(tests): Mirror EIP-7002/7251 tests * feat(test-types): Implement `SystemContractInteractionMeasuredOutOfGasContract` * fix(tests): Use `SystemContractInteractionMeasuredOutOfGasContract` * fix(test-forks): Lint * fix(tests): Improve modified contract tests * fix(tests): EIP-8282 builder request test fixes (#3090) * fix(tests): exceed per-block max in builder deposit carry-over test * chore(tests): clarify multi-type request test gas limit comment --------- Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Co-authored-by: spencer-tb <spencer.tb@ethereum.org> Co-authored-by: spencer <spencer.taylor-brown@ethereum.org> Co-authored-by: LouisTsai <q1030176@gmail.com>
🗒️ Description
Implements the current spec iteration of this PR: ethereum/EIPs#11760
🔗 Related Issues or PRs
N/A.
✅ Checklist
just 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