Conversation
LukasDeco
changed the title
crispheaney
reviewed
Feb 4, 2026
crispheaney
reviewed
Feb 4, 2026
crispheaney
reviewed
Feb 4, 2026
crispheaney
reviewed
Feb 4, 2026
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduce a new MarginTypeConfig enum and thread it through MarginContext, margin calculations, controller order/transfer flows, user APIs, SDK IDL, and tests to enable explicit isolated-vs-cross-margin override behavior and isolated-position-aware margin checks. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Controller as OrderController
participant MarginCtx as MarginContext
participant Calc as MarginCalculation
participant User as UserState
Client->>Controller: place_perp_order(params, is_isolated)
Controller->>Controller: determine isolated_market_index
alt isolated
Controller->>MarginCtx: standard_with_config(IsolatedPositionOverride{...})
else cross
Controller->>MarginCtx: standard_with_config(CrossMarginOverride{...})
end
Controller->>Calc: meets_place_order_margin_requirement(context, isolated_market_index)
Calc->>MarginCtx: get_cross_margin_requirement_type()
Calc->>MarginCtx: get_isolated_margin_requirement_type(isolated_market_index)
Calc->>User: evaluate user collateral & positions
alt collateral sufficient
Calc-->>Controller: Ok(true)
Controller-->>Client: accept order
else insufficient
Calc-->>Controller: Err(InsufficientCollateral)
Controller-->>Client: reject order
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. 📝 Coding Plan
Comment Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@programs/drift/src/controller/orders.rs`:
- Around line 2060-2072: The margin_type_config for makers is being built using
is_isolated_position (which was derived from the taker's perp); change the logic
to use the maker's own isolation flag instead: obtain the maker's perp position
isolation status (e.g., maker_isolated_position from the maker_perp_position
struct) and use that variable when selecting between
MarginTypeConfig::IsolatedPositionOverride and
MarginTypeConfig::CrossMarginOverride for the maker; update any references that
build margin_type_config for makers so they use maker_isolated_position (and
keep the same fields: market_index, margin_requirement_type,
default_*_margin_requirement_type) instead of the taker-derived
is_isolated_position.
In `@programs/drift/src/math/margin.rs`:
- Around line 599-609: The override branch currently sets
perp_position_custom_margin_ratio =
margin_ratio_override.max(perp_user_custom_margin_ratio) and thus drops
market_position.max_margin_ratio; change the logic so the final
perp_position_custom_margin_ratio is the max of all three relevant values
(margin_ratio_override, perp_user_custom_margin_ratio, and
market_position.max_margin_ratio as u32) so the per‑position cap is preserved;
update the code that sets perp_position_custom_margin_ratio (and the conditional
when position_margin_type == MarginRequirementType::Initial) to compute
max(margin_ratio_override, perp_user_custom_margin_ratio,
market_position.max_margin_ratio as u32) when context.margin_ratio_override is
Some(...).
In `@tests/orderMarginChecks.ts`:
- Around line 89-91: The test sets up oracles solUsd and ethUsd using
mockOracleNoProgram(bankrunContextWrapper, 1) but the test comments assume $100
per asset; update the oracle initialization to use price 100 (call
mockOracleNoProgram(bankrunContextWrapper, 100) for solUsd and ethUsd) so the
oracle prices match the scenario, or alternatively update all comments/expected
values to reflect the current price=1; locate uses of solUsd, ethUsd and
mockOracleNoProgram in the test to make the consistent change.
🧹 Nitpick comments (3)
README.md (1)
67-69: Add language identifier to fenced code block.The code block is missing a language identifier, which aids syntax highlighting and is flagged by markdownlint (MD040).
📝 Suggested fix
-``` +```bash cd .devcontainer && docker build -t drift-dev .</details> </blockquote></details> <details> <summary>tests/orderMarginChecks.ts (1)</summary><blockquote> `183-254`: **Consider logging cleanup failures for debugging.** The `resetUserState` function silently catches and ignores all errors during cleanup. While this is reasonable for test isolation, consider logging errors at a debug level to aid troubleshooting when tests behave unexpectedly. </blockquote></details> <details> <summary>programs/drift/src/controller/orders.rs (1)</summary><blockquote> `2246-2248`: **Address the TODO: HLM handling in isolated fill context.** The TODO comment raises a valid question about how High Leverage Mode (HLM) should be handled when filling orders for isolated positions. The current code uses the standard `is_high_leverage_mode(MarginRequirementType::Initial)` check without considering whether the position is isolated. This could affect fee tier determination. Consider clarifying the expected behavior and either: 1. Document why the current behavior is correct for isolated positions, or 2. Implement isolated-position-aware HLM checking if different behavior is required Would you like me to open an issue to track this TODO, or can you clarify the intended behavior here? </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/orderMarginChecks.ts`:
- Around line 93-94: Update the inline comments next to the mockOracleNoProgram
calls to reflect the actual prices passed (100 and 1000) instead of "$1 per SOL"
and "$1 per ETH"; change the comments for solUsd and ethUsd to indicate "$100
per SOL" and "$1000 per ETH" (or the correct currency units used by
mockOracleNoProgram) so the comment matches the values used in the calls to
mockOracleNoProgram.
🧹 Nitpick comments (3)
README.md (1)
67-69: Add language specifiers to fenced code blocks.The code blocks containing shell commands should have a language specifier (e.g.,
bashorshell) for proper syntax highlighting and to satisfy markdownlint rules.📝 Suggested fix
Build the container and tag it `drift-dev`: -``` +```bash cd .devcontainer && docker build -t drift-dev .Open a shell to the container:
-
+bashFind the container ID first
docker ps
Then exec into it
docker exec -it <CONTAINER_ID> /bin/bash
... Use the dev container as you would a local build environment: -``` +```bash # build program anchor buildAlso applies to: 73-79, 92-104
tests/orderMarginChecks.ts (2)
188-260: Consider logging suppressed errors in resetUserState for debuggability.The
resetUserStatefunction silently swallows all errors in multiple try-catch blocks. While ignoring errors during cleanup is common, completely suppressing them can make debugging test failures difficult when the cleanup itself has issues.Consider logging errors at a debug level or adding a flag to enable verbose cleanup output when troubleshooting:
💡 Example approach
async function resetUserState(verbose = false) { // ... try { await driftClient.closePosition(perpPosition.marketIndex); } catch (e) { if (verbose) console.debug('resetUserState: closePosition skipped:', e.message); } // ... }
495-504: Clarify margin requirement comments after oracle price changes.The comments reference "$333 MM required" which is calculated at the original $100 price. After
setFeedPriceNoProgramadjusts SOL to $80 (line 528), the actual MM requirement changes to approximately $266 (10 × $80 × 0.333). While the test outcomes remain valid (the effective collateral calculations account for the loss), the static MM figures in comments could be misleading.Consider updating comments to note that margin requirements are dynamic based on current oracle price, or explicitly state the post-move MM requirement.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/orderMarginChecks.ts`:
- Around line 188-260: resetUserState() currently clears positions and balances
but doesn't restore oracle prices, so tests leak price changes; update
resetUserState to also reset all test oracle feeds to their default values (for
example set SOL back to 100 and any other modified assets) by calling the
existing test oracle helper (e.g. setOraclePrice / setFeedPrice /
restoreDefaultOraclePrices) via the same test harness used elsewhere, then call
driftClient.fetchAccounts() afterwards to refresh state; modify the function
that closes positions and withdraws (resetUserState) to include this
oracle-reset step so each test starts with deterministic oracle prices.
Collaborator
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/isolatedTransferMarginChecks.ts`:
- Around line 571-620: The test descriptions for Scenario 7 and Scenario 8 say
"Cross: $700" but the tests call driftClient.deposit with 1000 USDC (new BN(1000
* 10 ** 6)), causing a mismatch; update the deposit calls in both scenarios (the
driftClient.deposit invocations in those test blocks) to deposit 700 USDC (new
BN(700 * 10 ** 6)) so the test setup matches the scenario description and
subsequent assertions.
- Around line 519-569: The test's setup deposits the wrong cross balance vs. the
scenario description: replace the initial cross deposit in the test (the call to
driftClient.deposit in this spec) so the cross balance matches the described
$700 scenario (or update the scenario text to reflect the actual $1000 deposit)
to ensure the post-transfer cross balance becomes $450 and fails IM;
specifically, fix the driftClient.deposit amount used in this test (and/or
clarify whether depositIntoIsolatedPerpPosition reduces cross balance) so the
behavior of transferIsolatedPerpPositionDeposit matches the intended assertion
that the transfer will cause cross IM failure.
- Around line 347-385: The test comment math is using entry price instead of
mark price; update the inline comments in the Scenario 3 and Scenario 4 test
blocks (near the describe for "Scenario 3: Cross fails IM, no other isolateds"
and the corresponding Scenario 4 block) to reflect mark price-based margin
calculations (e.g. Scenario 3: at $80 mark price with 50% IM -> 10 SOL × $80 ×
0.5 = $400 IM, not $500; Scenario 4: at $70 mark with 33% MM -> 10 SOL × $70 ×
0.33 ≈ $233, not $333), or simplify the comments to state "requirements are
computed from mark price" to avoid confusion; check the nearby calls to
driftClient.openPosition, setFeedPriceNoProgram and
transferIsolatedPerpPositionDeposit to locate the exact test code to edit.
🧹 Nitpick comments (1)
tests/isolatedTransferMarginChecks.ts (1)
210-228: Minor inconsistency in PNL settlement approach.
settleMultiplePNLsis used for market 0 (line 214) whilesettlePNLis used for market 1 (line 221-224). Consider usingsettleMultiplePNLswith[0, 1]for consistency and to reduce code duplication.♻️ Optional refactor
- try { - await driftClient.settleMultiplePNLs( - await driftClient.getUserAccountPublicKey(), - driftClient.getUserAccount(), - [0], - SettlePnlMode.TRY_SETTLE - ); - } catch (e) { - // Ignore - } - try { - await driftClient.settlePNL( - await driftClient.getUserAccountPublicKey(), - driftClient.getUserAccount(), - 1 - ); - } catch (e) { - // Ignore - } + try { + await driftClient.settleMultiplePNLs( + await driftClient.getUserAccountPublicKey(), + driftClient.getUserAccount(), + [0, 1], + SettlePnlMode.TRY_SETTLE + ); + } catch (e) { + // Ignore + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/isolatedTransferMarginChecks.ts`:
- Line 680: The test declaration currently uses it.only('should fail when any of
multiple other isolated fails MM', ...) which forces Mocha to run only that
test; change it to a normal it(...) by removing .only so the full test suite
runs in CI and no other tests are skipped.
---
Duplicate comments:
In `@tests/isolatedTransferMarginChecks.ts`:
- Around line 388-390: Update the scenario inline comments in the
isolatedTransferMarginChecks test cases so the described prices and mark-price
math match the actual feed values used in the test (e.g., fix Scenario 3 to say
$70 if feed is $70, reconcile Scenario 4's $333 MM reference to the entry/mark
price used, and correct Scenario 5 to $50 if feed is $50); locate and edit the
comment blocks labeled "Scenario 3", "Scenario 4", and "Scenario 5" (and the
related comment lines around the occurrences noted) to ensure all collateral/IM
calculations reflect the feed prices and mark-price math used by the test logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/isolatedTransferMarginChecks.ts`:
- Around line 475-496: Update the scenario comments so they accurately reflect
the code setup: change the Scenario 5 text to state the oracle is set to $50
(not $70) and adjust the MM math to use SOL price 50 yielding a $10*
(100-50)=500 PNL impact or recompute the stated numbers to match using
setFeedPriceNoProgram(bankrunContextWrapper, 50, solUsd); and modify Scenario 6
comments to match the actual deposits used by the code (driftClient.deposit(new
BN(2000 * 10 ** 6), ...) and driftClient.transferIsolatedPerpPositionDeposit(new
BN(600 * 10 ** 6), ...)) so the cross/isolated dollar amounts read $2000 cross
and $600 isolated (and update any resulting MM calculations); also make the same
comment fixes for the later block that the reviewer noted applies (the block
referencing lines 525-536).
- Around line 388-390: The Scenario 3 inline comment describing the oracle price
is incorrect: it says the price moves to $80 but the test actually sets the
oracle to $70; update the Scenario 3 comment (the block starting with "// Cross:
$600, 10 SOL long @ $100 -> $500 IM required" and the following lines) so the
described price change matches the test setup (change "$80" to "$70") or
alternatively change the oracle setup to $80 if you prefer the original
prose—ensure the comment and the oracle value are consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/isolatedTransferMarginChecks.ts (2)
233-261: Consider consolidating PnL settlement calls.The code uses
settleMultiplePNLsfor market 0 (line 235) but individualsettlePNLcalls for markets 1 and 2. For consistency and efficiency, consider settling all markets in a single call.♻️ Suggested consolidation
// Settle PNL for all markets try { await driftClient.settleMultiplePNLs( await driftClient.getUserAccountPublicKey(), driftClient.getUserAccount(), - [0], + [0, 1, 2], SettlePnlMode.TRY_SETTLE ); } catch (e) { // Ignore } - try { - await driftClient.settlePNL( - await driftClient.getUserAccountPublicKey(), - driftClient.getUserAccount(), - 1 - ); - } catch (e) { - // Ignore - } - try { - await driftClient.settlePNL( - await driftClient.getUserAccountPublicKey(), - driftClient.getUserAccount(), - 2 - ); - } catch (e) { - // Ignore - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/isolatedTransferMarginChecks.ts` around lines 233 - 261, Consolidate redundant PnL settlement calls by replacing the separate settleMultiplePNLs([...0...]) and two individual settlePNL calls with one call to settleMultiplePNLs passing all market indices (e.g., [0,1,2]); call await driftClient.settleMultiplePNLs(await driftClient.getUserAccountPublicKey(), driftClient.getUserAccount(), [0,1,2], SettlePnlMode.TRY_SETTLE) inside the existing try/catch and remove the subsequent settlePNL calls to keep behavior consistent and efficient.
601-620: Commented-outsuppressConsoleappears to be debug remnant.Other scenarios consistently use
suppressConsole()to silence expected failure output. Either remove the commented code or uncomment it for consistency.♻️ Suggested fix (option 1: use suppressConsole)
- // const restoreConsole = suppressConsole(); + const restoreConsole = suppressConsole(); try { ... } finally { - // restoreConsole(); + restoreConsole(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/isolatedTransferMarginChecks.ts` around lines 601 - 620, The test block around driftClient.transferIsolatedPerpPositionDeposit contains a commented-out suppressConsole/restoreConsole debug remnant; either remove those commented lines or re-enable them for consistency with other tests. Fix by calling suppressConsole() at the start of the try (e.g., const restoreConsole = suppressConsole();) and invoking restoreConsole() in the finally block, keeping the existing error check for transferIsolatedPerpPositionDeposit and the bn/args unchanged so the test output is suppressed only during the expected failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/isolatedTransferMarginChecks.ts`:
- Around line 568-577: The test comment header "Cross: $1000" is stale and
doesn't match the actual deposit call (driftClient.deposit) which uses 700e6;
update the comment in the 'Scenario 7: Cross passes IM before but fails after,
other isolated fails MM' block (the describe/it test and its inline scenario
comment) to read "Cross: $700" (or otherwise make the numeric narrative match
the deposit amount) so the explanatory math ("Cross after $450 < $500 IM",
transfer $250) is consistent with the driftClient.deposit value.
---
Duplicate comments:
In `@tests/isolatedTransferMarginChecks.ts`:
- Around line 521-538: The test comment is out-of-sync with the setup: update
the scenario text to reflect the actual deposits used by resetUserState and the
driftClient calls (driftClient.deposit with new BN(2000 * 10**6) and
driftClient.depositIntoIsolatedPerpPosition with new BN(600 * 10**6)), correct
the collateral and cross amounts/narrative and fix the MM calculation to show 10
× $80 × 0.33 = $264 so the comment matches the test logic in the "Scenario 6"
describe/it block.
- Around line 429-434: The comment inside the test block describe('Scenario 4:
Cross fails MM, no other isolateds') / it('should fail transfer when cross fails
MM') is using the entry price ($100) to compute maintenance margin ($333) but
the test scenario uses the mark price $70; update the comment to compute MM
using the mark price (10 × $70 × 0.33 = $231) or explicitly note that the $333
number was computed from entry price and that the test uses mark-price-based
calculations; adjust the explanatory text near the resetUserState() call so the
comment matches the mark-price-based MM math.
- Around line 471-478: Update the test comment in the "Scenario 5: Cross passes
IM before and after, other isolated fails MM" describe/it block: replace the
incorrect "$333 MM" with the correct maintenance margin calculation using the
mark price ($50) so it reads "MM = 10 × $50 × 0.33 = $165" and note that the
test uses the mark price (not entry price), keeping the remainder of the
scenario logic unchanged.
---
Nitpick comments:
In `@tests/isolatedTransferMarginChecks.ts`:
- Around line 233-261: Consolidate redundant PnL settlement calls by replacing
the separate settleMultiplePNLs([...0...]) and two individual settlePNL calls
with one call to settleMultiplePNLs passing all market indices (e.g., [0,1,2]);
call await driftClient.settleMultiplePNLs(await
driftClient.getUserAccountPublicKey(), driftClient.getUserAccount(), [0,1,2],
SettlePnlMode.TRY_SETTLE) inside the existing try/catch and remove the
subsequent settlePNL calls to keep behavior consistent and efficient.
- Around line 601-620: The test block around
driftClient.transferIsolatedPerpPositionDeposit contains a commented-out
suppressConsole/restoreConsole debug remnant; either remove those commented
lines or re-enable them for consistency with other tests. Fix by calling
suppressConsole() at the start of the try (e.g., const restoreConsole =
suppressConsole();) and invoking restoreConsole() in the finally block, keeping
the existing error check for transferIsolatedPerpPositionDeposit and the bn/args
unchanged so the test output is suppressed only during the expected failure.
98e4ccf to
c31ef21
Compare
crispheaney
reviewed
Feb 23, 2026
crispheaney
reviewed
Feb 23, 2026
crispheaney
reviewed
Feb 23, 2026
crispheaney
reviewed
Feb 23, 2026
LukasDeco
changed the title
…-checks-on-maintenance-margin
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 by CodeRabbit
New Features
Documentation
Tests