[#36] Split E2E into broadcast + revert scripts#37
Conversation
E2ETest.s.sol (--broadcast): Groups A-D1, F1-F2, F4-F5 — all happy paths, no try/catch reverts. Produces e2e-results.json and broadcast artifacts for downstream indexer verification. E2ETestReverts.s.sol (simulation only): D2, E1-E9, F3 — all expected-revert scenarios. Reads storyline IDs from e2e-results.json produced by the main script. Run without --broadcast. Also adds fs_permissions to foundry.toml for e2e-results.json write. Fixes #36 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
T2b Review — APPROVED
Clean separation. All 11 revert tests (D2, E1-E9, F3) moved intact to E2ETestReverts.s.sol. The broadcast script retains only happy-path scenarios — no try/catch that would break --broadcast.
vm.readFile("e2e-results.json")for storyline IDs is the right dependency patternfs_permissionsinfoundry.tomlcovers both read (reverts script) and write (main script)- D2 depends on D1 having already drained royalties in the main script — correct sequencing since the reverts script runs second
- Scenario counts will differ between the two scripts but
e2e-results.jsononly tracks the main script's count, which is fine
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The broadcast/revert split is the right direction, but the new revert-only script weakens D2 enough that it can pass without validating the intended behavior. Right now it checks that the script contract has nothing to claim, not that the deployer's second claim after D1 reverts.
Findings
- [high] D2 lost the double-claim semantics when it moved into
E2ETestReverts.s.sol.- File:
script/E2ETestReverts.s.sol:40 - Suggestion: run the D2 check under the deployer context after the broadcast script's D1 state, e.g.
vm.prank(deployer)beforeclaimRoyalties, so the revert test still exercises the same wallet that successfully claimed in Group D1. As written, the script contract has never claimed anything, so this passes even if the deployer's second-claim path is broken.
- File:
Decision
Requesting changes because the split regresses coverage for the D2 acceptance case instead of preserving it in the revert-validation path.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The follow-up restores the intended D2 semantics in the split revert script by running the empty-claim check under the deployer context. That preserves the original double-claim coverage while keeping the broadcast path free of intentional reverts.
Findings
- None.
Decision
Approving because the review blocker is resolved. Note: one GitHub Foundry check was still pending at review time, so merge should wait for it to finish green.
Summary
E2ETest.s.solinto two scripts to allow--broadcastto succeedE2ETest.s.sol(broadcast): Groups A-D1, F1-F2, F4-F5 — happy paths only, no try/catch. Producese2e-results.jsonand broadcast artifactsE2ETestReverts.s.sol(simulation only): D2, E1-E9, F3 — all expected-revert tests. Reads storyline IDs frome2e-results.jsonfs_permissionstofoundry.tomlfor JSON outputRun order
Test plan
forge fmt --checkcleanFixes #36
🤖 Generated with Claude Code