-
Notifications
You must be signed in to change notification settings - Fork 114
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.
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-103] Add subaccountNumber field in fills response result #1192
Conversation
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
WalkthroughThe recent update introduces a new field 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
indexer/services/comlink/public/swagger.json
is excluded by:!**/*.json
Files selected for processing (5)
- indexer/services/comlink/tests/controllers/api/v4/fills-controller.test.ts (1 hunks)
- indexer/services/comlink/public/api-documentation.md (4 hunks)
- indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (2 hunks)
- indexer/services/comlink/src/request-helpers/request-transformer.ts (2 hunks)
- indexer/services/comlink/src/types.ts (1 hunks)
Additional comments: 7
indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (3)
- 93-93: The addition of
subaccountNumber
as a parameter tofillToResponseObject
ensures that the new field is included in the response object. This change aligns with the PR's objective to addsubaccountNumber
to theFillResponseObject
.- 142-144: The conversion of
subaccountNumber
from a string to a number using the unary plus operator is a straightforward and effective way to ensure the correct data type is passed to thegetFills
method. However, it's important to ensure that the input is always a valid number to avoid unexpected results.Verification successful
The verification process confirms that
subaccountNumber
is expected to be a number at the point of receiving it as a query parameter, as indicated by@Query() subaccountNumber: number,
. This suggests that there is an inherent validation ensuringsubaccountNumber
is a valid number before its conversion with the unary plus operator. Therefore, the review comment is consistent with the codebase, emphasizing the importance of ensuring the input is always a valid number to avoid unexpected results.* 151-151: Passing the converted `subaccountNum` to the `getFills` method is correctly implemented. This ensures that the `subaccountNumber` is treated as a number throughout the method's logic, aligning with the changes made to support the new field in the response object.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that subaccountNumber is always a valid number before conversion. rg --type ts "subaccountNumber" src/controllers/api/v4/fills-controller.ts | grep -v "const subaccountNum"Length of output: 189
Script:
#!/bin/bash # First, find the correct path of fills-controller.ts fd fills-controller.ts # If found, the correct path will be used in a subsequent script to verify the handling of subaccountNumberLength of output: 92
Script:
#!/bin/bash # Verify that subaccountNumber is always a valid number before conversion using the correct file path. rg --type ts "subaccountNumber" indexer/services/comlink/src/controllers/api/v4/fills-controller.ts | grep -v "const subaccountNum"Length of output: 455
indexer/services/comlink/__tests__/controllers/api/v4/fills-controller.test.ts (1)
- 110-110: Adding
subaccountNumber
to the expected object in the test case is a necessary update to align the tests with the changes made to theFillResponseObject
. This ensures that the tests accurately reflect the new structure of the response object, including thesubaccountNumber
field.indexer/services/comlink/src/types.ts (1)
- 132-132: The addition of the
subaccountNumber
field of typenumber
to theFillResponseObject
interface is correctly implemented. This change is essential for integrating the new field into the application's data structure, ensuring that thesubaccountNumber
is included in the fill response objects as intended.indexer/services/comlink/src/request-helpers/request-transformer.ts (1)
- 134-140: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [137-153]
The addition of the
subaccountNumber
parameter to thefillToResponseObject
function and its inclusion in the returned object is correctly implemented. This change ensures that thesubaccountNumber
is properly transformed and included in response objects, aligning with the PR's objective to add this field to theFillResponseObject
.indexer/services/comlink/public/api-documentation.md (1)
- 583-584: The addition of the
subaccountNumber
field to theFillResponseObject
is correctly documented. This change aligns with the PR's objective to enhance data structure for transaction or order processing by including subaccount identification. Ensure that all relevant API responses and schemas are updated accordingly to reflect this new field.
Changelist
Add subAccountNumber as a field on FillResponseObject
Test Plan
Existing unit tests
Author/Reviewer Checklist
state-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.