Expand plugin service APIs, add wallet portfolio support (from PR9), and migrate build to Bun#16
Expand plugin service APIs, add wallet portfolio support (from PR9), and migrate build to Bun#16
Conversation
…nAccountsByKeypairs(), getTokensSymbols token2022 attempt (can't read pumpfun's token still), change verifySignature prototype order, deprecate funcs, modernize cstr, type fixes, notes
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughStandardizes environment keys to SOLANA_*, adds env.example and .gitignore entries, migrates build tooling toward Bun, expands SolanaService public API and token-metadata parsing, updates action handlers’ return types and logging, and changes plugin init to a runtime-guarded flow with conditional registration. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Plugin Init
participant Runtime as IAgentRuntime
participant Provider as walletProvider
participant Actions as ActionsRegistry
participant Chain as INTEL_CHAIN
Plugin->>Runtime: read SOLANA_RPC_URL
alt SOLANA_RPC_URL missing
Plugin-->>Plugin: early return (no init)
else present
Plugin->>Runtime: register walletProvider
Plugin->>Runtime: read SOLANA_NO_ACTIONS
alt actions enabled
Plugin->>Actions: register transferToken, executeSwap
else actions disabled
Plugin-->>Runtime: runtime.logger.log actions skipped
end
Plugin->>Chain: await INTEL_CHAIN
activate Chain
Plugin->>Chain: register Solana service (SOLANA_SERVICE_NAME, instance)
deactivate Chain
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/service.ts (2)
1696-1756: Cache key ignoresincludeZeroBalances(incorrect results)We cache results under a single key regardless of whether zero-balance accounts were filtered out. After a default call (zero balances dropped) any subsequent call with
includeZeroBalances: truewill still return the filtered list until the cache expires, so callers never see zero-balance accounts. Please fold the option into the cache key (or store it alongside the payload) before reusing cached data.- const key = 'solana_' + walletAddress.toString() + '_tokens' + const cacheKey = + `solana_${walletAddress.toString()}_${options.includeZeroBalances ? 'withZero' : 'noZero'}`; @@ - check = await this.runtime.getCache<any>(key) + check = await this.runtime.getCache<any>(cacheKey) @@ - await this.runtime.setCache<any>(key, { + await this.runtime.setCache<any>(cacheKey, { fetchedAt: now, data: haveAllTokens })
1970-1981: Delete subscription entry after unsubscribe
unsubscribeFromAccountremoves the listener from the cluster but leaves the entry inthis.subscriptions. A subsequentsubscribeToAccountfor the same address immediately returns the stale ID and never re-subscribes, so updates stop flowing. Please delete the map entry after removal.- await this.connection.removeAccountChangeListener(subscriptionId); - - return true; + await this.connection.removeAccountChangeListener(subscriptionId); + this.subscriptions.delete(accountAddress); + return true;
🧹 Nitpick comments (1)
package.json (1)
25-27: Pin@elizaos/coreto a concrete versionUsing
"latest"makes every install pick up whichever version was most recently published, which can pull in breaking changes unexpectedly. Please pin to a tested semver range (e.g.^1.5.7) so consumers get reproducible builds and upgrades are intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
.gitignore(1 hunks)README.md(3 hunks)env.example(1 hunks)package.json(1 hunks)src/environment.ts(3 hunks)src/index.ts(1 hunks)src/service.ts(36 hunks)tsconfig.build.json(1 hunks)tsconfig.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
.gitignore (1)
4-6: Good additions to .gitignore.The three new patterns follow best practices:
.envprevents accidental commits of environment variables and secrets (aligns with elizaOS guidance to keep keys in local env files).elizadband.elizadb-testprevent local development/test database artifacts from being trackedThese additions are appropriate for the runtime-driven initialization pattern described in the PR.
odilitime
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/environment.ts (1)
66-66: Remove the debug comment.The comment
// wtf is this?is unprofessional and should be removed from production code.Apply this diff:
- SOLANA_SECRET_SALT: runtime.getSetting('SOLANA_SECRET_SALT'),// wtf is this? + SOLANA_SECRET_SALT: runtime.getSetting('SOLANA_SECRET_SALT'),src/index.ts (1)
39-48: Add error handling for the service load promise.The promise returned by
getServiceLoadPromise('INTEL_CHAIN')lacks a.catch()handler. If the INTEL_CHAIN service fails to load or the promise rejects, this will result in an unhandled promise rejection that could crash the process or go unnoticed.Apply this diff to add error handling:
- runtime.getServiceLoadPromise('INTEL_CHAIN').then( () => { + runtime.getServiceLoadPromise('INTEL_CHAIN').then(() => { //runtime.logger.log('solana INTEL_CHAIN LOADED') const traderChainService = runtime.getService('INTEL_CHAIN') as any; const me = { name: 'Solana services', chain: 'solana', service: SOLANA_SERVICE_NAME, }; traderChainService.registerChain(me); + }).catch((error) => { + runtime.logger.error('Failed to register Solana service with INTEL_CHAIN:', error); })
🧹 Nitpick comments (1)
src/index.ts (1)
40-40: Remove commented-out code.The commented-out log statement should either be uncommented if needed or removed entirely to keep the codebase clean.
Apply this diff:
- //runtime.logger.log('solana INTEL_CHAIN LOADED')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/environment.ts(2 hunks)src/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
src/index.ts (3)
23-26: LGTM!The early return when
SOLANA_RPC_URLis missing is good defensive coding and prevents initialization errors downstream.
28-34: LGTM!The conditional action registration provides useful flexibility for deployments that don't need Solana actions.
54-57: LGTM!The additional exports provide a clean public API for other plugins to integrate with the Solana plugin.
src/environment.ts (2)
25-48: LGTM!The schema correctly reflects the new SOLANA_* naming convention and appropriately comments out the optional API keys (SOL_ADDRESS, HELIUS_API_KEY, BIRDEYE_API_KEY) that are no longer required. The union structure allows for flexible authentication via either private/public key pair or secret salt.
63-77: Configuration validation appears consistent.The commented-out fields in the config object (lines 67, 70-71) now match the commented-out schema fields (lines 42, 45-46), resolving the validation mismatch flagged in previous reviews. The validation should now pass correctly when either the private/public key pair or secret salt is provided.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
62-122: Align agentConfig with refactored SOLANA_ env schema; reconsider required API keys.*The agentConfig still mixes old
WALLET_*keys with newSOLANA_*keys, and marksHELIUS_API_KEYandBIRDEYE_API_KEYasrequired: true. Per PR objectives, env schema should use standardizedSOLANA_*keys as primary, relax API key requirements, and move validation to runtime.Update agentConfig to consolidate around
SOLANA_*keys and make API keys optional:"agentConfig": { "pluginType": "elizaos:plugin:1.0.0", "pluginParameters": { "SOL_ADDRESS": { "type": "string", "description": "The mint/contract address used to represent native SOL when interacting with token swap logic.", "required": true, "sensitive": false }, "SOLANA_RPC_URL": { "type": "string", "description": "The Solana RPC endpoint the application should connect to.", "required": false, "default": "https://api.mainnet-beta.solana.com", "sensitive": false }, "SOLANA_SECRET_SALT": { "type": "string", "description": "Salt used to derive or encrypt the Solana wallet's secret key; required if the direct secret key is not provided.", "required": false, "sensitive": false }, "SOLANA_PRIVATE_KEY": { "type": "string", "description": "Base58-encoded Solana wallet secret (private) key; required if SOLANA_SECRET_SALT is not supplied.", "required": false, "sensitive": true }, "SOLANA_PUBLIC_KEY": { "type": "string", "description": "Base58-encoded Solana wallet public key corresponding to SOLANA_PRIVATE_KEY.", "required": false, "sensitive": false }, "SLIPPAGE": { "type": "string", "description": "Maximum acceptable slippage (likely as a percentage or basis points) for Solana swaps/transactions.", "required": true, "sensitive": false }, "HELIUS_API_KEY": { "type": "string", "description": "API key for accessing the Helius Solana infrastructure services.", - "required": true, + "required": false, "sensitive": true }, "BIRDEYE_API_KEY": { "type": "string", "description": "API key for accessing Birdeye market data services.", - "required": true, + "required": false, "sensitive": true } } }Remove or deprecate the old
WALLET_SECRET_KEY,WALLET_PUBLIC_KEY, andWALLET_PRIVATE_KEYentries, keeping only the standardizedSOLANA_*names.
♻️ Duplicate comments (1)
src/index.ts (1)
39-48: Add error handling to the promise.The promise returned by
getServiceLoadPromiselacks error handling, which could lead to unhandled rejections if the INTEL_CHAIN service fails to load or encounters an error.Apply this diff to add error handling:
- runtime.getServiceLoadPromise('INTEL_CHAIN' as ServiceTypeName).then( () => { - //runtime.logger.log('solana INTEL_CHAIN LOADED') + runtime.getServiceLoadPromise('INTEL_CHAIN' as ServiceTypeName).then(() => { const traderChainService = runtime.getService('INTEL_CHAIN') as any; const me = { name: 'Solana services', chain: 'solana', service: SOLANA_SERVICE_NAME, }; traderChainService.registerChain(me); + }).catch((error) => { + runtime.logger.warn('Failed to register Solana with INTEL_CHAIN:', error); })
🧹 Nitpick comments (2)
src/index.ts (1)
40-41: Consider minor cleanup.Two optional improvements:
- Remove the commented-out log statement (line 40)
- If possible, replace
as anywith a proper type to improve type safetyApply this diff to remove the commented code:
- //runtime.logger.log('solana INTEL_CHAIN LOADED') const traderChainService = runtime.getService('INTEL_CHAIN') as any;package.json (1)
26-27: Version constraint for @solana/spl-token-metadata is defensible but low-urgency.The package is in maintenance mode (last update ~1 year ago) with no versions above 0.1.6. While the caret range
^0.1.6permits 0.2.0+ if released, pinning to0.1.6or tightening to~0.1.6remains a valid defensive best practice for 0.x pre-release packages. The recommendation is sound; implementation is optional given the package's stability and low update frequency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
package.json(2 hunks)src/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
src/index.ts (3)
20-26: LGTM! Good validation pattern.The early return when
SOLANA_RPC_URLis missing is a clean approach for optional plugin initialization, and the log message clearly explains why the plugin is being skipped.
28-34: LGTM! Clean conditional logic.The conditional action registration based on
SOLANA_NO_ACTIONSprovides good flexibility for configuration, and the log message helps with debugging when actions are intentionally skipped.
52-57: LGTM! Well-structured exports.The export pattern is clean: default export for the plugin and named exports for service name, service class, and type interface. This provides good flexibility for consumers of this plugin.
… around, tsup/engines key
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
package.json (1)
8-8: Types file exposed but likely not generated
"types": "dist/index.d.ts"conflicts with build.ts (generateDts: false). Ensure DTS are built.
- Set
generateDts: truein build.ts, or- Change
"types"to an existing file produced by your build.Also applies to: 26-26
src/actions/swap.ts (1)
283-289: Provide a default SOL mint; avoid undefined SOL_ADDRESS
process.env.SOL_ADDRESSmay be unset (schema/commented). Default to the canonical SOL mint and prefer runtime settings.+const SOL_MINT = (typeof process !== 'undefined' && process.env.SOL_ADDRESS) ?? + 'So11111111111111111111111111111111111111112'; // Handle SOL addresses if (response.inputTokenSymbol?.toUpperCase() === 'SOL') { - response.inputTokenCA = process.env.SOL_ADDRESS; + response.inputTokenCA = SOL_MINT; } if (response.outputTokenSymbol?.toUpperCase() === 'SOL') { - response.outputTokenCA = process.env.SOL_ADDRESS; + response.outputTokenCA = SOL_MINT; }And in
swapToken:- const decimals = - inputTokenCA === process.env.SOL_ADDRESS + const decimals = + inputTokenCA === SOL_MINT ? new BigNumber(9) : new BigNumber(await getTokenDecimals(connection, inputTokenCA));Optionally, read from
runtime.getSetting('SOL_ADDRESS')and pass it down.Also applies to: 66-70
src/actions/transfer.ts (1)
238-241: Avoid precision loss when computing SPL transfer amount
Number(amount) * 10 ** decimalscan round for large/decimal values. Use BigNumber → bigint.+import BigNumber from 'bignumber.js'; ... - const adjustedAmount = BigInt(Number(content.amount) * 10 ** decimals); + const adjustedAmount = BigInt( + new BigNumber(String(content.amount)) + .multipliedBy(new BigNumber(10).pow(decimals)) + .integerValue(BigNumber.ROUND_FLOOR) + .toString() + );src/service.ts (3)
1973-2001: Token-2022 support: derive correct ATA and unpack with correct programIdDerives ATAs with default SPL program and calls unpackAccount without programId. For Token‑2022 mints this yields wrong ATAs and throws on unpack. Detect mint owner and pass programId for both derivation and unpack.
public async getTokenBalanceForWallets(mint: PublicKey, walletAddresses: string[]): Promise<Record<string, number>> { const walletPubkeys = walletAddresses.map(a => new PublicKey(a)); - const atAs = walletPubkeys.map(w => getAssociatedTokenAddressSync(mint, w)); + // Detect token program (SPL vs Token-2022) once per mint + const mintInfo = await this.connection.getParsedAccountInfo(mint); + const progId = + mintInfo.value?.owner?.toBase58() === TOKEN_2022_PROGRAM_ID.toBase58() + ? TOKEN_2022_PROGRAM_ID + : TOKEN_PROGRAM_ID; + const atAs = walletPubkeys.map(w => getAssociatedTokenAddressSync(mint, w, false, progId)); @@ - infos.forEach((info, idx) => { + infos.forEach((info, idx) => { const walletKey = walletPubkeys[idx].toBase58(); let uiAmount = 0; if (info?.data) { - const account = unpackAccount(atAs[idx], info); + const account = unpackAccount(atAs[idx], info, progId); // address, mint, owner, amount, delegate, delegatedAmount, isInitiailized, isFrozen, isNative // rentExemptReserve, closeAuthority, tlvData const raw = account.amount; // bigint uiAmount = Number(raw) / 10 ** decimals; }
2573-2579: Don’t fail successful swaps on metadata fetch; pass PublicKey to getDecimalgetDecimal expects a PublicKey and hits RPC; if it throws, the whole swap is marked unsuccessful. Make it best‑effort and type‑correct.
- (swapResponses as any)[pubKey] = { - success: true, - outAmount, - outDecimal: await this.getDecimal(signal.targetTokenCA), + let outDecimal: number | undefined; + try { + outDecimal = await this.getDecimal(new PublicKey(signal.targetTokenCA)); + } catch {} + (swapResponses as any)[pubKey] = { + success: true, + outAmount, + outDecimal, signature: txid, fees, swapResponse, };
475-516: Security posture: public detectPrivateKeysFromString exposes sensitive materialThis helper returns decoded private key bytes and is publicly accessible. Accidental exposure/logging risk is high.
- Make it private or move to an internal module.
- Gate behind an explicit debug flag or remove from the service API.
- Add usage docs and ensure no logs of returned bytes.
♻️ Duplicate comments (4)
package.json (3)
43-45: Pin @elizaOS dependencies; avoid "latest"Floating versions risk breakage. Pin to a verified compatible release.
- "@elizaos/core": "latest", - "@elizaos/service-interfaces": "latest", + "@elizaos/core": "^1.6.1", + "@elizaos/service-interfaces": "^1.6.1",Do the same under peerDependencies.
Also applies to: 52-53
36-36: Avoid caret on pre-release dependencyPin
@solana/spl-token-metadataexactly to prevent unexpected updates.- "@solana/spl-token-metadata": "^0.1.6", + "@solana/spl-token-metadata": "0.1.6",
77-131: Complete migration to SOLANA_ keys and relax API key requirements*
agentConfigstill exposes WALLET_* keys and marks HELIUS/BIRDEYE as required, diverging from code/schema.
- Remove/replace WALLET_* with SOLANA_* equivalents.
- Set HELIUS_API_KEY and BIRDEYE_API_KEY to required: false (validate at runtime).
src/environment.ts (1)
66-66: Remove stray debug comment
// wtf is this?should not ship.- SOLANA_SECRET_SALT: runtime.getSetting('SOLANA_SECRET_SALT'),// wtf is this? + SOLANA_SECRET_SALT: runtime.getSetting('SOLANA_SECRET_SALT'),
🧹 Nitpick comments (16)
src/keypairUtils.ts (2)
47-47: Unify error logging payload and levelUse a consistent key and level to aid parsing. Prefer
logger.error({ error }, msg).- logger.log({ e }, 'Error decoding base58 private key:'); + logger.error({ error: e }, 'Error decoding base58 private key'); ... - logger.error({ e: e2 }, 'Error decoding private key: '); + logger.error({ error: e2 }, 'Error decoding private key');Also applies to: 54-54
58-69: Derive public key from private key when SOLANA_PUBLIC_KEY is absentAvoid failing read‑only flows if only the private key is configured. Fallback to derive the public key.
- const publicKeyString = - runtime.getSetting('SOLANA_PUBLIC_KEY') ?? runtime.getSetting('WALLET_PUBLIC_KEY'); + const publicKeyString = + runtime.getSetting('SOLANA_PUBLIC_KEY') ?? runtime.getSetting('WALLET_PUBLIC_KEY'); if (!publicKeyString) { - throw new Error( - 'Solana Public key not found in settings, but plugin was loaded, please set SOLANA_PUBLIC_KEY' - ); + // Fallback: derive from private key if available + const privateKeyString = + runtime.getSetting('SOLANA_PRIVATE_KEY') ?? runtime.getSetting('WALLET_PRIVATE_KEY'); + if (privateKeyString) { + try { + const secretKey = + (() => { + try { return bs58.decode(privateKeyString); } catch { return Uint8Array.from(Buffer.from(privateKeyString, 'base64')); } + })(); + const kp = Keypair.fromSecretKey(secretKey); + return { publicKey: kp.publicKey }; + } catch (e) { + logger.error({ error: e }, 'Failed to derive public key from private key'); + } + } + throw new Error('Solana public key not found; set SOLANA_PUBLIC_KEY or SOLANA_PRIVATE_KEY.'); }build.ts (2)
3-4: Fix copy text in headerMentions plugin-bootstrap; should reference plugin-solana.
- * Build script for @elizaos/plugin-bootstrap using standardized build utilities + * Build script for @elizaos/plugin-solana using standardized build utilities
35-37: Log build errors for diagnosabilitySilencing errors complicates CI triage.
-run().catch((error) => { - //console.error('Build script error:', error); - process.exit(1); -}); +run().catch((error) => { + console.error('Build script error:', error); + process.exit(1); +});src/actions/swap.ts (1)
320-321: Guard walletPublicKey presence
getWalletKey(runtime, false)can throw; if it returns undefined, avoid cast and throw a clear error.- const { publicKey: walletPublicKey } = await getWalletKey(runtime, false); + const { publicKey: walletPublicKey } = await getWalletKey(runtime, false); + if (!walletPublicKey) throw new Error('Public key not available');src/environment.ts (2)
42-47: Align schema with runtime usage and package configSOL_ADDRESS, HELIUS_API_KEY, BIRDEYE_API_KEY are commented here but still referenced or required elsewhere (package.json agentConfig, swap action).
- Decide: either include them as optional here or remove/rename in agentConfig and code. Ensure SOL_ADDRESS is available (or provide default in code).
4-24: Docblock still references WALLET_ keys*Update the schema JSDoc to reflect SOLANA_* keys to avoid confusion.
src/actions/transfer.ts (3)
202-208: Guard SOL lamports as a safe integerPrevent float rounding issues and unsafe integers.
- const lamports = Number(content.amount) * 1e9; + const lamports = Math.round(Number(content.amount) * 1e9); + if (!Number.isFinite(lamports) || !Number.isSafeInteger(lamports) || lamports <= 0) { + throw new Error('Invalid SOL amount'); + }
137-140: Use a single logger consistentlyMixed
runtime.loggerand importedlogger. Prefer one (e.g., modulelogger) for consistency.- runtime.logger.log('Validating transfer from entity:', message.entityId); + logger.log('Validating transfer from entity:', message.entityId); ... - logger.error({ error },'Error during transfer'); + logger.error({ error }, 'Error during transfer');Also applies to: 294-295
219-223: Optionally confirm transaction for reliabilityMirror swap flow: fetch latest blockhash and
confirmTransactionto ensure finality before responding.- signature = await connection.sendTransaction(transaction); + const latestBlockhash = await connection.getLatestBlockhash(); + signature = await connection.sendTransaction(transaction, { maxRetries: 3 }); + await connection.confirmTransaction( + { signature, blockhash: latestBlockhash.blockhash, lastValidBlockHeight: latestBlockhash.lastValidBlockHeight }, + 'confirmed' + );Apply similarly to the SPL token branch.
Also applies to: 277-281
src/service.ts (6)
2079-2082: Cleanup subscription map on unsubscriberemoveAccountChangeListener succeeds but the entry remains in this.subscriptions. Delete it to avoid leaks and duplicate checks.
public async unsubscribeFromAccount(accountAddress: string): Promise<boolean> { @@ - await this.connection.removeAccountChangeListener(subscriptionId); - - return true; + await this.connection.removeAccountChangeListener(subscriptionId); + this.subscriptions.delete(accountAddress); + return true;
2266-2267: Guard numeric math; Jupiter amounts can be stringsCoerce to numbers and guard division by zero for impliedSlippageBps.
- const impliedSlippageBps: number = ((initialQuote.outAmount - initialQuote.otherAmountThreshold) / initialQuote.outAmount) * 10_000; + const outAmt = Number(initialQuote.outAmount); + const minOut = Number(initialQuote.otherAmountThreshold); + const impliedSlippageBps: number = outAmt > 0 ? ((outAmt - minOut) / outAmt) * 10_000 : 0;
1864-1867: Clarify return shape or key by address to avoid misusegetTokenAccountsByKeypairs returns Array<Array>, but callers may expect a map keyed by address (as seen in getBalance). Either change the return type to Record<string, any[]> or document clearly.
- public async getTokenAccountsByKeypairs(walletAddresses: string[], options = {}) { - return Promise.all(walletAddresses.map(a => this.getTokenAccountsByKeypair(new PublicKey(a), options))) + public async getTokenAccountsByKeypairs(walletAddresses: string[], options = {}): Promise<Record<string, any[]>> { + const results = await Promise.all(walletAddresses.map(a => this.getTokenAccountsByKeypair(new PublicKey(a), options))); + return Object.fromEntries(walletAddresses.map((a, i) => [a, results[i]])); }
571-581: Price cache lacks TTL; use your setCacheExp/getCacheExp helpersPrices are cached indefinitely. Apply a short TTL (e.g., 60s) with the existing helpers.
private async fetchPrices(): Promise<Prices> { const cacheKey = 'prices'; - const cachedValue = await this.runtime.getCache<Prices>(cacheKey); + const cachedValue = await getCacheExp(this.runtime, cacheKey); @@ - await this.runtime.setCache<Prices>(cacheKey, prices); + await setCacheExp(this.runtime, cacheKey, prices, 60); return prices; }
424-427: Batch address typing to reduce RPC fan-outCurrent implementation fires N getAccountInfo calls. You already have batchGetMultipleAccountsInfo; leverage it here to cut latency and rate-limit pressure.
- async getAddressesTypes(addresses: string[]) { - // FIXME: use batchGetMultipleAccountsInfo to efficiently check multiple - return Promise.all(addresses.map(a => this.getAddressType(a))) - } + async getAddressesTypes(addresses: string[]) { + const pubkeys = addresses.map(a => new PublicKey(a)); + const infos = await this.batchGetMultipleAccountsInfo(pubkeys, 'getAddressesTypes'); + return infos.map((accountInfo) => { + if (!accountInfo) return 'Account does not exist'; + const len = accountInfo.data?.length ?? 0; + if (len === 0) return 'Wallet'; + if (len === SolanaService.TOKEN_ACCOUNT_DATA_LENGTH) return 'Token Account'; + if (len === SolanaService.TOKEN_MINT_DATA_LENGTH) return 'Token'; + return `Unknown (Data length: ${len})`; + }); + }
221-224: transferSol placeholder should throw or implementReturning '' masks missing behavior; throw NotImplementedError (or implement) to prevent silent failures.
async transferSol(from: any, to: any, lamports: number): Promise<string> { - return '' + throw new Error('transferSol not implemented'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (10)
build.ts(1 hunks)package.json(2 hunks)src/actions/swap.ts(9 hunks)src/actions/transfer.ts(6 hunks)src/environment.ts(2 hunks)src/keypairUtils.ts(1 hunks)src/service.ts(40 hunks)tsconfig.build.json(1 hunks)tsconfig.json(1 hunks)tsup.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tsup.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tsconfig.build.json
- tsconfig.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
build.ts (1)
14-14: Confirm target runtimeTargeting
bunmay limit Node consumers. If the plugin is consumed under Node, considertarget: 'node'or a dual build.
- Is the plugin guaranteed to run under Bun at runtime?
- If not, switch to
target: 'node'(or produce both).src/actions/swap.ts (1)
248-256: LGTM: handler contract and structured loggingReturn type aligned to
ActionResult | voidand error logs standardized. No concerns.Also applies to: 372-372
src/actions/transfer.ts (1)
49-61: LGTM: stricter, non-mutating type guardClear, side‑effect free validation. Good improvements.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/service.ts (1)
2573-2576: Type mismatch: getDecimal expects PublicKey, a string is passedThis will not type-check and risks runtime errors. Wrap with PublicKey.
- outDecimal: await this.getDecimal(signal.targetTokenCA), + outDecimal: await this.getDecimal(new PublicKey(signal.targetTokenCA)),
♻️ Duplicate comments (7)
package.json (3)
33-40: Pin @solana/spl-token-metadata to exact 0.1.6Avoid caret on early 0.x where minor bumps can include breaking/type changes. Pin to 0.1.6.
- "@solana/spl-token-metadata": "^0.1.6", + "@solana/spl-token-metadata": "0.1.6",
60-130: Complete SOLANA_ migration; relax API key requirements to match PR objectives*agentConfig still mixes WALLET_* keys, and HELIUS_API_KEY/BIRDEYE_API_KEY are required: true. Standardize on SOLANA_* and make API keys optional, with runtime checks in service.
"SOL_ADDRESS": { "type": "string", "description": "The mint/contract address used to represent native SOL when interacting with token swap logic.", "required": true, "sensitive": false }, @@ - "WALLET_SECRET_SALT": { - "type": "string", - "description": "Salt used to derive or encrypt the Solana wallet’s secret key; required if the direct secret key is not provided.", - "required": false, - "sensitive": false - }, - "WALLET_SECRET_KEY": { - "type": "string", - "description": "Base58-encoded Solana wallet secret (private) key; required if WALLET_SECRET_SALT is not supplied.", - "required": false, - "sensitive": true - }, - "WALLET_PUBLIC_KEY": { - "type": "string", - "description": "Base58-encoded Solana wallet public key corresponding to WALLET_SECRET_KEY.", - "required": false, - "sensitive": false - }, + // Deprecated WALLET_* keys removed; use SOLANA_* equivalents @@ "HELIUS_API_KEY": { "type": "string", "description": "API key for accessing the Helius Solana infrastructure services.", - "required": true, + "required": false, "sensitive": true }, "BIRDEYE_API_KEY": { "type": "string", "description": "API key for accessing Birdeye market data services.", - "required": true, + "required": false, "sensitive": true }, @@ - "WALLET_PRIVATE_KEY": { - "type": "string", - "description": "Alternative variable name for the Solana wallet private key in base58 or base64.", - "required": false, - "sensitive": true - } + // WALLET_PRIVATE_KEY deprecated; use SOLANA_PRIVATE_KEYAlso consider marking SOLANA_PUBLIC_KEY/PRIVATE_KEY as optional, allowing read-only mode if both are absent, consistent with runtime guards.
42-49: Pin @elizaos/ versions; avoid "latest" in dev/peer deps*"latest" is non-deterministic and risks breakage at install time. Pin to a known compatible range (e.g., ^1.6.1) in both devDependencies and peerDependencies.
Apply:
"devDependencies": { - "@elizaos/core": "latest", - "@elizaos/service-interfaces": "latest", + "@elizaos/core": "^1.6.1", + "@elizaos/service-interfaces": "^1.6.1", "bun-types": "^1.3.0", @@ "peerDependencies": { - "@elizaos/core": "latest", - "@elizaos/service-interfaces": "latest", + "@elizaos/core": "^1.6.1", + "@elizaos/service-interfaces": "^1.6.1",Also applies to: 51-53
src/environment.ts (2)
66-66: Remove stray debug commentStrip “// wtf is this?” from production code.
- SOLANA_SECRET_SALT: runtime.getSetting('SOLANA_SECRET_SALT'),// wtf is this? + SOLANA_SECRET_SALT: runtime.getSetting('SOLANA_SECRET_SALT'),
42-47: Align SOL_ADDRESS across schema and config (agentConfig requires it)Schema comments out SOL_ADDRESS and validateSolanaConfig omits it, but package.json agentConfig marks SOL_ADDRESS required. Pick one direction; to match agentConfig, re-enable here.
.and( z.object({ - //SOL_ADDRESS: z.string().min(1, 'SOL address is required'), + SOL_ADDRESS: z.string().min(1, 'SOL address is required'), SLIPPAGE: z.string().min(1, 'Slippage is required'), SOLANA_RPC_URL: z.string().min(1, 'RPC URL is required'), @@ - //SOL_ADDRESS: runtime.getSetting('SOL_ADDRESS'), + SOL_ADDRESS: runtime.getSetting('SOL_ADDRESS'),Alternatively, if you intend to drop SOL_ADDRESS, remove it from agentConfig instead. Keep both surfaces consistent.
Also applies to: 67-69
src/service.ts (2)
180-192: getPortfolio returns empty assets; populate from wallet itemsDownstream code expecting assets will break. Map wp.items into siWalletPortfolio.assets.
public async getPortfolio(owner?: string): Promise<siWalletPortfolio> { @@ - const out: siWalletPortfolio = { - totalValueUsd: parseFloat(wp.totalUsd), - assets: [] - } + const out: siWalletPortfolio = { + totalValueUsd: parseFloat(wp.totalUsd), + assets: wp.items.map(i => ({ + address: i.address, + symbol: i.symbol, + balance: Number(i.uiAmount ?? 0), + decimals: i.decimals, + valueUsd: Number(i.valueUsd ?? 0), + })), + }; return out; }
200-219: Fix getBalance: wrong data source and indexing; token balances always 0
- Don’t await a non-Promise on getPublicKey.
- For SPL tokens, query the specific mint via getTokenBalanceForWallets.
public async getBalance(assetAddress: string, owner?: string): Promise<number> { - const ownerAddress: string | undefined = owner || (this.getPublicKey()?.toBase58()); + const ownerAddress: string | undefined = owner ?? this.getPublicKey()?.toBase58(); if (!ownerAddress) { - return -1 + throw new Error('Wallet not initialized'); } @@ - const balances = await this.getBalancesByAddrs([ownerAddress]) - const balance = balances[ownerAddress] - return balance + const balances = await this.getBalancesByAddrs([ownerAddress]); + return balances[ownerAddress] ?? 0; } - const tokenBalances: any = await this.getTokenAccountsByKeypairs([ownerAddress]) - const balance: number = tokenBalances[ownerAddress]?.balanceUi || 0 - return balance + const balances = await this.getTokenBalanceForWallets(new PublicKey(assetAddress), [ownerAddress]); + return balances[ownerAddress] ?? 0; }
🧹 Nitpick comments (4)
src/environment.ts (2)
31-37: Schema allows empty auth config; add a minimal refine (while keeping keys optional)Current union + optional fields permits no key material. If that’s intended for read-only mode, keep optional but refine to require at least one of SOLANA_PRIVATE_KEY, SOLANA_PUBLIC_KEY, SOLANA_SECRET_SALT when signing is needed. Minimal schema improvement:
- .and( - z.union([ - z.object({ - SOLANA_PRIVATE_KEY: z.string().min(1).optional(), - SOLANA_PUBLIC_KEY: z.string().min(1, 'Solana public key is required'), - }), - z.object({ - SOLANA_SECRET_SALT: z.string().min(1).optional(), - }), - ]) - ) + .and( + z + .object({ + SOLANA_PRIVATE_KEY: z.string().min(1).optional(), + SOLANA_PUBLIC_KEY: z.string().min(1).optional(), + SOLANA_SECRET_SALT: z.string().min(1).optional(), + }) + .refine( + (v) => !!(v.SOLANA_PRIVATE_KEY || v.SOLANA_PUBLIC_KEY || v.SOLANA_SECRET_SALT), + { message: 'Provide SOLANA_PRIVATE_KEY, SOLANA_PUBLIC_KEY, or SOLANA_SECRET_SALT' } + ) + )This preserves optionality but prevents accidental empty configs when signing is required.
4-24: Docblock is stale (WALLET_ references)*Update the JSDoc to reflect SOLANA_* keys and removed API key requirements. Keeps generated types accurate for consumers.
src/service.ts (2)
2079-2082: unsubscribeFromAccount should clear subscriptions mapRemove the entry after detaching the listener to avoid stale state.
public async unsubscribeFromAccount(accountAddress: string): Promise<boolean> { @@ - await this.connection.removeAccountChangeListener(subscriptionId); - - return true; + await this.connection.removeAccountChangeListener(subscriptionId); + this.subscriptions.delete(accountAddress); + return true;
2266-2267: Be explicit with numeric coercion for impliedSlippageBpsEnsure fields are numbers (Jupiter often returns strings).
- const impliedSlippageBps: number = ((initialQuote.outAmount - initialQuote.otherAmountThreshold) / initialQuote.outAmount) * 10_000; + const outAmt = Number(initialQuote.outAmount); + const minOut = Number(initialQuote.otherAmountThreshold); + const impliedSlippageBps: number = outAmt ? ((outAmt - minOut) / outAmt) * 10_000 : 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
package.json(2 hunks)src/environment.ts(2 hunks)src/service.ts(40 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/service.ts (1)
206-225: getBalance still broken: incorrect method and wrong mint lookup.This duplicates the past critical review. Line 222 calls
getTokenAccountsByKeypairs([ownerAddress])which:
- Returns an array of token account arrays, not a mint-specific balance
- Doesn't filter by
assetAddress(the token mint)- Indexes by
ownerAddressexpecting a single balance objectFor token balances, use
getTokenBalanceForWallets(defined at line 1980) with the specific mint:- const tokenBalances: any = await this.getTokenAccountsByKeypairs([ownerAddress]) - const balance: number = tokenBalances[ownerAddress]?.balanceUi || 0 + const balances = await this.getTokenBalanceForWallets(new PublicKey(assetAddress), [ownerAddress]); return balancesrc/environment.ts (1)
66-66: Remove unprofessional comment.The comment "wtf is this?" should be removed. If clarification about
SOLANA_SECRET_SALTis needed, add a proper documentation comment explaining its purpose (e.g., "Used to derive wallet keypair when SOLANA_PRIVATE_KEY is not provided").
🧹 Nitpick comments (7)
src/service.ts (7)
63-92: Remove or document commented interface definitions.The ISolanaPluginServiceAPI contains extensive commented-out alternative method signatures. Either remove this unused code or add documentation explaining why these alternatives are preserved.
98-103: Remove commented-out serviceType declarations.Clean up the commented lines 100-101 that reference old static serviceType patterns no longer in use.
430-433: Performance: batch address type checks.As noted in the FIXME comment, this makes N separate RPC calls. Refactor to use
batchGetMultipleAccountsInfofor efficient batched lookups, similar to other methods in this file.
567-570: Performance: batch circulating supply queries.Same batching concern as
getAddressesTypes. Consider refactoring to batch-fetch mint account data and compute supplies in parallel.
891-1558: Complex but functional token account parsing.This method handles caching, Token-2022 TLV parsing, Metaplex metadata, and supply calculations. While it works correctly, consider extracting helper functions in a future refactor to improve maintainability (e.g., separate metadata parsing, caching logic, and supply calculations into smaller units).
2272-2273: Type precision: impliedSlippageBps calculation.The calculation involves floating-point arithmetic and multiplication by 10,000, which could introduce precision issues. Consider using integer-based calculations or explicitly rounding:
-const impliedSlippageBps: number = ((initialQuote.outAmount - initialQuote.otherAmountThreshold) / initialQuote.outAmount) * 10_000; +const impliedSlippageBps: number = Math.round( + ((initialQuote.outAmount - initialQuote.otherAmountThreshold) / initialQuote.outAmount) * 10_000 +);
2509-2510: Remove unnecessary type coercion.The
uiTokenAmount.uiAmountanduiTokenAmount.amountproperties already have defined types from the Solana SDK. The|| 0fallback is redundant and masks potential issues. These lines should rely on proper null-checking instead:-const lamDiff = (inBal.uiTokenAmount.uiAmount || 0) - (outBal.uiTokenAmount.uiAmount || 0) -const diff = Number(inBal.uiTokenAmount.amount || 0) - Number(outBal.uiTokenAmount.amount || 0) +const lamDiff = (inBal.uiTokenAmount.uiAmount ?? 0) - (outBal.uiTokenAmount.uiAmount ?? 0) +const diff = Number(inBal.uiTokenAmount.amount ?? '0') - Number(outBal.uiTokenAmount.amount ?? '0')Also applies to: 2533-2534
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
package.json(2 hunks)src/environment.ts(2 hunks)src/service.ts(40 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (15)
src/service.ts (12)
130-144: LGTM: Non-blocking Jupiter service detection.The async service detection pattern prevents blocking the constructor. However, the comment "shouldn't even be here..." suggests this may warrant refactoring in a future PR to extract Jupiter integration into a separate module.
171-198: LGTM: getPortfolio now populates assets array.The implementation correctly addresses the past review by mapping
wp.itemsinto theassetsarray with proper field mappings (address, symbol, balance, decimals, valueUsd).
308-329: LGTM: Clean deprecation pattern.The new
verifySignaturemethod removes redundant "Solana" from the name (already in class name), and the deprecated wrapper properly logs warnings while maintaining backward compatibility.
335-352: LGTM: Consistent deprecation pattern.Same clean deprecation approach as
verifySignature, removing the redundant "Solana" prefix while maintaining backward compatibility.
701-724: LGTM: Token-2022 metadata parsing.The helper correctly attempts inline
TokenMetadataextension first, then falls back toMetadataPointer. Proper error handling and null-safety are in place.
727-845: LGTM: Comprehensive token symbol resolution.The two-phase approach (Metaplex PDAs → Token-2022 TLV with pointer resolution) is well-structured. Batch fetching ensures efficiency, and the fallback chain handles both token standards correctly.
847-889: LGTM: Safe supply calculation with bigint.The method correctly handles Buffer/Uint8Array views, uses bigint for supply calculations to avoid overflow, and leverages BigNumber for decimal division. Good defensive programming.
1591-1598: LGTM: Public key getter.Clean public API method with proper null-safety.
1802-1868: LGTM: Flexible token account fetching.The options parameter adds caching control and zero-balance filtering. Default 1-minute cache is reasonable, and the implementation correctly respects
notOlderThanandincludeZeroBalancesoptions.
1870-1872: LGTM: Batch token account fetching.Simple and correct wrapper that maps addresses to individual calls, properly forwarding options.
2601-2606: LGTM: Service factory method.Clean static factory with proper return type.
2614-2639: LGTM: Proper service cleanup.The stop method correctly unsubscribes from all accounts with error handling, clears the subscriptions map, and logs appropriate messages. Clean shutdown logic.
src/environment.ts (3)
25-48: Schema updated: past SOL_ADDRESS issue resolved.The schema changes (commenting out SOL_ADDRESS at line 42 and updating to SOLANA_* keys) mean the validation will no longer fail for missing SOL_ADDRESS. The past critical review about this is now resolved by making these fields optional.
63-77: LGTM: Config validation aligned with schema.The validation function correctly reads only the fields required by the updated schema. The optional
SOLANA_PRIVATE_KEYandSOLANA_PUBLIC_KEYare properly included, and the commented-out fields match the schema's commented sections.
78-86: LGTM: Correct Zod error property.Using
error.issuesinstead oferror.errorsis correct for Zod's API. This ensures proper error message formatting.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…roperty instead of rederriving, getWalletBalances() start
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/service.ts (2)
130-132: Constructor parameter mismatch persists.The
runtimeparameter is declared optional (runtime?: IAgentRuntime) but the implementation immediately throws if it's not provided. This creates a mismatch between the type signature and runtime behavior.Apply this diff to make the parameter required:
- constructor(runtime?: IAgentRuntime) { + constructor(runtime: IAgentRuntime) { if (!runtime) throw new Error('runtime is required for solana service') super(runtime);
206-225: getBalance implementation remains incorrect.This issue was flagged in a previous review and has not been addressed:
- Line 207 incorrectly wraps
getPublicKey()in a redundant call (it's already synchronous and doesn't need the extra call wrapper).- Line 222 uses
getTokenAccountsByKeypairswhich returnsPromise<any[][]>(array of arrays), not a keyed object by address.- The method doesn't query the specific mint, so token balances will be wrong.
Apply this diff to fix the implementation:
public async getBalance(assetAddress: string, owner?: string): Promise<number> { - const ownerAddress: string | undefined = owner || (this.getPublicKey()?.toBase58()); + const ownerAddress: string | undefined = owner ?? this.getPublicKey()?.toBase58(); if (!ownerAddress) { - return -1 + throw new Error('Wallet not initialized'); } if ( assetAddress.toUpperCase() === 'SOL' || assetAddress === PROVIDER_CONFIG.TOKEN_ADDRESSES.SOL ) { - //return this.getSolBalance(ownerAddress); const balances = await this.getBalancesByAddrs([ownerAddress]) - const balance = balances[ownerAddress] - return balance + return balances[ownerAddress] ?? 0; } - //const tokenBalance = await this.getTokenBalance(ownerAddress, assetAddress); - //return tokenBalance?.uiAmount || 0; - const tokenBalances: any = await this.getTokenAccountsByKeypairs([ownerAddress]) - const balance: number = tokenBalances[ownerAddress]?.balanceUi || 0 - return balance + const balances = await this.getTokenBalanceForWallets(new PublicKey(assetAddress), [ownerAddress]); + return balances[ownerAddress] ?? 0; }
🧹 Nitpick comments (3)
src/service.ts (3)
2637-2638: Prefer nullish coalescing for numeric fallbacks.Lines 2637-2638 and 2661-2662 use
|| 0to guard against null/undefined, which works but can hide bugs if the value is legitimately0. The nullish coalescing operator??is more precise.Apply this diff:
- const lamDiff = (inBal.uiTokenAmount.uiAmount || 0) - (outBal.uiTokenAmount.uiAmount || 0) - const diff = Number(inBal.uiTokenAmount.amount || 0) - Number(outBal.uiTokenAmount.amount || 0) + const lamDiff = (inBal.uiTokenAmount.uiAmount ?? 0) - (outBal.uiTokenAmount.uiAmount ?? 0) + const diff = Number(inBal.uiTokenAmount.amount ?? 0) - Number(outBal.uiTokenAmount.amount ?? 0)And similarly at lines 2661-2662:
- const lamDiff = (outBal.uiTokenAmount.uiAmount || 0) - (inBal.uiTokenAmount.uiAmount || 0) - const diff = Number(outBal.uiTokenAmount.amount || 0) - Number(inBal.uiTokenAmount.amount || 0) + const lamDiff = (outBal.uiTokenAmount.uiAmount ?? 0) - (inBal.uiTokenAmount.uiAmount ?? 0) + const diff = Number(outBal.uiTokenAmount.amount ?? 0) - Number(inBal.uiTokenAmount.amount ?? 0)Also applies to: 2661-2662
348-348: Replace console.log with runtime.logger for consistency.Multiple
console.logcalls throughout the file (lines 348, 443, 663, 721, 777, 797, 823, 843, 847, 858, 873, 882, 885, 895, and many more) bypass the runtime's logging infrastructure.Using
this.runtime.loggerensures:
- Consistent log formatting
- Proper log level filtering
- Centralized log destination control
Example refactors:
- console.log('batchGetMultipleAccountsInfo(' + label + ') - getMultipleAccountsInfo', slice.length + '/' + pubkeys.length) + this.runtime.logger.debug(`batchGetMultipleAccountsInfo(${label}) - getMultipleAccountsInfo ${slice.length}/${pubkeys.length}`);- console.log('getTokensSymbols'); + this.runtime.logger.debug('getTokensSymbols');Consider a global search-and-replace for all
console.logcalls in this file.Also applies to: 443-443, 663-663, 721-721, 777-777
748-771: Consider logging Token-2022 metadata parsing failures.The
parseToken2022SymbolFromMintOrPtrhelper silently catches and ignores errors when unpacking Token-2022 metadata (line 756). While the fallback to pointer-based metadata is correct, lack of logging makes debugging metadata issues difficult.Add debug logging in the catch block:
if (inline) { try { const md = unpackToken2022Metadata(inline); const symbol = md?.symbol?.replace(/\0/g, '').trim() || null; return { symbol }; } catch { - // fall through to pointer + this.runtime.logger.debug('Token-2022 inline metadata parse failed, trying pointer'); } }This aids troubleshooting without changing behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/service.ts(41 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Cursor Bugbot
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/service.ts (1)
2271-2286: Memory leak: Missing subscription cleanup.After removing the listener (line 2279), the method should delete the entry from
this.subscriptionsmap to prevent memory leaks.Apply this diff:
await this.connection.removeAccountChangeListener(subscriptionId); + this.subscriptions.delete(accountAddress); return true;
♻️ Duplicate comments (4)
src/index.ts (1)
39-48: Missing error handling on INTEL_CHAIN integration.The promise chain lacks a
.catch()handler. If the INTEL_CHAIN service fails to load orregisterChainthrows, the error will be unhandled.Add error handling:
- runtime.getServiceLoadPromise('INTEL_CHAIN' as ServiceTypeName).then( () => { + runtime.getServiceLoadPromise('INTEL_CHAIN' as ServiceTypeName).then(() => { //runtime.logger.log('solana INTEL_CHAIN LOADED') const traderChainService = runtime.getService('INTEL_CHAIN') as any; const me = { name: 'Solana services', chain: 'solana', service: SOLANA_SERVICE_NAME, }; traderChainService.registerChain(me); - }) + }).catch((error) => { + runtime.logger.error('Failed to register with INTEL_CHAIN:', error); + });src/service.ts (3)
110-117: Type signature mismatch: optional parameter that throws.The constructor declares
runtimeas optional (runtime?: IAgentRuntime) but throws immediately if not provided. Either make the parameter required or handle the undefined case differently.Apply this diff to make the parameter required:
- constructor(runtime?: IAgentRuntime) { + constructor(runtime: IAgentRuntime) { if (!runtime) throw new Error('runtime is required for solana service')
296-297: Type signature mismatch: optional parameter that throws.Same issue as SolanaWalletService constructor - the parameter is typed as optional but required in practice.
Apply this diff:
- constructor(runtime?: IAgentRuntime) { + constructor(runtime: IAgentRuntime) { if (!runtime) throw new Error('runtime is required for solana service')
150-169: Critical: Token balance always returns 0.Lines 166-168 call
getTokenAccountsByKeypairswhich returnsRecord<string, unknown[]>(an array of token accounts), but the code tries to access.balanceUion the array itself. This will always be undefined and return 0.You need to either:
- Use
getTokenBalanceForWallets(line 2174) which is designed for single-mint balance queries, or- Parse the token accounts array to find the matching mint and extract its balance
Apply this diff to fix using the correct method:
public async getBalance(assetAddress: string, owner?: string): Promise<number> { - const ownerAddress: string | undefined = owner || (this.solanaService.getPublicKey()?.toBase58()); + const ownerAddress: string | undefined = owner ?? this.solanaService.getPublicKey()?.toBase58(); if (!ownerAddress) { return -1 } if ( assetAddress.toUpperCase() === 'SOL' || assetAddress === PROVIDER_CONFIG.TOKEN_ADDRESSES.SOL ) { - //return this.getSolBalance(ownerAddress); const balances = await this.solanaService.getBalancesByAddrs([ownerAddress]) - const balance = balances[ownerAddress] - return balance + return balances[ownerAddress] ?? 0; } - //const tokenBalance = await this.getTokenBalance(ownerAddress, assetAddress); - //return tokenBalance?.uiAmount || 0; - const tokenBalances: any = await this.solanaService.getTokenAccountsByKeypairs([ownerAddress]) - const balance: number = tokenBalances[ownerAddress]?.balanceUi || 0 - return balance + const balances = await this.solanaService.getTokenBalanceForWallets( + new PublicKey(assetAddress), + [ownerAddress] + ); + return balances[ownerAddress] ?? 0; }
🧹 Nitpick comments (4)
src/service.ts (4)
542-545: Performance: Sequential RPC calls for address type checking.The method calls
getAddressTypefor each address sequentially. As the FIXME notes, this should usebatchGetMultipleAccountsInfofor efficiency when checking multiple addresses.Consider this optimization:
async getAddressesTypes(addresses: string[]) { const pubkeys = addresses.map(a => new PublicKey(a)); const infos = await this.batchGetMultipleAccountsInfo(pubkeys, 'getAddressesTypes'); return addresses.map((addr, i) => { const info = infos[i]; if (!info) return 'Account does not exist'; const dataLength = info.data.length; if (dataLength === 0) return 'Wallet'; if (dataLength === SolanaService.TOKEN_ACCOUNT_DATA_LENGTH) return 'Token Account'; if (dataLength === SolanaService.TOKEN_MINT_DATA_LENGTH) return 'Token'; return `Unknown (Data length: ${dataLength})`; }); }
679-682: Performance: Consider batch optimization.Similar to
getAddressesTypes, this could benefit frombatchGetMultipleAccountsInfoto reduce RPC calls when checking multiple mints.
1003-1670: Complex but functional token account parsing.This method handles caching, Token-2022 extensions, Metaplex metadata, and supply data. While it works, consider extracting sub-functions for:
- Cache management (lines 1021-1073)
- Token-2022 TLV parsing (lines 1180-1338)
- Metaplex parsing (lines 1355-1424)
- Result assembly (lines 1451-1650)
This would improve maintainability and testability.
2374-2787: Complex swap execution with extensive logic.This 400+ line method handles quote fetching, balance validation, slippage calculation, transaction execution, and result parsing. While functional, consider extracting sub-functions for:
- Balance validation (lines 2395-2424)
- Quote and slippage calculation (lines 2430-2481)
- Transaction execution (lines 2522-2623)
- Result parsing (lines 2667-2748)
This would improve maintainability and testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/index.ts(1 hunks)src/service.ts(40 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (17)
src/index.ts (2)
23-26: LGTM: Safe early return when RPC URL is missing.The conditional initialization properly guards against missing configuration and logs clearly.
28-34: LGTM: Conditional action registration implemented correctly.The logic properly respects the SOLANA_NO_ACTIONS flag and provides clear logging.
src/service.ts (15)
10-13: LGTM: Imports added for transaction construction.The new imports (SystemProgram, Transaction, TransactionMessage) support the transferSol implementation.
20-21: LGTM: Token-2022 metadata parsing support added.The import of
unpackfrom @solana/spl-token-metadata enables parsing Token-2022 metadata extensions. Based on learnings.
124-142: LGTM: Portfolio assets now populated correctly.The assets array is properly mapped from wallet data, addressing the previous concern.
180-223: LGTM: transferSol properly implemented.The method correctly constructs and signs SOL transfer transactions with proper error handling.
420-441: LGTM: Signature verification with clean deprecation path.The implementation correctly uses tweetnacl for Ed25519 signature verification, and the deprecated method properly warns users. Based on learnings.
447-464: LGTM: Address validation with deprecation.Clean implementation with proper deprecation warning for the old method name.
765-768: LGTM: Batch decimals fetching.Simple, clear batch wrapper using Promise.all for parallel execution.
813-836: LGTM: Token-2022 metadata parsing with fallback.Properly handles both inline TokenMetadata and MetadataPointer extensions, following Token-2022 patterns. Based on learnings.
839-957: LGTM: Comprehensive symbol resolution for both token programs.The two-phase approach efficiently handles Metaplex metadata and Token-2022 extensions (including metadata pointers), with proper batching to minimize RPC calls. Based on learnings.
959-1001: LGTM: Safe supply calculation with bigint arithmetic.Properly handles large token supplies using bigint and BigNumber, avoiding floating-point precision issues. The Uint8Array view creation (lines 970-971) correctly handles Buffer compatibility.
1708-1710: LGTM: Public key accessor.Clean getter for the service's public key.
1914-1980: LGTM: Token account fetching with options.Properly supports both SPL Token and Token-2022, with configurable caching and zero-balance filtering. The options parameter provides good flexibility.
1982-1989: LGTM: Batch wrapper returns correct shape.The method now returns
Record<string, unknown[]>as declared, mapping wallet addresses to their token account arrays. This addresses the previous concern about return type mismatch.
2096-2171: LGTM: Comprehensive wallet balance fetching for multiple mints.The method correctly derives ATAs for both TOKEN and TOKEN_2022 programs, batch-fetches accounts efficiently, and returns the properly typed
Record<string, MintBalance | null>. This addresses the previous type mismatch concern.
2821-2833: LGTM: Proper cleanup in stop method.The instance stop method correctly unsubscribes from all accounts with error handling and clears the subscriptions map, preventing resource leaks.
odilitime
changed the title
odilitime
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/service.ts (5)
89-118: Consider removing commented-out methods or implementing the interface.The
ISolanaPluginServiceAPIinterface contains many commented-out method signatures. Either:
- Remove the commented methods if they're not part of the current API contract
- Implement them if they're planned
- Move them to a separate planning document if they're future work
Additionally, the active
executeSwapsignature uses weak types (any,Record<string, unknown>). Consider using the more strongly-typed commented signature or defining proper parameter/return types.
165-193: Consider throwing errors instead of returning -1 for error conditions.The method returns
-1in error cases (lines 168, 192), which is ambiguous:
-1could be confused with an actual balance value- It's not clear to callers whether
-1indicates an error or a legitimate negative balance- The IWalletService interface specifies
Promise<number>return, not a discriminated unionConsider either:
- Throwing descriptive errors for these cases (recommended)
- Returning
0if a zero balance is semantically equivalent to "not found"- Changing the return type to
Promise<number | null>and returningnullfor errorsAdditionally, line 151 uses
'' + Number(...)for string conversion. Consider using.toString()orString()for clarity.
566-569: Batch helper methods work correctly but could be optimized.The methods properly batch multiple requests using
Promise.all, but the FIXME comments correctly identify that usingbatchGetMultipleAccountsInfowould be more efficient. This optimization can be deferred as the current implementation is functional.Also applies to: 703-706, 789-792
1027-1694: Core token parsing logic is complex but necessary for performance.The
parseTokenAccountsmethod appropriately handles:
- Multi-level caching with freshness checks
- Both SPL Token and Token-2022 programs
- Metaplex and Token-2022 TLV metadata parsing
- Efficient batching to minimize RPC calls
However, consider these improvements:
- Replace
console.logstatements withthis.runtime.loggerfor consistency (e.g., lines 1097, 1866, 1867)- Line 1630: Avoid
anytype - use a more specific type forcopy- Line 1061: The complex type annotation could be extracted to a named type for clarity
2399-2812: Swap execution is complex but handles edge cases well.The method appropriately:
- Validates inputs and checks balances before expensive RPC calls
- Calculates optimal amounts with slippage adjustments
- Implements retry logic for slippage failures (lines 2604-2625)
- Parses transaction details to report actual amounts received
- Handles both buy and sell scenarios with different balance calculations
Consider replacing
console.logstatements withthis.runtime.loggerfor consistency (e.g., lines 2428, 2432, 2451, 2463, etc.). This is a large method that makes extensive use of console logging instead of the structured logger.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/index.ts(1 hunks)src/service.ts(42 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (14)
src/index.ts (4)
1-2: LGTM!The imports are appropriate for the runtime-guarded initialization flow and boolean parsing from environment settings.
11-14: LGTM!Service imports align with the expanded service model and are used correctly throughout the plugin initialization.
20-52: Initialization logic is well-structured with appropriate guards.The refactored init function properly:
- Guards against missing SOLANA_RPC_URL with early exit
- Conditionally registers actions based on SOLANA_NO_ACTIONS
- Registers the wallet provider
- Integrates with INTEL_CHAIN as an optional extension
Regarding the past review comment about the unhandled promise at line 39: The fire-and-forget pattern here is intentional and appropriate. The INTEL_CHAIN integration is an optional extension that doesn't need to block plugin initialization. The
.catch()handler properly logs any errors that occur during registration.
55-59: LGTM!The expanded exports appropriately expose the service constant, both service classes, and a type alias for external consumers.
src/service.ts (10)
1-29: LGTM!The expanded imports properly support Token-2022 metadata parsing, signature verification, and the transferSol implementation. The additions of
bs58andtweetnaclalign with the signature verification functionality, andunpackToken2022Metadataenables Token-2022 support as outlined in the PR objectives.
43-62: LGTM!The type definitions are clear and appropriate:
MintBalanceprovides a clean structure for token balance dataKeyedParsedTokenAccountproperly wraps account info with public keysParsedTokenAccountsResponseuses a type-safe approach withAwaited<ReturnType<...>>
320-369: Constructor properly initializes the service with appropriate fallbacks.The initialization flow correctly:
- Validates runtime presence
- Sets up RPC connection
- Handles optional Jupiter service integration
- Gracefully handles missing private key (some operations are read-only)
- Sets up wallet monitoring subscriptions
Minor typo at line 343: "useable" should be "usable".
444-488: LGTM!The signature verification and address validation implementations are correct:
verifySignatureproperly uses Ed25519 signature verification via tweetnaclisValidAddressvalidates Solana public keys with optional curve checking- Deprecated method wrappers follow good practices with warning logs
837-981: Token-2022 and Metaplex metadata parsing is comprehensive and well-structured.The three-phase approach (Metaplex PDAs → Token-2022 inline metadata → pointer metadata) properly handles the complexity of multiple token standards and metadata locations. The batching strategy is efficient.
1732-1734: LGTM!The public
getPublicKeymethod appropriately exposes the service's public key with correct null handling.
1938-2013: Token account fetching with caching is well-implemented.The methods properly:
- Cache results with configurable freshness checks
- Fetch from both SPL Token and Token-2022 programs
- Filter zero balances based on options
- Provide batch operations with correct return types
2120-2195: Wallet balance fetching efficiently handles multiple tokens and programs.The implementation correctly:
- Derives ATAs for both TOKEN_PROGRAM_ID and TOKEN_2022_PROGRAM_ID
- Batch fetches all accounts in parallel
- Prefers legacy program when both exist (line 2180)
- Returns null for missing/zero balances
2295-2311: LGTM!The unsubscribe logic properly cleans up listeners using the correct API (
removeAccountChangeListener) and maintains the subscriptions map.
2820-2858: Service lifecycle methods properly handle startup and cleanup.The static
startandstopmethods correctly:
- Initialize service instances
- Return the appropriate Service type
- Clean up subscriptions during stop
- Handle errors with appropriate logging
The instance
stopmethod ensures all account subscriptions are properly removed.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/service.ts (1)
358-363: Store subscription ID to enable cleanup.The comment at line 357 notes "FIXME store this subscriptions somewhere" but the subscription ID from
subscribeToAccountis not stored in the catch-allthis.subscriptionsMap. This could prevent proper cleanup in thestop()method.Consider storing the subscription:
- this.subscribeToAccount(this.publicKey.toBase58(), async (accountAddress: string, accountInfo: unknown, context: unknown) => { + const subId = await this.subscribeToAccount(this.publicKey.toBase58(), async (accountAddress: string, accountInfo: unknown, context: unknown) => { runtime.logger.log('Updating wallet data'); await this.updateWalletData(); // non-forced (respect: UPDATE_INTERVAL) - }).catch((error) => { + }); + }) + .catch((error) => { logger.error('Error subscribing to agent wallet updates:', error); - }); + });
♻️ Duplicate comments (1)
src/service.ts (1)
320-322: Make required parameter non-optional.Same issue as
SolanaWalletService:runtimeparameter is declared optional but immediately throws if missing.Apply this diff:
- constructor(runtime?: IAgentRuntime) { + constructor(runtime: IAgentRuntime) { if (!runtime) throw new Error('runtime is required for solana service')
🧹 Nitpick comments (5)
src/service.ts (5)
55-62: Remove or document commented type definition.The commented-out alternative for
ParsedTokenAccountsResponse(lines 55-62) is not used. Either remove it entirely or add a comment explaining why it's kept for reference.
93-116: Clean up commented interface members.The interface contains extensive commented-out method signatures (lines 93-116). Either remove these entirely or move them to documentation/ADRs if they represent future work.
165-193: Consider consistent error handling for missing owner/token.The method returns
-1in two cases: whenownerAddressis undefined (line 168) and when the token is not found (line 192). This mixes error signaling with numeric balance returns. Consider either:
- Throwing an error for missing owner (like in
getPortfolio)- Returning
0for not-found tokens (if zero balance is semantically correct)- Documenting that
-1signals "error/not-found" in the JSDoc
1004-1671: Comprehensive token metadata parsing with caching.The
parseTokenAccountsmethod handles the complexity of parsing SPL Token, Token-2022, and Metaplex metadata with intelligent caching based on mutability. The batched RPC calls and background cache saving are well-designed.Optional: Consider extracting helper functions (like
parseToken2022MetadataTLV,readBorshStringSafe,formatSupplyUiAmount) to a separate utility module to reduce this method's size (~667 lines) and improve testability.
2376-2789: Consider extracting swap execution logic.The
executeSwapmethod is very long (~413 lines) with complex nested logic including an inner recursiveexecuteSwapfunction. Consider extracting:
- Wallet validation/balance checking (lines 2384-2427)
- Quote optimization (lines 2433-2473)
- Transaction execution with retry (lines 2524-2625)
- Transaction analysis (lines 2627-2750)
This would improve readability, testability, and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/service.ts(41 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (14)
src/service.ts (14)
139-157: LGTM!The
getPortfoliomethod now properly populates the assets array by mappingwp.itemsto the requiredsiWalletPortfolioshape. All fields are correctly extracted and formatted.
204-247: LGTM!The
transferSolimplementation is now complete with proper transaction construction, signing, and error handling. The necessary imports (TransactionMessage, SystemProgram) are present, and the method correctly usesgetPublicKey()andgetWalletKeypair().
444-465: LGTM!The signature verification logic is correctly implemented using
tweetnacl. The deprecatedverifySolanaSignaturewrapper properly delegates to the newverifySignaturemethod with a deprecation warning.
520-546: Good batching pattern for address type checking.The
getAddressesTypesmethod efficiently batches address lookups and returns a keyed structure. The deprecatedgetAddressTypewrapper properly delegates to the batch method.
725-770: LGTM!The
getDecimalandgetDecimalsmethods properly cache results and handle both SPL Token and Token-2022 programs. The error handling gracefully returns -1 for unknown tokens.
960-1002: LGTM - Proper bigint handling for supply.The
getSupplymethod correctly handles large token supplies using bigint arithmetic (lines 982-983) and BigNumber for human-readable values, avoiding floating-point precision issues.
840-958: Complex but well-structured Token-2022 metadata handling.The
getTokensSymbolsmethod uses a two-phase approach (Metaplex PDAs first, then Token-2022 fallback) with efficient batching. The complexity is justified for supporting both SPL Token and Token-2022 metadata systems. The code correctly handles inline metadata and metadata pointer extensions.
1696-1711: LGTM!The
getWalletKeypairandgetPublicKeymethods correctly expose wallet access. Throwing an error when the keypair is unavailable protects against invalid private key operations, while allowing null return forgetPublicKeyenables safe checking.
1718-1855: LGTM - Robust wallet data fetching with fallback.The
updateWalletDatamethod properly implements a two-tier approach (Birdeye API with RPC fallback), includes intelligent caching with interval checking, and fetches missing token symbols using the batchgetTokensSymbolsmethod.
1915-1981: LGTM!The
getTokenAccountsByKeypairmethod efficiently queries both SPL Token and Token-2022 programs, supports flexible caching withnotOlderThanandincludeZeroBalancesoptions, and properly maintains the decimals cache.
1983-1990: LGTM - Correct return shape.The
getTokenAccountsByKeypairsmethod now correctly returnsRecord<string, KeyedParsedTokenAccount[]>keyed by wallet address, addressing the previous review concern about array-of-arrays return shape.
2097-2172: LGTM - Efficient ATA balance batching.The
getWalletBalancesmethod efficiently fetches balances for multiple mints by deriving ATAs for both token programs, batching RPC calls, and returning the correctRecord<string, MintBalance | null>type. The return type annotation now matches the implementation.
2797-2836: LGTM - Proper service lifecycle management.The
startandstopmethods correctly manage the service lifecycle. The instancestopmethod properly cleans up all account subscriptions with error handling and clears the subscriptions map.
2272-2288: LGTM!The
unsubscribeFromAccountmethod correctly removes account change listeners and cleans up the subscriptions map. The boolean return value clearly indicates success.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
1 participant
Note
Adds wallet/portfolio APIs and a new wallet service, enhances Solana service utilities and action handlers, updates env schema, and switches build/tooling to Bun.
SolanaWalletServicewith portfolio (getPortfolio), balance (getBalance), andtransferSolsupport; exports additional service types.SolanaService: publicgetPublicKey, batching helpers, address/signature validation, token metadata (incl. Token-2022) and decimals utilities, multi-wallet token lookups, wallet balances, supply helpers, and fixed unsubscribe.TRANSFER_SOLANAandSWAP_SOLANAtyped/structured logging, revised handler return types, improved validation and callbacks.SOLANA_NO_ACTIONS; registers withINTEL_CHAIN; requiresSOLANA_RPC_URLto init.SOLANA_*keys; loosens some required fields; updates validation and error handling; addsenv.example.build.ts,bunengine), updates TypeScript/tsup configs, externals, and scripts; adjusts deps/peers..gitignore.Written by Cursor Bugbot for commit fcf5815. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores