fix(polymarket): verify redeem effects and sweep legacy collateral#59
fix(polymarket): verify redeem effects and sweep legacy collateral#59KillerQueen-Z wants to merge 2 commits into
Conversation
VickyXAI
left a comment
There was a problem hiding this comment.
Reviewed with live on-chain verification of the assumptions rather than the description alone. The core mechanism is correct — I confirmed it independently:
- pUSD and USDC.e are both 6 decimals on-chain, so
pusdRaw + usdceRawis valid arithmetic. COLLATERAL_ONRAMPreally does implementwrap(address,address,uint256)— selector present in the deployed bytecode at0x93070a…, and a zero-amounteth_callsimulation succeeds.- 171 tests, typecheck and build pass. Merges cleanly into
maindespite being 3 commits behind. - I traced every abort point after a confirmed wrap: the funds land as pUSD in the same wallet and a re-run recomputes
wrapRawfrom fresh balances, so the wrap is idempotent and nothing is unrecoverable.
So this is not a disagreement with the approach. Every issue below is in failure handling, where the cost is that a successful money operation reports failure with misleading advice.
Blocking
1. A transient RPC blip is reported to the user as "your tokens didn't burn." (redeem.ts:163)
Read-failure and genuine-no-op collapse into one branch with no discriminator, and it discards stronger evidence computed 9 lines earlier: when paidOut is positive (pUSD demonstrably increased), it still returns isError: true while emitting paidOutUsd: paidOut — the text says nothing burned, the structured payload says money arrived. This also contradicts the comment this PR adds at constants.ts:104: "A failed post-redeem read must never mask a confirmed state change as an ambiguous payout." Note getPusdBalance retries 4x via withRpcRetry (setup.ts:182) while this read retries zero times — withRpcRetry is module-private, which is probably why it wasn't reused. Exporting it would fix both.
2. Transaction receipts are fetched and discarded. (withdraw.ts:124,126)
viem does not throw on revert; it resolves with status: "reverted". This repo already knows that — redeem.ts:148 and setup.ts:379 both check receipt.status !== "success". The result is an actively wrong diagnosis, detailed inline.
3. This PR changed which RPC broadcasts every money transaction, undeclared. (constants.ts:100)
The comment says the list is "Used only for read-only approval/balance checks". That is not true: POLYGON_RPC_URLS[0] is the sole, un-fallbacked transport for every signing client (withdraw.ts:122,156, redeem.ts:145, relayer.ts:50,76, setup.ts:356). Reads get fallback() across all four entries; writes get position 0 only. Reordering therefore moved all transaction broadcasting from 1rpc to publicnode. That may well be an improvement, but it should be stated in the PR description and reviewed as a write-path change.
Non-blocking
- The new money-moving code has no test at all. The one new withdraw test calls
withdrawFunds({})with noconfirm, so the entireif (wrapRaw > 0n)block never executes. The wrap calldata (target,_asset,_to,_amount) is the money-critical new artifact here, andbuildRedeemCallwas extracted specifically so calldata could be asserted — "the money-critical part of this file". ExtractingbuildWrapCalls(wrapRaw)and mirroringpolymarket-redeem.test.tswould close this. - The wrap tx hash is never surfaced in any path — not the success text, not
structured, not the error. A user whose USDC.e was converted has no on-chain reference for it.wrapHashis assigned and dropped; the sigType-3sendWalletBatchreturn value isn't captured. - Release hygiene: no
package.jsonbump, so merging will not triggerpublish.yml(it only fires onpackage.json/server.template.json/publish.yml) and the fix would sit onmainunreleased. No CHANGELOG entry either.
Thanks for chasing the zero-payout class down to the actual token effect — verifying the ERC-1155 delta instead of trusting the receipt is the right instinct, and the USDC.e sweep closes a real gap for older wallets.
| functionName: "balanceOf", | ||
| args: [owner, BigInt(t.token_id as string)], | ||
| }))).catch(() => null); | ||
| if (balancesAfter === null || !didRedeemAnyHeldPosition(balances, balancesAfter)) { |
There was a problem hiding this comment.
Blocking. balancesAfter === null (all RPCs failed the read) and "balances genuinely did not decrease" produce identical text, identical isError: true, and a structured payload with no field distinguishing them. The caller here is an LLM — it cannot tell "verification failed" from "your redeem was a no-op".
It also throws away better evidence from line 154. If paidOut is positive, the payout provably happened, yet this branch still errors while putting paidOutUsd: <positive> in the same object it says nothing decreased.
And it's the exact thing constants.ts:104 promises not to do: "A failed post-redeem read must never mask a confirmed state change as an ambiguous payout."
Suggested shape:
if (balancesAfter === null) {
// Verification unavailable — do NOT assert a no-op we never observed.
// paidOut > 0 is independent proof the redeem worked.
}
if (!didRedeemAnyHeldPosition(balances, balancesAfter)) { /* genuine no-op */ }Also worth exporting withRpcRetry (setup.ts:74) and wrapping this read — getPusdBalance on line 153 already retries 4x, so within one function the pUSD read is resilient and this one is not.
| const account = getPolymarketAccount(); | ||
| const wallet = createWalletClient({ account, chain: polygon, transport: http(POLYGON_RPC_URLS[0]) }); | ||
| const approveHash = await wallet.sendTransaction({ to: USDCE_COLLATERAL as Hex, data: approve, chain: polygon, account }); | ||
| await getPublicClient().waitForTransactionReceipt({ hash: approveHash }); |
There was a problem hiding this comment.
Blocking. The receipt is awaited and discarded. viem's waitForTransactionReceipt does not throw on a reverted transaction — it resolves with status: "reverted".
The house pattern in this repo checks it, in two places:
redeem.ts:148—if (receipt.status !== "success") throw new Error(...)setup.ts:379— same, for approvals
Failure path today: approve reverts (say, out of gas) -> ignored -> wrap is sent anyway -> reverts for no allowance -> also ignored -> the guard on line 129 fires and tells the user "legacy USDC.e wrap confirmed but the required pUSD balance was not observed; do not retry".
Both halves of that are wrong: the wrap was not confirmed, and since the operation is idempotent, retrying is exactly the right move. Checking status on both receipts turns a misleading dead end into "approve reverted".
| await getPublicClient().waitForTransactionReceipt({ hash: wrapHash }); | ||
| } | ||
| const normalizedPusd = await rawTokenBalance(PUSD_COLLATERAL as Hex, owner); | ||
| if (normalizedPusd < amountRaw) { |
There was a problem hiding this comment.
On the default path (no amount_usd), amountRaw === totalRaw === pusdRaw + usdceRaw, so this demands the post-wrap balance be exact to the wei. Any wrap fee, rounding, or simply a node one block behind trips it — and it trips after a confirmed on-chain state change, then tells the user not to retry.
rawTokenBalance (line 37) has no retry, unlike getPusdBalance, so a stale read right after a just-mined tx is the likeliest trigger.
Two things would harden it: reuse withRpcRetry for this read, and soften the message — the wrap is idempotent, so retry is safe and should be recommended, not warned against.
| test("legacy USDC.e is included and previewed as a pUSD wrap before withdrawal", async () => { | ||
| pusdRaw = 2_000_000n; | ||
| usdceRaw = 3_000_000n; | ||
| const res = await withdrawFunds({}); |
There was a problem hiding this comment.
This is the only new withdraw test, and it calls withdrawFunds({}) with no confirm — so it asserts the dry-run string and wrapUsd, and the entire if (wrapRaw > 0n) block at withdraw.ts:112-132 never executes.
Untested as a result: the wrap calldata itself (target, _asset, _to, _amount), the approve calldata, the sigType-3 two-call batch ordering, the EOA two-tx sequence, and the normalizedPusd < amountRaw guard.
That's the inverse of the standard this repo set for itself in redeem.ts:45-49 — buildRedeemCall is exported because "the target/calldata pair is the money-critical part of this file". The wrap encode is inline and can't be asserted without a small refactor; extracting buildWrapCalls(wrapRaw) and mirroring polymarket-redeem.test.ts:36-49 would cover it.
| // Used only for read-only approval/balance checks (viem public client). | ||
| // 1rpc first — polygon-rpc.com was observed lagging several blocks behind, which | ||
| // made freshly-confirmed deploys/approvals read as still-pending. | ||
| // PublicNode first: 1rpc requires registration for eth_call, Llama's Polygon |
There was a problem hiding this comment.
Two things here.
The 1rpc claim doesn't hold. I probed all four endpoints with a real eth_call (decimals() on USDC.e) just now:
| endpoint | result |
|---|---|
| polygon-bor-rpc.publicnode.com | OK, 0.2s |
| 1rpc.io/matic | OK, 0.1s |
| polygon.llamarpc.com | fetch fails |
| polygon-rpc.com | HTTP 401, "tenant disabled" |
The llamarpc and polygon-rpc.com clauses are correct. 1rpc served eth_call fine without registration.
The bigger point is the line above: "Used only for read-only approval/balance checks". That isn't true — POLYGON_RPC_URLS[0] is the only transport for every signing wallet client (withdraw.ts:122,156, redeem.ts:145, relayer.ts:50,76, setup.ts:356), none of which get fallback(). So this reorder silently moved all money-transaction broadcasting to publicnode. Probably fine or better, but it deserves to be called out in the PR description.
Since two of the four entries are confirmed dead, consider dropping them (each costs retryCount: 2 x 8s in deep fallback) and giving the wallet clients the same fallback() the public client has.
Summary\n- Require a confirmed ERC-1155 outcome-token decrease before redeem reports success, closing the successful no-op class.\n- Include legacy USDC.e in withdrawable collateral; wrap only the required amount to pUSD through the official collateral onramp before bridge withdrawal.\n- Add regressions for no-op detection and legacy USDC.e withdrawal previews.\n\n## Root cause\nDirect CTF redemption with pUSD-derived inputs can succeed while redeeming a zero balance. The current adapter route fixes the underlying path; this PR verifies its actual token effect and lets historical USDC.e residue leave the vault.\n\n## Validation\n- npm test (171 passing)\n- npm run typecheck\n- npm run build\n- Read-only Polygon/CLOB verification using the reported resolved BTC conditions and transactions: the zero-payout transaction had no ERC-1155 burn or USDC.e transfer; the corrected transaction burned positions and transferred 1.298700 USDC.e.\n\nNote: the current local wallet has no redeemable positions, so no user funds were moved for this test.