Skip to content

Commit 6230410

Browse files
gambinishgambinish
committed
fix: cleanup order errors
1 parent e975de7 commit 6230410

2 files changed

Lines changed: 263 additions & 0 deletions

File tree

‎app/controllers/perps/services/TradingService.test.ts‎

Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1002,6 +1002,33 @@ describe('TradingService', () => {
10021002
expect(mockDeps.metrics.trackPerpsEvent).toHaveBeenCalled();
10031003
});
10041004

1005+
it('logs error when provider returns a failure result without throwing', async () => {
1006+
const cancelParams: CancelOrderParams = {
1007+
orderId: 'order-123',
1008+
symbol: 'BTC',
1009+
};
1010+
mockProvider.cancelOrder.mockResolvedValue({
1011+
success: false,
1012+
error: 'Order already filled',
1013+
});
1014+
1015+
const result = await tradingService.cancelOrder({
1016+
provider: mockProvider,
1017+
params: cancelParams,
1018+
context: mockContext,
1019+
});
1020+
1021+
expect(result.success).toBe(false);
1022+
expect(mockDeps.logger.error).toHaveBeenCalledWith(
1023+
expect.objectContaining({ message: 'Order already filled' }),
1024+
expect.objectContaining({
1025+
controller: 'TradingService',
1026+
method: 'cancelOrder',
1027+
symbol: 'BTC',
1028+
}),
1029+
);
1030+
});
1031+
10051032
it('handles provider exception during order cancel', async () => {
10061033
const cancelParams: CancelOrderParams = {
10071034
orderId: 'order-123',
@@ -1258,6 +1285,80 @@ describe('TradingService', () => {
12581285
expect(result.results).toHaveLength(2);
12591286
expect(mockProvider.cancelOrder).toHaveBeenCalledTimes(2);
12601287
});
1288+
1289+
it('logs batch error when provider.cancelOrders returns partial/full failure', async () => {
1290+
const params: CancelOrdersParams = { cancelAll: true };
1291+
mockGetOpenOrders.mockResolvedValue(mockOrders);
1292+
mockWithStreamPause.mockImplementation(
1293+
async (callback) => await callback(),
1294+
);
1295+
(mockProvider.cancelOrders as jest.Mock).mockResolvedValue({
1296+
success: false,
1297+
successCount: 0,
1298+
failureCount: 2,
1299+
results: [
1300+
{
1301+
orderId: 'order-1',
1302+
symbol: 'BTC',
1303+
success: false,
1304+
error: 'rate limit',
1305+
},
1306+
{
1307+
orderId: 'order-2',
1308+
symbol: 'ETH',
1309+
success: false,
1310+
error: 'not found',
1311+
},
1312+
],
1313+
});
1314+
1315+
const result = await tradingService.cancelOrders({
1316+
provider: mockProvider,
1317+
params,
1318+
context: { ...mockContext, getOpenOrders: mockGetOpenOrders },
1319+
withStreamPause: mockWithStreamPause,
1320+
});
1321+
1322+
expect(result.success).toBe(false);
1323+
expect(mockDeps.logger.error).toHaveBeenCalledWith(
1324+
expect.objectContaining({
1325+
message: expect.stringContaining(
1326+
'cancelOrders batch failure: 2/2 failed',
1327+
),
1328+
}),
1329+
expect.objectContaining({
1330+
controller: 'TradingService',
1331+
method: 'cancelOrders',
1332+
}),
1333+
);
1334+
});
1335+
1336+
it('does NOT log batch error when using fallback path (provider.cancelOrders undefined)', async () => {
1337+
const params: CancelOrdersParams = { cancelAll: true };
1338+
mockGetOpenOrders.mockResolvedValue(mockOrders);
1339+
mockWithStreamPause.mockImplementation(
1340+
async (callback) => await callback(),
1341+
);
1342+
delete mockProvider.cancelOrders;
1343+
mockProvider.cancelOrder.mockResolvedValue({ success: true });
1344+
1345+
await tradingService.cancelOrders({
1346+
provider: mockProvider,
1347+
params,
1348+
context: { ...mockContext, getOpenOrders: mockGetOpenOrders },
1349+
withStreamPause: mockWithStreamPause,
1350+
});
1351+
1352+
// Batch-level log must not fire; individual leaf logs cover per-order failures
1353+
const batchErrorCalls = (
1354+
mockDeps.logger.error as jest.Mock
1355+
).mock.calls.filter(
1356+
([err]: [Error]) =>
1357+
err instanceof Error &&
1358+
err.message.includes('cancelOrders batch failure'),
1359+
);
1360+
expect(batchErrorCalls).toHaveLength(0);
1361+
});
12611362
});
12621363

12631364
describe('closePosition', () => {
@@ -1462,6 +1563,37 @@ describe('TradingService', () => {
14621563
}),
14631564
);
14641565
});
1566+
1567+
it('logs error when provider returns a failure result without throwing', async () => {
1568+
const params: ClosePositionParams = { symbol: 'BTC' };
1569+
const mockFailureResult: OrderResult = {
1570+
success: false,
1571+
error: 'Insufficient liquidity',
1572+
};
1573+
1574+
mockGetPositions.mockResolvedValue([mockPosition]);
1575+
mockProvider.closePosition.mockResolvedValue(mockFailureResult);
1576+
mockRewardsIntegrationService.calculateUserFeeDiscount.mockResolvedValue(
1577+
undefined,
1578+
);
1579+
1580+
const result = await tradingService.closePosition({
1581+
provider: mockProvider,
1582+
params,
1583+
context: { ...mockContext, getPositions: mockGetPositions },
1584+
reportOrderToDataLake: mockReportOrderToDataLake,
1585+
});
1586+
1587+
expect(result).toEqual(mockFailureResult);
1588+
expect(mockDeps.logger.error).toHaveBeenCalledWith(
1589+
expect.objectContaining({ message: 'Insufficient liquidity' }),
1590+
expect.objectContaining({
1591+
controller: 'TradingService',
1592+
method: 'closePosition',
1593+
symbol: 'BTC',
1594+
}),
1595+
);
1596+
});
14651597
});
14661598

14671599
describe('closePositions', () => {
@@ -1623,6 +1755,70 @@ describe('TradingService', () => {
16231755
expect(result.results).toHaveLength(1);
16241756
expect(mockProvider.closePosition).toHaveBeenCalledTimes(1);
16251757
});
1758+
1759+
it('logs batch error when provider.closePositions returns partial/full failure', async () => {
1760+
const params: ClosePositionsParams = { closeAll: true };
1761+
(mockProvider.closePositions as jest.Mock).mockResolvedValue({
1762+
success: false,
1763+
successCount: 0,
1764+
failureCount: 2,
1765+
results: [
1766+
{ symbol: 'BTC', success: false, error: 'insufficient liquidity' },
1767+
{ symbol: 'ETH', success: false, error: 'min size' },
1768+
],
1769+
});
1770+
mockRewardsIntegrationService.calculateUserFeeDiscount.mockResolvedValue(
1771+
undefined,
1772+
);
1773+
1774+
const result = await tradingService.closePositions({
1775+
provider: mockProvider,
1776+
params,
1777+
context: { ...mockContext, getPositions: mockGetPositions },
1778+
});
1779+
1780+
expect(result.success).toBe(false);
1781+
expect(mockDeps.logger.error).toHaveBeenCalledWith(
1782+
expect.objectContaining({
1783+
message: expect.stringContaining(
1784+
'closePositions batch failure: 2/2 failed',
1785+
),
1786+
}),
1787+
expect.objectContaining({
1788+
controller: 'TradingService',
1789+
method: 'closePositions',
1790+
}),
1791+
);
1792+
});
1793+
1794+
it('does NOT log batch error when using fallback path (provider.closePositions undefined)', async () => {
1795+
const params: ClosePositionsParams = { symbols: ['BTC'] };
1796+
mockGetPositions.mockResolvedValue(mockPositions);
1797+
delete mockProvider.closePositions;
1798+
mockProvider.closePosition.mockResolvedValue({
1799+
success: true,
1800+
orderId: 'close-1',
1801+
});
1802+
mockRewardsIntegrationService.calculateUserFeeDiscount.mockResolvedValue(
1803+
undefined,
1804+
);
1805+
1806+
await tradingService.closePositions({
1807+
provider: mockProvider,
1808+
params,
1809+
context: { ...mockContext, getPositions: mockGetPositions },
1810+
});
1811+
1812+
// Batch-level log must not fire; individual leaf logs cover per-position failures
1813+
const batchErrorCalls = (
1814+
mockDeps.logger.error as jest.Mock
1815+
).mock.calls.filter(
1816+
([err]: [Error]) =>
1817+
err instanceof Error &&
1818+
err.message.includes('closePositions batch failure'),
1819+
);
1820+
expect(batchErrorCalls).toHaveLength(0);
1821+
});
16261822
});
16271823

16281824
describe('updatePositionTPSL', () => {

‎app/controllers/perps/services/TradingService.ts‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1123,6 +1123,15 @@ export class TradingService {
11231123
},
11241124
);
11251125

1126+
this.#deps.logger.error(
1127+
ensureError(result.error, 'TradingService.cancelOrder'),
1128+
this.#getErrorContext('cancelOrder', {
1129+
symbol: params.symbol,
1130+
orderId: params.orderId,
1131+
providerError: result.error ?? 'Unknown error',
1132+
}),
1133+
);
1134+
11261135
traceData = { success: false, error: result.error ?? 'Unknown error' };
11271136
}
11281137

@@ -1298,6 +1307,31 @@ export class TradingService {
12981307
};
12991308
}, ['orders']); // Disconnect orders stream during operation
13001309

1310+
if (
1311+
provider.cancelOrders &&
1312+
operationResult &&
1313+
operationResult.failureCount > 0
1314+
) {
1315+
const failureSummary = operationResult.results
1316+
.filter((result) => !result.success)
1317+
.map(
1318+
(result) =>
1319+
`${result.symbol}/${result.orderId}: ${result.error ?? 'Unknown error'}`,
1320+
)
1321+
.join('; ');
1322+
1323+
this.#deps.logger.error(
1324+
new Error(
1325+
`cancelOrders batch failure: ${operationResult.failureCount}/${operationResult.results.length} failed - ${failureSummary}`,
1326+
),
1327+
this.#getErrorContext('cancelOrders', {
1328+
successCount: operationResult.successCount,
1329+
failureCount: operationResult.failureCount,
1330+
cancelAll: params.cancelAll,
1331+
}),
1332+
);
1333+
}
1334+
13011335
return operationResult;
13021336
} catch (error) {
13031337
operationError =
@@ -1416,6 +1450,14 @@ export class TradingService {
14161450
this.#deps.cacheInvalidator.invalidate({ cacheType: 'accountState' });
14171451
} else {
14181452
traceData = { success: false, error: result.error ?? 'Unknown error' };
1453+
1454+
this.#deps.logger.error(
1455+
ensureError(result.error, 'TradingService.closePosition'),
1456+
this.#getErrorContext('closePosition', {
1457+
symbol: params.symbol,
1458+
providerError: result.error ?? 'Unknown error',
1459+
}),
1460+
);
14191461
}
14201462

14211463
// Track analytics (success or failure, includes partial fills)
@@ -1602,6 +1644,31 @@ export class TradingService {
16021644
};
16031645
}
16041646

1647+
if (
1648+
provider.closePositions &&
1649+
operationResult &&
1650+
operationResult.failureCount > 0
1651+
) {
1652+
const failureSummary = operationResult.results
1653+
.filter((result) => !result.success)
1654+
.map(
1655+
(result) => `${result.symbol}: ${result.error ?? 'Unknown error'}`,
1656+
)
1657+
.join('; ');
1658+
1659+
this.#deps.logger.error(
1660+
new Error(
1661+
`closePositions batch failure: ${operationResult.failureCount}/${operationResult.results.length} failed - ${failureSummary}`,
1662+
),
1663+
this.#getErrorContext('closePositions', {
1664+
successCount: operationResult.successCount,
1665+
failureCount: operationResult.failureCount,
1666+
symbols: params.symbols?.length ?? 0,
1667+
closeAll: params.closeAll,
1668+
}),
1669+
);
1670+
}
1671+
16051672
return operationResult;
16061673
} catch (error) {
16071674
operationError =

0 commit comments

Comments
 (0)