From fce08dbcb9aa5de3451214675d0e4c869db9f663 Mon Sep 17 00:00:00 2001 From: dcccrypto Date: Sun, 19 Jul 2026 02:54:01 +0100 Subject: [PATCH] fix(trade): submit the worst-fill bound the user actually reviewed (GH#2362) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The confirmation modal computes a worstFillPriceE6 when it opens and displays it as the binding slippage limit — the existing comment says as much: "Showing it here means the user reviews the binding slippage limit before signing". But the confirm callback forwarded 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 therefore changed the protection bound out from under the user: they approved one number and the transaction carried another. Thread the reviewed bound through handleTrade to trade(), extending the snapshotSize pattern already established directly above it for exactly the same reason. Critically, do NOT forward a 0n snapshot. 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 deliberately preserves an explicit 0n as an escape hatch for keeper/bot paths. The modal's snapshot is ALSO 0n whenever its computation threw or no live price was available. Forwarding that zero would convert "the bound may not match what you saw" into "there is no bound at all" — strictly worse than the bug being fixed. 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 next to it. Closes #2362 Co-Authored-By: Claude Opus 4.8 --- .../lib/gh2362-submitted-limit-price.test.ts | 59 +++++++++++++++++++ app/components/trade/TradeForm.tsx | 25 +++++++- app/lib/submitted-limit-price.ts | 29 +++++++++ 3 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 app/__tests__/lib/gh2362-submitted-limit-price.test.ts create mode 100644 app/lib/submitted-limit-price.ts diff --git a/app/__tests__/lib/gh2362-submitted-limit-price.test.ts b/app/__tests__/lib/gh2362-submitted-limit-price.test.ts new file mode 100644 index 000000000..4da779569 --- /dev/null +++ b/app/__tests__/lib/gh2362-submitted-limit-price.test.ts @@ -0,0 +1,59 @@ +/** + * GH#2362 — the trade confirmation modal could display one worst-fill bound + * while the submitted transaction used another. + * + * TradeForm computes `worstFillPriceE6` when the modal opens and shows it to + * the user as the binding slippage limit, but the confirm callback forwarded + * only the position size. `trade()` was therefore called without an explicit + * `limitPriceE6`, so `useTrade` re-derived one from the live mark at submit + * time — any price move between opening the modal and pressing Confirm changed + * the protection bound the user had approved. + * + * The zero case is the trap. On-chain, `limit_price_e6 == 0` is a "no limit" + * sentinel that skips the slippage check entirely, and the modal's snapshot is + * `0n` whenever its computation failed. Blindly forwarding the snapshot would + * therefore convert a mismatched bound into NO bound. + */ + +import { describe, it, expect } from "vitest"; +import { resolveSubmittedLimitPriceE6 } from "@/lib/submitted-limit-price"; + +describe("GH#2362 submitted worst-fill bound", () => { + it("forwards the reviewed bound so the tx matches what the user approved", () => { + // The core fix: what was displayed is what gets submitted. + expect(resolveSubmittedLimitPriceE6(1_234_567n)).toBe(1_234_567n); + }); + + it("forwards the reviewed bound unchanged even if the market has since moved", () => { + // Staleness is the POINT — the user approved this number, and the tx should + // fail rather than execute at a bound they never saw. + const reviewed = 98_765_432n; + expect(resolveSubmittedLimitPriceE6(reviewed)).toBe(reviewed); + }); + + it("does NOT forward a 0n snapshot — that would disable slippage protection", () => { + // 0n means the modal's computation failed, not "no limit wanted". + // Returning undefined makes useTrade derive a fresh non-zero limit. + expect(resolveSubmittedLimitPriceE6(0n)).toBeUndefined(); + }); + + it("does not forward a negative bound", () => { + expect(resolveSubmittedLimitPriceE6(-1n)).toBeUndefined(); + expect(resolveSubmittedLimitPriceE6(-1_000_000n)).toBeUndefined(); + }); + + it("returns undefined when there is no snapshot at all", () => { + // Non-confirm code paths (mock mode, direct calls) keep the old behaviour. + expect(resolveSubmittedLimitPriceE6(undefined)).toBeUndefined(); + }); + + it("preserves bigint precision for large bounds", () => { + // Must not round-trip through Number. + const huge = 9_007_199_254_740_993n; // Number.MAX_SAFE_INTEGER + 2 + expect(resolveSubmittedLimitPriceE6(huge)).toBe(huge); + }); + + it("treats the smallest positive bound as forwardable", () => { + expect(resolveSubmittedLimitPriceE6(1n)).toBe(1n); + }); +}); diff --git a/app/components/trade/TradeForm.tsx b/app/components/trade/TradeForm.tsx index a2a472b37..696a52623 100644 --- a/app/components/trade/TradeForm.tsx +++ b/app/components/trade/TradeForm.tsx @@ -28,6 +28,7 @@ import { useMarketInfo } from "@/hooks/useMarketInfo"; import { formatTokenAmount, formatUsdPriceE6 } from "@/lib/format"; import { useClosePosition } from "@/hooks/useClosePosition"; import { saveEntryPrice, getEntryPrice, getEntryLeverage, clearEntryPrice } from "@/lib/entry-price"; +import { resolveSubmittedLimitPriceE6 } from "@/lib/submitted-limit-price"; import { isSentinelValue } from "@/lib/health"; import { DepositWithdrawCard } from "@/components/trade/DepositWithdrawCard"; import { useInitUser } from "@/hooks/useInitUser"; @@ -458,7 +459,7 @@ export const TradeForm: FC<{ slabAddress: string }> = ({ slabAddress }) => { const needsDeposit = connected && userAccount && capital === 0n; const canTrade = connected && userAccount && capital > 0n && !lpUnderfunded; - async function handleTrade(snapshotSize?: bigint) { + async function handleTrade(snapshotSize?: bigint, snapshotLimitPriceE6?: bigint) { // Use the snapshotted size from the confirmation modal so the submitted // trade matches what the user reviewed, even if the live price moved // between modal-open and confirm. Fall back to live size if no snapshot @@ -482,8 +483,23 @@ export const TradeForm: FC<{ slabAddress: string }> = ({ slabAddress }) => { setTradePhase("submitting"); try { const size = direction === "short" ? -effectiveSize : effectiveSize; + // GH#2362: submit the worst-fill bound the user actually reviewed. The + // modal already displays this value as the binding slippage limit, but it + // was never forwarded, so useTrade re-derived one from the live mark at + // submit time — a market move between opening the modal and pressing + // Confirm silently changed the protection bound out from under the user. + // See resolveSubmittedLimitPriceE6 for why a 0n snapshot is NOT forwarded. + const submittedLimitPriceE6 = resolveSubmittedLimitPriceE6(snapshotLimitPriceE6); + const tradeParams: { lpIdx: number; userIdx: number; size: bigint; limitPriceE6?: bigint } = { + lpIdx, + userIdx: userAccount!.idx, + size, + }; + if (submittedLimitPriceE6 !== undefined) { + tradeParams.limitPriceE6 = submittedLimitPriceE6; + } const sig = await withTransientRetry( - async () => trade({ lpIdx, userIdx: userAccount!.idx, size }), + async () => trade(tradeParams), { maxRetries: 2, delayMs: 3000 }, ); setTradePhase("confirming"); @@ -1048,9 +1064,12 @@ export const TradeForm: FC<{ slabAddress: string }> = ({ slabAddress }) => { decimals={decimals} onConfirm={() => { const snapSize = confirmSnapshot.positionSize; + // GH#2362: carry the reviewed worst-fill bound through to submission + // so the transaction is protected at the price the user approved. + const snapLimitPriceE6 = confirmSnapshot.worstFillPriceE6; setShowConfirmModal(false); setConfirmSnapshot(null); - handleTrade(snapSize); + handleTrade(snapSize, snapLimitPriceE6); }} onCancel={() => { setShowConfirmModal(false); setConfirmSnapshot(null); }} /> diff --git a/app/lib/submitted-limit-price.ts b/app/lib/submitted-limit-price.ts new file mode 100644 index 000000000..37507d022 --- /dev/null +++ b/app/lib/submitted-limit-price.ts @@ -0,0 +1,29 @@ +/** + * GH#2362 — which worst-fill bound actually gets submitted. + * + * The trade confirmation modal computes a `worstFillPriceE6` and shows it to + * the user as the binding slippage limit. That reviewed value must be the one + * the transaction carries; previously it was dropped and `useTrade` re-derived + * a bound from the live mark at submit time, so any price move between opening + * the modal and pressing Confirm silently changed the protection the user had + * approved. + * + * The subtlety is the zero case. 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); `useTrade` deliberately preserves + * an explicit `0n` as an opt-in escape hatch for keeper/bot paths. But the + * modal's snapshot is *also* `0n` whenever its computation threw or no live + * price was available. Forwarding that zero would convert "the bound might not + * match what you saw" into "there is no bound at all" — strictly worse than the + * bug being fixed. + * + * So: forward a positive reviewed bound, and otherwise return undefined, which + * makes `useTrade` derive a fresh non-zero limit exactly as it did before. + */ +export function resolveSubmittedLimitPriceE6( + reviewedWorstFillPriceE6: bigint | undefined, +): bigint | undefined { + if (reviewedWorstFillPriceE6 === undefined) return undefined; + if (reviewedWorstFillPriceE6 <= 0n) return undefined; + return reviewedWorstFillPriceE6; +}