sdk: fix helius-laserstream detection and prebuild:browser hook - #2199
sdk: fix helius-laserstream detection and prebuild:browser hook#2199Ysh204 wants to merge 4 commits into
Conversation
…ode engine constraint to >=18.0.0
…tially and add error handling to SDK dependency script
WalkthroughTransfer-hook token support is wired into ChangesTransfer Hook Support in Programs and SDK
SDK Optional Dependency Shim and Build Config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/driftClient.ts (1)
4108-4143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign token-program remaining accounts with the active transfer-hook side.
borrowToSpotMarkethook metas are appended (Lines 4133-4141), buttokenProgramsis built fromborrowFromSpotMarket(Lines 4104-4113). IfborrowTouses a different token program, the on-chain account contract can break for the borrow leg.Suggested fix
const tokenPrograms = new Set<string>(); const depositFromSpotMarket = this.getSpotMarketAccount( depositFromMarketIndex ); const borrowFromSpotMarket = this.getSpotMarketAccount( borrowFromMarketIndex ); + const borrowToSpotMarket = this.getSpotMarketAccount(borrowToMarketIndex); tokenPrograms.add( this.getTokenProgramForSpotMarket(depositFromSpotMarket).toBase58() ); tokenPrograms.add( this.getTokenProgramForSpotMarket(borrowFromSpotMarket).toBase58() ); + if (borrowAmount === undefined || !borrowAmount.isZero()) { + tokenPrograms.add( + this.getTokenProgramForSpotMarket(borrowToSpotMarket).toBase58() + ); + } @@ - if (borrowAmount === undefined || !borrowAmount.isZero()) { - const borrowToSpotMarket = this.getSpotMarketAccount(borrowToMarketIndex); + if (borrowAmount === undefined || !borrowAmount.isZero()) { this.addTokenMintToRemainingAccounts(borrowToSpotMarket, remainingAccounts); if (this.isTransferHook(borrowToSpotMarket)) { await this.addExtraAccountMetasToRemainingAccounts( borrowToSpotMarket.mint, remainingAccounts ); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/driftClient.ts` around lines 4108 - 4143, The tokenPrograms set is populated with token programs from depositFromSpotMarket and borrowFromSpotMarket, but transfer hook metadata is later added for borrowToSpotMarket. If borrowToSpotMarket uses a different token program than borrowFromSpotMarket, that token program will be missing from the remaining accounts, causing on-chain execution to fail. Add the token program for borrowToSpotMarket to the tokenPrograms set in the same conditional block where you call addExtraAccountMetasToRemainingAccounts for the borrow leg (around lines 4134-4141), ensuring all token programs used in the transaction are included.
🧹 Nitpick comments (1)
sdk/scripts/mock-optional-deps.js (1)
23-28: ⚡ Quick winConsider deriving the version from package.json.
The shim version is hardcoded to
0.1.8. If thehelius-laserstreamversion inoptionalDependenciesis updated, this hardcoded value will become stale and could cause version mismatch confusion.📦 Proposed fix to read version from package.json
+const pkgJson = require('../package.json'); +const shimVersion = pkgJson.optionalDependencies?.['helius-laserstream'] || '0.1.8'; + const packageJson = { name: 'helius-laserstream', - version: '0.1.8', + version: shimVersion, main: 'index.js', types: 'index.d.ts' };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/scripts/mock-optional-deps.js` around lines 23 - 28, The version property in the packageJson object is hardcoded to '0.1.8', which will become stale if the actual helius-laserstream version in optionalDependencies is updated. Instead of hardcoding the version, read it dynamically from the package.json file at runtime and assign it to the version property in the packageJson object. This ensures the mock always reflects the current version without manual synchronization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@programs/drift/src/instructions/keeper.rs`:
- Around line 3183-3189: The issue is that a single remaining_accounts_iter is
initialized once and reused across the entire function, but the transfer-hook
execution path consumes iterator entries. When the spot-position loop processes
multiple hook-enabled transfers, the first hook CPI drains accounts from the
shared iterator that are needed by subsequent hook CPIs. To fix this, create
isolated account windows or iterators for each transfer within the loop instead
of relying on one shared draining iterator. This means calculating the account
slice needed for each individual transfer (considering load_maps consumption and
hook-specific accounts) and passing only that slice to the hook execution, or
rebuilding the iterator state for each transfer iteration.
In `@programs/drift/src/instructions/user.rs`:
- Around line 1322-1328: The shared remaining_accounts_iter is being consumed by
both deposit-side and borrow-side token CPI calls, causing the first transfer
hook to deplete accounts needed by the second transfer. Either segment the
remaining_accounts into separate slices before passing to each transfer
operation (one for deposit operations and one for borrow operations), or
refactor the code to pass only the required account slice to each transfer
invocation instead of sharing a single peekable iterator. This applies to all
locations where dual hook transfers occur (around lines 1322-1328, 1698-1702,
and 1731-1735).
In `@sdk/package.json`:
- Line 16: The build:browser script currently does not invoke the
prebuild:browser script that is required to run beforehand. Modify the
build:browser command to call prebuild:browser first before executing the
TypeScript compilation and post-build steps. This ensures that the
helius-laserstream shim is created during the prebuild phase so it's available
for the subsequent browser build compilation to use. You should structure the
command so that prebuild:browser runs and completes successfully before the tsc
and postbuild commands execute.
---
Outside diff comments:
In `@sdk/src/driftClient.ts`:
- Around line 4108-4143: The tokenPrograms set is populated with token programs
from depositFromSpotMarket and borrowFromSpotMarket, but transfer hook metadata
is later added for borrowToSpotMarket. If borrowToSpotMarket uses a different
token program than borrowFromSpotMarket, that token program will be missing from
the remaining accounts, causing on-chain execution to fail. Add the token
program for borrowToSpotMarket to the tokenPrograms set in the same conditional
block where you call addExtraAccountMetasToRemainingAccounts for the borrow leg
(around lines 4134-4141), ensuring all token programs used in the transaction
are included.
---
Nitpick comments:
In `@sdk/scripts/mock-optional-deps.js`:
- Around line 23-28: The version property in the packageJson object is hardcoded
to '0.1.8', which will become stale if the actual helius-laserstream version in
optionalDependencies is updated. Instead of hardcoding the version, read it
dynamically from the package.json file at runtime and assign it to the version
property in the packageJson object. This ensures the mock always reflects the
current version without manual synchronization.
🪄 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: adecb57b-7714-41ac-96bc-f9ebda3ea5ae
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlprograms/drift/src/instructions/keeper.rsprograms/drift/src/instructions/user.rssdk/package.jsonsdk/scripts/mock-optional-deps.jssdk/src/driftClient.ts
| let mut remaining_accounts_iter = ctx.remaining_accounts.iter().peekable(); | ||
| let AccountMaps { | ||
| perp_market_map, | ||
| spot_market_map, | ||
| mut oracle_map, | ||
| } = load_maps( | ||
| &mut ctx.remaining_accounts.iter().peekable(), | ||
| &mut remaining_accounts_iter, |
There was a problem hiding this comment.
Shared transfer-hook iterator can break later CPIs in the same instruction.
A single remaining_accounts_iter is reused across the whole function, but the transfer-hook path consumes iterator entries. Since this function can perform multiple hook-enabled transfers (inside the spot-position loop), the first hook CPI can drain accounts required by later hook CPIs and cause runtime failure.
Use per-transfer account windows/iterators (or make hook-account consumption bounded to one transfer) instead of one shared draining iterator.
Also applies to: 3317-3321, 3339-3343
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@programs/drift/src/instructions/keeper.rs` around lines 3183 - 3189, The
issue is that a single remaining_accounts_iter is initialized once and reused
across the entire function, but the transfer-hook execution path consumes
iterator entries. When the spot-position loop processes multiple hook-enabled
transfers, the first hook CPI drains accounts from the shared iterator that are
needed by subsequent hook CPIs. To fix this, create isolated account windows or
iterators for each transfer within the loop instead of relying on one shared
draining iterator. This means calculating the account slice needed for each
individual transfer (considering load_maps consumption and hook-specific
accounts) and passing only that slice to the hook execution, or rebuilding the
iterator state for each transfer iteration.
| let mut remaining_accounts_iter = ctx.remaining_accounts.iter().peekable(); | ||
| let AccountMaps { | ||
| perp_market_map, | ||
| spot_market_map, | ||
| mut oracle_map, | ||
| } = load_maps( | ||
| &mut ctx.remaining_accounts.iter().peekable(), | ||
| &mut remaining_accounts_iter, |
There was a problem hiding this comment.
Dual hook transfers can fail due to shared draining iterator.
remaining_accounts_iter is shared between deposit-side and borrow-side token CPIs. When both sides require transfer-hook accounts, the first hook transfer can consume the iterator, leaving the second without required accounts.
Please split/segment remaining accounts per transfer invocation (or change consumption semantics so each call reads only its own required extras).
Also applies to: 1698-1702, 1731-1735
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@programs/drift/src/instructions/user.rs` around lines 1322 - 1328, The shared
remaining_accounts_iter is being consumed by both deposit-side and borrow-side
token CPI calls, causing the first transfer hook to deplete accounts needed by
the second transfer. Either segment the remaining_accounts into separate slices
before passing to each transfer operation (one for deposit operations and one
for borrow operations), or refactor the code to pass only the required account
slice to each transfer invocation instead of sharing a single peekable iterator.
This applies to all locations where dual hook transfers occur (around lines
1322-1328, 1698-1702, and 1731-1735).
| @@ -15,7 +15,9 @@ | |||
| "build": "yarn clean && tsc -p tsconfig.json && tsc -p tsconfig.browser.json && node scripts/postbuild.js", | |||
| "build:browser": "yarn clean && tsc -p tsconfig.json && tsc -p tsconfig.browser.json && node scripts/postbuild.js --force-env browser", | |||
There was a problem hiding this comment.
Critical: prebuild:browser is never invoked by build:browser.
The prebuild:browser script is defined on line 19 but the build:browser command does not call it. Per the PR objectives, the shim script must run before the browser build to create the helius-laserstream shim for compilation. Without this, the browser build will fail when the optional dependency is not installed.
🔧 Proposed fix to invoke prebuild:browser
-"build:browser": "yarn clean && tsc -p tsconfig.json && tsc -p tsconfig.browser.json && node scripts/postbuild.js --force-env browser",
+"build:browser": "yarn prebuild:browser && yarn clean && tsc -p tsconfig.json && tsc -p tsconfig.browser.json && node scripts/postbuild.js --force-env browser",📝 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.
| "build:browser": "yarn clean && tsc -p tsconfig.json && tsc -p tsconfig.browser.json && node scripts/postbuild.js --force-env browser", | |
| "build:browser": "yarn prebuild:browser && yarn clean && tsc -p tsconfig.json && tsc -p tsconfig.browser.json && node scripts/postbuild.js --force-env browser", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/package.json` at line 16, The build:browser script currently does not
invoke the prebuild:browser script that is required to run beforehand. Modify
the build:browser command to call prebuild:browser first before executing the
TypeScript compilation and post-build steps. This ensures that the
helius-laserstream shim is created during the prebuild phase so it's available
for the subsequent browser build compilation to use. You should structure the
command so that prebuild:browser runs and completes successfully before the tsc
and postbuild commands execute.
1 participant
Two small SDK build fixes:
fs.existsSyncwithrequire.resolve()inmock-optional-deps.jsso hoisted/non-local installs are detected correctly before creating the shim
prebuild:browserscript so the shim runs beforeyarn build:browser(the
prebuildlifecycle hook only fires forbuild, notbuild:browser)Summary by CodeRabbit
New Features
Chores