[CT-874] Add realizedPnl/unrealizedPnl to consolidated subaccount websocket msg sent from fills/liquidations/deleveraging Ender handlers #1603
Conversation
|
Warning Rate limit exceeded@dydxwill has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 51 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughRecent updates in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant OrderHandler
participant MarketDatabase
participant PnLAnnotator
participant PositionUpdater
OrderHandler->>MarketDatabase: Fetch Market Data
MarketDatabase-->>OrderHandler: Market Data
OrderHandler->>PnLAnnotator: Annotate with PnL
PnLAnnotator-->>OrderHandler: Annotated Data
OrderHandler->>PositionUpdater: Update Perpetual Position
PositionUpdater-->>OrderHandler: Confirmation
sequenceDiagram
participant LiquidationHandler
participant MarketDatabase
participant PnLAnnotator
participant PositionUpdater
LiquidationHandler->>MarketDatabase: Fetch Market Data
MarketDatabase-->>LiquidationHandler: Market Data
LiquidationHandler->>PnLAnnotator: Annotate with PnL
PnLAnnotator-->>LiquidationHandler: Annotated Data
LiquidationHandler->>PositionUpdater: Update Perpetual Position
PositionUpdater-->>LiquidationHandler: Confirmation
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 details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- indexer/services/ender/tests/helpers/indexer-proto-helpers.ts (3 hunks)
- indexer/services/ender/src/handlers/order-fills/order-handler.ts (2 hunks)
Additional context used
Path-based instructions (2)
indexer/services/ender/src/handlers/order-fills/order-handler.ts (1)
Pattern
**/**: 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/ender/__tests__/helpers/indexer-proto-helpers.ts (1)
Pattern
**/**: 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.
Biome
indexer/services/ender/src/handlers/order-fills/order-handler.ts
[error] 39-39: This type annotation is trivially inferred from its initialization.
[error] 49-49: Forbidden non-null assertion.
[error] 49-49: Forbidden non-null assertion.
[error] 50-50: Forbidden non-null assertion.
[error] 50-50: Forbidden non-null assertion.
[error] 52-52: Forbidden non-null assertion.
[error] 54-54: Forbidden non-null assertion.
[error] 85-85: Forbidden non-null assertion.
[error] 85-85: Forbidden non-null assertion.
[error] 87-87: Forbidden non-null assertion.
[error] 87-87: Forbidden non-null assertion.
[error] 119-119: Forbidden non-null assertion.
[error] 135-135: Forbidden non-null assertion.
[error] 1-20: Some named imports are only used as types.
[error] 22-25: All these imports are only used as types.
[error] 26-27: All these imports are only used as types.
[error] 27-28: All these imports are only used as types.
[error] 33-34: All these imports are only used as types.
[error] 34-35: All these imports are only used as types.
indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
[error] 96-96: This type annotation is trivially inferred from its initialization.
[error] 186-186: Unexpected any. Specify a different type.
[error] 201-201: Unexpected any. Specify a different type.
[error] 251-251: Unexpected any. Specify a different type.
[error] 262-262: Unexpected any. Specify a different type.
[error] 306-306: Unexpected any. Specify a different type.
[error] 319-319: Unexpected any. Specify a different type.
[error] 424-424: Unexpected any. Specify a different type.
[error] 666-666: This type annotation is trivially inferred from its initialization.
[error] 667-667: This type annotation is trivially inferred from its initialization.
[error] 668-668: This type annotation is trivially inferred from its initialization.
[error] 686-686: Forbidden non-null assertion.
[error] 690-690: Forbidden non-null assertion.
[error] 714-714: This type annotation is trivially inferred from its initialization.
[error] 715-715: This type annotation is trivially inferred from its initialization.
[error] 716-716: This type annotation is trivially inferred from its initialization.
[error] 722-722: Forbidden non-null assertion.
[error] 727-727: Forbidden non-null assertion.
[error] 728-728: Forbidden non-null assertion.
Additional comments not posted (6)
indexer/services/ender/src/handlers/order-fills/order-handler.ts (3)
14-19: The imports forMarketFromDatabase,MarketTable,MarketsMap,MarketColumns,perpetualMarketRefresher, andUpdatedPerpetualPositionSubaccountKafkaObjectare correctly added to support the new functionality related to market and position updates.
31-31: The import ofannotateWithPnlandconvertPerpetualPositionis essential for the new functionality that calculates and annotates profit and loss on perpetual positions. Ensure that these functions are thoroughly unit tested given their critical role in financial calculations.Verification successful
The functions
annotateWithPnlandconvertPerpetualPositionare indeed referenced in test files, specifically inindexer/services/ender/__tests__/handlers/subaccount-update-handler.test.tsandindexer/services/ender/__tests__/helpers/kafka-helper.test.ts. These references indicate that the functions are being tested.
indexer/services/ender/__tests__/handlers/subaccount-update-handler.test.tsindexer/services/ender/__tests__/helpers/kafka-helper.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify unit tests for `annotateWithPnl` and `convertPerpetualPosition`. # Test: Search for unit tests related to the functions. rg --type python "test_annotateWithPnl|test_convertPerpetualPosition"Length of output: 102
Script:
#!/bin/bash # Description: Verify unit tests for `annotateWithPnl` and `convertPerpetualPosition`. # Test: Search for the function definitions in the codebase. rg --type ts "function annotateWithPnl|function convertPerpetualPosition" # Test: Search for unit tests related to the functions. rg --type ts "annotateWithPnl|convertPerpetualPosition" --files-with-matches | xargs -I {} rg --type ts "test" {}Length of output: 3571
Script:
#!/bin/bash # Description: Verify unit tests for `annotateWithPnl` and `convertPerpetualPosition`. # Test: Search for test files in the repository. test_files=$(fd --type f --extension ts --glob '*test*') # Test: Check if any of these test files contain references to `annotateWithPnl` or `convertPerpetualPosition`. for file in $test_files; do if rg -q 'annotateWithPnl|convertPerpetualPosition' "$file"; then echo "Found references in: $file" rg 'annotateWithPnl|convertPerpetualPosition' "$file" fi doneLength of output: 36234
110-110: The methodgenerateConsolidatedKafkaEventis used here to push updates. It's crucial that this method handles the data correctly and ensures data integrity, especially since it deals with financial information.indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts (3)
32-38: The addition ofMarketFromDatabase,MarketTable,MarketsMap,MarketColumns, andUpdatedPerpetualPositionSubaccountKafkaObjectin the imports section supports the new functionality for handling market and position data in unit tests. This is a good practice to ensure that the new data structures are covered by tests.
67-67: The import ofannotateWithPnlandconvertPerpetualPositionhere is crucial for testing the new PnL functionality. It's important to ensure that these helper functions are used correctly in the tests to prevent any inaccuracies in PnL calculations.Verification successful
The search results indicate that both
annotateWithPnlandconvertPerpetualPositionare indeed used in various test files, confirming their integration into the testing framework for the new PnL functionality.
indexer/services/ender/__tests__/helpers/kafka-helper.test.tsindexer/services/ender/__tests__/handlers/subaccount-update-handler.test.tsThese files contain test cases that utilize the
annotateWithPnlandconvertPerpetualPositionfunctions, ensuring that the PnL calculations are being tested.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct usage of `annotateWithPnl` and `convertPerpetualPosition` in tests. # Test: Search for their usage in test scenarios. rg --type python "test_annotateWithPnl|test_convertPerpetualPosition"Length of output: 102
Script:
#!/bin/bash # Description: Verify correct usage of `annotateWithPnl` and `convertPerpetualPosition` in tests. # Test: Search for their usage in test scenarios. rg --type ts "annotateWithPnl|convertPerpetualPosition"Length of output: 4339
812-827: This block of code is responsible for fetching market data and updating perpetual positions with PnL information in a test environment. It's critical to ensure that the test mimics the production behavior accurately, especially in handling edge cases and error scenarios.
| const markets: MarketFromDatabase[] = await MarketTable.findAll( | ||
| {}, | ||
| [], | ||
| { txId: this.txId }, | ||
| ); | ||
| const marketIdToMarket: MarketsMap = _.keyBy( | ||
| markets, | ||
| MarketColumns.id, | ||
| ); | ||
| let perpUpdate: UpdatedPerpetualPositionSubaccountKafkaObject = convertPerpetualPosition( | ||
| position, | ||
| ); | ||
| perpUpdate = annotateWithPnl( | ||
| perpUpdate, | ||
| perpetualMarketRefresher.getPerpetualMarketsMap(), | ||
| marketIdToMarket, | ||
| ); |
There was a problem hiding this comment.
Tip
Codebase Verification
The implementations of MarketTable.findAll and annotateWithPnl do not show explicit handling for edge cases such as empty market data or incorrect market IDs. This is critical as it involves financial data processing.
MarketTable.findAlldoes not include checks for empty results or invalid IDs.annotateWithPnlassumes valid data is passed without additional validation.
Please review and ensure these methods handle edge cases appropriately.
indexer/packages/postgres/src/stores/market-table.tsindexer/services/ender/src/helpers/kafka-helper.ts
Analysis chain
The changes from lines 89 to 105 introduce a new logic for fetching market data and updating perpetual positions with PnL calculations. This is a critical section as it involves financial data processing. Ensure that the logic correctly handles edge cases such as empty market data or incorrect market IDs.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify handling of edge cases in market data processing.
# Test: Check for error handling related to empty market data or incorrect market IDs.
rg --type python "if not markets|if 'id' not in market"
Length of output: 94
Script:
#!/bin/bash
# Description: Verify handling of edge cases in market data processing.
# Test: Check for error handling related to empty market data or incorrect market IDs in TypeScript files.
rg --type ts "if (!markets|if (!marketIdToMarket\[)"
Length of output: 146
Script:
#!/bin/bash
# Description: Verify handling of edge cases in market data processing.
# Test: Check for error handling related to empty market data or incorrect market IDs in TypeScript files.
rg --type ts "if \(!markets\)|if \(!marketIdToMarket\["
Length of output: 57
Script:
#!/bin/bash
# Description: Verify handling of edge cases in market data processing.
# Test: Locate the implementation of the MarketTable.findAll method.
rg "MarketTable\.findAll"
# Test: Locate the implementation of the annotateWithPnl method.
rg "annotateWithPnl"
Length of output: 7630
Script:
#!/bin/bash
# Description: Verify handling of edge cases in market data processing.
# Test: Locate the definition of the MarketTable.findAll method.
rg "findAll" --type ts
# Test: Locate the definition of the annotateWithPnl method.
rg "function annotateWithPnl" --type ts
Length of output: 42530
Script:
#!/bin/bash
# Description: Retrieve the implementation details of the MarketTable.findAll method.
# Display the content of the market-table.ts file to inspect the findAll method.
rg "export async function findAll" -A 20 indexer/packages/postgres/src/stores/market-table.ts
# Display the content of the kafka-helper.ts file to inspect the annotateWithPnl method.
rg "export function annotateWithPnl" -A 20 indexer/services/ender/src/helpers/kafka-helper.ts
Length of output: 1254
dydxwill
changed the title
dydxwill
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- indexer/services/ender/tests/helpers/indexer-proto-helpers.ts (4 hunks)
- indexer/services/ender/src/handlers/order-fills/liquidation-handler.ts (4 hunks)
Files not reviewed due to errors (1)
- indexer/services/ender/src/handlers/order-fills/liquidation-handler.ts (no review received)
Additional context used
Path-based instructions (2)
indexer/services/ender/src/handlers/order-fills/liquidation-handler.ts (1)
Pattern
**/**: 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/ender/__tests__/helpers/indexer-proto-helpers.ts (1)
Pattern
**/**: 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.
Biome
indexer/services/ender/src/handlers/order-fills/liquidation-handler.ts
[error] 43-43: This type annotation is trivially inferred from its initialization.
[error] 55-55: Forbidden non-null assertion.
[error] 55-55: Forbidden non-null assertion.
[error] 57-57: Forbidden non-null assertion.
[error] 59-59: Forbidden non-null assertion.
[error] 67-80: This else clause can be omitted because previous branches break early.
[error] 68-68: Forbidden non-null assertion.
[error] 70-70: Forbidden non-null assertion.
[error] 131-131: Forbidden non-null assertion.
[error] 137-137: Forbidden non-null assertion.
[error] 137-137: Forbidden non-null assertion.
[error] 145-145: Forbidden non-null assertion.
[error] 145-145: Forbidden non-null assertion.
[error] 155-155: Forbidden non-null assertion.
[error] 155-155: Forbidden non-null assertion.
[error] 159-172: This else clause can be omitted because previous branches break early.
[error] 162-162: Forbidden non-null assertion.
[error] 1-19: Some named imports are only used as types.
[error] 21-24: All these imports are only used as types.
[error] 25-26: All these imports are only used as types.
indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
[error] 96-96: This type annotation is trivially inferred from its initialization.
[error] 186-186: Unexpected any. Specify a different type.
[error] 201-201: Unexpected any. Specify a different type.
[error] 251-251: Unexpected any. Specify a different type.
[error] 262-262: Unexpected any. Specify a different type.
[error] 306-306: Unexpected any. Specify a different type.
[error] 319-319: Unexpected any. Specify a different type.
[error] 424-424: Unexpected any. Specify a different type.
[error] 666-666: This type annotation is trivially inferred from its initialization.
[error] 667-667: This type annotation is trivially inferred from its initialization.
[error] 668-668: This type annotation is trivially inferred from its initialization.
[error] 685-685: Forbidden non-null assertion.
[error] 703-703: Forbidden non-null assertion.
[error] 731-731: This type annotation is trivially inferred from its initialization.
[error] 732-732: This type annotation is trivially inferred from its initialization.
[error] 733-733: This type annotation is trivially inferred from its initialization.
[error] 739-739: Forbidden non-null assertion.
[error] 744-744: Forbidden non-null assertion.
[error] 745-745: Forbidden non-null assertion.
Additional comments not posted (3)
indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts (3)
32-38: The addition of new type declarations such asMarketFromDatabase,MarketTable,MarketsMap,MarketColumns, andUpdatedPerpetualPositionSubaccountKafkaObjectsupports the enhanced handling of market data and perpetual positions. These types are essential for the new functionality introduced in the PR.
67-67: The import ofannotateWithPnlandconvertPerpetualPositionfrom../../src/helpers/kafka-helperis crucial for the new functionality to annotate perpetual positions with profit and loss data. This aligns with the changes in theliquidation-handler.tsfile.
684-700: This block introduces logic similar to that inliquidation-handler.ts, where perpetual positions are converted and annotated with PnL data. The use ofMarketTable.findAlland_.keyByfor handling market data is consistent with the other file. Ensure that the logic is correctly handling edge cases and that the data integrity is maintained throughout the process.
dydxwill
changed the title
|
https://github.com/Mergifyio backport release/indexer/v5.x |
✅ Backports have been created
|
Changelist
Add realizedPnl/unrealizedPnl to consolidated subaccount websocket msg sent from fills/liquidations/deleveraging Ender handlers.
Test Plan
Unit tested.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
These changes aim to provide more accurate market data and better performance in updating perpetual positions, enhancing overall user experience in market-related functionalities.