ethereum / ethereum/execution-specs
refactor(tests): Refactor system-request test helpers
- Dominant language
- Python
- Stars
- 1.2k
- Forks
- 505
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 116
Description
# Refactor system-request test helpers (EIPs 6110, 7002, 7251)
## Summary
The test helpers for the three system-contract EIPs under `tests/prague/`:
- `tests/prague/eip6110_deposits/helpers.py`
- `tests/prague/eip7002_el_triggerable_withdrawals/helpers.py`
- `tests/prague/eip7251_consolidations/helpers.py`
share two issues that are worth addressing together:
1. The request descriptor classes expose a `gas_limit` field that does not
belong on a "request" abstraction and only serves a handful of
gas/out-of-gas tests.
2. The three `helpers.py` files are near-identical, so there is a large
amount of duplication that could be reduced.
This was spotted while working on #2969, whose goal
is to remove hard-coded transaction `gas_limit`s from tests that are *not*
specifically exercising gas behavior, and instead let the gas limit be
resolved implicitly. The system-request helpers are an obstacle to that effort.
## Problem 1: `gas_limit` does not belong on the request descriptor
`DepositRequest`, `WithdrawalRequest`, and `ConsolidationRequest` are
descriptors of a *system-layer request* (validator pubkey, amount, fee,
target, validity, etc.). They each carry a `gas_limit: int | None` field that
is plumbed down into the `Transaction` they generate:
```python
# helpers.py — repeated in all three folders
gas_limit = request.gas_limit
if gas_limit is not None:
tx = Transaction(gas_limit=request.gas_limit, to=..., ...)
else:
tx = Transaction(to=..., ...)
```
and into the relay contract's call when the request comes from a contract:
```python
self.call_type(
Op.GAS if r.gas_limit is None else r.gas_limit,
...
)
```
Conceptually, the gas limit of the *transaction/call* is not a property of the
*request*. Mixing it in:
- Conflates the request descriptor with gas-specific test concerns.
- Forces the implicit-gas-limit work to special-case these helpers.
- Is only used by a small number of out-of-gas tests, which hard-code magic
numbers obtained from traces, e.g.:
```python
# tests/prague/eip7002_el_triggerable_withdrawals/test_withdrawal_requests.py
gas_limit=114_247 - 1, # "Value obtained from trace minus one"
gas_limit=80_047 - 1, # "Value obtained from trace minus one"
# tests/prague/eip6110_deposits/test_deposits.py
gas_limit=0x1431D,
gas_limit=0x10BF1,
# tests/prague/eip7251_consolidations/test_consolidations.py
gas_limit=136_534 - 1,
gas_limit=102_334 - 1,
```
These constants are brittle (any change to intrinsic/opcode gas costs silently
invalidates them) and the intent ("just under the gas needed to succeed") is
buried in a literal.
### Proposal
- Remove the `gas_limit` field from the request descriptor classes (and the
`gas_limit is None` branching in `transactions()` / `contract_code`).
- Move the out-of-gas / gas-boundary scenarios into dedicated test
function(s) that build the transaction with an explicit, *computed* gas
limit — derived from the EVM bytecode / intrinsic gas (e.g. cost of the
call data + the predeploy call) rather than a trace-derived literal.
- This lets all the non-gas request tests rely on the implicit gas limit,
unblocking the `implicit-gas-limit` refactor.
## Problem 2: duplication across the three helpers
The three `helpers.py` files are structurally identical:
- A `*Request` subclass adding `fee` / `valid` / `gas_limit` /
`calldata_modifier`, a `value` property, a `calldata` property, and
`with_source_address(...)`.
- A `*InteractionBase` dataclass (`sender_account`, `requests`,
`transactions()`, `update_pre()`, `valid_requests()`).
- A `*Transaction` subclass (EOA-originated) — byte-for-byte the same logic.
- A `*Contract` subclass (contract-originated) with `contract_balance`,
`contract_address`, `entry_address`, `call_type`, `call_depth`,
`extra_code`, `contract_code`, and the `call_depth > 2` relay-deployment
loop — essentially identical across the three.
- `get_n_fee_increments(n)` and `get_n_fee_increment_blocks(n)` — identical
apart from the per-EIP `TARGET_*_REQUESTS_PER_BLOCK` constant and the
request fields.
### Proposal
Factor the common structure into a shared base (e.g. a generic
`SystemContractRequestInteraction` base plus shared `*Transaction` /
`*Contract` mixins and a shared fee-increment helper), parameterized by the
per-EIP specifics (request type, predeploy address, target-per-block
constant, calldata layout). Each EIP module would then only declare what is
genuinely different.
> Note: this duplication is within a single fork (`tests/prague`), so reducing
> it does **not** conflict with the WET "complete copy per fork" rule that
> applies to `src/ethereum/forks/`.
## Scope / acceptance
- [ ] `gas_limit` removed from the request descriptor classes.
- [ ] Out-of-gas tests rewritten to compute the gas limit explicitly in a
dedicated helper/test function (no trace-derived magic numbers).
- [ ] Non-gas request tests rely on the implicit gas limit.
- [ ] Common helper code consolidated into a shared base, with each EIP
module declaring only its differences.
- [ ] Generated fixtures for EIP-6110 / 7002 / 7251 unchanged (or changes
explained and justified).
Contributor guide
Assessment
This issue has not been assessed yet.