fix: correct calculateLiquidationPrice (current-notional margin ratio)#64
fix: correct calculateLiquidationPrice (current-notional margin ratio)#64koko1123 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesLiquidation Price Calculation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/functions/position.ts (1)
110-112: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the
liqMarginRatio >= 1boundary.This branch is explicitly introduced by the PR, but the updated tests cover only ratios below 100%. Add a long-position case with
liq: 1_000_000and assert that the result isnull.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/functions/position.ts` around lines 110 - 112, Add a regression test for the long-position calculation covering liqMarginRatio equal to 1, using liq: 1_000_000 and asserting the result is null. Place it alongside the existing position tests and preserve the current expectations for ratios below 100%.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/functions/position.ts`:
- Around line 110-112: Add a regression test for the long-position calculation
covering liqMarginRatio equal to 1, using liq: 1_000_000 and asserting the
result is null. Place it alongside the existing position tests and preserve the
current expectations for ratios below 100%.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd70ab87-efc8-46d1-a3b2-084ee83a5121
📒 Files selected for processing (2)
src/__tests__/unit/position-calculations.test.tssrc/functions/position.ts
|
Addressed the CodeRabbit nitpick: added a regression test covering the Local |
… ratio The margin ratio is defined by IMarginRatios as equity / position value, where value is the *current* notional (size * price) -- not the entry notional. The old formula subtracted `(margin - liqRatio * entryNotional) / size` from the entry price, which is wrong. Solving `equity == liqRatio * size * liqPrice` gives: long: (entry - margin/size) / (1 - liqRatio) short: (entry + margin/size) / (1 + liqRatio) Also returns null for a long when liqRatio >= 100% (no positive liq price). Updated the unit tests with the corrected formula and exact-value locks (short = 142.857, high-lev long = 42.105). Full CI green (267 unit tests, tsc, biome). Mirrors the same fix in perpcity-zig-sdk so both SDKs report identical liquidation prices.
The denom <= 0 branch (liq ratio >= 100% -> null) introduced by this fix was untested. Adds a long-position case with liq: 1_000_000 asserting null, addressing the CodeRabbit nitpick.
222068d to
2e515d9
Compare
What
calculateLiquidationPriceused the wrong formula.IMarginRatiosdefines the margin ratio as equity / position value, where value is the current notional (size * price), not the entry notional. The old code computedentryPrice ∓ (margin - liqRatio * entryNotional) / size.Solving
equity == liqRatio * size * liqPriceat the liquidation price gives:(entry - margin/size) / (1 - liqRatio)(entry + margin/size) / (1 + liqRatio)Also returns
nullfor a long whenliqRatio >= 100%(no positive liquidation price exists).Example (entry 50, size 1, margin 100, liqRatio 0.05)
The old short value (147.5) overstated the liquidation price by ~3%, which for a bot reads as "safe" when the position is actually closer to liquidation.
Tests
Updated the unit tests with the corrected formula and added exact-value locks (short = 142.857, high-leverage long = 42.105). Full
pnpm run cigreen: build, 267 unit tests,tsc --noEmit, biome lint.Context
Found while porting these helpers to
perpcity-zig-sdk(verified against the deployed contract'sIMarginRatios). This mirrors the fix landing there so both SDKs report identical liquidation prices.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
0when applicable.Tests