Skip to content

feat(sdk-core): add getEddsaMPCv2RecoveryKeyShares helper#8918

Open
vibhavgo wants to merge 1 commit into
masterfrom
WCI-396/sdk-core
Open

feat(sdk-core): add getEddsaMPCv2RecoveryKeyShares helper#8918
vibhavgo wants to merge 1 commit into
masterfrom
WCI-396/sdk-core

Conversation

@vibhavgo
Copy link
Copy Markdown
Contributor

@vibhavgo vibhavgo commented Jun 2, 2026

Summary

Adds and exports getEddsaMPCv2RecoveryKeyShares — a standalone helper that decrypts both EdDSA MPCv2 reduced keycards in parallel, validates that pub and rootChainCode match independently, and returns typed Buffer key shares with the derived commonKeyChain.

Ticket: WCI-396

Changes

  • Parallel decryption of user and backup keycards via Promise.all
  • Validate pub and rootChainCode separately with distinct error messages
  • Wrap getDecodedReducedKeyShare in try-catch to surface a descriptive error for malformed/missing keycard fields
  • Clear unsupported error for v2 (Argon2id) envelopes detected via isV2Envelope
  • 4 unit tests: v1 happy path, v2 rejection, malformed keycard, mismatched wallet keys

Test Plan

  • yarn run unit-test --scope @bitgo/sdk-core -- -- --grep "getEddsaMPCv2RecoveryKeyShares" — all 4 tests pass

CLOSES TICKET: WCI-396

@linear-code
Copy link
Copy Markdown

linear-code Bot commented Jun 2, 2026

WCI-396

@vibhavgo vibhavgo force-pushed the WCI-396/sdk-core branch 3 times, most recently from e1e5266 to db49315 Compare June 3, 2026 19:10
@vibhavgo vibhavgo marked this pull request as ready for review June 3, 2026 19:35
@vibhavgo vibhavgo requested review from a team as code owners June 3, 2026 19:35
@vibhavgo vibhavgo force-pushed the WCI-396/sdk-core branch from db49315 to c447a30 Compare June 4, 2026 10:31
Ticket: WCI-396

Decrypt both keycards in parallel, validate pub and rootChainCode
separately with distinct errors, and surface a clear message when
keycard material is missing or a v2 Argon2id envelope is detected.
Adds 7 unit tests covering happy path, field validation, and
pub/rootChainCode mismatch cases.

Fix cbor-x missing-package CI error by exporting MPSUtil.cborEncode
from sdk-lib-mpc (which already declares cbor-x) and using that in
the sdk-core test instead of require('cbor-x') directly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vibhavgo vibhavgo force-pushed the WCI-396/sdk-core branch from c447a30 to 28cbfd6 Compare June 4, 2026 10:34
@vibhavgo vibhavgo requested a review from Marzooqa June 4, 2026 10:34
@rajangarg047
Copy link
Copy Markdown
Contributor

@claude

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 4, 2026

Claude finished @rajangarg047's task —— View job


Code Review for getEddsaMPCv2RecoveryKeyShares helper

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Review implementation code in eddsaMPCv2.ts
  • Review test coverage and quality
  • Analyze for bugs, security issues, and best practices
  • Provide detailed feedback and recommendations

Overall Assessment: ✅ Strong Implementation

This is a well-implemented feature that follows best practices. The code is clear, secure, and thoroughly tested.

🎯 Strengths

  1. Excellent Error Handling

    • Clear, descriptive error messages for all failure scenarios (modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts:942-986)
    • Proper distinction between v2 envelope rejection and malformed keycard errors
    • Defensive validation of required fields (keyShare, pub, rootChainCode)
  2. Security Best Practices

    • Parallel processing with Promise.all prevents timing side-channels (line 967)
    • Independent validation of pub and rootChainCode with separate error messages (lines 971-973, 977-979)
    • Try-catch wrapper around getDecodedReducedKeyShare provides graceful error handling (lines 950-957)
    • Clear v2 envelope rejection prevents incorrect decryption attempts (lines 942-946)
  3. Comprehensive Test Coverage

    • 4 test scenarios covering all critical paths (modules/sdk-core/test/unit/bitgo/utils/tss/eddsa/eddsaMPCv2.ts:344-394)
    • v1 happy path, v2 rejection, malformed keycard, and mismatched wallet keys
    • Tests use realistic mock data and validate both success and failure cases
  4. Clean API Design

    • Well-defined TypeScript interface (EddsaMPCv2RecoveryKeyShares, lines 47-51)
    • Optional walletPassphrase parameter with proper fallback handling
    • Returns typed Buffer objects for key shares

🔍 Technical Review

Implementation (eddsaMPCv2.ts:936-986)

  • Function signature is appropriate for standalone use
  • v2 envelope detection using isV2Envelope is correct
  • SJCL decryption usage aligns with existing patterns in the codebase
  • Buffer handling follows established conventions

Error Messages

  • Line 943-946: Clear explanation of v2 limitation with helpful guidance
  • Line 954-956: Specific error for malformed keycards mentioning required fields
  • Line 972: Precise error for mismatched pub keys
  • Line 978: Precise error for mismatched rootChainCode

Return Value Construction

  • Lines 981-985: Proper Buffer.from() usage and string concatenation for commonKeyChain

🧪 Test Quality Assessment

The test suite is exemplary:

  1. Realistic test data: Uses MPSUtil.generateEdDsaDKGKeyShares() for authentic key material
  2. Edge case coverage: Tests v2 envelope rejection, malformed data, and validation failures
  3. Clear test names: Each test clearly describes the scenario being tested
  4. Proper assertions: Tests both return values and error conditions

Recommendations

Minor Suggestions:

  1. Consider adding JSDoc: While the function is well-named, JSDoc comments would enhance maintainability
  2. Type annotation: The decodeKey inner function could benefit from explicit return type annotation for clarity

Future Enhancements (not blocking):

  1. v2 support: When v2 decryption support is added, this function provides a clear migration path
  2. Validation levels: Could potentially add different validation strictness levels in the future

🔒 Security Analysis

  • No timing vulnerabilities: Parallel processing prevents timing-based attacks
  • No secret leakage: Errors don't expose sensitive information
  • Proper input validation: All inputs are validated before processing
  • Safe error handling: Try-catch blocks prevent crashes from malformed input

📝 Conclusion

This implementation demonstrates excellent software engineering practices. The code is:

  • Secure with proper input validation and error handling
  • Performant with parallel processing
  • Maintainable with clear structure and comprehensive tests
  • User-friendly with descriptive error messages

The PR is ready for merge and serves as a good example of how to implement standalone utility functions in this codebase.


Job Run: View details

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