Skip to content

feat: enhance multisig recovery with split AWM support#251

Open
danielpeng1 wants to merge 1 commit into
masterfrom
WCN-1082/split-user-backup-awm
Open

feat: enhance multisig recovery with split AWM support#251
danielpeng1 wants to merge 1 commit into
masterfrom
WCN-1082/split-user-backup-awm

Conversation

@danielpeng1

Copy link
Copy Markdown
Contributor

Adds two-phase split-AWM recovery (user half-sign -> backup full-sign) for recovery and recoveryConsolidations, for UTXO + EVM, local + external signing.

  • AWM multisigRecovery.ts: implements keyToSign (user/backup) with runtime guards on the half-signed tx shape; external signing failures now throw BitgoApiResponseError (preserves upstream status) and reject EVM backup with a rich halfSigned object (requires flat txHex).
  • MBE orchestration: when user and backup AWMs differ, signs synchronously in two phases; single-AWM behavior unchanged.
  • submitMultisigRecoveryJob gains a sources param; worker prefers awmBackupResponse when present, this is forward-compatible scaffolding.
  • Note: Split-AWM recovery always runs synchronously, even when async is enabled. Async jobs fan out to user and backup AWMs in parallel, so they can’t do step 1 (user half-sign) then step 2 (backup full-sign).

Tests

  • AWM: UTXO + EVM two-phase (local + external), keyToSign isolation, half-signed-tx guards (400s).
  • MBE: UTXO + EVM split-AWM two-phase; sync path used even with asyncEnabled (bridge not called).
  • Worker: awmBackupResponse preferred; malformed rejected. Utils: sources param. testUtils: X-OSO-Source header nock.

Ticket: WCN-1082

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

linear-code Bot commented Jun 26, 2026

Copy link
Copy Markdown

WCN-1082

@danielpeng1 danielpeng1 marked this pull request as ready for review June 26, 2026 13:47
@danielpeng1 danielpeng1 requested review from a team as code owners June 26, 2026 13:47
@danielpeng1

Copy link
Copy Markdown
Contributor Author

@claude review and get_issue the ticket on linear for more context

@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown

Claude finished @danielpeng1's task —— View job


Code Review for Split AWM Recovery Enhancement

  • Read and analyze the PR changes across all 12 modified files
  • Attempt to get context from Linear ticket WCN-1082 (access denied, proceeding with PR context)
  • Review AWM multisig recovery implementation changes
  • Review MBE orchestration changes
  • Review worker and utility changes
  • Analyze test coverage and quality
  • Provide comprehensive code review feedback

Summary

This PR implements a significant enhancement to the multisig recovery system by adding split AWM support - enabling two-phase recovery where user keys and backup keys can be managed by separate AWM instances. The implementation is well-architected and addresses both UTXO and EVM recovery flows with proper error handling and comprehensive testing.

Key Strengths

1. Excellent Architecture Design

  • Clean separation of concerns: The keyToSign parameter elegantly controls which keys are used in each phase
  • Backward compatibility: Single-AWM mode remains unchanged, ensuring no breaking changes
  • Runtime guards: Strong validation ensures half-signed transactions match expected formats

2. Robust Error Handling

  • Preserves upstream errors: BitgoApiResponseError maintains original status codes from key provider failures (src/advancedWalletManager/handlers/multisigRecovery.ts:376-389)
  • Comprehensive validation: Both UTXO and EVM recovery have proper type guards and error messages
  • External signing safeguards: EVM backup signing properly validates flat txHex format

3. Comprehensive Test Coverage

  • 481 new test lines covering both local and external signing scenarios
  • Edge case testing: Malformed transaction validation, missing parameters, error conditions
  • Integration testing: End-to-end split-AWM recovery flows

Technical Analysis

AWM Recovery Handler (src/advancedWalletManager/handlers/multisigRecovery.ts)

Excellent additions:

// Lines 47-49: Clear validation for backup signing requirements
if (keyToSign === 'backup' && !halfSignedTransaction) {
  throw new BadRequestError('halfSignedTransaction is required when keyToSign is "backup"');
}

// Lines 58-67: Smart EVM external signing validation
if (keyToSign === 'backup' && baseCoin.isEVM()) {
  if (!halfSignedTransaction || typeof (halfSignedTransaction as { txHex?: unknown }).txHex !== 'string') {
    throw new BadRequestError('External backup signing for EVM coins requires halfSignedTransaction.txHex');
  }
}

Strong pattern for key retrieval optimization:

// Lines 80-87: Only fetch needed keys per phase
const userPrv = keyToSign === 'backup' ? undefined : await retrieveKeyProviderPrvKey(...);
const backupPrv = keyToSign === 'user' ? undefined : await retrieveKeyProviderPrvKey(...);

MBE Orchestration (src/masterBitgoExpress/handlers/recoveryWallet.ts)

Smart two-phase logic:

// Lines 119-127: Clear split-AWM detection and sequencing
if (userClient !== backupClient) {
  const halfSignedTx = await userClient.recoveryMultisigUserHalfSign(recoveryBody);
  return backupClient.recoveryMultisig({
    ...recoveryBody,
    keyToSign: 'backup',
    halfSignedTransaction: halfSignedTx,
  });
}

Worker Enhancement (src/masterBitgoExpress/workers/asyncJobWorker.ts)

Forward-compatible scaffolding:

// Lines 205-210: Intelligent response selection
const isSplitRecovery = job.awmBackupResponse !== undefined;
const finalResponse = isSplitRecovery ? job.awmBackupResponse : job.awmResponse;

Minor Recommendations

1. Code Duplication in EVM Recovery

In multisigRecovery.ts, the EVM signing logic for backup completion (lines 121-149) and full recovery (lines 204-232) share significant overlap. Consider extracting a helper function:

function buildEvmSigningParams(halfSignedTx, walletKeys, bitgoEnv) {
  return {
    gasPrice, gasLimit, // ... common params
    replayProtectionOptions: getReplayProtectionOptions(bitgoEnv, halfSignedTx?.replayProtectionOptions),
  };
}

2. Type Safety Enhancement

Consider making the keyToSign parameter required when halfSignedTransaction is provided to catch misconfigurations at compile time:

interface RecoveryMultisigOptions {
  // ... existing fields
  keyToSign: 'user' | 'backup';
  halfSignedTransaction?: keyToSign extends 'backup' ? SignedTransaction : never;
}

3. Test Organization

The test file recoveryMultisigTransaction.test.ts is quite large (481 lines). Consider splitting into separate files:

  • recoveryMultisigTransaction.utxo.test.ts
  • recoveryMultisigTransaction.evm.test.ts
  • recoveryMultisigTransaction.external.test.ts

4. Logging Consistency

Add structured logging for split recovery operations to aid in debugging:

logger.info('Split-AWM recovery initiated', { 
  userAwmUrl: userClient.baseUrl, 
  backupAwmUrl: backupClient.baseUrl,
  coin,
  phase: 'user-half-sign' 
});

Security Considerations

Proper validation of half-signed transaction formats prevents injection attacks
Key isolation ensures private keys are only retrieved when needed
Error wrapping preserves security context without leaking sensitive data
External signing validation prevents misconfiguration that could expose keys

Performance Impact

Minimal overhead for single-AWM scenarios (unchanged code path)
Optimized key retrieval (only fetch keys for current signing phase)
Efficient async job handling (smart response selection in worker)

Conclusion

This is a well-implemented, production-ready feature that significantly enhances the recovery system's flexibility. The code demonstrates excellent engineering practices with strong error handling, comprehensive testing, and thoughtful architecture. The backward compatibility and forward-compatible scaffolding show careful consideration for system evolution.

Recommendation: APPROVE

The minor suggestions above are enhancements rather than blockers. The implementation is solid and ready for production deployment.


Job Run: https://github.com/BitGo/advanced-wallets/actions/runs/
Branch: WCN-1082/split-user-backup-awm

@danielpeng1 danielpeng1 requested a review from pranavjain97 June 26, 2026 13:51
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