CSHLD-1451: Pass bridgingParams/sbtcWithdrawParams through to /tx/initiate for custody sBTC mint/burn - #9475

CSHLD-1451

@claude review this pr

Claude finished @abhi-bitgo's task —— View job

PR Review: CSHLD-1451 - Pass bridgingParams/sbtcWithdrawParams to /tx/initiate

Review Progress:

• Analyze PR context and changes
• Review BuildParams.ts changes
• Review wallet.ts implementation
• Review test coverage
• Check code quality and best practices
• Provide comprehensive feedback

Overall Assessment: ✅ APPROVED

This is a well-implemented PR that addresses a specific gap in the custody sBTC workflow. The implementation is clean, follows existing patterns, and includes comprehensive test coverage.

🔍 Detailed Review

Problem & Solution ✅

• Problem clearly identified: Custody sBTC operations were silently dropping bridgingParams/sbtcWithdrawParams because TxSendBody doesn't declare them
• Solution is appropriate: Local intersection with the missing fields for /tx/initiate only, preserving existing behavior for /tx/send
• Follows established patterns: Mirrors the existing attestation pass-through pattern

Code Quality & Architecture ✅

modules/sdk-core/src/bitgo/wallet/BuildParams.ts

• Excellent codec design: New SbtcBridgingParams and BridgingParams codecs properly mirror TypeScript interfaces
• Proper validation: SbtcBridgingParams requires all 4 fields, supports both number and string for amounts (matches interface)
• Good documentation: Clear JSDoc comments explaining purpose and relationship to interfaces

modules/sdk-core/src/bitgo/wallet/wallet.ts

• Smart scoping: whitelistedInitiateParams only affects /tx/initiate, leaving /tx/send untouched as intended
• Clean implementation: Uses t.intersection to extend TxSendBody locally without upstream changes
• Excellent documentation: Comprehensive comments explaining why this workaround is needed and when to remove it
• Proper routing: Custodial wallets → /tx/initiate, non-custodial → /tx/send (existing behavior preserved)

Test Coverage ✅

BuildParams.ts unit tests

• Comprehensive codec validation: Tests required fields, optional fields, type unions (number/string)
• Edge case coverage: Empty objects, incomplete nested shapes, wrong types
• Clear test structure: Well-organized with descriptive test names

Wallet integration tests

• End-to-end validation: Tests actual sendMany → API routing behavior
• Proper mocking: Uses nock to verify correct API calls with expected parameters
• Critical scenarios covered:
  • Custodial wallets pass bridgingParams to /tx/initiate ✅
  • Custodial wallets pass sbtcWithdrawParams to /tx/initiate ✅
  • Non-custodial wallets don't forward these params to /tx/send ✅

🔧 Technical Correctness

Type Safety ✅

• io-ts codecs properly validate runtime data
• TypeScript interfaces and codecs are kept in sync
• Union types correctly handle number | string for amounts

API Routing Logic ✅

// Custodial: sendMany → initiateTransaction → /tx/initiate (server builds/signs)
if (this._wallet.type === 'custodial') {
  return this.initiateTransaction(selectParams, reqId); // ✅ includes bridging params
}
// Non-custodial: sendMany → prebuildAndSignTransaction → sendTransaction → /tx/send
return this.sendTransaction(finalTxParams, reqId); // ✅ excludes build-time params

Backward Compatibility ✅

• No breaking changes to existing APIs
• Only additive changes to parameter handling
• Existing workflows unaffected

🧪 Testing Notes

• The PR mentions local test blockage due to missing @bitgo/sdk-coin-pearl workspace symlink
• This appears to be a local development environment issue, not a code problem
• The BuildParams tests should run independently and are well-structured

📋 Minor Suggestions

1. Consider adding JSDoc to whitelistedInitiateParams:

/** 
 * Parameters whitelisted for /tx/initiate requests. Extends whitelistedSendParams
 * with bridging-specific fields that are build-time only and shouldn't go to /tx/send.
 */
const whitelistedInitiateParams = [...whitelistedSendParams, 'bridgingParams', 'sbtcWithdrawParams'];

2. Test enhancement opportunity (non-blocking): Could add a test verifying that the parameters are properly validated by the new codecs in the integration flow.

✅ Approval Criteria Met

• Addresses the stated problem correctly
• Maintains backward compatibility
• Follows existing code patterns and conventions
• Includes comprehensive test coverage
• Uses proper TypeScript/io-ts patterns
• Clear documentation and comments
• No security concerns identified

Recommendation: LGTM - Ready to merge 🚀

Job run: View details