feat: Solana WalletConnect trading support

[arein]

Summary

• Enable Solana wallets (Phantom, Jupiter, Solflare) to sign DEX swap transactions via WalletConnect v2
• Remove the "EVM only" restriction from trading commands while keeping transfers EVM-only for now
• Add sendSolanaTransactionViaWalletConnect() handling both full signed tx and raw signature responses
• Fix Solana address comparison (case-sensitive vs EVM toLowerCase)
• Ed25519 signature length validation (64 bytes)

Test plan

• All 706 tests pass (npm test)
• Manual e2e: connected Jupiter wallet via WalletConnect, executed 0.01 SOL → USDC swap successfully
• EVM WalletConnect trading still works (no regression)
• Test with Jupiter wallet

Note: Requires WalletConnect/agent-sdk#22 for --chain solana to resolve correctly (already merged).

🤖 Generated with Claude Code

[nansen-pr-reviewer]

🟡 MEDIUM

Potential issue: Chain prefix matching may be too permissive

The code uses startsWith('solana:') to match Solana chains, but according to the CAIP-2 standard used by WalletConnect, the full chain identifier should be solana:<reference> where reference is a specific network identifier.

Current code:

const solAccount = accounts.find(a => a.chain?.startsWith('solana:'));

Issue: This will match ANY Solana network (mainnet, testnet, devnet), which could lead to using a testnet address when expecting mainnet, or vice versa.

From your test at line 86 and the constant at line 13, the expected mainnet chain ID is 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'.

Recommendation: Either:

1. Add a warning/validation that the chain matches the expected mainnet reference
2. Use exact matching: a.chain === SOLANA_MAINNET_CHAIN
3. Document that this intentionally supports any Solana network

Example:

if (chainType === 'solana') {
  const solAccount = accounts.find(a => a.chain?.startsWith('solana:'));
  if (solAccount && solAccount.chain !== SOLANA_MAINNET_CHAIN) {
    // Log warning: "Connected to Solana ${solAccount.chain}, expected mainnet"
  }
  return solAccount?.address || null;
}

Confidence: 82/100 - This could cause users to accidentally use testnet addresses for mainnet transactions, leading to failed transactions or lost funds.

[nansen-pr-reviewer]

🔵 LOW

Good: Clear and actionable error message

The updated error message is much better than the previous generic "WalletConnect is only supported for EVM chains":

✓ Specifies exactly what's not supported ("Solana transfers")
✓ Clarifies what IS supported (trading is supported per the PR)
✓ Provides a clear workaround ("Use a local wallet")

This follows the CLAUDE.md guideline: "Actionable errors — not generic failures"

The distinction between "transfers not supported" vs "trading is supported" (from the PR description) is correctly communicated to users.

Confidence: 100/100 - This is a quality improvement that reduces user confusion.

[nansen-pr-reviewer]

pr-reviewer Review Summary

📝 11 findings

Review completed. Please address the inline comments below.

Findings by Severity

Severity	Count
🔴 Critical	3
🟠 High	0
🟡 Medium	6
🔵 Low	2

Summary

The PR adds Solana WalletConnect support for trading, enabling wallets like Phantom to sign DEX transactions. The implementation is mostly solid but contains one critical buffer manipulation issue in the signature injection code (line 1039) that could corrupt transactions. Additionally, there are 6 medium-severity issues around error handling, validation, and test coverage that should be addressed. The code correctly maintains backward compatibility and includes comprehensive tests for the new WalletConnect functions.

Baseline Security Analysis: Found 2 potential security issue(s) using pattern-based detection (2 critical, 0 high, 0 medium, 0 low).

---

Token usage: 1,401,816 input, 6,178 output

[nansen-pr-reviewer]

🟡 MEDIUM

Potential Issue: Address Comparison Logic Inconsistency

The address comparison has different logic for Solana vs EVM chains:

• Solana: case-sensitive comparison with .trim()
• EVM: case-insensitive comparison with .toLowerCase().trim()

if (quoteWallet && (chainType === 'solana'
  ? wcAddress.trim() !== quoteWallet.trim()
  : wcAddress.toLowerCase().trim() !== quoteWallet.toLowerCase().trim())) {

Issue: Solana addresses are base58-encoded and ARE case-sensitive (unlike EVM addresses which are hex and can be case-insensitive with checksumming). However, the code doesn't validate that both addresses are well-formed before comparison.

Potential edge case: If wcAddress or quoteWallet is null or undefined, calling .trim() will throw a TypeError. The code checks if (quoteWallet && ...) which protects against quoteWallet being falsy, but if wcAddress comes back as null from getWalletConnectAddress(), it's already caught by the earlier check on line 972-976. So this is actually safe.

Additional concern: The .trim() is defensive against whitespace, but neither Solana nor EVM addresses should have whitespace. This suggests potential data quality issues from upstream. Consider logging a warning if trimming actually removes characters:

const wcTrimmed = wcAddress.trim();
const quoteTrimmed = quoteWallet.trim();
if (wcTrimmed !== wcAddress || quoteTrimmed !== quoteWallet) {
  log(`  ⚠ Whitespace detected in addresses (quote: "${quoteWallet}", wc: "${wcAddress}")`);
}

Confidence: 82/100

The code is functionally correct but could be more defensive about data quality issues.

[nansen-pr-reviewer]

🟠 HIGH

Potential Issue: Inconsistent Chain Prefix Matching

The chain filtering uses startsWith() for both Solana and EVM chains:

const solAccount = accounts.find(a => a.chain?.startsWith('solana:'));
// ...
const evmAccount = accounts.find(a => a.chain?.startsWith('eip155:'));

Issue: This could match unintended chains if the WalletConnect session includes variations like:

• solana:devnet (Solana devnet)
• solana:testnet (Solana testnet)
• eip155:5 (Goerli testnet)
• eip155:137 (Polygon)

The code would return the FIRST matching account, which might not be the mainnet account the user expects.

Recommendation: Use exact chain ID matching or add a suffix check:

if (chainType === 'solana') {
  // Match Solana mainnet specifically
  const solAccount = accounts.find(a => a.chain === 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp');
  return solAccount?.address || null;
}

Or at minimum, document this behavior and consider logging which chain was selected:

const solAccount = accounts.find(a => a.chain?.startsWith('solana:'));
if (solAccount) {
  // Could log: Selected Solana account on chain: ${solAccount.chain}
  return solAccount.address;
}

Confidence: 88/100

This is a real issue if users connect wallets with multiple network accounts. The CLAUDE.md specifies "Solana: mainnet-beta" only, so non-mainnet chains shouldn't be accepted.

[nansen-pr-reviewer]

🔵 LOW

Code Quality: Error Message Could Be More Specific

The parseWcJson() helper throws a generic error:

if (!jsonLine) throw new Error('No JSON output from walletconnect send-transaction');

Issue: This error message always references "send-transaction" even though parseWcJson() is a generic helper that could be used for other WalletConnect operations in the future. The error message is hard-coded rather than contextual.

Recommendation: Make the error message more generic or pass context:

function parseWcJson(output, context = 'walletconnect operation') {
  const jsonLine = output.split('\n').find(line => line.startsWith('{'));
  if (!jsonLine) throw new Error(`No JSON output from ${context}`);
  return JSON.parse(jsonLine);
}

Then call it as:

const result = parseWcJson(output, 'walletconnect send-transaction');

This is a minor quality issue but makes the code more maintainable.

Confidence: 80/100

[nansen-pr-reviewer]

🟡 MEDIUM

Potential Issue: Base58 Encoding Without Validation

The code converts base64 to base58 for WalletConnect:

const txBase58 = base58Encode(Buffer.from(txBase64, 'base64'));

Issue: If txBase64 is not a valid base64 string, Buffer.from() will silently produce an incorrect buffer rather than throwing an error. This could lead to confusing errors downstream when WalletConnect tries to parse an invalid transaction.

Example of silent failure:

Buffer.from('not!valid@base64', 'base64') // Returns: <Buffer 9e ad f2...> (garbage)

Recommendation: Add validation before encoding:

// Validate base64 format (optional but safer)
if (!/^[A-Za-z0-9+/]+=*$/.test(txBase64)) {
  throw new Error('Invalid base64 transaction data received from trading API');
}
const txBase58 = base58Encode(Buffer.from(txBase64, 'base64'));

Alternatively, wrap in try-catch with a clear error:

let txBase58;
try {
  txBase58 = base58Encode(Buffer.from(txBase64, 'base64'));
} catch (err) {
  throw new Error(`Failed to encode transaction for WalletConnect: ${err.message}`);
}

Confidence: 83/100

This is a defensive programming issue. In practice, the trading API should return valid base64, but validation would catch API bugs early.

[nansen-pr-reviewer]

🔵 LOW

Code Quality: Direct Buffer Mutation Without Copy

The code mutates txBytes directly by copying the signature into it:

const txBytes = Buffer.from(txBase64, 'base64');
// ... validation ...
sigBytes.copy(txBytes, sigCountSize);
signedTransaction = txBytes.toString('base64');

Analysis: This is actually correct because Buffer.from() creates a new buffer, so we're not mutating the original base64 string. However, the pattern could be clearer for future maintainers.

Best Practice: Consider making it explicit that we're working with a mutable copy:

const txBytes = Buffer.from(txBase64, 'base64'); // Creates a new mutable buffer
const { value: sigCount, size: sigCountSize } = readCompactU16(txBytes, 0);
// ... validation ...
// Inject signature into the first slot (in-place mutation of our copy)
sigBytes.copy(txBytes, sigCountSize);
signedTransaction = txBytes.toString('base64');

Or use a more explicit copy pattern:

const originalTx = Buffer.from(txBase64, 'base64');
const signedTxBytes = Buffer.from(originalTx); // Explicit copy
// ... validation ...
sigBytes.copy(signedTxBytes, sigCountSize);
signedTransaction = signedTxBytes.toString('base64');

Note: The current code is functionally correct but could benefit from a clarifying comment.

Confidence: 80/100

[nansen-pr-reviewer]

🟡 MEDIUM

Potential Issue: Inconsistent Field Name Handling

The function handles three possible response formats from WalletConnect wallets:

if (result.signedTransaction) return { signedTransaction: result.signedTransaction };
if (result.signature) return { signature: result.signature };
if (result.transaction) return { signedTransaction: result.transaction };

Issue: The third condition normalizes transaction → signedTransaction, but this creates an implicit assumption about what result.transaction contains. Different wallets might use different field names:

• Some return signedTransaction (expected format)
• Some return signature (raw sig only)
• Some return transaction (full signed tx)

Problem: Without documentation from WalletConnect spec or wallet implementations, we can't be sure result.transaction is the same as result.signedTransaction. If a wallet returns { transaction: "<unsigned_tx>" }, we'd incorrectly treat it as signed.

Recommendation:

1. Add a comment documenting which wallets use which format
2. Consider logging which field was returned for debugging:

if (result.signedTransaction) {
  return { signedTransaction: result.signedTransaction };
}
if (result.signature) {
  return { signature: result.signature };
}
if (result.transaction) {
  // Some wallets (e.g., Phantom) return 'transaction' instead of 'signedTransaction'
  return { signedTransaction: result.transaction };
}

3. Or be more strict and only accept the documented formats:

if (result.signedTransaction) return { signedTransaction: result.signedTransaction };
if (result.signature) return { signature: result.signature };
// Log unexpected format
throw new Error(`Unexpected WalletConnect response format: ${Object.keys(result).join(', ')}`);

Confidence: 82/100

This could lead to subtle bugs if a wallet returns unexpected field names.

[nansen-pr-reviewer]

pr-reviewer Review Summary

📝 8 findings

Review completed. Please address the inline comments below.

Findings by Severity

Severity	Count
🔴 Critical	2
🟠 High	1
🟡 Medium	4
🔵 Low	1

Summary

The PR adds Solana WalletConnect support for trading with solid implementation and comprehensive test coverage. Found one high-severity issue with chain prefix matching that could allow wrong network selection, and several medium-severity code quality improvements around validation, error handling, and defensive programming. The signature injection validation is correct but could benefit from clearer documentation. No critical security issues or data loss risks identified.

Baseline Security Analysis: Found 2 potential security issue(s) using pattern-based detection (2 critical, 0 high, 0 medium, 0 low).

---

Token usage: 1,901,399 input, 5,740 output

[arein]

Addressed remaining review feedback in e3fd3e0:

HIGH

• ✅ Chain prefix matching — Changed from startsWith('solana:') to exact === SOLANA_MAINNET_CHAIN match. Devnet/testnet accounts now return null. Added test.

MEDIUM

• ✅ parseWcJson multi-line JSON — Now handles pretty-printed JSON by tracking brace balance across lines. Added test.
• ✅ Ed25519 base58 decode error — Wrapped base58Decode(wcResult.signature) in try-catch with descriptive error message.
• ✅ Base58 tx encoding — Wrapped base58Encode() in try-catch for clearer error on malformed transaction data.
• ✅ Sig injection clarity — Added comments: "Buffer.from() creates a new buffer — safe to mutate in-place" and "Inject into the first signature slot (feePayer)".
• ✅ Response field docs — Added comment explaining the transaction → signedTransaction normalization for wallets like Phantom.

FALSE POSITIVES

• 💬 "CRITICAL - DES encryption" (×2) — Pattern matcher false positive triggered by the word describe in test files. No DES usage anywhere.

Previously addressed (b455a76)

• ✅ Sig injection validation (sigCount check, buffer size check)
• ✅ Address .trim() comparison
• ✅ Test assertion for getWalletConnectAddress('solana')

All 708 tests pass.

[nansen-pr-reviewer]

🟡 MEDIUM

Potential issue: Infinite loop if closing brace never found

The multi-line JSON parser collects lines until braces balance to zero, but if the JSON is malformed and missing a closing brace, this loop will run to the end of the lines array without ever breaking.

Consider adding a check to prevent reading beyond reasonable bounds:

for (let i = startIdx; i < lines.length; i++) {
  jsonLines.push(lines[i]);
  for (const ch of lines[i]) {
    if (ch === '{') braces++;
    else if (ch === '}') braces--;
  }
  if (braces === 0) break;
  // Add safety check for malformed JSON
  if (i - startIdx > 100) { // reasonable limit for JSON depth
    throw new Error('Malformed JSON from walletconnect: too many lines without closing brace');
  }
}

This prevents the loop from consuming excessive memory if the walletconnect output is unexpectedly large or malformed.

Confidence: 82/100

[nansen-pr-reviewer]

🟡 MEDIUM

Potential issue: Character-level brace counting can be fooled by strings

The current implementation counts { and } characters without considering whether they're inside JSON string values. For example:

{
  "message": "Failed to connect. Error: { network timeout }"
}

The { inside the error message string would incorrectly increment the brace counter.

While this is unlikely given the expected walletconnect output format, it's a potential edge case. Consider using JSON.parse() with error recovery instead:

function parseWcJson(output) {
  const lines = output.split('\n');
  const startIdx = lines.findIndex(l => l.trimStart().startsWith('{'));
  if (startIdx === -1) throw new Error('No JSON output from walletconnect');

  // Try parsing incrementally until JSON.parse succeeds
  for (let endIdx = startIdx; endIdx < lines.length; endIdx++) {
    const candidate = lines.slice(startIdx, endIdx + 1).join('\n');
    try {
      return JSON.parse(candidate);
    } catch {
      // Not valid JSON yet, keep collecting lines
    }
  }
  throw new Error('No valid JSON found in walletconnect output');
}

This approach is more robust and handles edge cases like escaped braces in strings.

Confidence: 85/100

[nansen-pr-reviewer]

🔵 LOW

Minor: Comment could be more precise

The comment says "Buffer.from() creates a new buffer — safe to mutate in-place" but this might be misleading. The safety comes from the fact that we're creating a copy of the transaction bytes, not that Buffer.from() returns a mutable buffer.

Consider clarifying:

// Create a copy of the transaction buffer so we can safely mutate it
const txBytes = Buffer.from(txBase64, 'base64');

This makes it clearer why it's safe (we're not modifying the original data) rather than just stating that it is safe.

Confidence: 80/100

[nansen-pr-reviewer]

🟠 HIGH

Potential security issue: Signature count validation happens too late

The validation if (sigCount < 1) happens after we've already called readCompactU16(txBytes, 0) which reads from the buffer. If the transaction buffer is too small (e.g., less than 1 byte), readCompactU16 will throw or return garbage data.

More critically, the buffer size check on line 1053-1055 validates that the buffer is large enough for signature injection, but this check should happen before we attempt to decode sigBytes from base58 (line 1040), since a corrupted transaction could have sigCount >= 1 but an invalid structure.

Recommended fix - validate the transaction structure before attempting any signature operations:

// Wallet returned raw Ed25519 sig → inject into unsigned tx
const txBytes = Buffer.from(txBase64, 'base64');
const { value: sigCount, size: sigCountSize } = readCompactU16(txBytes, 0);

// Validate transaction buffer BEFORE decoding signature
if (sigCount < 1) {
  throw new Error('Transaction has no signature slots');
}
if (txBytes.length < sigCountSize + 64) {
  throw new Error(`Transaction buffer too small for signature: need ${sigCountSize + 64}, got ${txBytes.length}`);
}

// Now safe to decode and inject signature
let sigBytes;
try {
  sigBytes = base58Decode(wcResult.signature);
} catch (err) {
  throw new Error(`Invalid base58 signature from WalletConnect: ${err.message}`);
}
if (sigBytes.length !== 64) {
  throw new Error(`Invalid Ed25519 signature length: expected 64 bytes, got ${sigBytes.length}`);
}
// Inject into the first signature slot (feePayer)
sigBytes.copy(txBytes, sigCountSize);

This ensures the transaction buffer is valid before we spend time decoding the signature.

Confidence: 88/100

[nansen-pr-reviewer]

🟡 MEDIUM

Missing test coverage: Signature injection validation

The new validation logic added in lines 1049-1055 (checking sigCount < 1 and buffer size) has no corresponding test coverage. According to grep results, there are no tests for:

• "Transaction has no signature slots"
• "Transaction buffer too small"

From CLAUDE.md line 49-51:

Testing: Vitest. Mock all RPC/API calls — never hit real networks.

Consider adding test cases to src/__tests__/trading.test.js or creating integration tests in the walletconnect trading test file:

it('throws when WC signature injection finds zero signature slots', async () => {
  // Mock transaction with sigCount = 0
  const txWithZeroSigs = Buffer.from([0x00]); // compact-u16: 0
  const txBase64 = txWithZeroSigs.toString('base64');
  
  // Mock WC response with signature
  mockWcResult({ signature: 'someBase58Sig' });
  
  await expect(execute(...)).rejects.toThrow('Transaction has no signature slots');
});

it('throws when transaction buffer too small for signature injection', async () => {
  // Mock transaction with sigCount = 1 but buffer too small
  const smallTx = Buffer.from([0x01, 0x00, 0x00]); // only 3 bytes
  const txBase64 = smallTx.toString('base64');
  
  await expect(execute(...)).rejects.toThrow('Transaction buffer too small');
});

This prevents regressions and documents the expected behavior.

Confidence: 85/100

[nansen-pr-reviewer]

🟡 MEDIUM

Potential issue: No error handling if braces never balance

If the brace counter never reaches zero (malformed JSON), the function will collect all lines from startIdx to the end and then call JSON.parse() on the entire remaining output. This could lead to:

1. Memory issues if the output is very large
2. Unhelpful error messages (JSON parse error instead of "braces never balanced")

Consider adding explicit handling:

function parseWcJson(output) {
  const lines = output.split('\n');
  const startIdx = lines.findIndex(l => l.trimStart().startsWith('{'));
  if (startIdx === -1) throw new Error('No JSON output from walletconnect');

  // Handle multi-line JSON: collect lines until braces balance
  let braces = 0;
  const jsonLines = [];
  for (let i = startIdx; i < lines.length; i++) {
    jsonLines.push(lines[i]);
    for (const ch of lines[i]) {
      if (ch === '{') braces++;
      else if (ch === '}') braces--;
    }
    if (braces === 0) break;
  }
  
  // If we got here and braces !== 0, JSON never closed
  if (braces !== 0) {
    throw new Error(`Malformed JSON from walletconnect: unbalanced braces (${braces})`);
  }
  
  return JSON.parse(jsonLines.join('\n'));
}

This provides a clearer error message when the JSON structure is invalid.

Confidence: 83/100

[nansen-pr-reviewer]

pr-reviewer Review Summary

📝 5 findings

Review completed. Please address the inline comments below.

Findings by Severity

Severity	Count
🔴 Critical	0
🟠 High	1
🟡 Medium	4
🔵 Low	0

Summary

This PR addresses PR review feedback with improvements to Solana chain validation, multi-line JSON parsing, and error handling. Found one high-severity issue: signature injection validation happens too late and should validate transaction buffer before decoding the signature. Four medium-severity findings related to JSON parsing robustness: the brace-counting approach can be fooled by strings containing braces, may run indefinitely on malformed JSON, and doesn't explicitly handle the case where braces never balance. Also missing test coverage for the new signature validation error paths.

---

Token usage: 1,836,927 input, 5,252 output

[nansen-pr-reviewer]

pr-reviewer Summary

✅ No issues found

The code review completed successfully with no findings.

Findings by Severity

Severity	Count
🔴 Critical	0
🟠 High	0
🟡 Medium	0
🔵 Low	0

Summary

No issues found. This PR adds { cause } to two error re-throw locations to preserve the original error stack trace, following JavaScript best practices. The changes are minimal, syntactically correct, compatible with the project's Node.js version requirement (>=18.0.0), and consistent with similar error handling patterns already present in the codebase (transfer.js). The implementation is clean and focused.

---

Token usage: 1,623,794 input, 3,945 output

[nansen-pr-reviewer]

🟡 MEDIUM

Potential infinite loop if JSON never closes

The multi-line JSON parsing logic collects lines until braces balance to zero. However, if the input has unclosed braces (malformed JSON), this loop will continue until i < lines.length becomes false, then JSON.parse() will throw.

Consider adding a safeguard:

if (braces === 0 && jsonLines.length > 0) break;

Or add a maximum line limit to prevent unbounded memory consumption:

if (i - startIdx > 1000) {
  throw new Error('JSON output too large or malformed');
}

Confidence: 82/100

[nansen-pr-reviewer]

🔵 LOW

Buffer size validation happens after buffer is allocated

The validation if (txBytes.length < sigCountSize + 64) occurs after Buffer.from(txBase64, 'base64') is created. While this prevents the signature injection from corrupting memory, it would be more defensive to validate the transaction structure before allocating the buffer.

Consider validating directly on the base64 string length or checking the decoded length first:

const txBytes = Buffer.from(txBase64, 'base64');
const { value: sigCount, size: sigCountSize } = readCompactU16(txBytes, 0);
if (sigCount < 1) {
  throw new Error('Transaction has no signature slots');
}
const minRequiredLength = sigCountSize + 64;
if (txBytes.length < minRequiredLength) {
  throw new Error(`Transaction buffer too small for signature: need ${minRequiredLength}, got ${txBytes.length}`);
}

The current code is correct, but the order of checks could be improved for clarity.

Confidence: 80/100

[nansen-pr-reviewer]

🔵 LOW

Address comparison uses trim() but original comparison doesn't

For Solana addresses, the code uses .trim() to guard against whitespace:

wcAddress.trim() !== quoteWallet.trim()

But for EVM addresses it uses:

wcAddress.toLowerCase().trim() !== quoteWallet.toLowerCase().trim()

This is actually correct—Solana addresses are case-sensitive (base58) while EVM addresses are not (hex). However, the asymmetry might be confusing. Consider adding a clarifying comment:

// Solana addresses are case-sensitive (base58), EVM addresses are case-insensitive (hex)
if (quoteWallet && (chainType === 'solana'
  ? wcAddress.trim() !== quoteWallet.trim()  // case-sensitive
  : wcAddress.toLowerCase().trim() !== quoteWallet.toLowerCase().trim())) {  // case-insensitive

Confidence: 75/100 (this is a minor clarity issue, not a bug)

[nansen-pr-reviewer]

🟠 HIGH

Character-by-character brace counting could fail on JSON strings containing braces

The current implementation counts braces character-by-character across all lines:

for (const ch of lines[i]) {
  if (ch === '{') braces++;
  else if (ch === '}') braces--;
}

This will incorrectly count braces that appear inside JSON string values. For example:

{
  "message": "Use { and } carefully",
  "data": "5K4Ld..."
}

The braces inside the string would be counted, causing the parser to stop prematurely or continue too far.

Recommended fix: Use a proper JSON parser state machine or rely on JSON.parse() error handling:

function parseWcJson(output) {
  const lines = output.split('\n');
  const startIdx = lines.findIndex(l => l.trimStart().startsWith('{'));
  if (startIdx === -1) throw new Error('No JSON output from walletconnect');

  // Try parsing progressively larger chunks until valid JSON
  for (let i = startIdx; i < lines.length; i++) {
    const jsonCandidate = lines.slice(startIdx, i + 1).join('\n');
    try {
      return JSON.parse(jsonCandidate);
    } catch {
      // Not yet complete, continue
    }
  }
  throw new Error('Malformed JSON output from walletconnect');
}

Confidence: 90/100 — This is a real bug that will manifest if the WalletConnect CLI ever outputs JSON with braces in string values.

[nansen-pr-reviewer]

pr-reviewer Summary

📝 4 findings

Review completed. Please address the inline comments below.

Findings by Severity

Severity	Count
🔴 Critical	0
🟠 High	1
🟡 Medium	1
🔵 Low	2

Summary

This PR adds Solana WalletConnect support for trading, enabling users to sign DEX swap transactions with Solana wallets like Phantom and Solflare. The implementation is mostly solid with good test coverage. However, there is one high-severity issue: the multi-line JSON parser uses character-by-character brace counting which will fail if JSON strings contain braces. The recommended fix is to use progressive JSON.parse() attempts instead. Additionally, there are minor improvements suggested for buffer validation ordering and code clarity around address comparison.

---

Token usage: 2,012,792 input, 4,899 output