[TRA-104] Add subaccountNumber to the OrderResponseObject #1296
Conversation
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
WalkthroughThe recent updates focus on integrating subaccount number handling across various components of the system. This includes modifications in testing, API documentation, and core functionality to ensure that subaccount numbers are now properly included and processed within order-related operations. The changes span from the inclusion of the subaccount number in test scenarios and API documentation to the implementation of logic for retrieving and incorporating subaccount numbers in order processing workflows. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
indexer/services/comlink/public/swagger.jsonis excluded by!**/*.json
Files selected for processing (6)
- indexer/services/comlink/tests/controllers/api/v4/orders-controller.test.ts (11 hunks)
- indexer/services/comlink/tests/lib/request-helpers/request-transformer.test.ts (7 hunks)
- indexer/services/comlink/public/api-documentation.md (5 hunks)
- indexer/services/comlink/src/controllers/api/v4/orders-controller.ts (3 hunks)
- indexer/services/comlink/src/request-helpers/request-transformer.ts (7 hunks)
- indexer/services/comlink/src/types.ts (1 hunks)
Additional Context Used
Path-based Instructions (6)
indexer/services/comlink/src/types.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/comlink/__tests__/lib/request-helpers/request-transformer.test.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/comlink/src/controllers/api/v4/orders-controller.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/comlink/src/request-helpers/request-transformer.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/comlink/__tests__/controllers/api/v4/orders-controller.test.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/comlink/public/api-documentation.md (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (52)
indexer/services/comlink/src/types.ts (1)
297-297: The addition ofsubaccountNumberto theOrderResponseObjectinterface aligns with the PR objectives and enhances the data model by incorporating crucial subaccount number information for order processing and management. This change is consistent and follows TypeScript best practices for extending interfaces.indexer/services/comlink/__tests__/lib/request-helpers/request-transformer.test.ts (7)
128-146: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [122-143]
The changes from lines 122 to 143 correctly add the
subaccountNumberparameter to thepostgresAndRedisOrderToResponseObjectfunction call within the test. This ensures that the test scenarios accurately reflect the updated functionality and validate the integration of thesubaccountNumberfield.
162-170: The changes from lines 162 to 170 correctly add thesubaccountNumberparameter to thepostgresAndRedisOrderToResponseObjectfunction call within the test. This modification is necessary for ensuring the test accurately reflects the updated functionality and validates the integration of thesubaccountNumberfield.
177-183: The changes from lines 177 to 183 correctly add thesubaccountNumberparameter to thepostgresAndRedisOrderToResponseObjectfunction call within the test. This modification is necessary for ensuring the test accurately reflects the updated functionality and validates the integration of thesubaccountNumberfield.
190-196: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [193-205]
The changes from lines 193 to 205 correctly add the
subaccountNumberparameter to thepostgresAndRedisOrderToResponseObjectfunction call within the test. This modification is necessary for ensuring the test accurately reflects the updated functionality and validates the integration of thesubaccountNumberfield.
205-215: The changes from lines 205 to 215 correctly add thesubaccountNumberparameter to thepostgresOrderToResponseObjectfunction call within the test. This ensures that the test scenarios accurately reflect the updated functionality and validate the integration of thesubaccountNumberfield.
229-237: The changes from lines 229 to 237 correctly add thesubaccountNumberparameter to thepostgresOrderToResponseObjectfunction call within the test. This modification is necessary for ensuring the test accurately reflects the updated functionality and validates the integration of thesubaccountNumberfield.
321-321: The changes from lines 321 correctly add thesubaccountNumberparameter to theredisOrderToResponseObjectfunction call within the test. This ensures that the test scenarios accurately reflect the updated functionality and validate the integration of thesubaccountNumberfield.indexer/services/comlink/src/controllers/api/v4/orders-controller.ts (2)
158-158: The logic to includesubaccountNumberin themergePostgresAndRedisOrdersToResponseObjectsfunction call within thelistOrdersmethod is correctly implemented. This ensures that thesubaccountNumberis properly integrated into the order response objects, aligning with the PR objectives.
294-301: The conversion ofsubaccountNumberfrom a potentially string type to a number using the unary plus operator is correctly implemented in the router middleware for thelistOrdersendpoint. This ensures type consistency and aligns with the expected type for thesubaccountNumberparameter in thelistOrdersmethod.indexer/services/comlink/src/request-helpers/request-transformer.ts (1)
426-435: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [391-496]
The changes from lines 391 to 496 introduce the
subaccountNumberparameter to various functions responsible for transforming database objects into response objects. This addition is consistent with the PR's objective to include subaccount information in theOrderResponseObject. However, it's crucial to ensure that thesubaccountNumberis correctly sourced and validated before being passed to these functions to avoid potential issues with incorrect or missing subaccount data.indexer/services/comlink/__tests__/controllers/api/v4/orders-controller.test.ts (1)
619-645: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [79-642]
The changes from lines 79 to 642 in the test file correctly update the function calls to include the
subaccountNumberparameter, ensuring that the tests align with the updated function signatures. This is a necessary step to validate the integration of thesubaccountNumberinto the system and to ensure that the new functionality behaves as expected. It's important to verify that thesubaccountNumberused in the tests accurately represents realistic scenarios and that edge cases related to subaccount handling are also considered in the test suite.indexer/services/comlink/public/api-documentation.md (40)
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
The documentation mentions the base URL but does not specify the API version in the URL path. It's good practice to include the API version in the base URL to manage versioning and backward compatibility effectively.
Consider specifying the API version in the base URL, for example,
https://dydx-testnet.imperator.co/v4/api.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [137-137]
The
AddressResponseschema is correctly updated to include thesubaccountNumberfield. This addition is consistent with the PR objectives.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [188-188]
The
subaccountNumberfield is correctly added as a required parameter in theGetSubaccountendpoint. This change aligns with the PR objectives.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [262-262]
The
SubaccountResponseObjectschema correctly includes thesubaccountNumberfield, ensuring the API documentation reflects the new data model accurately.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [313-313]
The addition of the
parentSubaccountNumberas a required parameter in theGetParentSubaccountendpoint is correctly documented.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [395-395]
The
ParentSubaccountResponseschema is updated to include theparentSubaccountNumberfield, which is essential for representing parent-subaccount relationships.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [448-448]
The
subaccountNumberfield is correctly added as a required query parameter in theGetAssetPositionsendpoint, ensuring the API can filter asset positions by subaccount.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [525-525]
The
parentSubaccountNumberfield is correctly added as a required query parameter in theGetAssetPositionsForParentSubaccountendpoint, allowing for the retrieval of asset positions associated with a parent subaccount.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [472-472]
The
AssetPositionResponseschema correctly includes thesubaccountNumberfield, ensuring the API documentation reflects the new data model accurately.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [549-549]
The
AssetPositionResponseschema for theGetAssetPositionsForParentSubaccountendpoint correctly includes thesubaccountNumberfield. This consistency in schema updates across endpoints is commendable.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [648-648]
The
CandleResponseschema is not directly related to the PR's primary objective but is essential for completeness in documenting the API's capabilities. Ensure all response schemas are kept up-to-date with any changes in the data model or functionality.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [768-768]
The inclusion of the
subaccountNumberfield as a required query parameter in theGetFillsendpoint is correctly documented, aligning with the PR objectives to enhance subaccount data representation.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [813-813]
The
FillResponseschema correctly reflects the addition of thesubaccountNumberfield, ensuring the API documentation is consistent with the updated data model.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [866-866]
The
parentSubaccountNumberfield is correctly added as a required query parameter in theGetFillsForParentSubaccountendpoint, allowing for the retrieval of fills associated with a parent subaccount.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [911-911]
The
FillResponseschema for theGetFillsForParentSubaccountendpoint correctly includes thesubaccountNumberfield. This consistency in schema updates across endpoints is commendable.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [972-972]
The
HeightResponseschema is not directly related to the PR's primary objective but is essential for completeness in documenting the API's capabilities. Ensure all response schemas are kept up-to-date with any changes in the data model or functionality.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1023-1023]
The inclusion of the
subaccountNumberfield as a required query parameter in theGetHistoricalPnlendpoint is correctly documented, aligning with the PR objectives to enhance subaccount data representation.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1209-1209]
The
HistoricalPnlResponseschema correctly reflects the addition of thesubaccountNumberfield, ensuring the API documentation is consistent with the updated data model.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1262-1262]
The
periodquery parameter in theGetAggregationsendpoint is correctly documented. It's essential to specify the available options for such parameters to guide API consumers effectively.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1298-1298]
The
HistoricalTradingRewardAggregationsResponseschema is not directly related to the PR's primary objective but is essential for completeness in documenting the API's capabilities. Ensure all response schemas are kept up-to-date with any changes in the data model or functionality.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1375-1375]
The
OrderbookResponseObjectschema is not directly related to the PR's primary objective but is essential for completeness in documenting the API's capabilities. Ensure all response schemas are kept up-to-date with any changes in the data model or functionality.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1428-1428]
The inclusion of the
subaccountNumberfield as a required query parameter in theListOrdersendpoint is correctly documented, aligning with the PR objectives to enhance subaccount data representation.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1501-1501]
The
OrderResponseObjectschema correctly reflects the addition of thesubaccountNumberfield, ensuring the API documentation is consistent with the updated data model.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1617-1617]
The
orderIdpath parameter in theGetOrderendpoint is correctly documented. It's essential to specify the type and requirements for such parameters to guide API consumers effectively.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1655-1655]
The
OrderResponseObjectschema for theGetOrderendpoint correctly includes thesubaccountNumberfield. This consistency in schema updates across endpoints is commendable.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1705-1705]
The inclusion of the
limitandtickerquery parameters in theListPerpetualMarketsendpoint is correctly documented. It's essential to specify the available options and default values for such parameters to guide API consumers effectively.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1763-1763]
The
PerpetualMarketResponseschema is not directly related to the PR's primary objective but is essential for completeness in documenting the API's capabilities. Ensure all response schemas are kept up-to-date with any changes in the data model or functionality.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1816-1816]
The inclusion of the
subaccountNumberfield as a required query parameter in theListPositionsendpoint is correctly documented, aligning with the PR objectives to enhance subaccount data representation.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1863-1863]
The
PerpetualPositionResponseschema correctly reflects the addition of thesubaccountNumberfield, ensuring the API documentation is consistent with the updated data model.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1916-1916]
The
parentSubaccountNumberfield is correctly added as a required query parameter in theListPositionsForParentSubaccountendpoint, allowing for the retrieval of positions associated with a parent subaccount.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1963-1963]
The
PerpetualPositionResponseschema for theListPositionsForParentSubaccountendpoint correctly includes thesubaccountNumberfield. This consistency in schema updates across endpoints is commendable.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2015-2015]
The
timePeriodquery parameter in theGetendpoint for sparklines is correctly documented. It's essential to specify the available options for such parameters to guide API consumers effectively.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2043-2043]
The
SparklineResponseObjectschema is not directly related to the PR's primary objective but is essential for completeness in documenting the API's capabilities. Ensure all response schemas are kept up-to-date with any changes in the data model or functionality.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2104-2104]
The
TimeResponseschema is not directly related to the PR's primary objective but is essential for completeness in documenting the API's capabilities. Ensure all response schemas are kept up-to-date with any changes in the data model or functionality.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2155-2155]
The inclusion of the
tickerfield as a required path parameter in theGetTradesendpoint is correctly documented. It's essential to specify the type and requirements for such parameters to guide API consumers effectively.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2183-2183]
The
TradeResponseschema correctly reflects the addition of thesubaccountNumberfield, ensuring the API documentation is consistent with the updated data model.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2236-2236]
The inclusion of the
subaccountNumberfield as a required query parameter in theGetTransfersendpoint is correctly documented, aligning with the PR objectives to enhance subaccount data representation.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2273-2273]
The
TransferResponseschema correctly reflects the addition of thesubaccountNumberfield, ensuring the API documentation is consistent with the updated data model.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2326-2326]
The
parentSubaccountNumberfield is correctly added as a required query parameter in theGetTransfersForParentSubaccountendpoint, allowing for the retrieval of transfers associated with a parent subaccount.
1480-1487: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2363-2363]
The
ParentSubaccountTransferResponseschema for theGetTransfersForParentSubaccountendpoint correctly includes thesubaccountNumberfield. This consistency in schema updates across endpoints is commendable.
| // 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}`); | ||
| } |
There was a problem hiding this comment.
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
* Add subaccountNumber to PerpetualPositionResponseObject (#1274) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * [CT-712] send updates for both normal order matches and liquidation (#1280) * Remove volatile market (#1263) * [SKI-21] Bump slinky version to v0.3.1 (#1275) * Bump slinky version * Add cp number method * clean up socks logging (#1285) * [CT-681] fix liquidated side and offsetting side for indexer delevera… (#1284) * [CT-681] fix liquidated side and offsetting side for indexer deleveraging events * fix test * fix test * fix lint * [TRA-105] Add API for parent subaccount perpetual positions (#1282) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * Use sample rate with stream destroyed stats. (#1294) * Revert "[CT-708] Indexer track e2e latency (#1237)" (#1292) This reverts commit 60b94df. * Fix swagger generation makefile command / regen swagger docs (#1299) * pull dydx fork to generate swagger properly * remove the print * remove vault constants (#1293) * Remove custom ping message from socks (#1301) * Add subaccountNumber to the OrderResponseObject (#1296) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * sample more metrics (#1304) * [OTE-256] Add upgrade handler to initialize OI during upgrade handler (#1302) * Add upgrade handler to initialize OI during upgrade handler * nits * Fix lib.ErrorLogWithError: properly pass in args (#1306) * fix broken tests (#1312) * Explicitly close websockets on errors (#1290) * Increase the number of allowed connections to 8000 (#1317) * [TRA-104] Add parentSubaccountNumber API for orders (#1313) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * Improve Slinky logs to prevent unnecessary logs (#1289) * [SKI-26]: Prevent funding index update with no oracle prices from (#1321) halting indexer * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment * fix lint (#1323) --------- Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> Co-authored-by: shrenujb <98204323+shrenujb@users.noreply.github.com> Co-authored-by: jayy04 <103467857+jayy04@users.noreply.github.com> Co-authored-by: Eric Warehime <eric.warehime@gmail.com> Co-authored-by: vincentwschau <99756290+vincentwschau@users.noreply.github.com> Co-authored-by: Jonathan Fung <121899091+jonfung-dydx@users.noreply.github.com> Co-authored-by: Tian <tian@dydx.exchange> Co-authored-by: Teddy Ding <teddy@dydx.exchange> Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com> Co-authored-by: Christopher-Li <Christopher-Li@users.noreply.github.com>
Changelist
Add subaccountNumber field to the OrderResponseObject struct
Test Plan
Modified existing tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.