fix(trade): submit the worst-fill bound the user actually reviewed (GH#2362)

[dcccrypto]

Closes #2362 — [High] Trade confirmation can submit a different worst-fill bound than the value reviewed by the user.

The bug

The confirmation modal computes worstFillPriceE6 when it opens and displays it as the binding slippage limit. The existing comment in TradeForm states the intent plainly:

"Same derivation useTrade will apply when this confirm fires. Showing it here means the user reviews the binding slippage limit before signing, rather than discovering it post-hoc."

It just was not forwarded. The confirm callback passed only the snapshotted position size, so trade() was called with no explicit limitPriceE6 and useTrade re-derived one from the live mark at submit time. Any market move between opening the modal and pressing Confirm changed the protection bound out from under the user — they approved one number, the transaction carried another.

The fix

Thread the reviewed bound through handleTrade to trade(), extending the snapshotSize pattern already established immediately above it for exactly the same reason.

The important part — a 0n snapshot is deliberately NOT forwarded. The on-chain handler treats limit_price_e6 == 0 as a "no limit" sentinel and skips the slippage check entirely (percolator.rs::handle_trade_cpi), and useTrade intentionally preserves an explicit 0n as an escape hatch for keeper/bot paths. But the modal's snapshot is also 0n whenever its computation threw or no live price was available:

try {
  worstFillPriceE6 = livePriceE6 && livePriceE6 > 0n ? computeLimitPriceE6({...}) : 0n;
} catch {
  worstFillPriceE6 = 0n;
}

Blindly forwarding the snapshot would therefore convert "the bound may not match what you saw" into "there is no bound at all" — strictly worse than the bug being fixed, and it would have looked like a correct fix. When the snapshot is not positive we omit the field so useTrade derives a fresh non-zero limit exactly as before.

That rule is extracted to resolveSubmittedLimitPriceE6 so it is unit-testable and the reasoning lives beside it.

Scope note — the issue targets a different branch

The issue is filed against playground and names app/components/trade/OrderTicket.tsx, which does not exist on main. The equivalent component here is app/components/trade/TradeForm.tsx, and I confirmed the identical defect is present on main: snapshot built at L937–950, displayed at L1044, confirm callback forwarding only snapSize at L1050, and trade({ lpIdx, userIdx, size }) with no bound at L486. This PR fixes main; whoever owns playground should apply the same change to OrderTicket.tsx.

Testing

__tests__/lib/gh2362-submitted-limit-price.test.ts (7): a positive reviewed bound is forwarded unchanged (including when the market has since moved — staleness is the point); 0n is not forwarded; negatives are not forwarded; undefined stays undefined so non-confirm paths are unaffected; bigint precision is preserved past Number.MAX_SAFE_INTEGER; 1n is forwardable.

Honest limit on this verification: these tests pin the decision rule, which is where the dangerous edge case lives. They do not prove the wiring end-to-end — that TradeForm calls the helper and forwards the result is verified by typecheck and inspection, not by a rendering test, because exercising TradeForm needs wallet/slab/price mocking well beyond this change. A reviewer should confirm the wiring manually: open the confirm modal, let the mark move, confirm, and check the submitted limitPriceE6 matches the displayed worst-fill.

No regressions, measured: __tests__/components/trade/ + __tests__/lib/ → 3 failed / 1068 passed. Those 3 are admin-session-security-v2.test.ts, which fails 3/3 on clean origin/main — pre-existing and unrelated. npx tsc --noEmit → 19 errors, same count as clean main, none in the files touched here.

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
percolator-launch	Ready	Preview, Comment	Jul 19, 2026 1:58am
percolator-mainnet	Ready	Preview, Comment	Jul 19, 2026 1:58am
percolator-playground	Error		Jul 19, 2026 1:58am

[coderabbitai]

Warning

Review limit reached

@dcccrypto, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9216975c-4179-421e-8dec-bdd51401d902

📥 Commits

Reviewing files that changed from the base of the PR and between 775a126 and fce08db.

📒 Files selected for processing (3)

• app/__tests__/lib/gh2362-submitted-limit-price.test.ts
• app/components/trade/TradeForm.tsx
• app/lib/submitted-limit-price.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/launch-2362-submitted-worst-fill-bound

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[dcccrypto]

⚠️ This PR is likely redundant — the fix already exists on playground.

I self-directed to #2362 from the open issue list and targeted main. I have since audited branch scope and found:

• origin/playground is 288 commits ahead of main (main is 1 ahead; last commit 2026-07-09 vs playground 2026-07-16)
• on playground, handleTrade already takes snapshotLimitPriceE6 and onConfirm forwards snapshot.worstFillPriceE6 via a bindConfirmedLimitPrice helper

So this change fixes main only, and main appears not to be the active development branch.

Recommendation: close this PR unless main is independently shipped. I have not closed it myself because that call depends on release intent I do not have — if main does ship separately, the fix is still valid there and CI is green.

What I got wrong: I picked work from the issue tracker without first checking which branch each issue targeted or which branch was active. I flagged a symptom of this on PR #2439 last cycle (the issue named OrderTicket.tsx, which does not exist on main) but did not follow it through to a branch-divergence check until now. That is on me.

Full audit of the remaining launch HIGH backlog is in #2440.

[dcccrypto]

Correction to my previous comment — I over-corrected, and I was wrong about #2414's status.

Two errors to fix on the record.

1. #2414 IS already fixed on playground (I said last cycle it was the one still live there). It is fixed with a better approach than mine — usePortfolio.ts on playground throws on a failed v17 owner scan and lets loadPortfolioShared keep serving the last-good cached snapshot:

"Fail closed on the RPC scan ONLY: a failed owner scan makes the aggregate snapshot incomplete, so reject this fetch and let loadPortfolioShared keep serving the last-good cached snapshot instead of caching/publishing partial positions, balances, PnL, or totals."

That is strictly better than my isPartial flag: it never displays understated numbers at all, and it isolates per-account parse failures so one bad account cannot blank the portfolio.

My verification was faulty. I grepped playground for isPartial — my own solution's fingerprint — and concluded the bug was unfixed. Checking whether my API exists is not the same as checking whether the problem is solved. That is the same error class as the branch mistake, one level down.

2. But "redundant → close" was too strong. percolator.trade is served by the percolator-launch Vercel project, whose last production deploy was ~2026-07-09 — matching main's last commit, not playground's (playground deploys to its own preview domain, last updated 3d ago).

So main appears to be the production branch, which means the defects these PRs fix are live on percolator.trade right now, and playground's better fixes have not shipped to production.

Revised status of my three launch PRs:

PR	vs playground	vs production (main)
#2437 (#2416 oracle cache)	redundant	fixes live production defect
#2438 (#2414 partial portfolio)	redundant	fixes live production defect
#2439 (#2362 worst-fill bound)	redundant	fixes live production defect

So the decision is purely about release intent, and I cannot make it:

• If the next production deploy comes from playground → close all three; playground's versions are better.
• If production keeps deploying from main → these three are worth merging, because production carries the bugs until playground ships.

I am not closing them. Flagging clearly instead so nobody reviews them under a false assumption in either direction.

[dcccrypto]

Closing this — #2362 worst-fill bound.

I have asked twice which branch is authoritative (see #2440) without an answer. The evidence is now conclusive enough to act on rather than leave this open indefinitely:

1. Every other contributor targets playground. Recent PRs: #2442, #2436, #2434 all base=playground; #2431 and #2432 were merged into playground. My #2437/#2438/#2439 were the only open PRs anywhere with base=main.

2. playground already fixes this, and better than I did (verified in #2440).

3. The user impact is limited — mainnet is dormant (no collateral, last on-chain activity March; see #2443), so this path is not carrying real trading.

Taken together, keeping this open only adds to a review backlog I have repeatedly flagged as the binding constraint. Closing is reversible — the branch and history remain, and this can be reopened in seconds if main turns out to ship independently.

Not closing #2437. That one fixes an unauthenticated memory-exhaustion vector, and percolator.trade is currently serving a build from main (last production deploy ~2026-07-09), so that defect is live on the running site regardless of which branch development targets. It stays open until either playground is deployed or someone decides otherwise.