Skip to content

feat(sdk-coin-sol): MPCv2 recoverNestedAta#9114

Merged
vibhavgo merged 1 commit into
masterfrom
WCI-495/sdk-coin-sol
Jun 26, 2026
Merged

feat(sdk-coin-sol): MPCv2 recoverNestedAta#9114
vibhavgo merged 1 commit into
masterfrom
WCI-495/sdk-coin-sol

Conversation

@bitgo-ai-agent-dev

Copy link
Copy Markdown

What

  • Update recoverNestedAta to detect MPCv2 keycards using isEddsaMpcV1SigningMaterial
  • Derive the wallet root address with deriveUnhardenedMps for MPCv2 wallets instead of MPC.deriveUnhardened, ensuring the RecoverNested instruction signer is correct
  • Pass the isMpcV2 flag to signAndGenerateBroadcastableTransaction (added in WCI-494) so signing follows the MPCv2 path
  • Add unit tests for: MPCv2 keycard routing, MPCv1 regression, mismatched bitgoKey vs keycard commonKeyChain, and missing address validation

Why

  • recoverNestedAta previously only supported MPCv1. For MPCv2 wallets, the wallet root address was derived with the wrong algorithm (MPC.deriveUnhardened instead of deriveUnhardenedMps), causing the RecoverNested instruction to use an invalid signer address — the transaction would be rejected even before signing
  • Part of the EDDSA upgrade from Zengo to Silence Labs (MPCv2) implementation

Test plan

  • recoverNestedAta MPCv2 keycard — uses MPS-derived wallet address, does not call getTSSSignature
  • recoverNestedAta MPCv1 keycard regression — uses MPC.deriveUnhardened, calls getTSSSignature once
  • recoverNestedAta mismatched bitgoKey vs keycard — throws 'EdDSA MPCv2 recovery: commonKeyChain from keycard does not match bitgoKey'
  • Missing nestedAtaAddress — throws 'invalid nestedAtaAddress'
  • Missing ownerAtaAddress — throws 'invalid ownerAtaAddress'
  • Full unit test suite: 607 passing, 0 failing

Ticket: WCI-495

@linear-code

linear-code Bot commented Jun 25, 2026

Copy link
Copy Markdown

WCI-495

@bitgo-ai-agent-dev bitgo-ai-agent-dev Bot force-pushed the WCI-495/sdk-coin-sol branch from b5427ad to 3637d31 Compare June 25, 2026 07:20
@vibhavgo vibhavgo marked this pull request as ready for review June 25, 2026 07:27
@vibhavgo vibhavgo requested a review from a team as a code owner June 25, 2026 07:27
@vibhavgo vibhavgo requested a review from hrishikeshjain June 25, 2026 08:25
Base automatically changed from WCI-494/sdk-coin-sol to master June 26, 2026 05:44
@vibhavgo

Copy link
Copy Markdown
Contributor

@claude

@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @vibhavgo's task —— View job


Code Review: MPCv2 recoverNestedAta Implementation ✅

Todo List:

  • Read and analyze the main implementation changes in sol.ts
  • Review the test implementation and coverage
  • Check for security considerations and edge cases
  • Verify adherence to coding standards and best practices
  • Provide comprehensive review feedback

Code Review Summary

This PR successfully adds MPCv2 support to the recoverNestedAta method. The implementation is solid with proper MPCv2 keycard detection, correct key derivation, and comprehensive test coverage.

Implementation Changes (sol.ts:1674-1711)

Strengths:

  1. Correct MPCv2 Detection: Uses EDDSAUtils.isEddsaMpcV1SigningMaterial() to properly detect keycard type
  2. Proper Key Derivation: Correctly switches between deriveUnhardenedMps() (MPCv2) and MPC.deriveUnhardened() (MPCv1)
  3. Clean Integration: Passes isMpcV2 flag to signAndGenerateBroadcastableTransaction for consistent signing path
  4. Backward Compatibility: Maintains full MPCv1 support without breaking changes

Security Considerations:

  • ✅ Proper validation of wallet passphrase before MPCv2 detection
  • ✅ Fallback to false when no wallet passphrase provided (unsigned sweep case)
  • ✅ Consistent error handling for decryption failures

Test Coverage (sol.ts test file)

Comprehensive Test Suite:

  1. MPCv2 Flow Test: Verifies MPS-derived wallet address and confirms no getTSSSignature calls
  2. MPCv1 Regression Test: Ensures existing MPCv1 functionality works with MPC.deriveUnhardened
  3. Security Test: Validates error thrown when bitgoKey doesn't match keycard commonKeyChain
  4. Validation Tests: Covers missing nestedAtaAddress and ownerAtaAddress error cases

Test Quality:

  • ✅ Uses proper spies to verify correct code paths taken
  • ✅ Tests both success and failure scenarios
  • ✅ Verifies no regression in existing MPCv1 behavior
  • ✅ Good edge case coverage for validation errors

📋 Minor Observations

  1. Code Organization: The MPCv2 detection logic in recoverNestedAta is duplicated from other methods. Consider extracting to a shared utility method in future refactoring.

  2. Error Messages: The error message "EdDSA MPCv2 recovery: commonKeyChain from keycard does not match bitgoKey" could be more user-friendly, but it's consistent with similar messages elsewhere in the codebase.

  3. Type Safety: Good use of optional chaining (?.) and proper fallback values.

🔧 Alignment with Requirements

The implementation fully addresses the stated objectives:

  • ✅ MPCv2 keycard detection using isEddsaMpcV1SigningMaterial
  • ✅ Correct wallet root address derivation with deriveUnhardenedMps
  • ✅ Proper isMpcV2 flag passing to signing method
  • ✅ Comprehensive unit test coverage for all scenarios
  • ✅ Full MPCv1 backward compatibility

📊 Code Quality Assessment

  • Security: Excellent ✅
  • Test Coverage: Comprehensive ✅
  • Maintainability: Very Good ✅
  • Performance: No concerns ✅
  • Documentation: Adequate ✅

Recommendation: APPROVE

This is a well-implemented feature that correctly adds MPCv2 support to recoverNestedAta while maintaining full backward compatibility. The code follows established patterns in the codebase, includes robust error handling, and has excellent test coverage covering all important scenarios including edge cases and regressions.

The changes are minimal, focused, and low-risk while solving the critical issue where MPCv2 wallets would fail due to incorrect signer address derivation.

Update recoverNestedAta to detect MPCv2 keycards via
isEddsaMpcV1SigningMaterial and derive the wallet root address
using deriveUnhardenedMps instead of MPC.deriveUnhardened. Pass
the isMpcV2 flag to signAndGenerateBroadcastableTransaction so
the RecoverNested instruction signer and the signing path are
both correct for MPCv2 wallets.

Add unit tests for MPCv2 keycard routing, MPCv1 regression,
mismatched bitgoKey, and missing address validation.

Ticket: WCI-495
Session-Id: 342e5b05-4d0f-487f-8709-7f2d871de396
Task-Id: 69cf21d5-0fa2-4660-83d9-7a4f6b417906
@vibhavgo vibhavgo force-pushed the WCI-495/sdk-coin-sol branch from 3637d31 to bc52daa Compare June 26, 2026 05:58
@vibhavgo vibhavgo merged commit bd16fa0 into master Jun 26, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants