Skip to content
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

[TRA-104] Add subaccountNumber to the OrderResponseObject #1296

Merged
merged 1 commit into from
Apr 1, 2024
Merged

[TRA-104] Add subaccountNumber to the OrderResponseObject #1296

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
39 changes: 23 additions & 16 deletions indexer/services/comlink/__tests__/controllers/api/v4/orders-controller.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ describe('orders-controller#V4', () => {
...testConstants.defaultOrder,
id: testConstants.defaultOrderId,
},
testConstants.defaultSubaccount.subaccountNumber,
));
});

Expand Down Expand Up @@ -117,6 +118,7 @@ describe('orders-controller#V4', () => {
...testConstants.defaultOrder,
id: testConstants.defaultOrderId,
},
testConstants.defaultSubaccount.subaccountNumber,
redisTestConstants.defaultRedisOrder,
),
);
Expand Down Expand Up @@ -391,19 +393,20 @@ describe('orders-controller#V4', () => {
postgresOrderToResponseObject({
...testConstants.defaultOrderGoodTilBlockTime,
id: getUuidForTest(testConstants.defaultOrderGoodTilBlockTime),
}),
}, testConstants.defaultSubaccount.subaccountNumber),
postgresOrderToResponseObject({
...secondOrderGoodTilBlockTime,
id: getUuidForTest(secondOrderGoodTilBlockTime),
}),
},
testConstants.defaultSubaccount.subaccountNumber),
postgresOrderToResponseObject({
...secondOrder,
id: getUuidForTest(secondOrder),
}),
}, testConstants.defaultSubaccount.subaccountNumber),
postgresOrderToResponseObject({
...testConstants.defaultOrder,
id: testConstants.defaultOrderId,
}),
}, testConstants.defaultSubaccount.subaccountNumber),
]);

const response2: request.Response = await sendRequest({
Expand All @@ -418,19 +421,19 @@ describe('orders-controller#V4', () => {
postgresOrderToResponseObject({
...testConstants.defaultOrder,
id: testConstants.defaultOrderId,
}),
}, testConstants.defaultSubaccount.subaccountNumber),
postgresOrderToResponseObject({
...secondOrder,
id: getUuidForTest(secondOrder),
}),
}, testConstants.defaultSubaccount.subaccountNumber),
postgresOrderToResponseObject({
...secondOrderGoodTilBlockTime,
id: getUuidForTest(secondOrderGoodTilBlockTime),
}),
}, testConstants.defaultSubaccount.subaccountNumber),
postgresOrderToResponseObject({
...testConstants.defaultOrderGoodTilBlockTime,
id: getUuidForTest(testConstants.defaultOrderGoodTilBlockTime),
}),
}, testConstants.defaultSubaccount.subaccountNumber),
]);
});

Expand All @@ -449,11 +452,11 @@ describe('orders-controller#V4', () => {
postgresOrderToResponseObject({
...testConstants.defaultOrder,
id: testConstants.defaultOrderId,
}),
}, testConstants.defaultSubaccount.subaccountNumber),
postgresOrderToResponseObject({
...untriggeredOrder,
id: untriggeredOrderId,
}),
}, testConstants.defaultSubaccount.subaccountNumber),
]);

const response2 = await sendRequest({
Expand All @@ -469,7 +472,7 @@ describe('orders-controller#V4', () => {
postgresOrderToResponseObject({
...untriggeredOrder,
id: untriggeredOrderId,
}),
}, testConstants.defaultSubaccount.subaccountNumber),
]);
});

Expand Down Expand Up @@ -503,6 +506,7 @@ describe('orders-controller#V4', () => {
...testConstants.defaultOrder,
id: testConstants.defaultOrderId,
},
testConstants.defaultSubaccount.subaccountNumber,
redisTestConstants.defaultRedisOrder,
),
]);
Expand All @@ -525,7 +529,7 @@ describe('orders-controller#V4', () => {
filledOrder.clobPairId,
filledOrder.orderFlags,
),
}),
}, testConstants.defaultSubaccount.subaccountNumber),
]);

response = await sendRequest({
Expand Down Expand Up @@ -554,7 +558,7 @@ describe('orders-controller#V4', () => {
postgresOrderToResponseObject({
...untriggeredOrder,
id: untriggeredOrderId,
}),
}, testConstants.defaultSubaccount.subaccountNumber),
]);

response = await sendRequest({
Expand All @@ -572,12 +576,13 @@ describe('orders-controller#V4', () => {
...testConstants.defaultOrder,
id: testConstants.defaultOrderId,
},
testConstants.defaultSubaccount.subaccountNumber,
redisTestConstants.defaultRedisOrder,
),
postgresOrderToResponseObject({
...untriggeredOrder,
id: untriggeredOrderId,
}),
}, testConstants.defaultSubaccount.subaccountNumber),
]);
});

Expand Down Expand Up @@ -614,25 +619,27 @@ describe('orders-controller#V4', () => {
postgresOrderToResponseObject({
...secondOrderGoodTilBlockTime,
id: getUuidForTest(secondOrderGoodTilBlockTime),
}),
}, testConstants.defaultSubaccount.subaccountNumber),
redisOrderToResponseObject(newerRedisOrderGoodTilBlockTime),
postgresAndRedisOrderToResponseObject(
{
...testConstants.defaultOrderGoodTilBlockTime,
id: getUuidForTest(testConstants.defaultOrderGoodTilBlockTime),
},
testConstants.defaultSubaccount.subaccountNumber,
redisTestConstants.defaultRedisOrderGoodTilBlockTime,
),
postgresOrderToResponseObject({
...secondOrder,
id: getUuidForTest(secondOrder),
}),
}, testConstants.defaultSubaccount.subaccountNumber),
redisOrderToResponseObject(redisOrderWithDifferentMarket),
postgresAndRedisOrderToResponseObject(
{
...testConstants.defaultOrder,
id: testConstants.defaultOrderId,
},
testConstants.defaultSubaccount.subaccountNumber,
redisTestConstants.defaultRedisOrder,
),
]);
Expand Down
28 changes: 22 additions & 6 deletions indexer/services/comlink/__tests__/lib/request-helpers/request-transformer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ describe('request-transformer', () => {
};
const responseObject: OrderResponseObject | undefined = postgresAndRedisOrderToResponseObject(
filledOrder,
testConstants.defaultSubaccount.subaccountNumber,
redisTestConstants.defaultRedisOrder,
);
const expectedRedisOrderTIF: TimeInForce = protocolTranslations.protocolOrderTIFToTIF(
Expand All @@ -127,13 +128,19 @@ describe('request-transformer', () => {

expect(responseObject).not.toBeUndefined();
expect(responseObject).not.toEqual(
postgresOrderToResponseObject(filledOrder),
postgresOrderToResponseObject(
filledOrder,
testConstants.defaultSubaccount.subaccountNumber,
),
);
expect(responseObject).not.toEqual(
redisOrderToResponseObject(redisTestConstants.defaultRedisOrder),
);
expect(responseObject).toEqual({
...postgresOrderToResponseObject(filledOrder),
...postgresOrderToResponseObject(
filledOrder,
testConstants.defaultSubaccount.subaccountNumber,
),
size: redisTestConstants.defaultRedisOrder.size,
price: redisTestConstants.defaultRedisOrder.price,
timeInForce: apiTranslations.orderTIFToAPITIF(expectedRedisOrderTIF),
Expand All @@ -152,27 +159,28 @@ describe('request-transformer', () => {
it('successfully converts a postgres order to a response object', () => {
const responseObject: OrderResponseObject | undefined = postgresAndRedisOrderToResponseObject(
order,
null,
testConstants.defaultSubaccount.subaccountNumber,
);

expect(responseObject).not.toBeUndefined();
expect(responseObject).not.toEqual(
redisOrderToResponseObject(redisTestConstants.defaultRedisOrder),
);
expect(responseObject).toEqual(
postgresOrderToResponseObject(order),
postgresOrderToResponseObject(order, testConstants.defaultSubaccount.subaccountNumber),
);
});

it('successfully converts a redis order to a response object', () => {
const responseObject: OrderResponseObject | undefined = postgresAndRedisOrderToResponseObject(
undefined,
testConstants.defaultSubaccount.subaccountNumber,
redisTestConstants.defaultRedisOrder,
);

expect(responseObject).not.toBeUndefined();
expect(responseObject).not.toEqual(
postgresOrderToResponseObject(order),
postgresOrderToResponseObject(order, testConstants.defaultSubaccount.subaccountNumber),
);
expect(responseObject).toEqual(
redisOrderToResponseObject(redisTestConstants.defaultRedisOrder),
Expand All @@ -182,6 +190,7 @@ describe('request-transformer', () => {
it('successfully converts undefined postgres order and null redis orderto undefined', () => {
const responseObject: OrderResponseObject | undefined = postgresAndRedisOrderToResponseObject(
undefined,
testConstants.defaultSubaccount.subaccountNumber,
null,
);

Expand All @@ -193,13 +202,17 @@ describe('request-transformer', () => {
it(
'successfully converts a postgres order with null `goodTilBlockTime` to a response object',
() => {
const responseObject: OrderResponseObject = postgresOrderToResponseObject(order);
const responseObject: OrderResponseObject = postgresOrderToResponseObject(
order,
testConstants.defaultSubaccount.subaccountNumber,
);

expect(responseObject).toEqual({
...order,
timeInForce: apiTranslations.orderTIFToAPITIF(order.timeInForce),
postOnly: apiTranslations.isOrderTIFPostOnly(order.timeInForce),
ticker,
subaccountNumber: testConstants.defaultSubaccount.subaccountNumber,
});
},
);
Expand All @@ -213,13 +226,15 @@ describe('request-transformer', () => {
};
const responseObject: OrderResponseObject = postgresOrderToResponseObject(
orderWithGoodTilBlockTime,
testConstants.defaultSubaccount.subaccountNumber,
);

expect(responseObject).toEqual({
...orderWithGoodTilBlockTime,
timeInForce: apiTranslations.orderTIFToAPITIF(order.timeInForce),
postOnly: apiTranslations.isOrderTIFPostOnly(order.timeInForce),
ticker,
subaccountNumber: testConstants.defaultSubaccount.subaccountNumber,
});
},
);
Expand Down Expand Up @@ -303,6 +318,7 @@ describe('request-transformer', () => {
goodTilBlockTime: protocolTranslations.getGoodTilBlockTime(redisOrder.order!),
ticker,
clientMetadata: redisOrder.order!.clientMetadata.toString(),
subaccountNumber: redisOrder.order!.orderId!.subaccountId!.number,
});
});
});
Expand Down
11 changes: 8 additions & 3 deletions indexer/services/comlink/public/api-documentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1480,7 +1480,8 @@ fetch('https://dydx-testnet.imperator.co/v4/orders?address=string&subaccountNumb
"postOnly": true,
"ticker": "string",
"updatedAt": "string",
"updatedAtHeight": "string"
"updatedAtHeight": "string",
"subaccountNumber": 0
}
]
```
Expand Down Expand Up @@ -1537,6 +1538,7 @@ Status Code **200**
|» ticker|string|true|none|none|
|» updatedAt|[IsoString](#schemaisostring)|false|none|none|
|» updatedAtHeight|string|false|none|none|
|» subaccountNumber|number(double)|true|none|none|

#### Enumerated Values

Expand Down Expand Up @@ -1641,7 +1643,8 @@ fetch('https://dydx-testnet.imperator.co/v4/orders/{orderId}',
"postOnly": true,
"ticker": "string",
"updatedAt": "string",
"updatedAtHeight": "string"
"updatedAtHeight": "string",
"subaccountNumber": 0
}
```

Expand Down Expand Up @@ -3777,7 +3780,8 @@ or
"postOnly": true,
"ticker": "string",
"updatedAt": "string",
"updatedAtHeight": "string"
"updatedAtHeight": "string",
"subaccountNumber": 0
}

```
Expand Down Expand Up @@ -3808,6 +3812,7 @@ or
|ticker|string|true|none|none|
|updatedAt|[IsoString](#schemaisostring)|false|none|none|
|updatedAtHeight|string|false|none|none|
|subaccountNumber|number(double)|true|none|none|

## PerpetualMarketStatus

Expand Down
7 changes: 6 additions & 1 deletion indexer/services/comlink/public/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,10 @@
},
"updatedAtHeight": {
"type": "string"
},
"subaccountNumber": {
"type": "number",
"format": "double"
}
},
"required": [
Expand All @@ -863,7 +867,8 @@
"timeInForce",
"status",
"postOnly",
"ticker"
"ticker",
"subaccountNumber"
],
"type": "object",
"additionalProperties": false
Expand Down
21 changes: 20 additions & 1 deletion indexer/services/comlink/src/controllers/api/v4/orders-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ class OrdersController extends Controller {
let mergedResponses: OrderResponseObject[] = mergePostgresAndRedisOrdersToResponseObjects(
postgresOrderMap,
redisOrderMap,
subaccountNumber,
);

if (status !== undefined) {
Expand Down Expand Up @@ -188,8 +189,23 @@ class OrdersController extends Controller {
OrdersCache.getOrder(orderId, redisClient),
]);

// Get subaccount number from either Redis or Postgres
let subaccountNumber: number | undefined;
if (redisOrder !== null) {
subaccountNumber = redisOrder.order!.orderId!.subaccountId!.number;
} else if (postgresOrder !== undefined) {
const subaccount = await SubaccountTable.findById(postgresOrder.subaccountId);
if (subaccount === undefined) {
throw new NotFoundError(`Unable to find subaccount id ${postgresOrder.subaccountId}`);
}
subaccountNumber = subaccount.subaccountNumber;
} else {
throw new NotFoundError(`Unable to find order id ${orderId}`);
}
Comment on lines +192 to +204
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The verification process indicates that the specific error handling for not finding a subaccount in the getOrder method of orders-controller.ts does not appear to be adequately covered in tests. This conclusion is based on the absence of direct references to tests that simulate the scenario where a subaccount cannot be found, leading to the throwing of the NotFoundError with the message "Unable to find subaccount id".

Analysis chain

The logic to retrieve the subaccountNumber from either Redis or Postgres in the getOrder method is correctly implemented. This approach ensures that the subaccountNumber is available for order processing, enhancing the handling of subaccount information in order-related operations. However, it's important to ensure that error handling for not finding a subaccount is adequately tested.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that error handling for not finding a subaccount is adequately covered in tests.
rg "Unable to find subaccount id" --type ts

Length of output: 209

Script:

#!/bin/bash
# Search for tests related to the getOrder method in orders-controller.ts
rg "getOrder" --type ts | rg "test"

Length of output: 19202


const order: OrderResponseObject | undefined = postgresAndRedisOrderToResponseObject(
postgresOrder,
subaccountNumber,
redisOrder,
);
if (order === undefined) {
Expand Down Expand Up @@ -275,11 +291,14 @@ router.get(
returnLatestOrders,
}: ListOrderRequest = matchedData(req) as ListOrderRequest;

// The schema checks allow subaccountNumber to be a string, but we know it's a number here.
const subaccountNum: number = +subaccountNumber;

try {
const controller: OrdersController = new OrdersController();
const response: OrderResponseObject[] = await controller.listOrders(
address,
subaccountNumber,
subaccountNum,
limit,
ticker,
side,
Expand Down
Loading
Loading