Conversation
jordy25519
force-pushed
the
chore/anchor-32
branch
from
2713263 to
f7c2e7e
Compare
* weow it compiels * update deps
jordy25519
force-pushed
the
chore/anchor-32
branch
from
570e5ba to
33a9c8e
Compare
jordy25519
force-pushed
the
chore/anchor-32
branch
from
512db3d to
1169388
Compare
crispheaney
left a comment
crispheaney
left a comment
There was a problem hiding this comment.
Can we confirm all the discriminators are same before and after the change?
✅ Actions performedReview triggered.
|
|
Important Review skippedToo many files! This PR contains 293 files, which is 143 over the limit of 150. You can disable this status message by setting the Use the checkbox below for a quick retry:
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
programs/drift/src/instructions/if_staker.rs (1)
489-513:⚠️ Potential issue | 🔴 CriticalAdd length check before discriminator comparison to prevent panic on malformed instructions.
The unchecked slice
&ix.data[0..8]on line 509 will panic ifix.data.len() < 8. Since this code validates cross-program instructions from the transaction sysvar, an attacker could construct a transaction with a Drift instruction containing fewer than 8 bytes of data, causing a transaction abort instead of a controlled program error.This same issue appears in 3 other instruction handlers:
programs/drift/src/instructions/user.rs:3880programs/drift/src/instructions/keeper.rs:1712programs/drift/src/instructions/lp_admin.rs:733Proposed fix (length-safe discriminator compare)
- let discriminator = crate::instruction::EndInsuranceFundSwap::DISCRIMINATOR; - validate!( - &ix.data[0..8] == discriminator, - ErrorCode::InvalidSwap, - "last drift ix must be end of swap" - )?; + let discriminator = crate::instruction::EndInsuranceFundSwap::DISCRIMINATOR; + validate!( + ix.data.len() >= discriminator.len() + && &ix.data[..discriminator.len()] == discriminator, + ErrorCode::InvalidSwap, + "last drift ix must be end of swap" + )?;programs/drift/src/instructions/admin.rs (1)
2138-2155:⚠️ Potential issue | 🟡 MinorPotential divide-by-zero when computing
cumulative_deposit_interest_after.
safe_div(spot_market.deposit_balance())?will fail ifdeposit_balance()is 0. Maybe that’s impossible by invariant for this instruction, but if it’s reachable it’ll be a confusing math error.Consider a targeted guard (and reusing the value so the getter isn’t called twice):
Proposed fix
let token_precision = spot_market.get_precision(); let cumulative_deposit_interest_before = spot_market.cumulative_deposit_interest(); + let deposit_balance = spot_market.deposit_balance(); + validate!( + deposit_balance > 0, + ErrorCode::DefaultError, + "spot_market.deposit_balance must be > 0" + )?; let cumulative_deposit_interest_after = deposit_token_amount_after .safe_mul(SPOT_CUMULATIVE_INTEREST_PRECISION)? - .safe_div(spot_market.deposit_balance())? + .safe_div(deposit_balance)? .safe_mul(SPOT_BALANCE_PRECISION)? .safe_div(token_precision.cast()?)?;programs/drift/src/math/funding.rs (1)
45-48:⚠️ Potential issue | 🟠 MajorFix unchecked narrowing cast for funding PnL.
uncapped_funding_pnlisi128, butnet_revenue_since_last_fundingisi64. The castuncapped_funding_pnl as i64silently truncates on overflow. Use the codebase'sCasttrait for checked conversion:- .safe_add(uncapped_funding_pnl as i64)?; + .safe_add(uncapped_funding_pnl.cast::<i64>()?)?;This follows the pattern already used elsewhere in the file (e.g., line 104) and will return a
CastingFailureerror if the value exceedsi64::MAX, preventing silent accounting corruption.
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 32-34: The package upgrades for `@switchboard-xyz/common` and
`@switchboard-xyz/on-demand` are major bumps; review their changelogs/migration
guides and confirm all breaking changes are handled: search the repo for usages
of "@switchboard-xyz/common" and "@switchboard-xyz/on-demand" (imports, types,
constructors, and method names), update any renamed APIs, adjust
types/signatures in Switchboard client code and tests (e.g., constructors or
method calls in your Switchboard client wrappers), run the TypeScript build and
unit/integration tests, and add any necessary adapters or code changes to match
the new major-version APIs.
In `@programs/drift/Cargo.toml`:
- Around line 34-49: Pin the git-based dependencies to immutable revisions: for
pyth-lazer-solana-contract, pyth-solana-receiver-sdk, pythnet-sdk, serum_dex,
and phoenix-v1 add a rev = "<commit-hash>" field (following the pattern used by
drift-macros) instead of relying on branch names or bare git URLs; update the
Cargo.toml entries for those package keys to include the specific commit SHA to
ensure reproducible, auditable builds.
In `@programs/drift/src/instructions/admin.rs`:
- Around line 1256-1260: The error message in
handle_delete_initialized_spot_market is out of sync: the validation uses
spot_market.deposit_balance() == 0 but the logged string says "number_of_users";
update the require/validation call that references
ErrorCode::InvalidMarketAccountforDeletion to use a clear message about
deposit_balance (e.g., "deposit_balance={} != 0") and include
spot_market.deposit_balance() so the logged error reflects the actual checked
field.
In `@programs/drift/src/instructions/keeper.rs`:
- Around line 1710-1712: The code slices ix.data[0..8] to compare against
LiquidateSpotWithSwapEnd::DISCRIMINATOR which will panic if ix.data is shorter
than 8 bytes; update the guard to first check ix.data.len() >= 8 (or use
ix.data.get(..8) and handle None) before performing the slice/comparison so the
validate! call never receives an out-of-bounds slice; ensure the check occurs
immediately before the line that references ix.data[0..8] (the comparison
against LiquidateSpotWithSwapEnd::DISCRIMINATOR) and return or fail validation
if the length is too short.
In `@programs/drift/src/instructions/lp_admin.rs`:
- Around line 731-736: The current discriminator check slices ix.data[0..8]
which will panic if ix.data is shorter than 8 bytes; before slicing, verify
ix.data.len() >= discriminator.len() (use
crate::instruction::EndLpSwap::DISCRIMINATOR and discriminator.len()) and return
ErrorCode::InvalidSwap via the existing validate! flow if the length is too
small, then compare &ix.data[0..discriminator.len()] == discriminator; apply the
same pattern to other files using similar discriminator checks (e.g., user.rs,
keeper.rs, if_staker.rs).
In `@programs/drift/src/state/events.rs`:
- Around line 912-922: Update the doc comment above the
_SignedMsgOrderParamsExport placeholder to correct the typo: change "unusued" to
"unused" so the comment reads e.g. "unused placeholder event to force include
signed msg types into drift IDL"; locate the comment near the struct
_SignedMsgOrderParamsExport and adjust only the comment text (no code changes to
the struct or impl Size).
🧹 Nitpick comments (13)
.github/workflows/main.yml (2)
76-79: Updateactions/setup-nodefrom v2 to v4.Static analysis flags
actions/setup-node@v2as outdated. This action is used in multiple jobs (lines 77, 92, 135, 169, 232) whileactions/checkoutwas already updated to v4. Update to v4 for consistency and better compatibility with Node 22.x.♻️ Proposed fix
- name: Setup node - uses: actions/setup-node@v2 + uses: actions/setup-node@v4 with: node-version: ${{ env.NODE_VERSION }}Apply similar changes at lines 92, 135, 169, and 232.
126-132: Cache bypass defeats caching purpose.The cache-hit check is commented out (line 127), causing
cargo install avm --forceto run on every build even when the Anchor binary is cached. This adds unnecessary build time.♻️ Proposed fix to restore cache utilization
- name: Install Anchor CLI - # if: steps.cache-anchor.outputs.cache-hit != 'true' + if: steps.cache-anchor.outputs.cache-hit != 'true' run: | sudo apt update -y && sudo apt install libudev-dev cargo install --git https://github.com/coral-xyz/anchor avm --force avm install ${{ env.ANCHOR_VERSION }} avm use ${{ env.ANCHOR_VERSION }}If AVM is intentionally needed on every run for
avm use, consider splitting into two steps—one for installation (cached) and one for activation.programs/drift/src/instructions/optional_accounts.rs (1)
293-297: Consider aligning with the slice-based pattern used elsewhere.Lines 137, 167, and 263 use
&data[..8]for discriminator extraction, but this location still usesarray_ref!. While functionally correct (the comparison works via coercion), unifying the pattern would improve consistency and potentially allow removing thearrayrefimport if it's not used elsewhere.♻️ Optional: Use slice extraction for consistency
let borrowed_data = account_info.data.borrow(); - let account_discriminator = array_ref![&borrowed_data, 0, 8]; + let account_discriminator = &borrowed_data[..8]; if account_discriminator != RevenueShareEscrow::DISCRIMINATOR {If
array_refis no longer used elsewhere in this file, you could also remove the import on line 28..github/actions/setup-solana/scripts/solana-install-init.sh (1)
105-105: Consider renaming internal variable for consistency.The local variable
solana_install_initstill uses the old naming while the actual binary and references have been updated toagave-install-init. For internal consistency, consider renaming this variable.♻️ Optional: Rename variable for consistency
- solana_install_init="$temp_dir/agave-install-init" + agave_install_init="$temp_dir/agave-install-init"And update all references to the variable (lines 110-114, 120, 122, 126):
- ensure downloader "$download_url" "$solana_install_init" - ensure chmod u+x "$solana_install_init" - if [ ! -x "$solana_install_init" ]; then - printf '%s\n' "Cannot execute $solana_install_init (likely because of mounting /tmp as noexec)." 1>&2 + ensure downloader "$download_url" "$agave_install_init" + ensure chmod u+x "$agave_install_init" + if [ ! -x "$agave_install_init" ]; then + printf '%s\n' "Cannot execute $agave_install_init (likely because of mounting /tmp as noexec)." 1>&2- ignore "$solana_install_init" $SOLANA_INSTALL_INIT_ARGS + ignore "$agave_install_init" $SOLANA_INSTALL_INIT_ARGS- ignore "$solana_install_init" "$@" + ignore "$agave_install_init" "$@"- ignore rm "$solana_install_init" + ignore rm "$agave_install_init"programs/drift/src/math/repeg.rs (1)
300-304: Consider cachingsqrt_k()result.The
sqrt_k()getter is called twice in quick succession. While the compiler likely inlines this, caching in a local variable would make the intent clearer and avoid any overhead if the getter becomes more complex in the future.♻️ Optional refactor
let new_sqrt_k_lower_bound = market.amm.get_lower_bound_sqrt_k()?; + let current_sqrt_k = market.amm.sqrt_k(); let new_sqrt_k = market .amm - .sqrt_k() - .safe_sub(market.amm.sqrt_k().safe_div(1000)?)? + .sqrt_k() + .safe_sub(current_sqrt_k.safe_div(1000)?)? .max(new_sqrt_k_lower_bound);programs/drift/src/lib.rs (1)
1-1: Consider declaring expected cfgs explicitly in Cargo.toml instead of blanket suppression.The crate-level
#![allow(unexpected_cfgs)]attribute suppresses warnings about unrecognized configuration predicates across the entire crate. While all cfgs currently in use are legitimate (built-intestand defined features likemainnet-beta,cpi,no-entrypoint,anchor-test,drift-rs, andidl-build), blanket suppression is overly broad and could hide typos if misconfigurations are introduced later.For better maintainability, declare expected cfgs explicitly in
Cargo.toml:[lints.rust] unexpected_cfgs = { level = "warn", check-cfg = ['cfg(test)', 'cfg(feature, values("mainnet-beta", "cpi", "no-entrypoint", "anchor-test", "drift-rs", "idl-build"))'] }This documents which cfgs are intentional while still catching accidental typos in other predicates.
package.json (1)
99-99: Missing newline at end of file.The file is missing a trailing newline, which is a common convention for text files and helps with version control diffs.
programs/drift/Cargo.toml (1)
15-26: Makeproc-macro2gating explicit (or drop the redundant feature flag).
proc-macro2already enablesspan-locationsunconditionally in dependencies, so theidl-buildentry doesn’t change behavior. If the intent is to only enable it for IDL builds, markproc-macro2as optional and gate it; otherwise remove it from the feature list to avoid confusion.♻️ Optional cleanup (remove redundant feature flag)
idl-build = [ "anchor-lang/idl-build", "anchor-spl/idl-build", "openbook-v2-light/idl-build", "pythnet-sdk/idl-build", "switchboard-on-demand/idl-build", - "proc-macro2/span-locations" ]Also applies to: 54-54
programs/drift/src/controller/lp.rs (1)
42-62: Consider computing the new per-LP values before calling setters (readability + avoids borrow subtleties).Current code relies on Rust’s two-phase borrow behavior (
set_*receiver + getter in the arg). It should work, but it’s fragile to future refactors and harder to read.Proposed refactor (no behavior change)
if expo_diff > 0 { - perp_market.amm.set_base_asset_amount_per_lp(perp_market - .amm - .base_asset_amount_per_lp() - .safe_mul(rebase_divisor)?); - - perp_market.amm.set_quote_asset_amount_per_lp(perp_market - .amm - .quote_asset_amount_per_lp() - .safe_mul(rebase_divisor)?); + let new_base = perp_market + .amm + .base_asset_amount_per_lp() + .safe_mul(rebase_divisor)?; + perp_market.amm.set_base_asset_amount_per_lp(new_base); + + let new_quote = perp_market + .amm + .quote_asset_amount_per_lp() + .safe_mul(rebase_divisor)?; + perp_market.amm.set_quote_asset_amount_per_lp(new_quote); } else { - perp_market.amm.set_base_asset_amount_per_lp(perp_market - .amm - .base_asset_amount_per_lp() - .safe_div(rebase_divisor)?); - - perp_market.amm.set_quote_asset_amount_per_lp(perp_market - .amm - .quote_asset_amount_per_lp() - .safe_div(rebase_divisor)?); + let new_base = perp_market + .amm + .base_asset_amount_per_lp() + .safe_div(rebase_divisor)?; + perp_market.amm.set_base_asset_amount_per_lp(new_base); + + let new_quote = perp_market + .amm + .quote_asset_amount_per_lp() + .safe_div(rebase_divisor)?; + perp_market.amm.set_quote_asset_amount_per_lp(new_quote); }programs/drift/src/instructions/lp_pool.rs (1)
721-883:set_last_aum(...)updates are clearer with thelast_aumsnapshot; consider pre-checkingmax_aumbefore writing (optional).The new pattern:
- snapshot
let last_aum = lp_pool.last_aum();- compute delta notional
lp_pool.set_last_aum(last_aum.safe_add(delta)?)…is a good move for consistency. One optional tightening: compute
new_aumand validatenew_aum <= lp_pool.max_aum()before callingset_last_aum(new_aum)(avoids a write+read pattern; still reverts either way on Solana).programs/drift/src/controller/isolated_position.rs (1)
132-151: Prefer SpotMarket getters over direct field reads +.into()for DepositRecord market fields.Right now you’re reading
spot_market.deposit_balance/borrow_balance/cumulative_*directly and converting with.into(). Given the rest of the PR guidance (“prefer generated getter/setter methods”), it’d be safer/clearer to use accessors here too (and reduces the chance of accidentally recording the raw compat field later if types change again).Proposed refactor
oracle_price, - market_deposit_balance: spot_market.deposit_balance.into(), - market_withdraw_balance: spot_market.borrow_balance.into(), - market_cumulative_deposit_interest: spot_market.cumulative_deposit_interest.into(), - market_cumulative_borrow_interest: spot_market.cumulative_borrow_interest.into(), + market_deposit_balance: spot_market.deposit_balance(), + market_withdraw_balance: spot_market.borrow_balance(), + market_cumulative_deposit_interest: spot_market.cumulative_deposit_interest(), + market_cumulative_borrow_interest: spot_market.cumulative_borrow_interest(), total_deposits_after, total_withdraws_after, market_index: spot_market_index,market_index: spot_market_index, - market_deposit_balance: spot_market.deposit_balance.into(), - market_withdraw_balance: spot_market.borrow_balance.into(), - market_cumulative_deposit_interest: spot_market.cumulative_deposit_interest.into(), - market_cumulative_borrow_interest: spot_market.cumulative_borrow_interest.into(), + market_deposit_balance: spot_market.deposit_balance(), + market_withdraw_balance: spot_market.borrow_balance(), + market_cumulative_deposit_interest: spot_market.cumulative_deposit_interest(), + market_cumulative_borrow_interest: spot_market.cumulative_borrow_interest(), total_deposits_after: user.total_deposits, total_withdraws_after: user.total_withdraws,Also applies to: 428-447
programs/drift/src/controller/pnl/tests.rs (1)
2266-2267: Prefer setters forexpected_marketupdates to stay consistent with new accessors.These blocks still mutate fields directly; using setters aligns with the rest of the file and reduces future breakage if fields are hidden.
♻️ Suggested refactor
- expected_market.pnl_pool.scaled_balance = (100 * SPOT_BALANCE_PRECISION).into(); - expected_market.amm.quote_asset_amount = (-100 * QUOTE_PRECISION_I128).into(); + expected_market.pnl_pool.set_scaled_balance(100 * SPOT_BALANCE_PRECISION); + expected_market.amm.set_quote_asset_amount(-100 * QUOTE_PRECISION_I128); - expected_market.pnl_pool.scaled_balance = (25 * SPOT_BALANCE_PRECISION).into(); - expected_market.amm.quote_asset_amount = (-175 * QUOTE_PRECISION_I128).into(); + expected_market.pnl_pool.set_scaled_balance(25 * SPOT_BALANCE_PRECISION); + expected_market.amm.set_quote_asset_amount(-175 * QUOTE_PRECISION_I128);Also applies to: 2400-2401
programs/drift/src/controller/position/tests.rs (1)
2503-2511: Inconsistent field access vs getter method usage.Line 2504 uses direct field access (
base_asset_amount_with_unsettled_lpwithout parentheses), while the analogous code at line 2252 uses the getter method (base_asset_amount_with_unsettled_lp()). This creates inconsistent type handling:
- Line 2252 (getter) returns
i128, used directly insafe_addat line 2255- Line 2504 (field) returns wrapped type, requiring
.as_i128()at line 2508For consistency with the getter/setter migration pattern, consider using the getter method here as well.
♻️ Suggested fix for consistency
let base_asset_amount_with_amm = perp_market.amm.base_asset_amount_with_amm; -let base_asset_amount_with_unsettled_lp = perp_market.amm.base_asset_amount_with_unsettled_lp; +let base_asset_amount_with_unsettled_lp = perp_market.amm.base_asset_amount_with_unsettled_lp(); perp_market.amm.set_base_asset_amount_with_amm( base_asset_amount_with_amm .as_i128() - .safe_add(base_asset_amount_with_unsettled_lp.as_i128()) + .safe_add(base_asset_amount_with_unsettled_lp) .unwrap(), );
| "@switchboard-xyz/common": "5.6.1", | ||
| "@switchboard-xyz/on-demand": "3.8.2", | ||
| "anchor-bankrun": "0.5.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the package.json file to see the current state
cat -n package.json | head -50Repository: drift-labs/protocol-v2
Length of output: 1970
🏁 Script executed:
# Search for all Switchboard package references in the codebase
rg -n '@switchboard-xyz' -C 2Repository: drift-labs/protocol-v2
Length of output: 32602
🏁 Script executed:
# Check git history to confirm previous versions if available
git log --oneline --all package.json | head -20Repository: drift-labs/protocol-v2
Length of output: 975
🏁 Script executed:
# Look for any migration docs or comments related to Switchboard
rg -n -i 'switchboard|migration|breaking.change' -C 1Repository: drift-labs/protocol-v2
Length of output: 50378
🏁 Script executed:
# Check git history for package.json changes to find previous versions
git log -p package.json | grep -A 5 -B 5 "@switchboard-xyz" | head -100Repository: drift-labs/protocol-v2
Length of output: 48
🏁 Script executed:
# Search for commits that mention the specific versions claimed
git log --all --oneline | grep -i "switchboard\|3.0.14\|2.4.1" | head -20Repository: drift-labs/protocol-v2
Length of output: 1050
🏁 Script executed:
# Look for the actual previous version by checking a broader git history
git log --all -S "@switchboard-xyz/common" --oneline | head -10Repository: drift-labs/protocol-v2
Length of output: 152
🏁 Script executed:
# Check specific commit that mentions version bump
git show 85c32101 --statRepository: drift-labs/protocol-v2
Length of output: 526
🏁 Script executed:
# Look for any old package.json versions in recent history
git log --oneline -20 package.jsonRepository: drift-labs/protocol-v2
Length of output: 80
🏁 Script executed:
# Search for version patterns in commit messages
git log --all --grep="switchboard" --oneline | head -15Repository: drift-labs/protocol-v2
Length of output: 777
🏁 Script executed:
# Get the detailed diff for the version bump commit
git show 85c32101 -- package.json sdk/package.jsonRepository: drift-labs/protocol-v2
Length of output: 1169
🏁 Script executed:
# Check the parent commit to see what versions were before
git show 85c32101^:package.json | grep -A 2 "@switchboard-xyz"Repository: drift-labs/protocol-v2
Length of output: 157
🏁 Script executed:
# Also search for any reference to old versions in recent commits
git log -p -S "3.0.14" --all | head -50Repository: drift-labs/protocol-v2
Length of output: 48
🏁 Script executed:
# Now find when the current versions (5.6.1 and 3.8.2) were introduced
git log --oneline -p package.json | grep -B 10 "5.6.1" | head -30Repository: drift-labs/protocol-v2
Length of output: 365
🏁 Script executed:
# Check the timeline - when did 5.6.1 and 3.8.2 get added
git log --all --oneline -- package.json | grep -A 5 -B 5 "85c32101"Repository: drift-labs/protocol-v2
Length of output: 599
🏁 Script executed:
# Get the most recent package.json to confirm current versions
git show HEAD:package.json | grep "@switchboard-xyz"Repository: drift-labs/protocol-v2
Length of output: 144
Verify Switchboard package compatibility with major version updates.
The version changes are accurate: @switchboard-xyz/common has jumped from 3.0.14 to 5.6.1 (two major versions), and @switchboard-xyz/on-demand from 2.4.1 to 3.8.2 (one major version). While the codebase's Switchboard client implementations and imports appear to be functioning with these versions, major version bumps warrant explicit verification that API changes have been fully integrated. Review the changelog or migration guides for these packages to confirm all breaking changes are properly addressed in the Drift codebase.
🤖 Prompt for AI Agents
In `@package.json` around lines 32 - 34, The package upgrades for
`@switchboard-xyz/common` and `@switchboard-xyz/on-demand` are major bumps; review
their changelogs/migration guides and confirm all breaking changes are handled:
search the repo for usages of "@switchboard-xyz/common" and
"@switchboard-xyz/on-demand" (imports, types, constructors, and method names),
update any renamed APIs, adjust types/signatures in Switchboard client code and
tests (e.g., constructors or method calls in your Switchboard client wrappers),
run the TypeScript build and unit/integration tests, and add any necessary
adapters or code changes to match the new major-version APIs.
| pyth-lazer-solana-contract = { git = "https://github.com/jordy25519/pyth-crosschain", features = ["no-entrypoint"]} | ||
| pyth-solana-receiver-sdk = { git = "https://github.com/jordy25519/pyth-crosschain" } | ||
| pythnet-sdk = { git = "https://github.com/jordy25519/pyth-crosschain" } | ||
| bytemuck = "1.4.0" | ||
| hex = "0.4.3" | ||
| num-traits = "0.2" | ||
| uint = { version = "0.9.1", default-features = false } | ||
| num-integer = "0.1.44" | ||
| num-traits = "0.2" | ||
| arrayref = "0.3.6" | ||
| base64 = "0.13.0" | ||
| serum_dex = { git = "https://github.com/project-serum/serum-dex", rev = "85b4f14", version = "0.5.6", features = ["no-entrypoint"] } | ||
| serum_dex = { git = "https://github.com/drift-labs/serum-dex", branch = "master", features = ["no-entrypoint"] } | ||
| enumflags2 = "0.6.4" | ||
| phoenix-v1 = { git = "https://github.com/drift-labs/phoenix-v1", rev = "7703c5", version = "0.2.4", features = ["no-entrypoint"] } | ||
| phoenix-v1 = { git = "https://github.com/jordy25519/phoenix-v1", branch = "master", features = ["no-entrypoint"] } | ||
| solana-security-txt = "1.1.0" | ||
| static_assertions = "1.1.0" | ||
| drift-macros = { git = "https://github.com/drift-labs/drift-macros.git", rev = "c57d87" } | ||
| drift-macros = { git = "https://github.com/drift-labs/drift-macros", rev = "6e246ba" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the Cargo.toml file
find . -path "*/programs/drift/Cargo.toml" -type fRepository: drift-labs/protocol-v2
Length of output: 93
🏁 Script executed:
# Read the Cargo.toml file to see the dependencies
cat -n ./programs/drift/Cargo.tomlRepository: drift-labs/protocol-v2
Length of output: 2410
Pin all git dependencies to immutable revisions for reproducible, auditable builds.
The repository has five unpinned git dependencies that should be pinned:
pyth-lazer-solana-contract,pyth-solana-receiver-sdk, andpythnet-sdkhave no rev/tagserum_dexandphoenix-v1trackmasterbranch
Without pinning, these can change unexpectedly during lockfile updates, complicating audits and potentially breaking builds. Follow the pattern already used for drift-macros by adding rev = "<commit>" to each dependency.
🤖 Prompt for AI Agents
In `@programs/drift/Cargo.toml` around lines 34 - 49, Pin the git-based
dependencies to immutable revisions: for pyth-lazer-solana-contract,
pyth-solana-receiver-sdk, pythnet-sdk, serum_dex, and phoenix-v1 add a rev =
"<commit-hash>" field (following the pattern used by drift-macros) instead of
relying on branch names or bare git URLs; update the Cargo.toml entries for
those package keys to include the specific commit SHA to ensure reproducible,
auditable builds.
| spot_market.deposit_balance() == 0, | ||
| ErrorCode::InvalidMarketAccountforDeletion, | ||
| "spot_market.number_of_users={} != 0", | ||
| spot_market.deposit_balance, | ||
| spot_market.deposit_balance(), | ||
| )?; |
There was a problem hiding this comment.
Stale error message in handle_delete_initialized_spot_market for deposit balance check.
The validation is now spot_market.deposit_balance() == 0, but the error string still references number_of_users, which will be confusing during incident/debugging.
Proposed fix
validate!(
spot_market.deposit_balance() == 0,
ErrorCode::InvalidMarketAccountforDeletion,
- "spot_market.number_of_users={} != 0",
+ "spot_market.deposit_balance={} != 0",
spot_market.deposit_balance(),
)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spot_market.deposit_balance() == 0, | |
| ErrorCode::InvalidMarketAccountforDeletion, | |
| "spot_market.number_of_users={} != 0", | |
| spot_market.deposit_balance, | |
| spot_market.deposit_balance(), | |
| )?; | |
| spot_market.deposit_balance() == 0, | |
| ErrorCode::InvalidMarketAccountforDeletion, | |
| "spot_market.deposit_balance={} != 0", | |
| spot_market.deposit_balance(), | |
| )?; |
🤖 Prompt for AI Agents
In `@programs/drift/src/instructions/admin.rs` around lines 1256 - 1260, The error
message in handle_delete_initialized_spot_market is out of sync: the validation
uses spot_market.deposit_balance() == 0 but the logged string says
"number_of_users"; update the require/validation call that references
ErrorCode::InvalidMarketAccountforDeletion to use a clear message about
deposit_balance (e.g., "deposit_balance={} != 0") and include
spot_market.deposit_balance() so the logged error reflects the actual checked
field.
| let discriminator = crate::instruction::LiquidateSpotWithSwapEnd::DISCRIMINATOR; | ||
| validate!( | ||
| ix.data[0..8] == discriminator, | ||
| &ix.data[0..8] == discriminator, |
There was a problem hiding this comment.
Guard discriminator slicing to avoid a panic on short instruction data.
ix.data[0..8] will panic if the instruction data is shorter than 8 bytes. Add a length check before slicing.
🛠️ Proposed fix
// must be the SwapEnd instruction
let discriminator = crate::instruction::LiquidateSpotWithSwapEnd::DISCRIMINATOR;
+ let disc_len = discriminator.len();
+ validate!(
+ ix.data.len() >= disc_len,
+ ErrorCode::InvalidLiquidateSpotWithSwap,
+ "swap end ix data too short for discriminator"
+ )?;
validate!(
- &ix.data[0..8] == discriminator,
+ &ix.data[..disc_len] == discriminator,
ErrorCode::InvalidLiquidateSpotWithSwap,
"last drift ix must be end of swap"
)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let discriminator = crate::instruction::LiquidateSpotWithSwapEnd::DISCRIMINATOR; | |
| validate!( | |
| ix.data[0..8] == discriminator, | |
| &ix.data[0..8] == discriminator, | |
| let discriminator = crate::instruction::LiquidateSpotWithSwapEnd::DISCRIMINATOR; | |
| let disc_len = discriminator.len(); | |
| validate!( | |
| ix.data.len() >= disc_len, | |
| ErrorCode::InvalidLiquidateSpotWithSwap, | |
| "swap end ix data too short for discriminator" | |
| )?; | |
| validate!( | |
| &ix.data[..disc_len] == discriminator, | |
| ErrorCode::InvalidLiquidateSpotWithSwap, | |
| "last drift ix must be end of swap" | |
| )?; |
🤖 Prompt for AI Agents
In `@programs/drift/src/instructions/keeper.rs` around lines 1710 - 1712, The code
slices ix.data[0..8] to compare against LiquidateSpotWithSwapEnd::DISCRIMINATOR
which will panic if ix.data is shorter than 8 bytes; update the guard to first
check ix.data.len() >= 8 (or use ix.data.get(..8) and handle None) before
performing the slice/comparison so the validate! call never receives an
out-of-bounds slice; ensure the check occurs immediately before the line that
references ix.data[0..8] (the comparison against
LiquidateSpotWithSwapEnd::DISCRIMINATOR) and return or fail validation if the
length is too short.
| let discriminator = crate::instruction::EndLpSwap::DISCRIMINATOR; | ||
| validate!( | ||
| ix.data[0..8] == discriminator, | ||
| &ix.data[0..8] == discriminator, | ||
| ErrorCode::InvalidSwap, | ||
| "last drift ix must be end of swap" | ||
| )?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the specific lines
cat -n programs/drift/src/instructions/lp_admin.rs | sed -n '725,745p'Repository: drift-labs/protocol-v2
Length of output: 1048
🏁 Script executed:
# Search for the DISCRIMINATOR definition in the codebase
rg "DISCRIMINATOR" programs/drift/src/instructions/lp_admin.rs -B 2 -A 2Repository: drift-labs/protocol-v2
Length of output: 263
🏁 Script executed:
# Check the Anchor instruction macro and DISCRIMINATOR definition
find . -name "*.rs" -type f | xargs rg "const DISCRIMINATOR" | head -20Repository: drift-labs/protocol-v2
Length of output: 186
🏁 Script executed:
# Look for EndLpSwap definition
rg "struct EndLpSwap\|enum EndLpSwap" -A 5 programs/drift/src/Repository: drift-labs/protocol-v2
Length of output: 48
🏁 Script executed:
# Search more broadly for EndLpSwap
rg "EndLpSwap" programs/drift/src/ --type rust -B 2 -A 5Repository: drift-labs/protocol-v2
Length of output: 723
🏁 Script executed:
# Check how discriminator validation is done elsewhere in this file
rg "DISCRIMINATOR" programs/drift/src/instructions/lp_admin.rs -B 3 -A 3Repository: drift-labs/protocol-v2
Length of output: 333
🏁 Script executed:
# Look for other instruction data slicing patterns to understand conventions
rg "&ix\.data\[0\.\." programs/drift/src/instructions/ --type rust -B 1 -A 1 | head -40Repository: drift-labs/protocol-v2
Length of output: 1052
🏁 Script executed:
# Check the actual EndLpSwap definition in the instructions
fd "*.rs" programs/drift/src/instructions/ -exec rg -l "EndLpSwap" {} \;Repository: drift-labs/protocol-v2
Length of output: 551
🏁 Script executed:
# Fix the fd command - search for files containing EndLpSwap
fd "\.rs" programs/drift/src/instructions/ | xargs rg "EndLpSwap" -lRepository: drift-labs/protocol-v2
Length of output: 109
🏁 Script executed:
# Search for EndLpSwap definition more directly
rg "struct EndLpSwap|pub struct EndLpSwap" programs/drift/ --type rust -A 10Repository: drift-labs/protocol-v2
Length of output: 48
🏁 Script executed:
# Check the context around the slicing - is there any validation of ix.data length before?
cat -n programs/drift/src/instructions/lp_admin.rs | sed -n '700,750p'Repository: drift-labs/protocol-v2
Length of output: 2195
🏁 Script executed:
# Look for how other similar validation patterns handle length checking
rg "ix\.data\.len()" programs/drift/src/instructions/ --type rust -B 2 -A 2Repository: drift-labs/protocol-v2
Length of output: 48
🏁 Script executed:
# Search for load_instruction_at_checked to understand what it guarantees
rg "load_instruction_at_checked" programs/drift/src/ --type rust -B 5 -A 5Repository: drift-labs/protocol-v2
Length of output: 9573
🏁 Script executed:
# Check if this is from solana_program crate
rg "use.*load_instruction_at_checked|fn load_instruction_at_checked" programs/drift/src/ --type rustRepository: drift-labs/protocol-v2
Length of output: 48
🏁 Script executed:
# Search for any comments about instruction data minimum length
rg "instruction.*data|discriminator" programs/drift/src/lib.rs --type rust -B 2 -A 2 | head -30Repository: drift-labs/protocol-v2
Length of output: 390
🏁 Script executed:
# Search for Anchor instruction format documentation or comments
rg "Instruction|discriminator" programs/drift/src/ --type rust -B 1 -A 1 | grep -i "8\|byte\|length" | head -20Repository: drift-labs/protocol-v2
Length of output: 2153
🏁 Script executed:
# Check if instructions are created with minimum data requirements
rg "Instruction\s*{" programs/drift/src/ --type rust -A 5 | head -40Repository: drift-labs/protocol-v2
Length of output: 159
🏁 Script executed:
# Look for any defensive checks on instruction data in similar contexts
rg "\.data\.len()" programs/drift/src/ --type rust -B 2 -A 2Repository: drift-labs/protocol-v2
Length of output: 531
🏁 Script executed:
# Search for Solana documentation comments about instruction data format
rg "solana_program::instruction::Instruction" programs/drift/src/ --type rust -B 3 -A 3 | head -30Repository: drift-labs/protocol-v2
Length of output: 1955
🏁 Script executed:
# Verify the exact type and length of DISCRIMINATOR
rg "pub const DISCRIMINATOR" programs/drift/src/ --type rust -B 2 -A 2Repository: drift-labs/protocol-v2
Length of output: 48
🏁 Script executed:
# Check if there are any existing panic guards in the codebase for instruction data
rg "panic|unwrap|expect" programs/drift/src/instructions/lp_admin.rs --type rust -B 2 -A 2 | head -30Repository: drift-labs/protocol-v2
Length of output: 1161
🏁 Script executed:
# Look for how Anchor defines DISCRIMINATOR size
rg "DISCRIMINATOR.*8|DISCRIMINATOR.*\[u8\].*8" . --type rust | head -20Repository: drift-labs/protocol-v2
Length of output: 321
🏁 Script executed:
# Search for any comments or issues related to instruction data validation
rg "instruction.*data.*length|instruction.*size|minimum.*data" programs/drift/src/ --type rust -B 1 -A 1Repository: drift-labs/protocol-v2
Length of output: 1655
🏁 Script executed:
# Check Solana instruction structure - look for any minimum size requirements
rg "Instruction" . --type rust -B 2 -A 5 | grep -i "size\|len\|byte" | head -20Repository: drift-labs/protocol-v2
Length of output: 1620
🏁 Script executed:
# See how user.rs handles the same pattern - any differences?
cat -n programs/drift/src/instructions/user.rs | sed -n '450,480p'Repository: drift-labs/protocol-v2
Length of output: 1592
🏁 Script executed:
# Check if there's any validation after loading the instruction
cat -n programs/drift/src/instructions/lp_admin.rs | sed -n '710,740p'Repository: drift-labs/protocol-v2
Length of output: 1486
🏁 Script executed:
# Look for Anchor discriminator size specification
web_search
Anchor framework instruction discriminator size bytes specificationRepository: drift-labs/protocol-v2
Length of output: 161
🌐 Web query:
Anchor framework instruction discriminator size specification documentation
💡 Result:
In Anchor (Solana), an instruction discriminator is the prefix placed at the start of the instruction’s data so the program can identify which instruction handler to run.
Default size + how it’s computed
- Default discriminator size: 8 bytes.
- Derivation: first 8 bytes of
sha256("global:<instruction_name>"), where<instruction_name>is the instruction name in snake_case.- This is widely documented (including in tooling that re-implements Anchor’s logic). [1][2]
Where it appears in instruction data
Anchor’s instruction data layout is:
instruction_data = discriminator || borsh_serialize(args)
Meaning the discriminator bytes come first, then Borsh-serialized arguments. [3]
Custom / non-8-byte discriminators (current Anchor)
As of Anchor’s newer releases, discriminators are customizable:
- You can override the default 8-byte instruction discriminator with:
#[instruction(discriminator = <CONST_EXPR>)][4]
- The discriminator can be any length (including zero) at the trait level, though IDL generation currently disallows empty discriminators. [5]
- Anchor also removed hard “8-byte requirement” messaging and made discriminator sizing more flexible across the ecosystem (including TS changes). [6]
Sources: [1] Anchor instructions explainer (shows default formula) • [2] Rust helper function documenting the exact rule • [3] InstructionData trait docs • [4] #[instruction] attribute macro docs • [5] Anchor 0.31.0 release notes (“Custom discriminators”) • [6] Anchor changelog entries about discriminator sizing/messages and TS constant removal.
Add length validation before slicing ix.data[0..8] for discriminator checks.
&ix.data[0..8] will panic if the instruction data is shorter than 8 bytes. Since ix.data is attacker-controlled transaction data, this should fail gracefully with ErrorCode::InvalidSwap instead of panicking.
Also, since Anchor's discriminator is a slice, using discriminator.len() makes the code more robust to potential future changes.
Proposed fix
- let discriminator = crate::instruction::EndLpSwap::DISCRIMINATOR;
- validate!(
- &ix.data[0..8] == discriminator,
- ErrorCode::InvalidSwap,
- "last drift ix must be end of swap"
- )?;
+ let discriminator = crate::instruction::EndLpSwap::DISCRIMINATOR;
+ validate!(
+ ix.data.len() >= discriminator.len(),
+ ErrorCode::InvalidSwap,
+ "drift ix data too short"
+ )?;
+ validate!(
+ &ix.data[..discriminator.len()] == discriminator,
+ ErrorCode::InvalidSwap,
+ "last drift ix must be end of swap"
+ )?;Note: This pattern appears in multiple instruction files (user.rs, keeper.rs, if_staker.rs) and should be addressed consistently.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let discriminator = crate::instruction::EndLpSwap::DISCRIMINATOR; | |
| validate!( | |
| ix.data[0..8] == discriminator, | |
| &ix.data[0..8] == discriminator, | |
| ErrorCode::InvalidSwap, | |
| "last drift ix must be end of swap" | |
| )?; | |
| let discriminator = crate::instruction::EndLpSwap::DISCRIMINATOR; | |
| validate!( | |
| ix.data.len() >= discriminator.len(), | |
| ErrorCode::InvalidSwap, | |
| "drift ix data too short" | |
| )?; | |
| validate!( | |
| &ix.data[..discriminator.len()] == discriminator, | |
| ErrorCode::InvalidSwap, | |
| "last drift ix must be end of swap" | |
| )?; |
🤖 Prompt for AI Agents
In `@programs/drift/src/instructions/lp_admin.rs` around lines 731 - 736, The
current discriminator check slices ix.data[0..8] which will panic if ix.data is
shorter than 8 bytes; before slicing, verify ix.data.len() >=
discriminator.len() (use crate::instruction::EndLpSwap::DISCRIMINATOR and
discriminator.len()) and return ErrorCode::InvalidSwap via the existing
validate! flow if the length is too small, then compare
&ix.data[0..discriminator.len()] == discriminator; apply the same pattern to
other files using similar discriminator checks (e.g., user.rs, keeper.rs,
if_staker.rs).
| /// unusued placeholder event to force include signed msg types into drift IDL | ||
| #[event] | ||
| #[derive(Default)] | ||
| struct _SignedMsgOrderParamsExport { | ||
| _a: SignedMsgOrderParamsMessage, | ||
| _b: SignedMsgOrderParamsDelegateMessage, | ||
| } | ||
|
|
||
| impl Size for _SignedMsgOrderParamsExport { | ||
| const SIZE: usize = 0; | ||
| } |
There was a problem hiding this comment.
Fix typo in doc comment.
“unusued” → “unused”.
📝 Suggested fix
-/// unusued placeholder event to force include signed msg types into drift IDL
+/// unused placeholder event to force include signed msg types into drift IDL📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// unusued placeholder event to force include signed msg types into drift IDL | |
| #[event] | |
| #[derive(Default)] | |
| struct _SignedMsgOrderParamsExport { | |
| _a: SignedMsgOrderParamsMessage, | |
| _b: SignedMsgOrderParamsDelegateMessage, | |
| } | |
| impl Size for _SignedMsgOrderParamsExport { | |
| const SIZE: usize = 0; | |
| } | |
| /// unused placeholder event to force include signed msg types into drift IDL | |
| #[event] | |
| #[derive(Default)] | |
| struct _SignedMsgOrderParamsExport { | |
| _a: SignedMsgOrderParamsMessage, | |
| _b: SignedMsgOrderParamsDelegateMessage, | |
| } | |
| impl Size for _SignedMsgOrderParamsExport { | |
| const SIZE: usize = 0; | |
| } |
🤖 Prompt for AI Agents
In `@programs/drift/src/state/events.rs` around lines 912 - 922, Update the doc
comment above the _SignedMsgOrderParamsExport placeholder to correct the typo:
change "unusued" to "unused" so the comment reads e.g. "unused placeholder event
to force include signed msg types into drift IDL"; locate the comment near the
struct _SignedMsgOrderParamsExport and adjust only the comment text (no code
changes to the struct or impl Size).
3 participants
Program Changes
zero_copy(unsafe)and u/i128 fields are now marked withlegacy_layoutmacro (rewrites definition to use compat::u128/i128 + adds getter/setters with internal conversions to/from std::primitive types)&[u8]instead of[u8; 8]TS SDK Changes
'PerpMarket','State'was used it should now be replaced withperpMarketor 'state' respectively. few struct fields also auto renamed e.g.volume24H->volume24hNotes
self.scaled_balance()idl-buildfeature is required inCargo.tomls for IDL build from now on and added to all deps involvedComplementary PRs
- part 1: #2019Summary by CodeRabbit
Release Notes
New Features
Chores