WCI-398

@claude

Claude finished @Marzooqa's task —— View job

Code Review for MPCv2 Signed Recovery

Tasks:

• Read repository guidelines (CLAUDE.md)
• Analyze main implementation changes in sol.ts
• Review utility function changes in eddsaMPCv2.ts
• Examine test coverage and quality
• Check for potential security issues
• Verify error handling and edge cases
• Provide comprehensive feedback

Overall Assessment

This PR successfully implements MPCv2 signed recovery for Solana, adding automatic keycard detection and routing to the MPS DSG signing path. The implementation is well-structured and maintains backward compatibility with MPCv1 paths.

Detailed Review

✅ Strengths

1. Clean Architecture: The early keycard detection pattern at lines 1240-1250 in sol.ts is excellent - it fails fast if decryption fails and determines the signing path upfront.

2. Proper Address Derivation: Using deriveUnhardenedMps for MPCv2 vs MPC.deriveUnhardened for MPCv1 (lines 1258-1263) correctly accounts for the different derivation formulas between MPS and TSS stacks.

3. Security Validation: The commonKeyChain validation at lines 1495-1497 prevents key material mismatches, which is crucial for security.

4. Comprehensive Test Coverage: The test suite covers all major scenarios:

   • Native SOL recovery
   • SPL token recovery
   • Token-2022 with programId preservation
   • Durable nonce handling
   • Error cases (commonKeyChain mismatch)

5. Sinon-Friendly Design: The plain object export pattern for EddsaMPCv2RecoveryFunctions elegantly solves the TypeScript/Sinon stubbing issue without affecting production code.

⚠️ Areas for Improvement

1. Error Handling & Context (Medium Priority)

// Line 1247-1249 in sol.ts
catch (e) {
  throw new Error(`Error decrypting user keychain: ${e instanceof Error ? e.message : String(e)}`);
}

Issue: Generic error message loses important context about which keychain failed.
Suggestion: Include keychain type in error message:

throw new Error(`Error decrypting user keychain for MPCv2 detection: ${e instanceof Error ? e.message : String(e)}`);

2. Magic Number (Low Priority)

// Line 1259 & 1262 in sol.ts  
accountId = deriveUnhardenedMps(bitgoKey, currPath).slice(0, 64);
accountId = MPC.deriveUnhardened(bitgoKey, currPath).slice(0, 64);

Issue: Magic number 64 appears twice without explanation.
Suggestion: Add a constant:

const ED25519_PUBLIC_KEY_HEX_LENGTH = 64; // 32 bytes * 2 hex chars
accountId = deriveUnhardenedMps(bitgoKey, currPath).slice(0, ED25519_PUBLIC_KEY_HEX_LENGTH);

3. Code Duplication (Low Priority)

The MPCv1 and MPCv2 signing blocks (lines 1456-1507) have similar structure but different implementations. While the duplication is justified due to different protocols, consider extracting common setup logic if this pattern repeats elsewhere.

🔒 Security Analysis

✅ Proper key validation: commonKeyChain matching prevents key substitution attacks
✅ Error propagation: Decryption errors properly bubble up rather than being silently caught
✅ No key material leakage: Error messages don't expose sensitive data
✅ Consistent derivation: Uses appropriate derivation functions for each MPC version

📋 Test Quality

Excellent coverage with realistic scenarios:

• ✅ All major code paths covered
• ✅ Error conditions tested (commonKeyChain mismatch)
• ✅ Token-specific logic verified (SPL, Token-2022)
• ✅ Durable nonce integration tested
• ✅ Proper mocking strategy using sinon sandbox

🔧 Technical Implementation

eddsaMPCv2.ts Changes

The addition of standalone functions and the EddsaMPCv2RecoveryFunctions export object is well-designed:

• Clean separation: Recovery functions are isolated from the main class
• Testability: Plain object properties are easily stubbed by Sinon
• Maintainability: Functions can be used independently of the class instance

Recovery Function Design

The recovery functions follow good patterns:

• isEddsaMpcV1SigningMaterial: Robust detection with proper error handling
• getEddsaMpcV2RecoveryKeySharesFromReducedKey: Thorough validation of key shares
• signRecoveryEddsaMPCv2: Includes signature verification for additional security

📝 Minor Suggestions

1. Comment Enhancement (line 1238-1239): Consider expanding the comment to mention the key difference:

// Detect MPCv2 keycards (CBOR format) vs MPCv1 (JSON format) — 
// will throw if decryption fails (e.g., wrong password).

2. Type Safety: Consider adding a type guard for the keycard detection result to make the isMpcV2 boolean more explicit.

Recommendation

✅ APPROVE - This is a well-implemented feature that properly extends the Solana recovery functionality to support MPCv2. The code is secure, well-tested, and maintains backward compatibility. The minor suggestions above are enhancements rather than blocking issues.

Job Run: Link
Branch: WCI-398/sdk-coin-sol

@claude review the pr, Are there bugs introduced? Is it well tested? Is it consistent with the ecdsa MPCv2 approach?

Claude finished @Marzooqa's task —— View job

PR Review: MPCv2 Signed Recovery for Solana

Tasks:

• Read repository guidelines (CLAUDE.md)
• Analyze main implementation changes in sol.ts
• Review utility function changes in eddsaMPCv2.ts
• Examine test coverage and quality
• Check for potential security issues
• Compare with ECDSA MPCv2 approach for consistency
• Address existing review comments
• Provide comprehensive feedback

Overall Assessment

🔍 BUGS FOUND: No critical bugs, but there are issues that need addressing based on existing review comments and code analysis.

✅ WELL TESTED: Yes, comprehensive test coverage with 5 new test cases covering all major scenarios.

⚠️ CONSISTENCY ISSUES: The approach differs from ECDSA MPCv2 pattern but is justified by different requirements.

Detailed Analysis

1. Existing Review Comments to Address

HIGH PRIORITY - Must Fix:

Duplicate Method (Comment by Marzooqa)

modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts:930

There's already a method isEddsaMpcV1SigningMaterial in the EddsaMPCv2Utils class (lines 56-68 in original). The standalone function at line 930 duplicates this functionality.

Recommendation: Move the existing class method out as a standalone function instead of creating a new one.

Unnecessary Try-Catch (Comment by Marzooqa)

modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts:943

} catch (e) {
  // Decryption errors (e.g., wrong password) should propagate; don't catch them.
  throw e;
}

This try-catch block simply re-throws the error, serving no purpose.

Recommendation: Remove the try-catch block around the decryption logic.

2. Code Quality Issues

Error Context Loss

modules/sdk-coin-sol/src/sol.ts:1247-1249

catch (e) {
  throw new Error(`Error decrypting user keychain: ${e instanceof Error ? e.message : String(e)}`);
}

Issue: Generic error message loses context about which operation failed.
Fix: Be more specific: "Error decrypting user keychain for MPCv2 detection: ..."

Magic Numbers

modules/sdk-coin-sol/src/sol.ts:1259 & 1262

accountId = deriveUnhardenedMps(bitgoKey, currPath).slice(0, 64);
accountId = MPC.deriveUnhardened(bitgoKey, currPath).slice(0, 64);

Issue: Magic number 64 appears without explanation.
Fix: Add constant const ED25519_PUBLIC_KEY_HEX_LENGTH = 64; // 32 bytes * 2 hex chars

3. Security Analysis ✅

• ✅ Proper key validation: commonKeyChain matching prevents key substitution attacks
• ✅ Early failure: MPCv2 detection fails fast on decryption errors
• ✅ No key leakage: Error messages don't expose sensitive data
• ✅ Signature verification: MPCv2 path includes signature verification for additional security

4. Test Coverage Analysis ✅

Excellent coverage with 5 comprehensive tests:

• ✅ Native SOL MPCv2 signed recovery (routes to MPCv2 path correctly)
• ✅ commonKeyChain mismatch error handling
• ✅ SPL token recovery with correct getTokenAccountsByOwner routing
• ✅ Token-2022 recovery preserving programId
• ✅ Durable nonce + MPCv2 integration

Test Quality: Proper use of sinon sandbox, realistic stubs, good assertion coverage.

5. Consistency with ECDSA MPCv2 ⚠️

Key Differences (Justified):

• Pattern: ECDSA MPCv2 uses class methods; EdDSA MPCv2 uses standalone functions + export object
• Reason: TypeScript compilation + Sinon stubbing compatibility (excellent technical explanation in PR description)
• Recovery Functions: EdDSA has dedicated recovery functions; ECDSA doesn't have equivalent recovery pattern
• Derivation: EdDSA uses deriveUnhardenedMps vs ECDSA's approach due to different crypto libraries

Verdict: The differences are well-justified and necessary due to different requirements.

6. Architecture Review ✅

Strengths:

• Early Detection: MPCv2 keycard detection at lines 1240-1250 is well-placed
• Clean Separation: MPCv1 and MPCv2 paths are clearly separated
• Backward Compatibility: All existing paths remain unchanged
• Proper Derivation: Uses correct derivation functions for each MPC version

Technical Implementation:

• Address Derivation: Correctly uses deriveUnhardenedMps for MPCv2 vs MPC.deriveUnhardened for MPCv1
• Export Object Pattern: Smart solution to TypeScript/Sinon compatibility issues
• Recovery Flow: Well-structured with proper error handling and validation

Recommendations

Must Fix (Blocking):

1. Address Marzooqa's comments:
   • Move existing class method out instead of creating duplicate function
   • Remove unnecessary try-catch block that just re-throws

Should Fix (Quality):

2. Improve error context in sol.ts line 1247
3. Add constant for magic number 64
4. Consider extracting common setup logic if this pattern repeats elsewhere

Code Example for Fix #1:

// Remove the duplicate function at line 930, instead move the existing class method:
export async function isEddsaMpcV1SigningMaterial(
  encryptedKeyShare: string, 
  walletPassphrase: string,
  bitgo?: BitGoBase
): Promise<boolean> {
  const prv = bitgo
    ? await bitgo.decryptAsync({ input: encryptedKeyShare, password: walletPassphrase })
    : sjcl.decrypt(walletPassphrase, encryptedKeyShare);
  
  try {
    const m = JSON.parse(prv);
    return (
      typeof m?.uShare?.seed === 'string' &&
      typeof m?.bitgoYShare?.u === 'string' &&
      (typeof m?.backupYShare?.u === 'string' || typeof m?.userYShare?.u === 'string')
    );
  } catch {
    return false;
  }
}

Final Verdict

NEEDS REVISION - The implementation is solid but must address the duplicate method and unnecessary try-catch issues identified in the existing review comments. Once those are fixed, this will be a well-implemented feature.

Job Run: Link
Branch: WCI-398/sdk-coin-sol