Skip to content

chore(advanced-wallets): improve error handling #67

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pranavjain97 wants to merge 1 commit into from
Closed

chore(advanced-wallets): improve error handling #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/__tests__/api/enclaved/signMpcTransaction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ describe('signMpcTransaction', () => {
.set('Authorization', `Bearer ${accessToken}`)
.send(input);

response.status.should.equal(500);
response.status.should.equal(404);
response.body.should.have.property('error');
kmsNock.done();
});
Expand Down
8 changes: 6 additions & 2 deletions src/__tests__/api/master/accelerate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,10 @@ 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');
response.body.error.should.equal('ApiResponseError');
response.body.details.should.deepEqual({ error: 'Wallet not found' });

walletGetNock.done();
});
Expand Down Expand Up @@ -190,8 +192,10 @@ 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');
response.body.error.should.equal('ApiResponseError');
response.body.details.should.deepEqual({ error: 'Keychain not found' });

walletGetNock.done();
keychainGetNock.done();
Expand Down
20 changes: 15 additions & 5 deletions src/__tests__/api/master/consolidate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,21 @@ describe('POST /api/:coin/wallet/:walletId/consolidate', () => {
consolidateAddresses: ['0x1234567890abcdef', '0xfedcba0987654321'],
});

response.status.should.equal(500);
response.body.should.have.property('error', 'Internal Server Error');
response.body.should.have
.property('details')
.which.match(/Consolidations failed: 1 and succeeded: 1/);
response.status.should.equal(202);
response.body.should.deepEqual({
success: [
{
txid: 'consolidation-tx-1',
status: 'signed',
},
],
failure: [
{
error: 'Insufficient funds',
address: '0xfedcba0987654321',
},
],
});

walletGetNock.done();
keychainGetNock.done();
Expand Down
4 changes: 0 additions & 4 deletions src/__tests__/api/master/consolidateUnspents.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@ describe('POST /api/:coin/wallet/:walletId/consolidateunspents', () => {

const mockError = {
error: 'Internal Server Error',
name: 'ApiResponseError',
details:
'There are too few unspents that meet the given parameters to consolidate (1 available).',
};
Expand All @@ -147,9 +146,6 @@ describe('POST /api/:coin/wallet/:walletId/consolidateunspents', () => {
});

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();
Expand Down
33 changes: 17 additions & 16 deletions src/api/master/clients/enclavedExpressClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
MpcV2RoundResponseType,
} from '../../../enclavedBitgoExpress/routers/enclavedApiSpec';
import { FormattedOfflineVaultTxInfo } from '@bitgo/abstract-utxo';
import { EnclavedError } from '../../../errors';

const debugLogger = debug('bitgo:express:enclavedExpressClient');

Expand Down Expand Up @@ -242,7 +243,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to create independent keychain: %s', err.message);
throw err;
throw new EnclavedError(`Failed to create independent keychain: ${err.message}`, 500);
}
}

Expand Down Expand Up @@ -272,7 +273,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to sign multisig: %s', err.message);
throw err;
throw new EnclavedError(`Failed to sign multisig transaction: ${err.message}`, 500);
}
}

Expand All @@ -296,7 +297,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Enclaved express service ping failed: %s', err.message);
throw err;
throw new EnclavedError(`Failed to ping enclaved express service: ${err.message}`, 500);
}
}

Expand All @@ -319,7 +320,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to get version information: %s', err.message);
throw err;
throw new EnclavedError(`Failed to get version information: ${err.message}`, 500);
}
}

Expand All @@ -344,7 +345,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to recover multisig: %s', err.message);
throw err;
throw new EnclavedError(`Failed to recover multisig transaction: ${err.message}`, 500);
}
}

Expand Down Expand Up @@ -376,7 +377,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to initialize MPC key generation: %s', err.message);
throw err;
throw new EnclavedError(`Failed to initialize MPC key generation: ${err.message}`, 500);
}
}

Expand Down Expand Up @@ -422,7 +423,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to finalize MPC key generation: %s', err.message);
throw err;
throw new EnclavedError(`Failed to finalize MPC key generation: ${err.message}`, 500);
}
}

Expand All @@ -446,7 +447,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to sign mpc commitment: %s', err.message);
throw err;
throw new EnclavedError(`Failed to sign MPC commitment: ${err.message}`, 500);
}
}

Expand All @@ -470,7 +471,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to sign mpc r-share: %s', err.message);
throw err;
throw new EnclavedError(`Failed to sign MPC R-share: ${err.message}`, 500);
}
}

Expand All @@ -494,7 +495,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to sign mpc g-share: %s', err.message);
throw err;
throw new EnclavedError(`Failed to sign MPC G-share: ${err.message}`, 500);
}
}

Expand Down Expand Up @@ -524,7 +525,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to initialize MPCv2 key generation: %s', err.message);
throw err;
throw new EnclavedError(`Failed to initialize MPCv2 key generation: ${err.message}`, 500);
}
}

Expand Down Expand Up @@ -561,7 +562,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to execute MPCv2 round: %s', err.message);
throw err;
throw new EnclavedError(`Failed to execute MPCv2 round: ${err.message}`, 500);
}
}

Expand Down Expand Up @@ -595,7 +596,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to finalize MPCv2 key generation: %s', err.message);
throw err;
throw new EnclavedError(`Failed to finalize MPCv2 key generation: ${err.message}`, 500);
}
}

Expand Down Expand Up @@ -628,7 +629,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to sign mpcv2 round 1: %s', err.message);
throw err;
throw new EnclavedError(`Failed to sign MPCv2 Round 1: ${err.message}`, 500);
}
}

Expand Down Expand Up @@ -661,7 +662,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to sign mpcv2 round 2: %s', err.message);
throw err;
throw new EnclavedError(`Failed to sign MPCv2 Round 2: ${err.message}`, 500);
}
}

Expand Down Expand Up @@ -694,7 +695,7 @@ export class EnclavedExpressClient {
} catch (error) {
const err = error as Error;
debugLogger('Failed to sign mpcv2 round 3: %s', err.message);
throw err;
throw new EnclavedError(`Failed to sign MPCv2 Round 3: ${err.message}`, 500);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/api/master/handlers/handleConsolidate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export async function handleConsolidate(
msg = `Consolidations failed: ${result.failure.length} and succeeded: ${result.success.length}`;
} else {
// All failed

Copilot AI Jul 15, 2025

Copy link

Choose a reason for hiding this comment

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

This comment implies a 400 status for all failures, but the code now sets status = 500. Update the comment to reflect that a 500 is returned on complete failure.

Suggested change
// All failed
// All consolidations failed, returning status 500

Copilot uses AI. Check for mistakes.
status = 400;
status = 500;
msg = 'All consolidations failed';
}

Expand Down
8 changes: 8 additions & 0 deletions src/api/master/routers/enclavedExpressHealth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ const PingEnclavedResponse: HttpResponse = {
status: t.string,
enclavedResponse: PingResponseType,
}),
404: t.type({
error: t.string,
details: t.string,
}),
500: t.type({
error: t.string,
details: t.string,
Expand All @@ -22,6 +26,10 @@ const PingEnclavedResponse: HttpResponse = {

const VersionEnclavedResponse: HttpResponse = {
200: VersionResponseType,
404: t.type({
error: t.string,
details: t.string,
}),
500: t.type({
error: t.string,
details: t.string,
Expand Down
9 changes: 9 additions & 0 deletions src/api/master/routers/healthCheck.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,23 @@ import { Response } from '@api-ts/response';
import pjson from '../../../../package.json';
import { responseHandler } from '../../../shared/middleware';
import { PingResponseType, VersionResponseType } from '../../../types/health';
import * as t from 'io-ts';

// API Response types
const PingResponse: HttpResponse = {
200: PingResponseType,
404: t.type({
error: t.string,
details: t.string,
}),
};

const VersionResponse: HttpResponse = {
200: VersionResponseType,
404: t.type({
error: t.string,
details: t.string,
}),
};

// API Specification
Expand Down
41 changes: 40 additions & 1 deletion src/api/master/routers/masterApiSpec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ export function parseBody(req: express.Request, res: express.Response, next: exp
const GenerateWalletResponse: HttpResponse = {
// TODO: Get type from public types repo
200: t.any,
400: t.type({
error: t.string,
details: t.string,
}),
404: t.type({
error: t.string,
details: t.string,
}),
500: t.type({
error: t.string,
details: t.string,
Expand All @@ -57,6 +65,7 @@ export const SendManyRequest = {
t.undefined,
t.literal('transfer'),
t.literal('acceleration'),
t.literal('fillNonce'),
t.literal('accountSet'),
t.literal('enabletoken'),
t.literal('stakingLock'),
Expand All @@ -66,7 +75,7 @@ export const SendManyRequest = {
]),
commonKeychain: t.union([t.undefined, t.string]),
source: t.union([t.literal('user'), t.literal('backup')]),
recipients: t.array(t.any),
recipients: t.union([t.undefined, t.array(t.any)]),
numBlocks: t.union([t.undefined, t.number]),
feeRate: t.union([t.undefined, t.number]),
feeMultiplier: t.union([t.undefined, t.number]),
Expand Down Expand Up @@ -98,6 +107,11 @@ export const SendManyRequest = {
export const SendManyResponse: HttpResponse = {
// TODO: Get type from public types repo / Wallet Platform
200: t.any,
400: t.any,
404: t.type({
error: t.string,
details: t.string,
}),
500: t.type({
error: t.string,
details: t.string,
Expand All @@ -115,7 +129,12 @@ export const ConsolidateRequest = {
// Response type for /consolidate endpoint
const ConsolidateResponse: HttpResponse = {
200: t.any,
202: t.any,
400: t.any, // All failed
404: t.type({
error: t.string,
details: t.string,
}),
500: t.type({
error: t.string,
details: t.string,
Expand All @@ -140,6 +159,14 @@ const AccelerateResponse: HttpResponse = {
txid: t.string,
tx: t.string,
}),
400: t.type({
error: t.string,
details: t.string,
}),
404: t.type({
error: t.string,
details: t.any,
}),
500: t.type({
error: t.string,
details: t.string,
Expand All @@ -152,6 +179,14 @@ const RecoveryWalletResponse: HttpResponse = {
200: t.type({
txHex: t.string, // the full signed transaction hex
}),
400: t.type({
error: t.string,
details: t.string,
}),
404: t.type({
error: t.string,
details: t.string,
}),
500: t.type({
error: t.string,
details: t.string,
Expand Down Expand Up @@ -212,6 +247,10 @@ const ConsolidateUnspentsResponse: HttpResponse = {
txid: t.string,
}),
400: t.any,
404: t.type({
error: t.string,
details: t.string,
}),
500: t.type({
error: t.string,
details: t.string,
Expand Down
Loading