Skip to content

feat: implement async multisig recovery operations#250

Merged
danielpeng1 merged 1 commit into
masterfrom
danielpeng/wcn-888-mbe-async-ify-multisig_recovery-endpoints
Jun 24, 2026
Merged

feat: implement async multisig recovery operations#250
danielpeng1 merged 1 commit into
masterfrom
danielpeng/wcn-888-mbe-async-ify-multisig_recovery-endpoints

Conversation

@danielpeng1

Copy link
Copy Markdown
Contributor

Support for asynchronous onchain multisig recovery in masterBitgoExpress (recovery + recoveryConsolidations).

  • Added submitMultisigRecoveryJob / recoverMultisigOrSubmitJob to submit recovery jobs to the bridge in async mode
  • Added handleMultisigRecoveryOperation to complete multisig_recovery jobs from AWM (no WP submit)
  • Return 202 { jobId, status: 'pending' } on both recovery endpoints when async mode is enabled
  • Reject async mode for TSS/MPC recovery and multi-tx recovery consolidations
  • Updated route codecs, toApiResponse, and OpenAPI spec with 202 responses
  • Added/updated unit + integration tests (UTXO/EVM recovery, consolidations, worker, utils)

Ticket: WCN-888

@danielpeng1 danielpeng1 self-assigned this Jun 22, 2026
@linear-code

linear-code Bot commented Jun 22, 2026

Copy link
Copy Markdown

WCN-888

@danielpeng1

Copy link
Copy Markdown
Contributor Author

@claude review this PR. If possible, focus on async recovery implementation parity with the WCN-887 multisig_sign asyncify pattern.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown

Claude finished @danielpeng1's task —— View job


PR Review: Async Multisig Recovery Implementation

Reviewing this PR with focus on async recovery implementation parity with WCN-887 multisig_sign asyncify pattern.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze PR changes and key files
  • Find and study WCN-887 multisig_sign asyncify pattern
  • Compare implementation patterns for consistency
  • Review code for bugs, security, and best practices
  • Provide comprehensive review feedback

Overall Assessment

Excellent implementation parity with WCN-887 async pattern. The recovery implementation closely follows the established asyncify patterns used in multisig_sign, accelerate, consolidate, and sendMany operations.

Pattern Consistency Analysis

✅ Core Async Pattern Adherence

The implementation correctly follows the established async pattern:

  1. Utility Function: submitMultisigRecoveryJob() mirrors submitMultisigSignJob()
  2. Async Gateway: Uses submitJobViaBridgeClient() consistently
  3. Bridge Communication: Correct operationType (multisig_recovery) and key source headers
  4. Response Format: Proper { jobId, status: 'pending' } on 202 responses
  5. Worker Integration: New handleMultisigRecoveryOperation() in async job worker

✅ Request/Response Flow Consistency

  • Sync path fallback: Both recovery handlers properly fall back when asyncResult is null
  • API Response Mapping: toApiResponse() correctly maps async jobs to 202 Accepted
  • Error Handling: Consistent async mode rejection for TSS/MPC operations
  • Job Context: Recovery jobs don't need WP submission (unlike sign operations) ✓

Detailed Code Review

multisigRecoveryUtils.ts (NEW)

Strengths:

  • Clean separation of concerns with dedicated recovery utilities
  • Consistent function naming and parameter patterns
  • Proper TypeScript typing with MultisigRecoveryBody
  • Reuses parsing logic from multisigSignUtils

recoveryWallet.ts Changes

Line 115-125 - recoverMultisigOrSubmitJob():

async function recoverMultisigOrSubmitJob(/* ... */): Promise<SignedTransaction | AsyncJobResponse> {
  const asyncResult = await submitMultisigRecoveryJob(req, req.decoded.coin, recoveryBody);
  if (asyncResult) {
    return asyncResult;  // 202 response
  }
  return awmClient.recoveryMultisig(recoveryBody);  // sync fallback
}

✅ Perfect pattern match with other async handlers (sendMany, accelerate, etc.)

Line 256-258 - TSS Recovery Rejection:

if (req.config.asyncModeConfig.enabled) {
  throw new BadRequestError('Async mode is not yet supported for TSS/MPC recovery');
}

✅ Consistent with recoveryConsolidations rejection pattern

handleRecoveryConsolidations.ts Changes

Line 96-112 - Async Mode Handling:

  • ✅ Single transaction validation matches async limitations in other operations
  • ✅ Proper error messaging for multi-tx batches
  • ✅ Consistent TSS/MPC rejection (lines 52-56)

asyncJobWorker.ts Changes

Line 196-217 - handleMultisigRecoveryOperation():

export async function handleMultisigRecoveryOperation(/* ... */): Promise<void> {
  const signedTx = parseSignedRecoveryTransaction(parseAwmResponseBody(job.awmResponse, 'awmResponse'));
  // ... 
  await bridge.updateJob({ jobId, version, status: 'complete', result: signedTx });
}

✅ Excellent implementation:

  • No WP submission needed (unlike sign operations) - correctly simplified
  • Proper error handling and logging pattern
  • Consistent job completion flow

✅ Route and API Spec Changes

  • ✅ Proper OpenAPI spec updates with 202 responses
  • ✅ Route codecs correctly updated
  • toApiResponse() handles async jobs uniformly

Test Coverage Analysis

✅ Comprehensive Test Suite

New test file: multisigRecoveryUtils.test.ts

  • ✅ Tests async mode enabled/disabled scenarios
  • ✅ Validates bridge communication (correct headers, paths, body)
  • ✅ Tests response parsing with schema validation
  • ✅ Proper nock cleanup patterns

Enhanced existing tests:

  • recoveryWallet.test.ts: +202 lines of async test coverage
  • recoveryConsolidationsWallet.test.ts: +117 lines
  • ✅ Integration tests updated appropriately

Security & Best Practices

✅ Security Considerations

  • Input Validation: Proper validation of public keys and parameters
  • Error Handling: No sensitive data leakage in error messages
  • Type Safety: Strong TypeScript typing throughout
  • Bridge Communication: Secure header-based authentication

✅ Code Quality

  • DRY Principle: Good reuse of existing utilities (parseSignedMultisigTransaction)
  • Error Messages: Clear, actionable error messages
  • Logging: Appropriate logging levels and structured logs
  • Async Patterns: Proper Promise handling, no race conditions

Minor Suggestions

1. Documentation Consistency (Optional)

Consider adding JSDoc comments to match the pattern in multisigSignUtils.ts:

/** Submits a multisig recovery job to the bridge, or returns null when async mode is off. */
export async function submitMultisigRecoveryJob(/* ... */) {

2. Error Message Alignment (Optional)

The TSS rejection messages could be more consistent between recovery and recoveryConsolidations:

// Current (recoveryConsolidations.ts:53)
'Async mode is not yet supported for TSS/MPC recovery consolidations'

// Current (recoveryWallet.ts:257)  
'Async mode is not yet supported for TSS/MPC recovery'

// Suggested: Make them consistent
'Async mode is not yet supported for TSS/MPC recovery operations'

Recommendation

✅ APPROVE - This PR demonstrates excellent implementation parity with the WCN-887 asyncify pattern. The code follows established conventions, includes comprehensive testing, and maintains consistency with existing async operations. The implementation is clean, secure, and well-structured.

Key Strengths:

  • Perfect adherence to established async patterns
  • Comprehensive test coverage
  • Clean separation of concerns
  • Proper error handling and validation
  • No breaking changes to existing functionality

Job Run: Link to run
Branch: danielpeng/wcn-888-mbe-async-ify-multisig_recovery-endpoints

@danielpeng1 danielpeng1 marked this pull request as ready for review June 22, 2026 14:40
@danielpeng1 danielpeng1 requested review from a team as code owners June 22, 2026 14:40

@pranishnepal pranishnepal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if i'm wrong, but it seems that this is only doing user sign, i don't see backup sign happening -- without that, the recovery signing wouldn't be complete

@danielpeng1

Copy link
Copy Markdown
Contributor Author

Correct me if i'm wrong, but it seems that this is only doing user sign, i don't see backup sign happening -- without that, the recovery signing wouldn't be complete

Discussed earlier, but the asyncifying should be good for what's here right now; supporting split user/backup AWM + external recovery orchestration is a separate work we need to build.

@pranishnepal pranishnepal dismissed their stale review June 22, 2026 19:57

discussions ongoing

@danielpeng1 danielpeng1 merged commit cedbbfd into master Jun 24, 2026
22 checks passed
@danielpeng1 danielpeng1 deleted the danielpeng/wcn-888-mbe-async-ify-multisig_recovery-endpoints branch June 24, 2026 20:21
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.

3 participants