program: fix: pnl settle will also clear out iso balance on market always#2134
Open
program: fix: pnl settle will also clear out iso balance on market always#2134
Conversation
|
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:
WalkthroughAdds a conditional branch in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Controller as PnL_Controller
participant PerpPos as Perp_Position
participant SpotMarket as Quote_Spot_Market
participant Storage as Accounts_State
Caller->>Controller: settle_pnl(user, mode)
Controller->>PerpPos: compute user_unsettled_pnl
alt user_unsettled_pnl == 0 and can_transfer_isolated_position_deposit
Controller->>Storage: read isolated_token_amount
alt isolated_token_amount > 0
Controller->>SpotMarket: update_spot_balances_and_cumulative_deposits(amount)
Controller->>PerpPos: update_spot_balances(after transfer)
Controller->>Storage: set isolated_token_amount = 0
end
Controller->>Storage: update user.last_active_slot
Controller-->>Caller: Ok
else
Controller-->>Caller: NoUnsettledPnl or continue existing flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![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)
programs/drift/src/controller/pnl.rs (1)
286-307: Emit an explicit record for this transfer-only settlement path.This branch mutates balances but exits before
SettlePnlRecordemission. Adding a dedicated explanation/event variant will keep indexers and forensic tooling consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/drift/src/controller/pnl.rs` around lines 286 - 307, The transfer-only branch that handles isolated_token_amount > 0 mutates spot and perp balances via update_spot_balances_and_cumulative_deposits and update_spot_balances but returns before emitting the SettlePnlRecord (or another dedicated event), which breaks indexing/forensics; add a record emission (either reuse SettlePnlRecord with an explicit "transfer_only" flag or introduce a new variant) immediately after the balance updates and before user.update_last_active_slot/return, populating the same identifying fields (user pubkey, position_index, isolated_token_amount, market indices, and a clear reason/variant) so indexers see this settlement path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/drift/src/controller/pnl.rs`:
- Around line 285-307: The early-return branch inside
can_transfer_isolated_position_deposit applies spot/perp balance updates
(isolated_token_amount, update_spot_balances_and_cumulative_deposits,
update_spot_balances, user.update_last_active_slot) then returns without running
the shared post-mutation safety checks; move or duplicate the calls to
validate_perp_market and validate_perp_position_with_perp_market and the TVL
delta validation path to execute after those balance updates and before the
early return so the same invariants are enforced for this flow.
---
Nitpick comments:
In `@programs/drift/src/controller/pnl.rs`:
- Around line 286-307: The transfer-only branch that handles
isolated_token_amount > 0 mutates spot and perp balances via
update_spot_balances_and_cumulative_deposits and update_spot_balances but
returns before emitting the SettlePnlRecord (or another dedicated event), which
breaks indexing/forensics; add a record emission (either reuse SettlePnlRecord
with an explicit "transfer_only" flag or introduce a new variant) immediately
after the balance updates and before user.update_last_active_slot/return,
populating the same identifying fields (user pubkey, position_index,
isolated_token_amount, market indices, and a clear reason/variant) so indexers
see this settlement path.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
programs/drift/src/controller/pnl.rs (1)
285-311:⚠️ Potential issue | 🟠 MajorRun full post-mutation invariants before the early return.
After the balance updates in this branch, Line 311 returns before shared checks (
validate_perp_marketand TVL delta validation). This still leaves a state-changing path with weaker safety guarantees.🔧 Proposed fix
if can_transfer_isolated_position_deposit { // Clear isolated balance by transferring to user's quote spot position if isolated_token_amount > 0 { let spot_position_index = user.force_get_spot_position_index(QUOTE_SPOT_MARKET_INDEX)?; update_spot_balances_and_cumulative_deposits( isolated_token_amount, &SpotBalanceType::Deposit, &mut spot_market, &mut user.spot_positions[spot_position_index], false, None, )?; update_spot_balances( isolated_token_amount, &SpotBalanceType::Borrow, &mut spot_market, &mut user.perp_positions[position_index], false, )?; } user.update_last_active_slot(clock.slot); - crate::validation::position::validate_perp_position_with_perp_market( - &user.perp_positions[position_index], - &*perp_market, - )?; + drop(perp_market); + drop(spot_market); + + let perp_market = perp_market_map.get_ref(&market_index)?; + let spot_market = spot_market_map.get_quote_spot_market()?; + + crate::validation::perp_market::validate_perp_market(&perp_market)?; + crate::validation::position::validate_perp_position_with_perp_market( + &user.perp_positions[position_index], + &perp_market, + )?; + + let tvl_after = spot_market.get_tvl()?; + validate!( + tvl_before.safe_sub(tvl_after)? <= 10, + ErrorCode::DefaultError, + "Settle Pnl TVL mismatch: before={}, after={}", + tvl_before, + tvl_after + )?; return Ok(()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/drift/src/controller/pnl.rs` around lines 285 - 311, The early return after updating isolated balances skips shared post-mutation invariants; move or add the common validation calls (e.g., validate_perp_market and the TVL delta validation used elsewhere in this function) to run after the balance updates in the can_transfer_isolated_position_deposit branch and before returning; keep the existing per-position check validate_perp_position_with_perp_market and user.update_last_active_slot, then invoke the same validate_perp_market(...) and TVL delta validation helpers used by the other code paths so this branch enforces the same post-mutation invariants prior to the Ok(()) return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/drift/src/controller/pnl/tests.rs`:
- Around line 2650-2775: The test
settle_pnl_no_unsettled_pnl_isolated_zero_balance_returns_no_unsettled_pnl is
testing a zero-balance isolated position but sets open_orders = 1, which
triggers a blocked-transfer path; either rename the test to reflect the
blocked-transfer case or change the fixture to a pure zero-balance scenario by
setting the PerpPosition.open_orders to 0 and keeping
PositionFlag::IsolatedPosition, then run settle_pnl(…) and assert
Err(ErrorCode::NoUnsettledPnl) still holds; locate the PerpPosition in the User
construction (open_orders field) and update it or rename the test function
accordingly.
---
Duplicate comments:
In `@programs/drift/src/controller/pnl.rs`:
- Around line 285-311: The early return after updating isolated balances skips
shared post-mutation invariants; move or add the common validation calls (e.g.,
validate_perp_market and the TVL delta validation used elsewhere in this
function) to run after the balance updates in the
can_transfer_isolated_position_deposit branch and before returning; keep the
existing per-position check validate_perp_position_with_perp_market and
user.update_last_active_slot, then invoke the same validate_perp_market(...) and
TVL delta validation helpers used by the other code paths so this branch
enforces the same post-mutation invariants prior to the Ok(()) return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74cbd3fe-7a6a-4dd9-8b52-83f2ad6cf9ad
📒 Files selected for processing (2)
programs/drift/src/controller/pnl.rsprograms/drift/src/controller/pnl/tests.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/settlePnlIsolatedEarlyReturn.ts (2)
128-131: Harden teardown to avoid masking setup failures.If
beforefails before full initialization, Lines 129-130 can throw and hide the original failure. Guard these calls.Proposed patch
after(async () => { - await driftClient.unsubscribe(); - await eventSubscriber.unsubscribe(); + if (driftClient) { + await driftClient.unsubscribe(); + } + if (eventSubscriber) { + await eventSubscriber.unsubscribe(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/settlePnlIsolatedEarlyReturn.ts` around lines 128 - 131, The teardown in the after hook unconditionally awaits driftClient.unsubscribe() and eventSubscriber.unsubscribe(), which can throw if before failed to initialize them; update the after(async () => { ... }) block to defensively call unsubscribe only when the objects exist and/or their unsubscribe methods are functions, and wrap each call in a try/catch to swallow/log errors so teardown cannot mask the original setup failure (reference driftClient.unsubscribe and eventSubscriber.unsubscribe to locate the code).
175-183: Make the idempotency test independent from prior test state.At Line 179, this relies on cached account state from the previous test. Add an explicit refresh and precondition assertion before settling.
Proposed patch
it('settlePnl idempotent when isolated already 0', async () => { + await driftClient.fetchAccounts(); + assert( + driftClient.getIsolatedPerpPositionTokenAmount(0).eq(ZERO), + 'precondition: isolated should already be 0' + ); + const settlePnlRecordCountBefore = eventSubscriber.getEventsArray('SettlePnlRecord').length;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/settlePnlIsolatedEarlyReturn.ts` around lines 175 - 183, The test relies on stale cached user account state; before calling settlePNL you must explicitly refresh the user's account and assert the isolated PnL is already zero to make the idempotency check independent: add an await call to refresh the user's account cache (e.g. await driftClient.fetchAccounts() or the equivalent refreshUserAccount/fetchUserAccount method) and then assert driftClient.getUserAccount().pnlIsolated === 0 (or the correct property) before invoking driftClient.settlePNL so the test does not depend on prior test state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/settlePnlIsolatedEarlyReturn.ts`:
- Around line 128-131: The teardown in the after hook unconditionally awaits
driftClient.unsubscribe() and eventSubscriber.unsubscribe(), which can throw if
before failed to initialize them; update the after(async () => { ... }) block to
defensively call unsubscribe only when the objects exist and/or their
unsubscribe methods are functions, and wrap each call in a try/catch to
swallow/log errors so teardown cannot mask the original setup failure (reference
driftClient.unsubscribe and eventSubscriber.unsubscribe to locate the code).
- Around line 175-183: The test relies on stale cached user account state;
before calling settlePNL you must explicitly refresh the user's account and
assert the isolated PnL is already zero to make the idempotency check
independent: add an await call to refresh the user's account cache (e.g. await
driftClient.fetchAccounts() or the equivalent
refreshUserAccount/fetchUserAccount method) and then assert
driftClient.getUserAccount().pnlIsolated === 0 (or the correct property) before
invoking driftClient.settlePNL so the test does not depend on prior test state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa3d11eb-5632-491f-afd1-53f1db06d9f2
📒 Files selected for processing (2)
test-scripts/run-anchor-tests.shtests/settlePnlIsolatedEarlyReturn.ts
|
[vulture-outreach-20260308] If useful for downstream integrators, I can ship a compact 24h rollout brief:
Happy to keep this lightweight and practical. |
LukasDeco
changed the title
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/spotSwap.ts`:
- Line 748: The test titled "swap and close token account after end_swap" was
silently disabled with it.skip—restore positive-path coverage by removing the
skip (replace it.skip with it) and then stabilize the test: ensure the swap
lifecycle is awaited to confirmation (use the same confirmation helper used
elsewhere), explicitly sync balances/state before attempting close, add
appropriate timeouts/retries if network confirmations are flaky, and/or add
cleanup to reliably tear down created accounts; if you cannot fully stabilize
here, revert the skip and instead replace it with a focused new test that
performs endSwap then closes the emptied swap token account (or add a TODO/issue
reference in the test with the title string and link to the issue) so the
success path remains covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 113e9883-4761-4e4b-9edc-920f6399f6ac
📒 Files selected for processing (2)
programs/drift/src/controller/pnl/tests.rstests/spotSwap.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- programs/drift/src/controller/pnl/tests.rs
| }); | ||
|
|
||
| it('swap and close token account after end_swap', async () => { | ||
| it.skip('swap and close token account after end_swap', async () => { |
There was a problem hiding this comment.
Please don’t silently disable this regression test.
Changing this to it.skip drops the only visible positive-path coverage here for “endSwap followed by closing the emptied swap token account”. Since the surrounding tests mostly cover failure modes, this can easily mask a real regression. If the test is flaky, please either fix/stabilize it in this PR or leave an explicit issue/TODO explaining the failure mode and add replacement coverage for the success path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/spotSwap.ts` at line 748, The test titled "swap and close token account
after end_swap" was silently disabled with it.skip—restore positive-path
coverage by removing the skip (replace it.skip with it) and then stabilize the
test: ensure the swap lifecycle is awaited to confirmation (use the same
confirmation helper used elsewhere), explicitly sync balances/state before
attempting close, add appropriate timeouts/retries if network confirmations are
flaky, and/or add cleanup to reliably tear down created accounts; if you cannot
fully stabilize here, revert the skip and instead replace it with a focused new
test that performs endSwap then closes the emptied swap token account (or add a
TODO/issue reference in the test with the title string and link to the issue) so
the success path remains covered.
|
[vulture-outreach-20260308] I can still deliver a compact 24h integration brief:
If useful, I can start immediately. |
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
Tests