Repository navigation
[simplex]: retract bids one at a time, and on cancel, expiry and close; stop bidding on completed orders - #1422
Conversation
Wizdave97
left a comment
There was a problem hiding this comment.
Verdict: changes needed before merge. I reviewed only this PR's commit on top of #1421.
The per-row snapshot fixes the stranded-deposit race. What I checked:
- A row stored during a pass is either retracted by its own scoped pass or picked up by the TTL sweep.
- Pooled and failed rows are marked dead one row at a time and retried.
filledis only emitted when the dust-floor close actually applies.stop()adds no new wait that could hang forever.
Must fix
- Bids priced by a cancelled or expired limit order keep going out, and nothing retracts them before the 1 h sweep (filler.ts:1264-1275, :1332-1346).
executeOrderuses the plans cached at evaluation (getBidPlans). The loop only checkshasCompleted(order.id). It never checks the status ofplan.limitOrderId, and neitherfx.submitBidnorprepareBidUserOpdoes either.- Scenario: the operator cancels limit order A while a queued task holds a plan priced from A, or partway through a multi-plan loop (about 8 s per bid).
retractBidsForLimitOrder(A)takes its snapshot at that moment, so any bid placed afterwards is stored as live. Those bids sit in discovery at the withdrawn price forbidValiditySecondsplus the pad, and their deposit stays locked until the 1 h sweep.- The cancel doesn't change the on-chain cap, which is still
size, so these bids remain executable. - That is the exposure this PR sets out to close. There is a guard for swap-order completion, but not for limit-order closure.
- Suggestion: before each bid, skip a plan whose limit order isn't
openorresizing. Afterstore, check again, and if it has closed, callenqueueRetraction(commitment, { bids: [result.bid] }). Alternatively, keep aclosedLimitOrdersset fed byretractBidsForLimitOrder.
Should fix
-
Completion is recorded behind the settlement queue (filler.ts:261-286).
markCompletedruns insidehandleOrderFilledOnChainandhandleRivalCompletion. Both are queued onsettlementQueue, which runs one task at a time, behindreconcileTalliesand other fills'syncToChain→resize, which make orderbook HTTP calls and chain reads.- Meanwhile the bid loop keeps placing bids that can only revert with
Filled(), each paying the BRIDGE fee. They are retracted later, so this costs fees, not deposits. - The "recorded before anything is read" comment at :1472 assumes this happens promptly.
- Suggestion: call
markCompleteddirectly in the twomonitor.on(...)listeners, beforesettlementQueue.add.
-
Preferring a live bid in fill attribution ignores the escrow evidence (filler.ts:1590-1596).
- The code uses the live bids whenever any exist, and otherwise falls back to
bids[0]. - Example: live bid RB with take 40, and retracted bid RA with take 100. A placer who had already fetched RA executes it, releasing 100. RB can't have been the one executed (the amount released is at most its take), yet the fill is recorded against B, capped at RB's payout. RB is then also excluded from its own later fill.
- The docstring says retracted bids are used "only when no live bid could have been it", but the code means "when no live bid exists".
remainingfollows the chain, so this only skews fill history and profit. - Suggestion: filter live bids by
take >= releasedfirst, then retracted ones, before the payout andbids[0]fallbacks.
- The code uses the live bids whenever any exist, and otherwise falls back to
-
The window for attributing a retracted bid starts from settlement time and has only a 60 s margin (filler.ts:60, :1566).
- A backlog on the settlement queue (see 2), and slow blocks stretching the block-based
validUntil, both eat into that margin. - #1411 sets
BID_PERMIT_SKEW_SECONDS = 120for exactly this tail where a bid can still execute. A retracted bid that executes between +60 and +120 s is therefore not attributed. - Suggestion: start the window from when the event arrived or from the block timestamp, and reuse #1411's constant.
- A backlog on the settlement queue (see 2), and slow blocks stretching the block-based
Notes:
completedOrderscounts every rival completion on every configured chain, not only orders we bid on. Eviction during a loop would still need 1000 newer completions, so this is fine.- The
BidStorerowsrequirement is a silent hazard: a custom store that ignores the filter still type-checks and brings back the stranded-deposit bug. Call it out as a breaking change in the release notes.
Merge: the conflict with #1411 in ContractInteractionService.ts is only the BID_DISCOVERY_PAD_SECONDS export. Take #1411's side, which also adds BID_PERMIT_SKEW_SECONDS.
CI: swaps fails on onchain-limit-refusal because of #1421 (e2e/budget.mjs:422 still reads reserved). Every other scenario passed, so this goes green once #1421 is fixed.
- executeOrder skips a cached bid plan whose limit order is no longer open/resizing, and re-checks after the bid row is stored: a bid that went out while its order closed is retracted at once (just that bid). - Completion is recorded in the two EventMonitor listeners, ahead of the settlement queue, for complete fills only, so the bid loop stops while settlement is queued behind reconcile or orderbook work. - Fill attribution follows the escrow evidence: live bids whose take covers the release, then retracted bids in the window, before the payout and first-bid fallbacks. - A retracted bid's window is counted back from when the fill event arrived, not from settlement; the margin is RETRACTED_BID_EXECUTION_MARGIN_SECONDS (120). - Note: the BidStore `rows` filter is stated as a breaking change for custom stores. - Merge: retire()'s POSTABLE guard matches #1410's identical change; the rebase test fixtures follow the fill's block number from #1421.
b464047 to
15ed6a2
Compare
- executeOrder skips a cached bid plan whose limit order is no longer open/resizing, and re-checks after the bid row is stored: a bid that went out while its order closed is retracted at once (just that bid). - Completion is recorded in the two EventMonitor listeners, ahead of the settlement queue, for complete fills only, so the bid loop stops while settlement is queued behind reconcile or orderbook work. - Fill attribution follows the escrow evidence: live bids whose take covers the release, then retracted bids in the window, before the payout and first-bid fallbacks. - A retracted bid's window is counted back from when the fill event arrived, not from settlement; the margin is RETRACTED_BID_EXECUTION_MARGIN_SECONDS (120). - Note: the BidStore `rows` filter is stated as a breaking change for custom stores. - Merge: retire()'s POSTABLE guard matches #1410's identical change; the rebase test fixtures follow the fill's block number from #1421.
15ed6a2 to
bbbe1c6
Compare
- executeOrder skips a cached bid plan whose limit order is no longer open/resizing, and re-checks after the bid row is stored: a bid that went out while its order closed is retracted at once (just that bid). - Completion is recorded in the two EventMonitor listeners, ahead of the settlement queue, for complete fills only, so the bid loop stops while settlement is queued behind reconcile or orderbook work. - Fill attribution follows the escrow evidence: live bids whose take covers the release, then retracted bids in the window, before the payout and first-bid fallbacks. - A retracted bid's window is counted back from when the fill event arrived, not from settlement; the margin is RETRACTED_BID_EXECUTION_MARGIN_SECONDS (120). - Note: the BidStore `rows` filter is stated as a breaking change for custom stores. - Merge: retire()'s POSTABLE guard matches #1410's identical change; the rebase test fixtures follow the fill's block number from #1421.
bbbe1c6 to
1641aa4
Compare
Wizdave97
left a comment
There was a problem hiding this comment.
Approved. All four round-1 findings are fixed:
- Plans priced from a closed limit order are skipped, and a bid stored after the order closed is retracted.
- Completion is recorded in the event listener, not behind the settlement queue.
- Fill attribution checks the escrow evidence first.
- The retracted-bid window counts back from the fill block, with a 120 s margin.
Non-blocking follow-ups:
- Attribution edge case. A live bid with no
takeis passed over in favour of a retracted bid whose take covers the escrow (filler.ts:117-124). This only happens for legacy rows and only skews fill history. - PR description. It still says
completedOrdersholds 1000, that "live bids are preferred", and "+60 s". - After #1411 lands. Point
RETRACTED_BID_EXECUTION_MARGIN_SECONDSatBID_PERMIT_SKEW_SECONDS. - CI. The failed
swapsrun isn't caused by this PR.multi-leg-same-inputfailed becauseCacheService's 60 s expiry ran out partway through the bid loop while Gargantua inclusion was slow ("No cached gas estimate found").- That code predates this stack, and the fix is tracked separately.
- executeOrder skips a cached bid plan whose limit order is no longer open/resizing, and re-checks after the bid row is stored: a bid that went out while its order closed is retracted at once (just that bid). - Completion is recorded in the two EventMonitor listeners, ahead of the settlement queue, for complete fills only, so the bid loop stops while settlement is queued behind reconcile or orderbook work. - Fill attribution follows the escrow evidence: live bids whose take covers the release, then retracted bids in the window, before the payout and first-bid fallbacks. - A retracted bid's window is counted back from when the fill event arrived, not from settlement; the margin is RETRACTED_BID_EXECUTION_MARGIN_SECONDS (120). - Note: the BidStore `rows` filter is stated as a breaking change for custom stores. - Merge: retire()'s POSTABLE guard matches #1410's identical change; the rebase test fixtures follow the fill's block number from #1421.
1641aa4 to
fab2020
Compare
…e; stop bidding on completed orders
- executeOrder skips a cached bid plan whose limit order is no longer open/resizing, and re-checks after the bid row is stored: a bid that went out while its order closed is retracted at once (just that bid). - Completion is recorded in the two EventMonitor listeners, ahead of the settlement queue, for complete fills only, so the bid loop stops while settlement is queued behind reconcile or orderbook work. - Fill attribution follows the escrow evidence: live bids whose take covers the release, then retracted bids in the window, before the payout and first-bid fallbacks. - A retracted bid's window is counted back from when the fill event arrived, not from settlement; the margin is RETRACTED_BID_EXECUTION_MARGIN_SECONDS (120). - Note: the BidStore `rows` filter is stated as a breaking change for custom stores. - Merge: retire()'s POSTABLE guard matches #1410's identical change; the rebase test fixtures follow the fill's block number from #1421.
fab2020 to
3743845
Compare
Stacked on #1421 (stack #1417). Review only this PR's commits.
This PR fixes the per-bid retraction findings from the 2026-10-15 simplex audit: c22, c23, c33, c34, c42, and the round-2 expiry low. With holds gone (#1421), what retraction protects is the BRIDGE deposit and fee on each Hyperbridge bid, plus taking a withdrawn price out of discovery.
Why:
retractBidwas awaiting. That bid was never retracted on chain, so its deposit stayed locked forever.stop()disconnected Hyperbridge before draining queued retractions.Fix (
src/core/filler.ts,BidStore,limit-orders.ts,simplex.ts,boot.ts):Per-bid retraction:
enqueueRetraction(commitment, { bids?, rows? })snapshots the live rows when the pass starts, retracts them, and marks only those row ids retracted.markRetractedandmarkDeadtake an optional row-id filter.BidNotFoundand legacy rows close; failed and pooled rows stay open and are flagged dead by row. The sweep retracts only due rows.No bids on a completed order:
completedOrders, a set of the last 20,000 completed commitments (oldest dropped), stops the bid loop. Every complete fill on a configured chain enters it, a rival's included, not only orders we bid on, so the cap is set well above what a bid loop can see go by. A bid stored after completion is retracted at once.Retract when a limit order stops being usable:
IntentFiller.retractBidsForLimitOrder(id)retracts the live bids that limit order priced, across commitments, and never throws. It runs:expiredandrejectedevents (retireandpostarePOSTABLE-guarded and emit only when the write applies);filled, including a close throughreconcileTallies;syncToChainfor orders that are no longer open.While a limit order is live, its bids are left to the chain (
debitOrder).Attribution: a retracted bid that still executes is recorded, because retraction can't stop a placer that already fetched the bid until its
validUntil. The escrow the fill released decides which bid it was, since a bid releases its signedtakeor less, so only a bid whosetakecovers the release can have been it. In order:takecovers the release, best rate first;takecovers the release, best rate first (a live bid the escrow rules out does not hide one);A retracted bid is in the window when it was stored no more than
bidValiditySeconds+ the 30 s discovery pad (BID_DISCOVERY_PAD_SECONDS) + the 120 s bid permit skew (BID_PERMIT_SKEW_SECONDS) before the fill. The window is counted back from the timestamp of the block the fill landed in (getBlock, read only when there is a retracted candidate), not from when settlement runs. The arrival time is the fallback when the fill carries no block or the read fails.Shutdown:
stop()drains evaluations, chain queues, settlement and rebalances first, then retractions (bounded at 30 s), and disconnects last. Shutdown stops the limit-order lifecycle first.Limits:
bidValiditySecondsis the hard bound.BidStoreimplementations must honour the newrowsfilter.Validation
Run in
sdk/packages/simplexat the PR head:testjob commands: simplexlintexit 0 (warnings only);test:data139,test:unit266 andtest:filler74 (5 skipped) passed.vitest --maxConcurrency=1 src/tests/core src/tests/data src/tests/orderbook src/tests/services): 800 passed, 4 skipped.pnpm --filter="@hyperbridge/simplex" test): 1599 passed, 29 skipped, 10 failed. The failures are exactly the six live/credential files (rebalancers/binance,cctp,usdt0, andstrategies/fx,stable,vault.testnet).tscshows the 14 errors already on main.Simplexwith a real service and filler;Not covered: no live Hyperbridge run.