Fix bug with PnL aggregation. #2446
Conversation
WalkthroughThe pull request introduces significant modifications to the handling of Profit and Loss (PnL) ticks across multiple files. Key changes include the renaming of the Changes
Possibly related PRs
Suggested labels
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 4
🧹 Outside diff range and nitpick comments (1)
indexer/services/comlink/__tests__/lib/helpers.test.ts (1)
883-895: Clarify time calculations for better readabilityIn the test case,
blockTime3is calculated by adding 61 minutes topnlTick.createdAt:const blockTime3: string = DateTime.fromISO(pnlTick.createdAt).plus({ minute: 61 }).toISO();Adding 61 minutes achieves the desired time offset, but it might be clearer to add
{ hour: 1, minute: 1 }to represent 1 hour and 1 minute explicitly. This enhances readability and reduces the potential for confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- indexer/services/comlink/tests/lib/helpers.test.ts (5 hunks)
- indexer/services/comlink/src/controllers/api/v4/historical-pnl-controller.ts (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3 hunks)
- indexer/services/comlink/src/lib/helpers.ts (2 hunks)
🔇 Additional comments (9)
indexer/services/comlink/src/controllers/api/v4/historical-pnl-controller.ts (5)
22-22: LGTM: Import statement updated correctly.The import statement has been updated to include
aggregateHourlyPnlTicksinstead ofaggregatePnlTicks, which aligns with the PR objective of modifying the PnL aggregation method.
159-159: LGTM: Aggregation function updated to group by hour.The change from
aggregatePnlTickstoaggregateHourlyPnlTicksaligns with the PR objective of grouping PnL ticks by hour instead of by block. This should address the issue of inaccurate PnL aggregation due to ticks being generated at different block heights within the same hour.
159-159: Verify performance impact of new data structure.The change from
Map<number, PnlTicksFromDatabase>toPnlTicksFromDatabase[]represents a shift in the data structure. While this change is necessary for the new aggregation method, it's important to verify that it doesn't negatively impact performance or memory usage, especially for large datasets.Could you provide some benchmarks or performance tests comparing the old and new implementations, particularly for scenarios with a large number of PnL ticks?
Line range hint
162-166: LGTM: historicalPnl construction updated correctly.The historicalPnl construction has been appropriately updated to directly map over the aggregatedPnlTicks array. This change is consistent with the new data structure and should maintain the correct functionality.
Line range hint
1-266: Summary: Changes address the PnL aggregation issue, but require further verification.The modifications to the
getHistoricalPnlForParentSubaccountmethod successfully implement the new hourly PnL aggregation, addressing the issue of inaccurate aggregation due to block-based grouping. The changes are well-implemented and consistent with the PR objectives.However, to ensure the robustness and efficiency of these changes, please address the following points:
Provide benchmarks or performance tests comparing the old and new implementations, especially for large datasets, to verify that the change in data structure (from Map to Array) doesn't negatively impact performance or memory usage.
Implement comprehensive unit tests to ensure the new aggregation method works as expected across various scenarios, including edge cases.
Update the relevant documentation to reflect the changes in PnL aggregation methodology.
Could you please provide information on the test plan implementation and any performance comparisons you've conducted?
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1)
96-96: Updated to use hourly PnL aggregationGreat job updating the code to use
aggregateHourlyPnlTicksfor hourly aggregation of PnL ticks. This aligns well with the PR objective to aggregate PnL ticks by hour instead of by block.indexer/services/comlink/src/lib/helpers.ts (1)
687-687: Ensure consistent time zone handling when truncating timestamps.Using
DateTime.fromISO(pnlTick.createdAt).startOf('hour').toISO()relies on the default time zone, which may lead to inconsistent aggregation ifcreatedAttimestamps are not in UTC. Explicitly specifying the time zone can prevent potential issues.Run the following script to check if all
createdAttimestamps are in UTC:✅ Verification successful
Consistent UTC time zone handling verified for
createdAttimestamps.All
createdAtfields are in UTC, ensuring consistent aggregation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all `createdAt` timestamps in PnL ticks are in UTC. # Test: Search for `createdAt` values with time zone offsets. Expect: No matches if all are in UTC. rg --type js --no-heading --no-filename 'createdAt:\s*"[^"]+"' | grep -E '\+\d{2}:\d{2}|\-\d{2}:\d{2}'Length of output: 104
Script:
#!/bin/bash # Description: Verify that all `createdAt` timestamps in PnL ticks are in UTC by checking for 'Z' suffix. # Test: Search for `createdAt` values not ending with 'Z'. Expect: No matches if all are in UTC. rg --type js 'createdAt:\s*"[^"]+[^Z]"'Length of output: 41
Script:
#!/bin/bash # Description: Check for `createdAt` fields without any time zone information. # Test: Search for `createdAt` values without time zone offset or 'Z'. Expect: Minimal or no matches. rg --type js 'createdAt:\s*"[^"]+(?<!Z)"'Length of output: 293
indexer/services/comlink/__tests__/lib/helpers.test.ts (2)
47-47: Import of 'aggregateHourlyPnlTicks' is appropriateThe addition of
aggregateHourlyPnlTicksto the import statement is necessary for the new test cases.
63-63: Importing 'DateTime' from 'luxon' is necessaryThe import of
DateTimefrom theluxonlibrary correctly facilitates date and time manipulations in the test cases.
|
@Mergifiyio backport release/indexer/v7.x |
(cherry picked from commit 8fdf8c5)
Co-authored-by: vincentwschau <[email protected]>
Changelist
PnL aggregation was by block, however PnL ticks can be generated for the same hour at separate block heights (seconds to minutes apart).
Instead, aggregate PnL ticks by hour.
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.Summary by CodeRabbit
New Features
Bug Fixes
Documentation