[TRA-102] add subaccountNumber field in assetPositions return result #1187
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
WalkthroughThe recent updates introduce the 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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
indexer/services/comlink/public/swagger.jsonis excluded by:!**/*.json
Files selected for processing (9)
- indexer/services/comlink/tests/controllers/api/v4/asset-positions-controller.test.ts (4 hunks)
- indexer/services/comlink/tests/lib/helpers.test.ts (4 hunks)
- indexer/services/comlink/tests/lib/validation/schemas.test.ts (1 hunks)
- indexer/services/comlink/public/api-documentation.md (12 hunks)
- indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts (4 hunks)
- indexer/services/comlink/src/lib/constants.ts (1 hunks)
- indexer/services/comlink/src/request-helpers/request-transformer.ts (1 hunks)
- indexer/services/comlink/src/types.ts (1 hunks)
Additional comments: 13
indexer/services/comlink/src/lib/constants.ts (1)
- 16-16: The addition of
subaccountNumber: 0to theZERO_USDC_POSITIONconstant aligns with the PR's objectives. Ensure that the default value0is consistent with the system's handling of subaccount numbers.indexer/services/comlink/__tests__/lib/validation/schemas.test.ts (1)
- 13-13: The addition of
stringNumberwith typeanyis noted. Consider adding a comment or documentation explaining its use in tests for clarity.indexer/services/comlink/__tests__/controllers/api/v4/asset-positions-controller.test.ts (4)
- 49-49: The addition of
subaccountNumberto the test cases ensures thorough testing of the new functionality. Well done on covering this aspect.- 79-79: The inclusion of
subaccountNumberin test cases is appropriate for verifying the new functionality. Good job on ensuring comprehensive test coverage.- 115-115: Including
subaccountNumberin test cases is crucial for testing the new feature comprehensively. This is a good practice.- 160-160: The addition of
subaccountNumberin test cases demonstrates attention to detail in testing the new feature. This is commendable.indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts (1)
- 55-56: Renaming the controller to
AssetPositionsControllerand updating the@Getdecorator to include:subaccountNumberare appropriate changes that align with the PR's objectives.indexer/services/comlink/src/types.ts (1)
- 108-108: Adding
subaccountNumberto theAssetPositionResponseObjectinterface is a necessary update to support the new functionality. This ensures consistency in type definitions across the application.indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts (1)
- 381-381: The addition of
subaccount.subaccountNumberto theassetPositionToResponseObjectfunction call is appropriate and ensures the new functionality is correctly integrated.indexer/services/comlink/src/request-helpers/request-transformer.ts (1)
- 118-126: The addition of the
subaccountNumberparameter to theassetPositionToResponseObjectfunction is correctly implemented. It enhances the function's capability to include subaccount information in the response object, aligning with the PR's objectives. The change is straightforward, maintains code quality, and follows best practices.indexer/services/comlink/__tests__/lib/helpers.test.ts (1)
- 395-395: The addition of the
subaccountNumber: 0field to various objects within the test cases is correctly implemented. These updates ensure that the test suite accurately reflects the system's changes, maintaining the quality and relevance of the tests. The changes are straightforward, maintain code quality, and follow best practices.Also applies to: 462-462, 518-518, 575-575
indexer/services/comlink/public/api-documentation.md (2)
- 113-114: The addition of the
subaccountNumberfield to the asset positions in various API responses is consistent and correctly implemented across the document. This change enhances the API's functionality by allowing users to associate asset positions with specific subaccount numbers, improving data granularity and user experience.Also applies to: 120-121, 239-240, 246-247, 324-325
- 276-276: The inclusion of
subaccountNumberin the API endpoint examples forGetAssetPositionsis correctly done, ensuring that users are aware of how to use this new field in their requests. This change is crucial for maintaining the API's usability and ensuring that the documentation stays up-to-date with the codebase.Also applies to: 290-290, 304-304
Comment on lines
+164
to
+171
| // The schema checks allow subaccountNumber to be a string, but we know it's a number here. | ||
| const subaccountNum : number = +subaccountNumber; | ||
|
|
||
| try { | ||
| const controller: AddressesController = new AddressesController(); | ||
| const controller: AssetPositionsController = new AssetPositionsController(); | ||
| const response: AssetPositionResponse = await controller.getAssetPositions( | ||
| address, | ||
| subaccountNumber, | ||
| subaccountNum, |
There was a problem hiding this comment.
Tip
Codebase Verification
The script output indicates that subaccountNumber is directly converted to a number without explicit error handling or range checking in the provided code snippets. This could potentially lead to issues if subaccountNumber is not a valid number. Therefore, the review comment highlighting the need for proper error handling and range checking for subaccountNumber is justified based on the available evidence.
Analysis chain
The handling of subaccountNumber as a number, including its parsing and usage in getAssetPositions, is crucial for the new functionality. Ensure that there's proper error handling for parsing and that subaccountNumber falls within expected ranges or values.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that subaccountNumber parsing handles errors and checks for valid ranges
grep -r "parseNumber" src/controllers/api/v4/asset-positions-controller.ts | grep -v "//"
Length of output: 173
Script:
#!/bin/bash
# Find the correct path for asset-positions-controller.ts
fd asset-positions-controller.ts
Length of output: 112
Script:
#!/bin/bash
# Inspect handling of subaccountNumber in asset-positions-controller.ts for error handling and range checking
grep -C 3 "subaccountNumber" indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts
Length of output: 1200
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- indexer/services/comlink/tests/controllers/api/v4/addresses-controller.test.ts (3 hunks)
- indexer/services/comlink/tests/lib/helpers.test.ts (12 hunks)
- indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/comlink/tests/lib/helpers.test.ts
- indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts
Additional comments: 5
indexer/services/comlink/__tests__/controllers/api/v4/addresses-controller.test.ts (5)
- 116-116: The addition of
subaccountNumberto the asset position object in the test case is consistent with the PR objectives to include this new field in the asset positions functionality. This change ensures that the test suite accurately reflects the updated system behavior.- 123-123: Similar to the previous comment, the inclusion of
subaccountNumberhere aligns with the PR's goal to integrate this field into the asset positions. It's good to see that the test cases are being updated to cover this new field, ensuring comprehensive testing.- 167-167: Again, the addition of
subaccountNumberin this test case is appropriate and follows the PR's objectives. It's crucial for the test suite to validate the presence and correct handling of thesubaccountNumberfield in all relevant scenarios.- 276-276: The consistent update of test cases to include the
subaccountNumberfield is commendable. This ensures that the new functionality is thoroughly tested across different scenarios, which is essential for maintaining the reliability of the system.- 283-283: The inclusion of
subaccountNumberin this part of the test suite further demonstrates a thorough approach to testing the new field's integration. It's important to cover as many scenarios as possible to ensure the system behaves as expected with the addition of new fields.
indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts
Show resolved
Hide resolved
indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts
Outdated
Show resolved
Hide resolved
vincentwschau
approved these changes
Mar 18, 2024
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
indexer/services/comlink/public/swagger.jsonis excluded by:!**/*.json
Files selected for processing (2)
- indexer/services/comlink/public/api-documentation.md (9 hunks)
- indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts
Additional comments: 5
indexer/services/comlink/public/api-documentation.md (5)
- 113-114: The addition of the
subaccountNumberfield in theassetPositionsobject is clear and consistent with the PR's objectives. However, ensure that the default value of 0 forsubaccountNumberis intended and correctly implemented in the backend logic.- 120-121: Similar to the previous comment, the consistent addition of
subaccountNumberhere is noted. It's crucial to verify that all instances whereassetPositionsare returned or processed have been updated to include this new field.- 239-240: The inclusion of
subaccountNumberin theGET /addresses/{address}/subaccountNumber/{subaccountNumber}endpoint's response is appropriate. This ensures that the API's response structure is updated to reflect the new field across relevant endpoints.- 246-247: Again, the addition is consistent across different properties within the
assetPositionsobject. This consistency is key to ensuring that the API's contract is clear and predictable for consumers.- 324-325: The inclusion of
subaccountNumberin theGET /assetPositionsendpoint's response is noted. This change aligns with the PR's objectives to enhance the asset positions functionality with subaccount tracking.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelist
Add subAccountNumber as a field on AssetPositionResponseObject
Test Plan
Unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.