Skip to content

feat: refactor MPC/TSS wallet gen to use SDK callback#260

Open
danielpeng1 wants to merge 2 commits into
masterfrom
WCN-683/use-callbacks-for-mpctss-wallet-gen
Open

feat: refactor MPC/TSS wallet gen to use SDK callback#260
danielpeng1 wants to merge 2 commits into
masterfrom
WCN-683/use-callbacks-for-mpctss-wallet-gen

Conversation

@danielpeng1

Copy link
Copy Markdown
Contributor

Replaces hand-rolled ECDSA MPCv2 and EdDSA MPCv1 DKG orchestrators with the SDK's generateWalletWithExternalSigner callback pattern, consolidating onchain and TSS wallet generation into a single handleGenerateWallet handler.

  • Add createEcdsaMPCv2KeyGenCallbacks and createEddsaKeyGenCallbacks factories in walletGenerationCallbacks.ts; each drives both the user and backup AWM. User↔backup messages are carried in the factory closure (which is the same pattern as signing). ECDSA AWM rounds 2+3 are folded into round2Callback because both their inputs arrive with BitGo's round 1 response.
  • Replace handleGenerateMpcWallet + orchestrateEcdsaKeyGen + orchestrateEddsaKeyGen with a single keyGenCallbacks() dispatch keyed on baseCoin.getMPCAlgorithm().
  • Delete walletCreationUtils.ts (unused now but we can always get it back if needed).
  • Bump @bitgo-beta/sdk-core to 8.2.1-beta.1928 and @bitgo-beta/sdk-api to 1.10.1-beta.1923, to use EcdsaMPCv2KeyGenCallbacks, EddsaKeyGenCallbacks, and generateWalletWithExternalSigner.

Ticket: WCN-683

@danielpeng1 danielpeng1 self-assigned this Jul 27, 2026
@linear-code

linear-code Bot commented Jul 27, 2026

Copy link
Copy Markdown

WCN-683

@danielpeng1

Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown

Claude finished @danielpeng1's task —— View job


Code Review for MPC/TSS Wallet Generation Refactoring

Review Checklist:

  • Analyze the overall refactoring approach and architecture changes
  • Review the new callback factories implementation
  • Examine the consolidated wallet generation handler
  • Check test coverage and quality
  • Review deleted/modified files for completeness
  • Assess SDK integration and version compatibility
  • Provide comprehensive feedback and recommendations

Overall Assessment: STRONG APPROVE

This is an excellent refactoring that significantly improves the codebase architecture by consolidating wallet generation logic and leveraging the SDK's standardized callback pattern. The implementation is well-structured, thoroughly tested, and follows best practices.

🏆 Strengths

Architecture Improvements

  1. Excellent Consolidation: Replacing multiple handler functions with a single handleGenerateWallet that dispatches based on baseCoin.getMPCAlgorithm() is a clean architectural improvement - see handleGenerateWallet.ts:55-89.

  2. SDK Integration: The adoption of generateWalletWithExternalSigner from the SDK standardizes the approach and should reduce maintenance burden while improving reliability.

  3. Clean Abstraction: The callback factory pattern in walletGenerationCallbacks.ts properly encapsulates the complexity of different MPC protocols while maintaining a consistent interface.

Code Quality

  1. Robust Error Handling: Comprehensive assertions throughout the callbacks ensure protocol integrity (e.g., lines 123-124, 147-148 in walletGenerationCallbacks.ts).

  2. Clear State Management: The toMpcState() helper function (lines 49-52) provides a clean abstraction for threading encrypted state between callbacks.

  3. Proper Message Formatting: The formatBroadcastMessage() and formatP2PMessage() functions (lines 54-73) ensure correct message structure for AWM endpoints.

🧪 Test Coverage Excellence

The test suite is comprehensive and well-structured:

  1. Full Protocol Coverage: Tests cover all three wallet types (onchain, EdDSA MPCv1, ECDSA MPCv2) with both single and separate HSM modes.

  2. Detailed Unit Tests: The new walletGenerationCallbacks.test.ts provides excellent unit test coverage with proper mocking and assertion validation.

  3. Error Path Testing: Good coverage of error conditions (e.g., missing broadcast messages, mismatched keychains).

📋 Detailed Technical Review

ECDSA MPCv2 Implementation (createEcdsaMPCv2KeyGenCallbacks)

  • Smart Round Consolidation: The decision to fold AWM rounds 2+3 into round2Callback is well-documented and handles the protocol mismatch elegantly.
  • Message Threading: The closure-based approach for user↔backup message passing is clean and prevents state leakage.
  • Validation: Comprehensive party index validation ensures message routing integrity.

EdDSA MPCv1 Implementation (createEddsaKeyGenCallbacks)

  • Proper Sequencing: The two-phase finalize approach (user first, then backup) correctly handles the dependency on counterparty key shares.
  • Key Share Management: Proper handling of the complex key share exchange protocol.

General Handler Logic (handleGenerateWallet)

  • Clean Dispatch: The keyGenCallbacks() function provides a clear algorithm-based dispatch mechanism.
  • Async Mode Handling: Proper separation of async and sync modes with appropriate error messages.
  • EVM Keyring Support: Clean handling of the special case for EVM keyring wallets.

🎯 Minor Suggestions

1. Enhanced Error Context

Consider adding more context to error messages in callback failures:

// Current (line 88)
throw new BadRequestError(`Unsupported MPC algorithm: ${algorithm}`);

// Suggested
throw new BadRequestError(`Unsupported MPC algorithm: ${algorithm}. Supported algorithms: ecdsa, eddsa`);

2. Type Safety Enhancement

The algorithm dispatch could be more type-safe:

// Consider using a const assertion or enum for algorithm types
const SUPPORTED_ALGORITHMS = ['ecdsa', 'eddsa'] as const;
type SupportedAlgorithm = typeof SUPPORTED_ALGORITHMS[number];

3. Documentation Enhancement

Consider adding JSDoc comments to the callback factory functions explaining the protocol flow and round mapping, especially for the ECDSA MPCv2 round consolidation.

4. Magic Number Constants

The party indices could be more discoverable:

// Consider moving to a shared constants file
const MPC_PARTY_INDICES = { USER: 0, BACKUP: 1, BITGO: 2 } as const;

Verification Points

  • SDK Version Compatibility: The dependency bumps to @bitgo-beta/sdk-core@8.2.1-beta.1928 and @bitgo-beta/sdk-api@1.10.1-beta.1923 are appropriate for the new callback interfaces.
  • Backward Compatibility: The refactoring maintains the same API surface, ensuring no breaking changes.
  • State Management: Proper handling of encrypted session state throughout all protocols.
  • Security: No exposure of sensitive data in logs or error messages.

🚀 Impact Assessment

Positive Impacts:

  • Reduced code duplication and maintenance burden
  • Standardized approach across all wallet generation types
  • Improved error handling and protocol validation
  • Better separation of concerns
  • Enhanced testability

Risk Assessment: LOW - The comprehensive test coverage and incremental refactoring approach minimize risk.


Final Recommendation: This PR represents a significant improvement to the codebase architecture and should be merged. The refactoring is well-executed, thoroughly tested, and aligns with best practices for SDK integration.

Job run: WCN-683/use-callbacks-for-mpctss-wallet-gen

@danielpeng1
danielpeng1 marked this pull request as ready for review July 27, 2026 15:07
@danielpeng1
danielpeng1 requested review from a team as code owners July 27, 2026 15:07
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.

1 participant