Merged
Conversation
Adds a new public method that sums isolated USDC deposit values across all active perp positions by aggregating getIsolatePerpPositionTokenAmount results. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…alue, and getLeverageComponents Wire getTotalIsolatedPositionDeposits() into the three aggregate value methods so that isolated perp position deposits are counted in net USD value, total asset value, and leverage component calculations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughAdded User.getTotalIsolatedPositionDeposits() and updated User valuation methods so isolated perp-position deposits are included in leverage components, total asset value, and net USD value; tests added to validate valuation, oracle pricing, inclusion/exclusion semantics, and margin behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant PerpPos as PerpPosition (loop)
participant PerpMarket as PerpMarket
participant SpotMarket as SpotMarket
participant Oracle as Oracle
User->>PerpPos: iterate active perp positions
PerpPos-->>User: return position with isolatedPositionScaledBalance
User->>PerpMarket: fetch perp market by marketIndex
PerpMarket-->>User: returns perp market (includes quoteSpotMarketIndex)
User->>SpotMarket: fetch quote spot market
SpotMarket-->>User: returns spot market (includes oracle data)
User->>Oracle: derive strict oracle price from spot market
Oracle-->>User: strict price
User->>User: convert isolatedPositionScaledBalance -> quote token amount -> USD using price
User->>User: sum across positions -> getTotalIsolatedPositionDeposits()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sdk/src/user.ts (1)
381-414: Consider centralizing isolated-deposit valuation logic.This
isolatedPositionScaledBalance -> tokenAmount -> USD valueflow now exists here, in the single-marketgetLeverageComponents()branch, and again ingetMarginCalculation(). A shared private helper would make future pricing changes much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/user.ts` around lines 381 - 414, Centralize the isolated-position valuation: extract the repeated flow (isolatedPositionScaledBalance -> getTokenAmount -> getStrictTokenValue using quoteSpotMarket, quoteOraclePriceData/StrictOracle) into a single private helper (e.g., computeIsolatedDepositUsdValue or isolatedScaledBalanceToUsd) and call it from getTotalIsolatedPositionDeposits, the single-market branch in getLeverageComponents, and getMarginCalculation; the helper should accept the perpPosition (or isolatedPositionScaledBalance and market indices) plus driftClient/getOracleData accessors and return a BN USD value so callers simply sum or add the returned BN.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/src/user.ts`:
- Around line 1823-1828: getLeverageComponents currently adds
getTotalIsolatedPositionDeposits() into spotAssetValue for every marginCategory,
which diverges from getTotalAssetValue(marginCategory) that only includes
isolated deposits when marginCategory === undefined; update
getLeverageComponents to only add isolatedDeposits to spotAssetValue when
marginCategory is undefined (same condition used by getTotalAssetValue), keeping
calculateLeverageFromComponents behavior consistent; refer to
getLeverageComponents, getTotalAssetValue, calculateLeverageFromComponents, and
getTotalIsolatedPositionDeposits and adjust the spotAssetValue +
isolatedDeposits logic to be conditional.
---
Nitpick comments:
In `@sdk/src/user.ts`:
- Around line 381-414: Centralize the isolated-position valuation: extract the
repeated flow (isolatedPositionScaledBalance -> getTokenAmount ->
getStrictTokenValue using quoteSpotMarket, quoteOraclePriceData/StrictOracle)
into a single private helper (e.g., computeIsolatedDepositUsdValue or
isolatedScaledBalanceToUsd) and call it from getTotalIsolatedPositionDeposits,
the single-market branch in getLeverageComponents, and getMarginCalculation; the
helper should accept the perpPosition (or isolatedPositionScaledBalance and
market indices) plus driftClient/getOracleData accessors and return a BN USD
value so callers simply sum or add the returned BN.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02bb1c09-6315-41fd-896c-0e346f07eadd
📒 Files selected for processing (2)
sdk/src/user.tssdk/tests/user/test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/tests/user/test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
sdk/tests/user/marginCalculations.test.ts (2)
193-199: Avoid brittle long positional-argument calls in tests.These invocations are hard to read and easy to mis-order. A tiny wrapper/helper with named intent would make failures easier to reason about.
🧩 Suggested refactor
+const getIsolatedCollateral = (user: any, perpMarketIndex: number) => + user.getTotalCollateral('Maintenance', false, true, undefined, perpMarketIndex); + +const getIsolatedLiqPrice = (user: any, perpMarketIndex: number) => + user.liquidationPrice( + perpMarketIndex, + ZERO, + ZERO, + 'Maintenance', + false, + ZERO, + false, + 'Isolated' + );Also applies to: 308-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tests/user/marginCalculations.test.ts` around lines 193 - 199, The tests call mockUser.getTotalCollateral with many positional args which is brittle and hard to read; refactor by adding a small test helper or wrapper (e.g., a helper function like getTotalCollateralForTests or a wrapper around mockUser.getTotalCollateral) that accepts a descriptive options object or named parameters and forwards them to mockUser.getTotalCollateral, then replace the long positional calls (including the occurrences around the second range) with calls to that helper to improve readability and reduce ordering errors.
23-39: Reduce repeated mock setup with a small fixture builder.The repeated clone-and-build blocks make the suite harder to maintain. A local helper would keep tests shorter and reduce edit drift.
♻️ Suggested refactor
+const buildMockUser = async ( + mutate?: (ctx: { + myMockPerpMarkets: typeof mockPerpMarkets; + myMockSpotMarkets: typeof mockSpotMarkets; + myMockUserAccount: typeof mockUserAccount; + }) => void +) => { + const myMockPerpMarkets = _.cloneDeep(mockPerpMarkets); + const myMockSpotMarkets = _.cloneDeep(mockSpotMarkets); + const myMockUserAccount = _.cloneDeep(mockUserAccount); + mutate?.({ myMockPerpMarkets, myMockSpotMarkets, myMockUserAccount }); + return makeMockUserFromHelpers( + myMockPerpMarkets, + myMockSpotMarkets, + myMockUserAccount, + DEFAULT_ORACLES, + DEFAULT_ORACLES + ); +};Also applies to: 47-73, 82-99, 104-122, 130-160, 167-191, 204-228, 237-268, 276-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tests/user/marginCalculations.test.ts` around lines 23 - 39, Extract the repeated clone-and-setup into a small local fixture builder function (e.g., buildMockUser) that accepts optional overrides for perpMarkets, spotMarkets, userAccount adjustments and internally clones mockPerpMarkets/mockSpotMarkets/mockUserAccount, applies modifications like setting spotPositions[0].marketIndex, balanceType = SpotBalanceType.DEPOSIT and scaledBalance = new BN(...).mul(SPOT_MARKET_BALANCE_PRECISION), then calls makeMockUserFromHelpers(myMockPerpMarkets, myMockSpotMarkets, myMockUserAccount, DEFAULT_ORACLES, DEFAULT_ORACLES) and returns the result; replace each repeated block that constructs myMockPerpMarkets, myMockSpotMarkets, myMockUserAccount and calls makeMockUserFromHelpers with calls to this builder (passing only the small differences per test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sdk/tests/user/marginCalculations.test.ts`:
- Around line 193-199: The tests call mockUser.getTotalCollateral with many
positional args which is brittle and hard to read; refactor by adding a small
test helper or wrapper (e.g., a helper function like getTotalCollateralForTests
or a wrapper around mockUser.getTotalCollateral) that accepts a descriptive
options object or named parameters and forwards them to
mockUser.getTotalCollateral, then replace the long positional calls (including
the occurrences around the second range) with calls to that helper to improve
readability and reduce ordering errors.
- Around line 23-39: Extract the repeated clone-and-setup into a small local
fixture builder function (e.g., buildMockUser) that accepts optional overrides
for perpMarkets, spotMarkets, userAccount adjustments and internally clones
mockPerpMarkets/mockSpotMarkets/mockUserAccount, applies modifications like
setting spotPositions[0].marketIndex, balanceType = SpotBalanceType.DEPOSIT and
scaledBalance = new BN(...).mul(SPOT_MARKET_BALANCE_PRECISION), then calls
makeMockUserFromHelpers(myMockPerpMarkets, myMockSpotMarkets, myMockUserAccount,
DEFAULT_ORACLES, DEFAULT_ORACLES) and returns the result; replace each repeated
block that constructs myMockPerpMarkets, myMockSpotMarkets, myMockUserAccount
and calls makeMockUserFromHelpers with calls to this builder (passing only the
small differences per test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d351669-d67f-4e03-8463-e06a2b7e6de9
📒 Files selected for processing (1)
sdk/tests/user/marginCalculations.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Several SDK methods that compute aggregate account value and leverage (
getNetUsdValue,getTotalAssetValue,getLeverageComponents) did not account for USDC deposits held in isolated perp positions (PerpPosition.isolatedPositionScaledBalance). This caused under-reporting of total account value, incorrect leverage/margin ratio calculations, and incorrect all-time PnL. This PR fixes that by summing isolated deposits into these value/leverage methods.Health/margin methods (e.g.
getTotalCollateral,getMarginCalculation) are intentionally not changed — isolated positions have their own independent health tracked viaisolatedMarginCalculations.Changes
New helper method:
getTotalIsolatedPositionDeposits()— sumsgetIsolatePerpPositionTokenAmount()across all active perp positions to get total isolated USDC deposit valueMethods fixed (
sdk/src/user.ts):getNetUsdValue()— now adds isolated deposits (cascading fix togetTotalAllTimePnl())getTotalAssetValue()— now adds isolated depositsgetLeverageComponents()aggregate path (noperpMarketIndex) — adds isolated deposits tospotAssetValue(cascading fix togetLeverage(),getMarginRatio(),getMaxLeverageForPerp(),getMaxLeverageForSpot(),getMaxSwapAmount(),calculateMaxSwapOutput())Tests (
sdk/tests/user/test.ts):Test plan
getTotalIsolatedPositionDepositscorrectly sums multiple isolated position depositsgetNetUsdValuereturns cross-margin spot value + unrealized PnL + isolated depositsgetTotalAssetValuereturns spot asset value + unrealized PnL + isolated depositsgetLeverageComponentsaggregate path includes isolated deposits inspotAssetValueSummary by CodeRabbit
New Features
Tests