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; +}