Merge pull request #75 from BitGo/WP-5260-fix-api-error-handling

pranavjain97 · web-flow · commit 63acfb2dd387 · 2025-07-18T17:14:10.000-04:00
chore(mbe): handle bitgo api error responses
diff --git a/src/__tests__/api/master/accelerate.test.ts b/src/__tests__/api/master/accelerate.test.ts
@@ -157,7 +157,7 @@ describe('POST /api/:coin/wallet/:walletId/accelerate', () => {
         cpfpTxIds: ['b8a828b98dbf32d9fd1875cbace9640ceb8c82626716b4a64203fdc79bb46d26'],
       });
 
-    response.status.should.equal(500);
+    response.status.should.equal(404);
     response.body.should.have.property('error');
 
     walletGetNock.done();
@@ -190,7 +190,7 @@ describe('POST /api/:coin/wallet/:walletId/accelerate', () => {
         cpfpTxIds: ['b8a828b98dbf32d9fd1875cbace9640ceb8c82626716b4a64203fdc79bb46d26'],
       });
 
-    response.status.should.equal(500);
+    response.status.should.equal(404);
     response.body.should.have.property('error');
 
     walletGetNock.done();
diff --git a/src/__tests__/api/master/consolidateUnspents.test.ts b/src/__tests__/api/master/consolidateUnspents.test.ts
@@ -107,55 +107,6 @@ describe('POST /api/:coin/wallet/:walletId/consolidateunspents', () => {
     sinon.assert.calledOnce(consolidateUnspentsStub);
   });
 
-  it('should return error, name, and details on failure', async () => {
-    const walletGetNock = nock(bitgoApiUrl)
-      .get(`/api/v2/${coin}/wallet/${walletId}`)
-      .matchHeader('any', () => true)
-      .reply(200, {
-        id: walletId,
-        type: 'cold',
-        subType: 'onPrem',
-        keys: ['user-key-id', 'backup-key-id', 'bitgo-key-id'],
-      });
-
-    const keychainGetNock = nock(bitgoApiUrl)
-      .get(`/api/v2/${coin}/key/user-key-id`)
-      .matchHeader('any', () => true)
-      .reply(200, {
-        id: 'user-key-id',
-        pub: 'xpub_user',
-      });
-
-    const mockError = {
-      error: 'Internal Server Error',
-      name: 'ApiResponseError',
-      details:
-        'There are too few unspents that meet the given parameters to consolidate (1 available).',
-    };
-
-    const consolidateUnspentsStub = sinon
-      .stub(Wallet.prototype, 'consolidateUnspents')
-      .throws(Object.assign(new Error(mockError.details), mockError));
-
-    const response = await agent
-      .post(`/api/${coin}/wallet/${walletId}/consolidateunspents`)
-      .set('Authorization', `Bearer ${accessToken}`)
-      .send({
-        source: 'user',
-        pubkey: 'xpub_user',
-        feeRate: 1000,
-      });
-
-    response.status.should.equal(500);
-    response.body.should.have.property('error', mockError.error);
-    response.body.should.have.property('name', mockError.name);
-    response.body.should.have.property('details', mockError.details);
-
-    walletGetNock.done();
-    keychainGetNock.done();
-    sinon.assert.calledOnce(consolidateUnspentsStub);
-  });
-
   it('should throw error when provided pubkey does not match wallet keychain', async () => {
     const walletGetNock = nock(bitgoApiUrl)
       .get(`/api/v2/${coin}/wallet/${walletId}`)
diff --git a/src/__tests__/api/master/sendMany.test.ts b/src/__tests__/api/master/sendMany.test.ts
@@ -5,7 +5,7 @@ import * as request from 'supertest';
 import nock from 'nock';
 import { app as expressApp } from '../../../masterExpressApp';
 import { AppMode, MasterExpressConfig, TlsMode } from '../../../shared/types';
-import { Environments, Wallet } from '@bitgo/sdk-core';
+import { ApiResponseError, Environments, Wallet } from '@bitgo/sdk-core';
 import { Coin } from 'bitgo';
 import assert from 'assert';
 
@@ -522,7 +522,7 @@ describe('POST /api/:coin/wallet/:walletId/sendmany', () => {
           pubkey: 'xpub_backup',
         });
 
-      response.status.should.equal(500);
+      response.status.should.equal(400);
       response.body.details.should.equal('Backup MPC signing not supported for sendMany');
 
       walletGetNock.done();
@@ -567,8 +567,7 @@ describe('POST /api/:coin/wallet/:walletId/sendmany', () => {
         pubkey: 'wrong_pubkey',
       });
 
-    // TODO: Fix this to expect the error message when the middleware on MBE is fixed to handle errors
-    response.status.should.equal(500);
+    response.status.should.equal(400);
 
     walletGetNock.done();
     keychainGetNock.done();
@@ -648,7 +647,7 @@ describe('POST /api/:coin/wallet/:walletId/sendmany', () => {
         pubkey: 'xpub_user',
       });
 
-    response.status.should.equal(500);
+    response.status.should.equal(400);
 
     walletGetNock.done();
     keychainGetNock.done();
@@ -706,11 +705,85 @@ describe('POST /api/:coin/wallet/:walletId/sendmany', () => {
         pubkey: 'xpub_user',
       });
 
+    response.status.should.equal(400);
+
+    walletGetNock.done();
+    keychainGetNock.done();
+    sinon.assert.calledOnce(prebuildStub);
+    sinon.assert.calledOnce(verifyStub);
+  });
+
+  it('should handle BitGoApiResponseError correctly', async () => {
+    // Mock wallet get request
+    const walletGetNock = nock(bitgoApiUrl)
+      .get(`/api/v2/${coin}/wallet/${walletId}`)
+      .matchHeader('any', () => true)
+      .reply(200, {
+        id: walletId,
+        type: 'cold',
+        subType: 'onPrem',
+        keys: ['user-key-id', 'backup-key-id', 'bitgo-key-id'],
+      });
+
+    // Mock keychain get request
+    const keychainGetNock = nock(bitgoApiUrl)
+      .get(`/api/v2/${coin}/key/user-key-id`)
+      .matchHeader('any', () => true)
+      .reply(200, {
+        id: 'user-key-id',
+        pub: 'xpub_user',
+      });
+
+    const prebuildStub = sinon.stub(Wallet.prototype, 'prebuildTransaction').resolves({
+      txHex: 'prebuilt-tx-hex',
+      txInfo: {
+        nP2SHInputs: 1,
+        nSegwitInputs: 0,
+        nOutputs: 2,
+      },
+      walletId,
+    });
+
+    const verifyStub = sinon.stub(Coin.Btc.prototype, 'verifyTransaction').resolves(true);
+
+    // Mock enclaved express sign request to return an error
+    const signNock = nock(enclavedExpressUrl)
+      .post(`/api/${coin}/multisig/sign`)
+      .replyWithError(
+        new ApiResponseError('Custom API error', 500, {
+          error: 'Custom API error',
+          requestId: 'test-request-id',
+        }),
+      );
+
+    const response = await agent
+      .post(`/api/${coin}/wallet/${walletId}/sendMany`)
+      .set('Authorization', `Bearer ${accessToken}`)
+      .send({
+        recipients: [
+          {
+            address: 'tb1qtest1',
+            amount: '100000',
+          },
+        ],
+        source: 'user',
+        pubkey: 'xpub_user',
+      });
+
+    // The response should be a 500 error with the enclaved error details
     response.status.should.equal(500);
+    response.body.should.have.property('error');
+    response.body.should.have.property('details');
+    response.body.error.should.equal('BitGoApiResponseError');
+    response.body.details.should.deepEqual({
+      error: 'Custom API error',
+      requestId: 'test-request-id',
+    });
 
     walletGetNock.done();
     keychainGetNock.done();
     sinon.assert.calledOnce(prebuildStub);
     sinon.assert.calledOnce(verifyStub);
+    signNock.done();
   });
 });
diff --git a/src/api/master/handlers/handleSendMany.ts b/src/api/master/handlers/handleSendMany.ts
@@ -13,6 +13,7 @@ import { MasterApiSpecRouteRequest } from '../routers/masterApiSpec';
 import { createEcdsaMPCv2CustomSigners } from './ecdsaMPCv2';
 import { EnclavedExpressClient } from '../clients/enclavedExpressClient';
 import { createEddsaCustomSigningFunctions } from './eddsa';
+import { BadRequestError, NotFoundError } from '../../../shared/errors';
 
 /**
  * Defines the structure for a single recipient in a send-many transaction.
@@ -41,7 +42,7 @@ function createMPCSendParamsWithCustomSigningFns(
   const mpcAlgorithm = coin.getMPCAlgorithm();
 
   if (!commonKeychain) {
-    throw new Error('Common keychain is required for MPC signing');
+    throw new BadRequestError('Common keychain is required for MPC signing');
   }
 
   if (mpcAlgorithm === 'ecdsa') {
@@ -66,7 +67,7 @@ function createMPCSendParamsWithCustomSigningFns(
     };
   }
 
-  throw new Error(`Unsupported MPC algorithm: ${mpcAlgorithm}`);
+  throw new BadRequestError(`Unsupported MPC algorithm: ${mpcAlgorithm}`);
 }
 
 export async function handleSendMany(req: MasterApiSpecRouteRequest<'v1.wallet.sendMany', 'post'>) {
@@ -81,11 +82,11 @@ export async function handleSendMany(req: MasterApiSpecRouteRequest<'v1.wallet.s
   const walletId = req.params.walletId;
   const wallet = await baseCoin.wallets().get({ id: walletId, reqId });
   if (!wallet) {
-    throw new Error(`Wallet ${walletId} not found`);
+    throw new NotFoundError(`Wallet ${walletId} not found`);
   }
 
   if (wallet.type() !== 'cold' || wallet.subType() !== 'onPrem') {
-    throw new Error('Wallet is not an on-prem wallet');
+    throw new NotFoundError('Wallet is not an on-prem wallet');
   }
 
   const keyIdIndex = params.source === 'user' ? KeyIndices.USER : KeyIndices.BACKUP;
@@ -98,13 +99,15 @@ export async function handleSendMany(req: MasterApiSpecRouteRequest<'v1.wallet.s
   });
 
   if (!signingKeychain) {
-    throw new Error(`Signing keychain for ${params.source} not found`);
+    throw new NotFoundError(`Signing keychain for ${params.source} not found`);
   }
   if (params.pubkey && signingKeychain.pub !== params.pubkey) {
-    throw new Error(`Pub provided does not match the keychain on wallet for ${params.source}`);
+    throw new BadRequestError(
+      `Pub provided does not match the keychain on wallet for ${params.source}`,
+    );
   }
   if (params.commonKeychain && signingKeychain.commonKeychain !== params.commonKeychain) {
-    throw new Error(
+    throw new BadRequestError(
       `Common keychain provided does not match the keychain on wallet for ${params.source}`,
     );
   }
@@ -113,7 +116,7 @@ export async function handleSendMany(req: MasterApiSpecRouteRequest<'v1.wallet.s
     // Create MPC send parameters with custom signing functions
     if (wallet.multisigType() === 'tss') {
       if (signingKeychain.source === 'backup') {
-        throw new Error('Backup MPC signing not supported for sendMany');
+        throw new BadRequestError('Backup MPC signing not supported for sendMany');
       }
       const mpcSendParams = createMPCSendParamsWithCustomSigningFns(
         req,
@@ -150,14 +153,14 @@ export async function handleSendMany(req: MasterApiSpecRouteRequest<'v1.wallet.s
         walletType: wallet.multisigType(),
       });
       if (!verified) {
-        throw new Error('Transaction prebuild failed local validation');
+        throw new BadRequestError('Transaction prebuild failed local validation');
       }
       logger.debug('Transaction prebuild verified');
     } catch (e) {
       const err = e as Error;
       logger.error('transaction prebuild failed local validation:', err.message);
       logger.error('transaction prebuild:', JSON.stringify(txPrebuilt, null, 2));
-      throw new Error(`Transaction prebuild failed local validation: ${err.message}`);
+      throw new BadRequestError(`Transaction prebuild failed local validation: ${err.message}`);
     }
 
     logger.debug('Tx prebuild: %s', JSON.stringify(txPrebuilt, null, 2));
@@ -188,7 +191,7 @@ export async function signAndSendMultisig(
   reqId: RequestTracer,
 ) {
   if (!signingKeychain.pub) {
-    throw new Error(`Signing keychain pub not found for ${source}`);
+    throw new BadRequestError(`Signing keychain pub not found for ${source}`);
   }
   logger.info(`Signing with ${source} keychain, pub: ${signingKeychain.pub}`);
   logger.debug(`Signing keychain: ${JSON.stringify(signingKeychain, null, 2)}`);
diff --git a/src/api/master/routers/masterApiSpec.ts b/src/api/master/routers/masterApiSpec.ts
@@ -31,6 +31,7 @@ import {
   BadRequestResponse,
   InternalServerErrorResponse,
   UnprocessableEntityResponse,
+  NotFoundResponse,
 } from '../../../shared/errors';
 
 export type ScriptType2Of3 = utxolib.bitgo.outputScripts.ScriptType2Of3;
@@ -160,6 +161,8 @@ export const SendManyRequest = {
 export const SendManyResponse: HttpResponse = {
   // TODO: Get type from public types repo / Wallet Platform
   200: t.any,
+  ...BadRequestResponse,
+  ...NotFoundResponse,
   ...InternalServerErrorResponse,
 };
 
diff --git a/src/shared/errors.ts b/src/shared/errors.ts
@@ -84,3 +84,4 @@ const ErrorResponse = t.type({
 export const BadRequestResponse = { 400: ErrorResponse };
 export const UnprocessableEntityResponse = { 422: ErrorResponse };
 export const InternalServerErrorResponse = { 500: ErrorResponse };
+export const NotFoundResponse = { 404: ErrorResponse };
diff --git a/src/shared/responseHandler.ts b/src/shared/responseHandler.ts
@@ -1,7 +1,7 @@
 import { Request, Response as ExpressResponse, NextFunction } from 'express';
 import { Config } from '../shared/types';
 import { BitGoRequest } from '../types/request';
-import { EnclavedError } from '../errors';
+import { ApiResponseError, EnclavedError } from '../errors';
 import {
   BitgoExpressError,
   ValidationError,
@@ -47,6 +47,15 @@ export function responseHandler<T extends Config = Config>(fn: ServiceFunction<T
         return res.sendEncoded(apiError.type, apiError.payload);
       }
 
+      if ((error as any).name === 'ApiResponseError') {
+        const apiError = error as ApiResponseError;
+        const body = {
+          error: 'BitGoApiResponseError',
+          details: apiError.result,
+        };
+        return res.status(apiError.status).json(body);
+      }
+
       // If it's a BitgoExpressError, map to appropriate status code
       if (error instanceof BitgoExpressError) {
         let statusCode = 500;
