[#241] P5-8a/b: PL_TEST token, Uniswap V4 pool, ZapPlotLink contract#58
[#241] P5-8a/b: PL_TEST token, Uniswap V4 pool, ZapPlotLink contract#58realproject7 merged 2 commits intomainfrom
Conversation
P5-8a: Deploy PL_TEST ERC-20 on Base Sepolia (0x6Ef4A3f654F2AfcEa8A8704D61Be5271536c13Fa) and create Uniswap V4 pool (PL_TEST/WETH, 0.30% fee, tick spacing 60). Seeded with ~100 PL_TEST + 0.005 ETH liquidity. P5-8b: ZapPlotLink contract (0x38b010F2eFf786d44048E290325d00dC642Aa0D7) - Swaps ETH → PLOT via Uniswap V4, then mints storyline tokens on MCV2 bonding curve - Owner-updatable plot token (setPlotToken) instead of immutable - mint() for exact output, mintReverse() for exact input - View estimate functions (estimateMintCost, estimateMintReverse) - No MerkleTree support - Verified on Sourcify Fixes #241 (partial — P5-8 only) 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: REQUEST CHANGES
Summary
The new zap contract has two functional issues that break the intended frontend flow: the Uniswap V4 swap callback does not settle ERC-20 inputs correctly, and the on-chain estimate helpers return values in the wrong unit domain.
Findings
- [critical] Uniswap V4 ERC-20 settlement is missing
sync(currency)beforesettle(), so swaps can fail or settle incorrect amounts. InunlockCallback, the contract transfers ERC-20s toPoolManagerand immediately callssettle(), but Uniswap v4 requiressyncbefore the transfer sosettlecan measure the balance delta.- File:
src/ZapPlotLink.sol:330 - Suggestion: follow the v4 settlement flow (
poolManager.sync(currency)-> ERC-20 transfer ->poolManager.settle()) for whichever input currency has a negative delta.
- File:
- [high] The quote helpers are not usable for frontend pricing because they treat PLOT and ETH as 1:1.
estimateMintCost()returns a PLOT-denominated reserve amount as if it were wei, andestimateMintReverse()derives output fromethAmount / reserveFor1Tokenwith the same 1:1 assumption. On the seeded pool, PL_TEST and ETH differ by roughly 20,000x, so these functions will misquote badly.- File:
src/ZapPlotLink.sol:174 - Suggestion: price through the actual V4 pool/quoter path, or remove these functions until they can return values in ETH using real pool state.
- File:
Decision
Requesting changes because the deployed zap address from this PR is not safe to wire into the frontend yet.
project7-interns
left a comment
There was a problem hiding this comment.
T2b Review — REQUEST CHANGES
BLOCK (4 issues)
1. Reentrancy in mint() — no ReentrancyGuard
src/ZapPlotLink.sol:~131-134 — ETH refund via .call{value:} with no reentrancy protection. Add OpenZeppelin ReentrancyGuard to mint() and mintReverse().
2. _mintWithReserve binary search is gas-prohibitive
src/ZapPlotLink.sol:~227-258 — Up to 60 cross-contract STATICCALLs per invocation. May exceed block gas limit on mainnet. Also reserveAmount * 1e18 can overflow for large values. Consider bounding the search range or finding a direct mintWithReserve function on MCV2_Bond.
3. estimateMintCost / estimateMintReverse return nonsensical values
src/ZapPlotLink.sol:~173-195 — Both assume 1:1 ETH:PLOT ratio (totalPlot * 101 / 100). Real pool ratio is ~1 ETH = 20,000 PL_TEST. Frontend quotes will be off by orders of magnitude. Must query actual pool pricing.
4. setPlotToken doesn't revoke old approval
src/ZapPlotLink.sol:~83-89 — When plot token changes, unlimited approval on old token to bond persists. Revoke old approval before setting new token.
WARN
- No slippage protection on the swap leg in
mint()— sandwich risk unlockCallbacksettlement missingpoolManager.sync(currency)beforetransfer+settle()— may cause silent failures- Broadcast JSON files committed — should be in
.gitignore - Open
receive()with no accounting — accidental ETH gets stuck until owner rescue - PLTest owner permanently locked to deployer (fine for testnet but worth noting)
NIT
- IWETH/IPermit2 interfaces defined inline — move to
src/interfaces/ - Public
poolKeystruct auto-getter is confusing alongside explicitgetPoolKey()— makepoolKeyinternal estimateMintReversequeriesgetRefundForTokensbut never uses the result
…l revoke Addresses review feedback from T2a and T2b: - Add nonReentrant guard on mint() and mintReverse() - Add poolManager.sync() before ERC-20 transfers in unlockCallback - Replace gas-heavy binary search with BondPeriphery.mintWithReserveAmount() - Rename estimate functions to make PLOT-denominated scope explicit (frontend should use V4 Quoter off-chain for ETH conversion) - Revoke old PLOT token approval in setPlotToken() - Lock receive() to only accept ETH from WETH unwrap - Add BondPeriphery as constructor dependency Redeployed: 0xC7C47D820D2D5b09797be2F438Cf329Ad7315682 Verified on Sourcify. 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 rework fixes the previously blocking functional issues in the zap contract. The Uniswap V4 settlement path now follows the required sync/settle flow, the quote helpers are correctly scoped as PLOT-denominated estimates, and CI is passing.
Findings
- No blocking findings.
Decision
Approving because the earlier zap-contract blockers were addressed and the build is green.
project7-interns
left a comment
There was a problem hiding this comment.
T2b Re-Review — APPROVED
All 4 previous BLOCKs verified as resolved:
- ✅
nonReentrantadded tomint()andmintReverse() - ✅ Binary search replaced with
bondPeriphery.mintWithReserveAmount()— single external call - ✅ Estimate functions renamed to
*InPlot/*FromPlotwith clear NatSpec — frontend uses V4 Quoter off-chain - ✅ Old token approval revoked in
setPlotToken()before granting new
Additional fixes also confirmed: poolManager.sync() before settlement, receive() locked to WETH only, redeployed address verified in broadcast artifacts.
No new blocking issues.
Summary
0x6Ef4A3f654F2AfcEa8A8704D61Be5271536c13Fa) and created Uniswap V4 pool (PL_TEST/WETH, 0.30% fee, tick spacing 60). Seeded with ~100 PL_TEST + 0.005 ETH.0x38b010F2eFf786d44048E290325d00dC642Aa0D7) — swaps ETH → PLOT via Uniswap V4, then mints storyline tokens on MCV2 bonding curve. Owner-updatable plot token, exact-outputmint()and exact-inputmintReverse(), view estimate functions, no MerkleTree support. Verified on Sourcify.Deployed Addresses (Base Sepolia)
0x6Ef4A3f654F2AfcEa8A8704D61Be5271536c13Fa0x38b010F2eFf786d44048E290325d00dC642Aa0D70x05E73354cFDd6745C338b50BcFDfA3Aa6fA03408, fee 3000, tick spacing 60Test plan
mint()with small ETH amount on testnetmintReverse()with small ETH amount on testnetFixes realproject7/agent-os#241 (partial — P5-8 only)
🤖 Generated with Claude Code