Conversation
Foundry script covering Groups A-F: story lifecycle (create/chain), trading variations (buy/sell/monotonicity), donations, royalty claims, 9 validation barrier reverts, and edge cases (CID boundaries, royalty diff). Run: source .env && forge script script/E2ETest.s.sol --rpc-url base --broadcast -vvvv 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 — PR #28
Overall the script is well-structured: clean group separation, correct broadcast management for Group E reverts, proper try/catch usage, and good scenario coverage. Three issues to address:
1. Missing e2e-results.json output (blocking)
The issue explicitly requires: "Output tx hashes + storyline IDs to e2e-results.json" — and ticket 2 (plotlink#374) depends on this file. The script only logs to console. Foundry's vm.writeJson cheatcode can write structured JSON from a script. Add a vm.writeJson call at the end that writes storyline IDs, token addresses, and scenario results to e2e-results.json. Without this, the indexer verification script has no input.
2. Missing estimate-vs-actual assertions for Group B (non-blocking but recommended)
The issue specifies: "getReserveForToken() estimate matches actual reserve spent (within 1 wei tolerance)" and same for getRefundForTokens(). B1–B5 log both values but never assert they match. Add require checks with a small tolerance, e.g.:
require(spent1 <= estReserve1 + 1 && spent1 >= estReserve1 - 1, "B1: estimate mismatch");3. Missing totalSupply assertions for Group B
The issue requires: "totalSupply changes by exact trade amount". Not currently checked. Add totalSupply() checks before/after each mint/burn.
Items 2–3 are assertion gaps that reduce confidence but don't break the script. Item 1 is blocking — the downstream ticket depends on the JSON output file.
…s, totalSupply checks - Add vm.writeJson to output storyline IDs, token addresses, trading costs, and scenario results to e2e-results.json (blocking — needed by downstream plotlink#374) - Add estimate-vs-actual require checks (1 wei tolerance) for all Group B mint/burn operations - Add totalSupply assertions before/after every mint and burn 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 Re-review — APPROVED
All 3 requested changes addressed cleanly in f8e10b0:
e2e-results.json—vm.serializeUint/Address/String+vm.writeJsonwrites structured JSON with storyline IDs, token addresses, trading costs/refunds, royalties claimed, and edge case data. Downstream plotlink#374 is unblocked.- Estimate-vs-actual assertions — B1–B5 now assert
require(spent <= est + 1 && spent + 1 >= est)for both mints and burns. totalSupplychecks — AddedIERC20Extendedinterface;totalSupply()verified before/after every mint/burn in Groups B and F.
Code is clean, all 30 scenarios covered with proper assertions. LGTM.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The script is close, but it still misses one acceptance-criteria requirement and one of the donation scenarios is not actually being tested as specified. The PR also cannot merge in its current state because the Foundry check is failing on formatting.
Findings
-
[high] Group C uses the writer as the donor, so the donation scenarios never verify the required transfer behavior between two parties.
- File:
script/E2ETest.s.sol:280 - Suggestion: run donations from a second funded address (or otherwise separate donor from writer) and assert the donor balance decreases while the writer balance increases by the donation amount, per the issue requirements.
- File:
-
[medium]
e2e-results.jsonstill omits the broadcast transaction hashes that issue #27 explicitly requires and that the downstream indexer-verification ticket needs.- File:
script/E2ETest.s.sol:100 - Suggestion: capture and persist the tx hash for each broadcast transaction alongside the storyline IDs / token addresses in
e2e-results.json.
- File:
-
[low] GitHub checks are still red because
forge fmt --checkfails on this file.- File:
script/E2ETest.s.sol:238 - Suggestion: run
forge fmt(or apply the equivalent formatting at the flagged lines) before re-requesting review.
- File:
Decision
Requesting changes because the script does not yet satisfy the donation assertions from the issue, does not emit the tx-hash output required by the acceptance criteria, and currently fails CI.
- Group C: Use separate DONOR_PRIVATE_KEY for two-party donation verification — assert both donor-decreases and writer-increases - e2e-results.json: Add chainId, donor address, and broadcastArtifact path for per-broadcast tx hash resolution by downstream plotlink#374 - Fix forge fmt violations (lines 238, 405 area) 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 updated script addresses the requested changes. Group C now uses a separate donor wallet with exact donor/writer balance assertions, the JSON output now provides the chain and Forge broadcast artifact path needed to resolve transaction hashes downstream, and CI is green.
Findings
- None.
Decision
Approving because the PR now satisfies the previously-blocking review items and the GitHub checks are passing.
Summary
script/E2ETest.s.sol— a Foundry script that runs the full StoryFactory lifecycle on Base mainnet with real transactionsTest plan
forge buildcompiles without errorsforge test)source .env && forge script script/E2ETest.s.sol --rpc-url base --broadcast -vvvvFixes #27
🤖 Generated with Claude Code