enhance-ported-test

# Enhance Ported Test Future-proof and clean up a test under `tests/ported_static/`. These tests were machine-ported from the legacy `ethereum/tests` static fillers (YAML/JSON) and carry a lot of boilerplate, hardcoded values, and weak/incomplete post-state checks. This skill is the ordered methodology for turning one into idiomatic, robust Python. This skill is a **living document**: it captures the cases we have validated so far. Real tests will hit shapes not covered here — that is expected. When you find one, solve it, then add the new case/step to this file. ## Goal The end state is a test that **passes on every fork from its `valid_from` onward** (not just the baseline), expresses its intent explicitly, and has no fragile hardcoded constants. "Future-proof" = a later fork that re-prices gas, adds state costs, or changes account rules should not silently break it. ## Core loop (subtractive) Most of the work is **removing** boilerplate one piece at a time and proving the test still passes after each removal: 1. **Baseline first.** Before touching anything, fill the test and confirm it is green: `uv run fill <path> --fork=<valid_from-fork> -q --clean`. 2. Make **one** change. 3. Fill again (same fast command). Green → keep, move on. 4. **Red → roll back that one change and analyze.** A break is information: it tells you the thing you removed was load-bearing. Understand *why* before deciding whether to keep it, replace it with a dynamic equivalent, or leave it. Never paste a new expected value just to make red go green without understanding the change (see "Re-pinning" below). Do low-risk, independent removals in small batches if you like, but anything that can plausibly interact (addresses, contracts, gas) goes **one at a time** so a failure is attributable. ## Verification cadence - **Iterating:** `--fork=<baseline>` (usually Cancun) — fast. - **Checkpoint / done:** fill the whole `valid_from` range (omit `--fork`) so all deployed forks are exercised. - **Probe the future fork:** explicitly `--fork Amsterdam` (or the latest fork that enables new EIPs). A gas/state-cost change there is the most likely future breakage. (Historical note: broken tests used to be parked in a `tests/ported_static/amsterdam_skip_list.txt` consumed by a local conftest; the list was emptied and both were removed. If a future fork's repricing breaks tests en masse, the same parking pattern — a substring-matched skip list plus a `pytest_collection_modifyitems` hook — is in git history.) - **`fill` output:** writes to `./fixtures` (`--clean` resets it), or pass `--output <dir>` for a scratch location. Do **not** use `-o` — that is pytest's `--override-ini`, not the output dir. ## Ordered steps Do them roughly in this order. Earlier steps unblock later ones (notably: audit the bytecode *before* touching gas, since restoring elided opcodes moves the budget; and max out gas *before* strengthening post-state, so added opcodes don't hit a gas ceiling). ### 0. Audit the bytecode against its `# Source: yul` comment The `# Source: yul` blocks are the *filler's* source; the bytecode beside them is what **solc emitted**, and the optimizer is free to delete operations the test depends on. A port that faithfully reproduces the compiled bytecode therefore faithfully reproduces the *hole* the optimizer left. Do this **first** — restoring elided operations changes gas, so it must precede any budget work (steps 2 / 10). **The canonical fold: a self-cancelling `SSTORE` pair.** Refund tests set a slot then clear it (`sstore(k, 1); sstore(k, 0)`) to earn a refund. In a fresh `CREATE` frame slot `k` is already zero, so solc folds the pair down to `sstore(k, 0)` — a no-op that generates **no refund at all**, leaving the test vacuous while still passing. Validated on `test_create_oog_from_call_refunds`, where 2 of 24 init codes had lost their `sstore(1, 1)`: the OoG arms assert the sender's balance reaches exactly zero, which *is* the "refund earned inside a reverted frame must be discarded" check — and it was asserting nothing. **How to check.** Disassemble every bytecode blob and diff it against the comment above it. Comparing opcode *counts* per mnemonic (`sstore(` in the Yul vs. `SSTORE` in the asm) catches the whole class in one pass. A throwaway script that `ast`-parses the test, `eval`s each `Op...` assignment against a namespace of the test's constants, and walks `bytes(...)` through a `PUSH*`-aware opcode table is enough — there is no disassembler in `execution_testing`. **Tells in the ported source.** Dense `DUP`/`SWAP` juggling (`Op.SSTORE(key=Op.DUP2, value=Op.DUP2)`, a bare `Op.PUSH1[0x1] + Op.PUSH1[0x0]` prologue, a trailing argument-less `Op.RETURN`) is solc's stack reuse — the shape most likely to hide a fold, and unreadable regardless. Rewrite those from the Yul into explicit `Op.SSTORE(key=..., value=...)` / `Op.RETURN(offset=..., size=...)` form: it restores the intent and makes the next audit trivial. **Benign deviations — do not "fix" them.** solc drops a `POP` before a terminator (`pop(call(...)); return(0, 1)` compiles without the `POP`, as `RETURN` ignores leftover stack) and encodes repeated literal zeros as `DUP1` chains (`Op.CALL(..., args_offset=Op.DUP1, ...)`). Both are semantically identical to the Yul. Only a **missing or added state-changing operation** is a real deviation. **Expect to re-budget afterwards.** Restoring an elided op adds its cost — a zero->non-zero `SSTORE` is ~22.1k pre-EIP-8037 and ~97.9k of *state* gas on Amsterdam — so a test with a hardcoded `gas_limit` may now OOG. That is usually not a regression you introduced: it reveals that the sibling cases which never lost their op were *already* failing on the future fork for the same reason. Establish this before re-budgeting by copying the pre-change file aside under a different test name, filling both, and diffing the failure sets — in the validated case that separated 12 pre-existing Amsterdam failures from the 3 the fix added. **Verify the restoration is observable, not merely green.** Fill before and after and reconcile the gas delta. Above, consumption moved 77731 -> 97857 (the added cold `SSTORE`, minus the reset dropping to a warm 100) and the 19900 refund was capped by EIP-3529 at `97857 // 5 = 19571`, giving the reported 78286 exactly. A delta you cannot account for means the rewrite changed the program (see "Re-pinning" below). ### 1. Remove `env` Delete the `Environment(...)` block, the `env=env` arg to `state_test`, and any now-orphaned vars (`coinbase`) and the `Environment` import. The framework supplies sensible defaults. **Keep `env` only if** the post asserts on the coinbase/`fee_recipient` balance, or the bytecode reads block fields (`NUMBER`, `TIMESTAMP`, `PREVRANDAO`, `BASEFEE`, `GASLIMIT`, `COINBASE`). `fee_recipient=sender` alone is not a reason to keep it. ### 2. Remove `gas_limit` from the transaction (if gas is not the subject) This is the common case and belongs early. Omitting `gas_limit` maxes out the gas the tx receives, so the body executes fully. See the [Transactions section] (../write-test/SKILL.md#transactions) of the `write-test` skill. - **Remove it** when the test is about *behavior* and just needs to run to completion. This also lets you delete any per-fork gas band-aids (e.g. `fork.is_eip_enabled(8037)` budget bumps) and often the `fork` param itself. - **Keep it** only for genuinely gas-sensitive tests (OOG boundaries, intrinsic-gas, code-deposit limits, or gas metering) — see step 10. - **Gas-snapshot tests are gas-sensitive.** If the post asserts a stored `GAS` reading or a `SUB(@gas_before, GAS)` delta (legacy slots `0` / `0x64`), the test *measures gas* — handle it under step 10 (preserve via `CodeGasMeasure`), do not just strip `gas_limit`. This was the dominant skip-list shape: the stored gas value is exactly what EIP-8037 re-prices and breaks. - **EIP-8037 caveat:** when you omit `gas_limit` on a test that *measures* an operation incurring **state gas** (account creation, storage writes), add `state_gas_reservoir=0` to the tx, or that state gas is silently dropped from the measurement on EIP-8037 forks (see step 10). Pure-execution opcodes (e.g. `PUSH0`, arithmetic) have no state gas and do not need it. - Do **not** add a comment explaining the absence of `gas_limit`; omission is the default. ### 3. Remove hardcoded contract `nonce` Drop `nonce=0` from `pre.deploy_contract(...)`. If a `compute_create_address(..., nonce=N)` in the post depends on it, keep them consistent. ### 4. Remove hardcoded addresses (one contract at a time) Two sub-cases: - **Value discarded:** a `contract = Address(0x...)` literal that is immediately overwritten by `pre.deploy_contract(...)` (no `address=`). Just delete the literal; the deploy returns a `fill`-generated address. - **Value passed to `address=`:** remove both the literal *and* the `address=` argument, per contract, filling after each. - **No-op case:** `to=None` creation tests often have no hardcoded address at all (the created address is `compute_create_address(sender, nonce=0)`). Confirm by grepping for `Address(0x` / `address=`. - **On break:** some bytecode hardcodes that address as a CALL/CREATE target (or the tx `to`/`data`). Thread the dynamic address through the caller and the tx entry point instead. - **Self-reference:** a contract that hardcodes its *own* deploy address (e.g. `Op.BALANCE(0xF172…)` where `0xF172…` is its own `address=`). Threading a `fill`-generated address in is impossible (chicken-and-egg), so replace the self-reference with the opcode that yields it at runtime — `Op.BALANCE(Op. ADDRESS)`. Don't substitute a *different* opcode that happens to be shorter (e.g. `Op.SELFBALANCE`) if it changes what the test exercises. - **Remove `@pytest.mark.pre_alloc_mutable`** once the test no longer hardcodes addresses/nonces or assigns `pre[...]` directly — i.e. all allocation now goes through `fund_eoa` / `deploy_contract` / `nonexistent_account`. Fill to confirm. ### 5. Remove easy boilerplate values Independent and usually safe (batchable): `pre.fund_eoa(amount=...)` → `fund_eoa()`; tx `value`; tx `data` when it is empty (`Bytes("")`); explicit gas price fields. Keep any of these that the post actually checks or that triggers the behavior under test. - **Drop opcode args that just pass their default.** Ported bytecode often spells out zero operands that are already the default, e.g. `Op.CALL(..., args_offset=0, args_size=0, ret_offset=0, ret_size=0)` — all four are `0` by default. Removing them is a no-op on the assembled bytecode (verify once with `bytes(a) == bytes(b)`) and cuts noise. Applies to any opcode arg equal to its default. - **Drop the hardcoded subcall `gas` operand — this is a correctness fix, not cosmetics.** `Op.CALL`/`CALLCODE`/`DELEGATECALL`/`STATICCALL` default `gas` to `Op.GAS` (forward all remaining). Ported fillers hardcode a constant (`gas=0xEA60`, `gas=0x186A0`) that was sized for the *old* gas schedule; once EIP-8037 inflates the callee's state gas (e.g. a zero→non-zero SSTORE jumps to ~97920), that fixed budget no longer covers the callee and the subcall OOGs on Amsterdam — a common reason a pure-behavior test lands on the skip list. Omit the operand so it forwards everything. **Caveat:** forwarding all gas via `Op.GAS` misbehaves on **pre-EIP-150 (Homestead)** — the sweep (step 11) fails only there, so such tests floor at **TangerineWhistle**. Keep an explicit `gas` operand *only* when the amount forwarded is the subject (an OOG-boundary test). **Budget vs. subject:** before dropping the operand, ask *why* the constant has its value. A mid-sized constant (`0xEA60`) is a *budget* sized for the old schedule — drop it. An absurd or boundary constant (`2**256 - 20`) is the *subject*: it exercises the 63/64 clamp on an oversized ask (a client that computed e.g. `requested + stipend` in wrapping arithmetic would forward almost nothing and fail). Keep it, name it (`OVERSIZED_GAS_ASK`), and state the intent in a comment. Validated on `test_make_money`. - **A codeless / absent call target is `pre.nonexistent_account()`**, not `pre.fund_eoa(amount=0)`. It yields an address guaranteed to hold no code and no state, which is what "call an empty contract" tests mean. - **Drop a stale `# noqa: F841`** on `contract = pre.deploy_contract(...)` once the variable is actually used (in `to=` / the post); leaving it triggers `RUF100`. ### 6. (Parametrized tests) Analyze what the `data` parameter is Look at `tx.data` / `tx.to`: - **Scenario A — data is a target contract address:** the tx lands in a thin entry-point contract that just `CALL`s the address from calldata. Usually you can **delete the entry-point** and call the target directly, and the N targets are near-identical → replace N bytecode copies with a **dynamic generator** parameterized by the small difference. When the targets are *gas-measurement* contracts differing only by the measured opcode, the dedup collapses all the way to a single `CodeGasMeasure(code=opcode)` parametrized on the opcode (step 10) — the entry-point's `CALL` was only a delivery mechanism. Validated on `test_push0_gas2` (PUSH0 vs PUSH1 0x00). - **Scenario B — data is initcode:** spotted by **`to=None`**. Decide whether running inside initcode is *required* by the test (e.g. the test is about initcode-context behavior, per its title/docstring) or just an artifact of the static-filler format (most common — then the logic can move to a normal deployed contract). If required, convert the `tx_data` array into an `initcode(d)` **generator function**: even when variants are genuinely different programs, the function form lets each branch be labeled by intent, surfacing the one thing that varies. ### 7. (Parametrized tests) Simplify `expect_entries_` / `resolve_expect_post` **First, identify which index actually discriminates — it is *not* always `d`.** Ported tests also key on `g` (gas) or `v` (value); check both the `expect_entries_` `indexes` (which axis is non-`-1`) and which of `tx_data[d]`/`tx_gas[g]`/`tx_value[v]` is the list with >1 entry. The other two indexes are pinned/wildcard. (Example: `test_add_non_const` varies `v` — `d`/`g` are fixed at 0 and the `indexes` match on `"value"`.) **Precondition** (to collapse to a per-case form): every entry's `network` is implied by `valid_from` and there is no `expect_exception`. Then the post is a pure function of the discriminating index. - Convert `expect_entries_` into a plain **list of `result` dicts indexed by the discriminator** — duplicating identical entries (e.g. data `[0,1]` → two slots) is fine and preferred; an explicit flat list is easiest to reason about. - **When the discriminator is a real quantity** (the tx `value` or `gas`), parametrize *directly on that quantity* (`parametrize("tx_value", [0, 1])`) rather than an opaque index, feed it straight into the `Transaction`, and express the post as a function of it. A clean closed form is ideal — e.g. `Account(storage={0: 2 * tx_value})` for a contract that stores `ADD(BALANCE, BALANCE)` of a balance equal to the sent value (this is the "encode relationships" idea from step 9 applied to the post). - Cascade: delete the `resolve_expect_post` import, the `_exc` it returned, and the tx's `error=_exc`. - **Optionally merge** the data-generator and the post-list into **one `if/elif/else` on `d`** that sets both `initcode` and `post` per case. This co-locates each case's bytecode with its expected state — the strongest readability win, and it tends to *reveal* incomplete verification. Use a final `else` so every branch binds both vars; declare `initcode: Bytecode` and `post: dict` above the switch. Prefer the array form when cases are many or the switch would be unwieldy; this is a judgment call. - **Clean up the `parametrize` signature.** The ported `"d, g, v"` triple is usually overkill: drop the pinned/unused indexes from both the `parametrize` and the function signature, keep the discriminator, and rename it to something meaningful (and `fork` too, if no longer used). Parametrize on the renamed axis: - **String values** (e.g. `parametrize("opcode", ["calldataload", "calldatacopy", "codecopy"])`) read best when the cases are distinct programs; pytest derives the test ids straight from the strings (matching the old `id=`s), and the switch branches become `if opcode == "calldataload"`.