program: allow transfer between fee and pnl pool#2167
Open
program: allow transfer between fee and pnl pool#2167
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 an admin instruction to move tokens between fee and PnL pools (same- and cross-market), exposes it through the SDK and IDL, adds tests for happy/failure paths, and updates devcontainer, Node, and Rust/tooling setup. Changes
Sequence DiagramsequenceDiagram
participant AdminClient as Admin Client
participant Program as On-chain Program
participant Helper as Transfer Helper
participant SpotCtrl as Spot Balance Controller
AdminClient->>Program: transfer_fee_and_pnl_pool(amount, direction, accounts)
Program->>Program: validate admin & update spot market interest
Program->>Program: load perp markets & spot market/state
Program->>Helper: execute_transfer_between_pools(amount, spot_market, fee_pool, pnl_pool, direction)
Helper->>SpotCtrl: compute token amounts & validate source balance
Helper->>SpotCtrl: transfer_spot_balances(source_pool -> dest_pool)
Helper-->>Program: return success
Program-->>AdminClient: emit events / tx success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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. 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 |
![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
🧹 Nitpick comments (2)
.devcontainer/Dockerfile (1)
50-52: Optimize zsh installation: redundant packages and missing best practices.This block has several inefficiencies:
apt-get updateruns again (already executed on line 17)curlandgitare already installed in lines 19-20- Missing
--no-install-recommends(flagged by static analysis)- Missing
rm -rf /var/lib/apt/lists/*cleanup♻️ Proposed optimization
-RUN apt-get update && apt-get install -y zsh curl git \ +RUN apt-get update && apt-get install -y --no-install-recommends zsh \ && sh -c "$(curl -fsSL https://raw.githubusercontent.com/ohmyzsh/ohmyzsh/master/tools/install.sh)" "" --unattended \ - && chsh -s /usr/bin/zsh root + && chsh -s /usr/bin/zsh root \ + && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/Dockerfile around lines 50 - 52, The RUN installing zsh/oh-my-zsh is redundant and missing best-practices: remove the extra apt-get update and omit curl/git since they're already installed earlier, add --no-install-recommends to apt-get install to avoid extra packages, and ensure you clean up apt lists with rm -rf /var/lib/apt/lists/* after installation; update the RUN that invokes sh -c "$(curl...)" to still run the unattended oh-my-zsh installer and keep chsh -s /usr/bin/zsh root, but do this in a single optimized RUN that only installs zsh (using apt-get install -y --no-install-recommends zsh), then runs the installer and removes apt lists for a smaller, cleaner image.tests/admin.ts (1)
544-685: Comprehensive test coverage for the new admin instruction.The test thoroughly covers all 6 combinations of transfer directions and market configurations:
- Same-market fee↔pnl transfers (cases 1-2)
- Cross-market fee↔pnl transfers in both directions (cases 3-6)
Each case correctly verifies that the source pool decreases and destination pool increases by the exact transfer amount.
Consider adding negative test cases for robustness:
- Attempt transfer with insufficient source pool balance
- Attempt transfer with zero amount (if that should fail)
- Unauthorized caller attempting transfer (non-admin)
These could be deferred to a follow-up PR if the program-level validation is already tested elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/admin.ts` around lines 544 - 685, Add negative tests for transferFeeAndPnlPool: create three new cases that assert the instruction fails — (1) attempt a transfer with amt > source pool balance (use readFeePool/readPnlPool to set/inspect balances and pass a larger BN to transferFeeAndPnlPool), (2) attempt transfer with amt = 0 and assert it rejects if zero-amount transfers should be invalid, and (3) attempt the transfer from a non-admin wallet (use a different client/wallet or omit admin signer) and assert an unauthorized error; ensure each case calls fetchAccounts() only after expecting success cases and uses try/catch or assertion helpers to verify failures.
🤖 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/instructions/admin.rs`:
- Around line 5253-5280: handle_transfer_fee_and_pnl_pool moves funds between
amm.fee_pool and perp_market.pnl_pool but doesn’t update
amm.total_fee_minus_distributions, causing spendable-fee drift for both
same-market and cross-market transfers; update the appropriate perp market's
amm.total_fee_minus_distributions whenever you mutate amm.fee_pool (and do so
for both the same_market branch and the cross-market branch), adjusting by
+amount or -amount depending on TransferFeeAndPnlPoolDirection (e.g., for
FeeToPnlPool decrement total_fee_minus_distributions when removing from
fee_pool; for PnlToFeePool increment when adding), using the same loaded mut
refs (perp_market / perp_market_with_fee_pool) so execute_transfer_between_pools
sees consistent balances.
- Around line 5258-5280: The quote spot market's cumulative interest must be
refreshed on this code path before mutating fee/PnL pools to avoid using a stale
deposit index; call the same update/refresh routine used elsewhere to update the
quote spot market's cumulative interest on spot_market (the same code invoked at
other paths that update the quote spot market) immediately before calling
execute_transfer_between_pools in both the same_market and else branches (i.e.,
before the unsafe fee/pnl pointer usage and before passing fee_pool/pnl_pool to
execute_transfer_between_pools), by invoking the existing function that updates
cumulative interest for spot markets on the loaded spot_market.
In `@sdk/src/idl/drift.json`:
- Around line 9260-9307: The IDL removed the Pyth pull-oracle instructions but
the public methods remain; remove or migrate the following obsolete APIs:
getPostPythPullOracleUpdateAtomicIxs, getUpdatePythPullOracleIxs,
updatePythPullOracle and any internal calls to
postMultiPythPullOracleUpdatesAtomic, postPythPullOracleUpdateAtomic,
updatePythPullOracle; update the module's exported API to drop these functions,
delete or refactor their implementations and any helper serializers, and
search/replace all call sites to either remove usage or replace with the new
supported instruction flows so consumers won't attempt to serialize unknown
instructions.
---
Nitpick comments:
In @.devcontainer/Dockerfile:
- Around line 50-52: The RUN installing zsh/oh-my-zsh is redundant and missing
best-practices: remove the extra apt-get update and omit curl/git since they're
already installed earlier, add --no-install-recommends to apt-get install to
avoid extra packages, and ensure you clean up apt lists with rm -rf
/var/lib/apt/lists/* after installation; update the RUN that invokes sh -c
"$(curl...)" to still run the unattended oh-my-zsh installer and keep chsh -s
/usr/bin/zsh root, but do this in a single optimized RUN that only installs zsh
(using apt-get install -y --no-install-recommends zsh), then runs the installer
and removes apt lists for a smaller, cleaner image.
In `@tests/admin.ts`:
- Around line 544-685: Add negative tests for transferFeeAndPnlPool: create
three new cases that assert the instruction fails — (1) attempt a transfer with
amt > source pool balance (use readFeePool/readPnlPool to set/inspect balances
and pass a larger BN to transferFeeAndPnlPool), (2) attempt transfer with amt =
0 and assert it rejects if zero-amount transfers should be invalid, and (3)
attempt the transfer from a non-admin wallet (use a different client/wallet or
omit admin signer) and assert an unauthorized error; ensure each case calls
fetchAccounts() only after expecting success cases and uses try/catch or
assertion helpers to verify failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9da02c0-73b7-4127-a15f-a542bdc8e668
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.devcontainer/Dockerfile.mocharc.ymlpackage.jsonprograms/drift/src/instructions/admin.rsprograms/drift/src/lib.rsprograms/drift/src/state/events.rssdk/src/adminClient.tssdk/src/idl/drift.jsonsdk/src/types.tstests/admin.ts
Comment on lines
+9260
to
9307
| }, | ||
| { | ||
| "name": "transferFeeAndPnlPool", | ||
| "accounts": [ | ||
| { | ||
| "name": "state", | ||
| "isMut": false, | ||
| "isSigner": false | ||
| }, | ||
| { | ||
| "name": "admin", | ||
| "isMut": false, | ||
| "isSigner": true | ||
| }, | ||
| { | ||
| "name": "perpMarketWithFeePool", | ||
| "isMut": true, | ||
| "isSigner": false | ||
| }, | ||
| { | ||
| "name": "perpMarketWithPnlPool", | ||
| "isMut": true, | ||
| "isSigner": false | ||
| }, | ||
| { | ||
| "name": "spotMarket", | ||
| "isMut": true, | ||
| "isSigner": false | ||
| }, | ||
| { | ||
| "name": "spotMarketVault", | ||
| "isMut": true, | ||
| "isSigner": false | ||
| } | ||
| ], | ||
| "args": [ | ||
| { | ||
| "name": "amount", | ||
| "type": "u64" | ||
| }, | ||
| { | ||
| "name": "direction", | ||
| "type": { | ||
| "defined": "TransferFeeAndPnlPoolDirection" | ||
| } | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Retire the old Pyth pull-oracle SDK entry points in the same change.
This IDL update removes the Pyth pull-oracle instructions, but sdk/src/driftClient.ts:11192-11375 still exposes getPostPythPullOracleUpdateAtomicIxs, getUpdatePythPullOracleIxs, and updatePythPullOracle, which call postMultiPythPullOracleUpdatesAtomic, postPythPullOracleUpdateAtomic, and updatePythPullOracle. Once consumers pick up this IDL, those methods become broken public API and will fail when they try to serialize/send the now-unknown instructions, so they should be removed or migrated in the same PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/idl/drift.json` around lines 9260 - 9307, The IDL removed the Pyth
pull-oracle instructions but the public methods remain; remove or migrate the
following obsolete APIs: getPostPythPullOracleUpdateAtomicIxs,
getUpdatePythPullOracleIxs, updatePythPullOracle and any internal calls to
postMultiPythPullOracleUpdatesAtomic, postPythPullOracleUpdateAtomic,
updatePythPullOracle; update the module's exported API to drop these functions,
delete or refactor their implementations and any helper serializers, and
search/replace all call sites to either remove usage or replace with the new
supported instruction flows so consumers won't attempt to serialize unknown
instructions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
programs/drift/src/instructions/admin.rs (1)
6347-6352: Dropspot_market_vaultunless you validate against it here.The handler never reads this account, so the new instruction expands the IDL and takes an unnecessary writable lock on the quote vault. If the intent was a post-transfer invariant check, wire it into
validate_spot_market_vault_amount; otherwise this account can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/drift/src/instructions/admin.rs` around lines 6347 - 6352, The spot_market_vault account is declared but never read, causing an unnecessary writable lock; either remove the spot_market_vault field from the instruction accounts (and its seeds/bump) or actually validate it: if you intended a post-transfer invariant check, pass the spot_market_vault Account<InterfaceAccount<TokenAccount>> into validate_spot_market_vault_amount (or call a new helper that reads its amount) and perform the comparison there; if you only need to read it, make it non-mut to avoid an extra writable lock.
🤖 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/instructions/admin.rs`:
- Around line 5280-5289: Before subtracting fees in the
TransferFeeAndPnlPoolDirection::FeeToPnlPool branch, enforce the minimum allowed
fee by calling get_total_fee_lower_bound(...) and ensure
perp_market.amm.total_fee_minus_distributions - amount.cast()? does not fall
below that bound; compute new_total =
perp_market.amm.total_fee_minus_distributions.safe_sub(amount.cast()?)?, check
new_total >= get_total_fee_lower_bound(perp_market, ...) (or return an error),
then assign perp_market.amm.total_fee_minus_distributions = new_total; apply the
same guard to the other similar subtraction site (the other FeeToPnlPool block).
- Around line 5335-5372: Before performing the first pool mutation in
transfer_fee_and_pnl_pool, preflight-check the source pool has >= amount to
avoid draining it and causing a later CantUpdateSpotBalanceType failure;
specifically, for TransferFeeAndPnlPoolDirection::FeeToPnlPool verify
fee_pool.balance (the Deposit/available amount) >= amount, and for PnlToFeePool
verify pnl_pool.balance >= amount, returning an explicit insufficient-balance
error if not. Do this check prior to calling
controller::spot_balance::update_spot_balances in each match arm (referencing
amount, fee_pool, pnl_pool, update_spot_balances, SpotBalanceType, and
TransferFeeAndPnlPoolDirection) so the function fails fast with a clear error
instead of relying on rollback.
---
Nitpick comments:
In `@programs/drift/src/instructions/admin.rs`:
- Around line 6347-6352: The spot_market_vault account is declared but never
read, causing an unnecessary writable lock; either remove the spot_market_vault
field from the instruction accounts (and its seeds/bump) or actually validate
it: if you intended a post-transfer invariant check, pass the spot_market_vault
Account<InterfaceAccount<TokenAccount>> into validate_spot_market_vault_amount
(or call a new helper that reads its amount) and perform the comparison there;
if you only need to read it, make it non-mut to avoid an extra writable lock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e035f5c-0a7f-4194-a29a-0c4472b02eb1
📒 Files selected for processing (1)
programs/drift/src/instructions/admin.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/transferFeeAndPnlPool.ts (2)
49-56: Consider making error assertion more specific.The
expectFailhelper only checks for'custom program error'which is generic. For more robust tests, consider matching against the specific error code or message to ensure the correct validation is triggering.Example of more specific error matching
const expectFail = async (fn: () => Promise<any>) => { try { await fn(); assert.fail('Should have thrown'); } catch (e) { - assert(e.message.includes('custom program error')); + assert( + e.message.includes('custom program error') || + e.message.includes('insufficient') || + e.message.includes('exceeds spendable fee pool'), + `Unexpected error: ${e.message}` + ); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transferFeeAndPnlPool.ts` around lines 49 - 56, The expectFail helper currently asserts only that the thrown error includes 'custom program error', which is too generic; update the expectFail function to accept an expected error identifier (string or number) and assert against that (for example check e.message.includes(expectedMessage) or e.code === expectedCode or inspect nested error fields), or add a new assertion inside expectFail that matches the specific program error string/code used by your program (refer to the expectFail helper function and the caught exception variable `e` when modifying this behavior).
246-277: Missing test case for oversized cross-market pnl → fee transfer.The test suite covers oversized transfers for:
- Same-market fee → pnl (Line 246)
- Same-market pnl → fee (Line 257)
- Cross-market fee → pnl (Line 268)
But it's missing the cross-market pnl → fee oversized transfer test for completeness.
Add missing test case
it('rejects oversized fee -> pnl cross market', async () => { await expectFail(() => driftClient.transferFeeAndPnlPool( SOL_PERP, ETH_PERP, hugeAmt, TransferFeeAndPnlPoolDirection.FEE_TO_PNL_POOL ) ); }); + +it('rejects oversized pnl -> fee cross market', async () => { + await expectFail(() => + driftClient.transferFeeAndPnlPool( + SOL_PERP, + ETH_PERP, + hugeAmt, + TransferFeeAndPnlPoolDirection.PNL_TO_FEE_POOL + ) + ); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transferFeeAndPnlPool.ts` around lines 246 - 277, Add a fourth test mirroring the other oversized-transfer tests: create an it block named like "rejects oversized pnl -> fee cross market" that calls driftClient.transferFeeAndPnlPool with SOL_PERP as the source market, ETH_PERP as the destination market, hugeAmt as the amount, and TransferFeeAndPnlPoolDirection.PNL_TO_FEE_POOL as the direction, wrapped in expectFail; this uses the same symbols as the other tests (driftClient.transferFeeAndPnlPool, hugeAmt, TransferFeeAndPnlPoolDirection.PNL_TO_FEE_POOL, expectFail, SOL_PERP, ETH_PERP) so it checks the missing cross-market pnl→fee oversized transfer case.
🤖 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/transferFeeAndPnlPool.ts`:
- Around line 49-56: The expectFail helper currently asserts only that the
thrown error includes 'custom program error', which is too generic; update the
expectFail function to accept an expected error identifier (string or number)
and assert against that (for example check e.message.includes(expectedMessage)
or e.code === expectedCode or inspect nested error fields), or add a new
assertion inside expectFail that matches the specific program error string/code
used by your program (refer to the expectFail helper function and the caught
exception variable `e` when modifying this behavior).
- Around line 246-277: Add a fourth test mirroring the other oversized-transfer
tests: create an it block named like "rejects oversized pnl -> fee cross market"
that calls driftClient.transferFeeAndPnlPool with SOL_PERP as the source market,
ETH_PERP as the destination market, hugeAmt as the amount, and
TransferFeeAndPnlPoolDirection.PNL_TO_FEE_POOL as the direction, wrapped in
expectFail; this uses the same symbols as the other tests
(driftClient.transferFeeAndPnlPool, hugeAmt,
TransferFeeAndPnlPoolDirection.PNL_TO_FEE_POOL, expectFail, SOL_PERP, ETH_PERP)
so it checks the missing cross-market pnl→fee oversized transfer case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4f9e75c-4238-4d5c-868e-81dfc70f5e62
📒 Files selected for processing (3)
programs/drift/src/instructions/admin.rstests/admin.tstests/transferFeeAndPnlPool.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/admin.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/admin.ts (1)
52-52: Remove unnecessaryas anycast.The
startAnchor()function returnsProgramTestContext, which is exactly whatBankrunContextWrapperexpects. Other test files (e.g.,userAccount.tsat lines 63-66) pass the context directly without casting. Theas anysuppresses type safety without benefit.Suggested fix
- bankrunContextWrapper = new BankrunContextWrapper(context as any); + bankrunContextWrapper = new BankrunContextWrapper(context);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/admin.ts` at line 52, Remove the unnecessary type assertion on the context when constructing BankrunContextWrapper: locate where bankrunContextWrapper is assigned with new BankrunContextWrapper(context as any) and change it to new BankrunContextWrapper(context). The startAnchor() return type is ProgramTestContext which matches BankrunContextWrapper's constructor, so drop the "as any" cast to restore proper type checking for BankrunContextWrapper and ensure consistency with other tests like userAccount.ts.
🤖 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/admin.ts`:
- Line 52: Remove the unnecessary type assertion on the context when
constructing BankrunContextWrapper: locate where bankrunContextWrapper is
assigned with new BankrunContextWrapper(context as any) and change it to new
BankrunContextWrapper(context). The startAnchor() return type is
ProgramTestContext which matches BankrunContextWrapper's constructor, so drop
the "as any" cast to restore proper type checking for BankrunContextWrapper and
ensure consistency with other tests like userAccount.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d61ebf79-c649-4005-9eba-ea1b9a65ca59
📒 Files selected for processing (2)
tests/admin.tstests/transferFeeAndPnlPool.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/transferFeeAndPnlPool.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
1 participant
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
Removals
Chores
Tests