Price the fresh-open liquidation preview against full cross-margin capital - #2492
Price the fresh-open liquidation preview against full cross-margin capital#24920x-SquidSol wants to merge 1 commit into
Conversation
…n capital OrderTicket's liquidation preview priced a FRESH open (no existing position) with computePreTradeLiqPrice against only the order's margin, while the scale-in branch (and PositionsDock / useLiqPrice) use full account capital. Percolator v17 is cross-margined — the whole account backs the position — so the margin-only preview understated liquidation distance: the confirm modal showed a scarier (closer) liq price than the real on-chain one, which then visibly jumped once the position opened and recomputed on full capital (e.g. a preview liq of ~$81 for a position that is actually unliquidatable). When there is no existing position, combinedSignedSize / combinedEntryPriceE6 already reduce to this order's own signed size and estimated entry, so the existing capital-based computeLiqPrice path is correct for both fresh-open and scale-in. Drop the fresh-open special case (and the now-unused computePreTradeLiqPrice import) so the preview matches what the position shows once open. - OrderTicket: single capital-based afterLiqPrice branch for fresh-open + scale-in - add a PoC/regression using the real SDK math (margin-only preview shows a scary liq for a position that is unliquidatable / safer at full capital) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@0x-SquidSol is attempting to deploy a commit to the Khubair Nasir's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe order ticket now uses full account capital and combined position data for after-liquidation-price calculations. A regression test covers fresh-open pricing and verifies the matching-capital control case. ChangesFresh-open liquidation pricing
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
dcccrypto
left a comment
dcccrypto
left a comment
There was a problem hiding this comment.
Reviewed on head 6901d574. Fix is correct, and the load-bearing claim in the
comment holds exactly. Suite clean: 2921 passed / 0 failed.
The reduction claim checks out
The change collapses two branches into one on the strength of "when there is no
existing position, combinedSignedSize and combinedEntryPriceE6 already reduce
to this order's own signed size and estimated entry". If that were even slightly
off, the fresh-open preview would silently show the wrong number rather than
fail, so I worked it through with existingPositionSize === 0n:
combinedSignedSize = 0n + newSignedSize→newSignedSize✓sameDirectionshort-circuits totrueonexistingPositionSize === 0n✓combinedEntryPriceE6 = (existingEntryPriceE6 * 0n + estEntry * positionSize) / (0n + positionSize)
→estEntry✓ (andpositionSize > 0nis guaranteed by the
combinedSignedSize !== 0nguard)
So the merged branch is exactly the old fresh-open inputs, priced against
capital instead of marginNative. Clean collapse, not an approximation.
One edge I chased and it doesn't bite
Switching the denominator from marginNative to capital made me check the
first-timer case: capital = userAccount ? userAccount.account.capital : 0n, and
computeLiqPrice(entry, 0n, size, mm) on a zero-capital account would be
meaningless — where the old marginNative was always non-zero.
Unreachable: needsDeposit = connected && userAccount && capital === 0n feeds
ticketLocked, and :1250 renders the deposit prompt instead of the order UI, so
no order can be previewed at capital === 0n. Recording it because "we changed
the denominator to something that can be zero" is the obvious thing to worry
about here, and the answer isn't visible from the diff.
The test doesn't bind the fix — eighth in the run
order-ticket-fresh-open-liq.test.ts calls computePreTradeLiqPrice and
computeLiqPrice directly and compares them. It never renders or imports
OrderTicket, so it asserts that the two SDK functions differ, which was true
before the PR and is still true after.
I restored the original branch verbatim:
const afterLiqPrice = hasOrder
? (existingPositionSize === 0n
? computePreTradeLiqPrice(oracleE6, marginNative, positionSize, maintenanceMarginBps, tradingFeeBps, direction)
: ...)2/2 still green with the bug fully back.
This one is cheap to bind without rendering the component, because the whole
change is a pure expression over already-computed values. Extracting it — say
computeAfterLiqPrice({ hasOrder, combinedSignedSize, combinedEntryPriceE6, capital, maintenanceMarginBps })
into lib/trading beside computeEstimatedEntryPrice — makes it directly
testable, and the fresh-open reduction above becomes an assertion rather than a
comment. That also puts it next to PositionsDock / useLiqPrice, which is where
the inconsistency came from in the first place: three surfaces computing liq three
ways is what produced both this bug and the H7-style drift on the stale-oracle
gate (#2484).
Worth noting the computePreTradeLiqPrice import is now dropped from
OrderTicket entirely — after this, is it used anywhere? If not, the SDK export
being unreferenced is a useful signal that the margin-only convention is gone
repo-wide rather than just here.
|
Answering my own closing question so nobody re-checks it:
But nothing calls it. After this PR the only references left in |
2 participants
What
Price the order ticket's fresh-open liquidation preview against the full
cross-margined account capital, matching the scale-in branch and PositionsDock.
Why
OrderTicket priced a FRESH open (
existingPositionSize === 0) withcomputePreTradeLiqPriceagainst only the order'smarginNative, while every otherliq surface (the scale-in branch,
PositionsDock,useLiqPrice) uses fullcapital. Percolator v17 is cross-margined (withdraw is blocked while any leg isopen), so the whole account backs the position. The margin-only preview therefore
understated liquidation distance — the confirm modal showed a scarier (closer) liq
price than reality, which then jumped once the position opened and recomputed on
full capital (e.g. a preview liq of ~$81 for a position that is actually
unliquidatable at full capital).
Changes
position,
combinedSignedSize/combinedEntryPriceE6already equal this order'sown signed size and estimated entry, so the existing capital-based
computeLiqPricebranch is correct for both fresh-open and scale-in. Removed thenow-unused
computePreTradeLiqPriceimport.the capital-based value (margin-only shows a real liq for a position that is
unliquidatable / safer at full capital), and that when order margin == capital the
two agree (no false positive).
Testing
npx tsc --noEmit— clean.liquidation-distancesuite — 13 tests, all pass.Notes
corrected so it matches what the opened position displays (no more jump). The old
preview erred conservative (scarier); this shows the true (farther / unliquidatable)
liq.
Summary by CodeRabbit
Bug Fixes
Tests