Allow configuring day vault PnL starts. #2570
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request enhance the functionality and testing of PnL tick retrieval in the indexer module. A new method for retrieving the latest PnL ticks for subaccounts at specified dates is introduced, along with modifications to existing methods for improved date filtering. The test suite is expanded to cover edge cases and ensure accuracy in PnL calculations. Additionally, a new configuration parameter for the vault's PnL start date is added, affecting how PnL is calculated in related controllers. Changes
Possibly related PRs
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (5)
indexer/services/comlink/src/config.ts (1)
65-65: Consider enhancing date handling robustnessTo improve the robustness of the date configuration:
- Add runtime validation of the date string format
- Make the timezone handling more explicit in the parameter name
- Add JSDoc comments to document the expected format and timezone
Consider applying this enhancement:
+ /** + * UTC start date for vault PnL calculations. + * Must be in ISO 8601 format: YYYY-MM-DDThh:mm:ssZ + */ - VAULT_PNL_START_DATE: parseString({ default: '2024-01-01T00:00:00Z' }), + VAULT_PNL_START_DATE_UTC: parseString({ + default: '2024-01-01T00:00:00Z', + validate: (value: string) => { + if (!/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$/.test(value)) { + throw new Error('Invalid date format. Expected: YYYY-MM-DDThh:mm:ssZ'); + } + return value; + }, + }),indexer/packages/postgres/src/stores/pnl-ticks-table.ts (1)
469-473: LGTM with a minor security suggestion!The changes to
getPnlTicksAtIntervalslook good:
- Early return for empty subaccountIds is an optimization
- Proper UTC conversion for timestamp comparison
Consider using parameterized queries instead of string interpolation for the
earliestDateto prevent potential SQL injection, even though the DateTime type provides some safety.- "blockTime" >= '${earliestDate.toUTC().toISO()}'::timestamp AND + "blockTime" >= :earliestDate ANDAlso applies to: 501-501
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (2)
145-148: Consider enhancing test coverage and documentation.While the test cases effectively verify the start date functionality, consider these improvements:
- Add a comment explaining why 10000 is subtracted from the total PnL (is this the PnL value at the start date?)
- Add test cases for edge cases:
- Start date after all PnL ticks
- Start date between PnL ticks
- Invalid start date format
Also applies to: 154-170
Line range hint
516-627: Consider refactoring the test helper function.The
createPnlTicksfunction could be improved for better maintainability:
- Consider extracting the PnL adjustment logic into the helper function:
async function createPnlTicks( createMainSubaccountPnlTicks: boolean = false, + startDate?: string, ): Promise<PnlTicksFromDatabase[]> { const createdTicks: PnlTicksFromDatabase[] = await Promise.all([/*...*/]); + if (startDate !== undefined) { + for (const createdPnlTick of createdTicks) { + createdPnlTick.totalPnl = Big(createdPnlTick.totalPnl).sub('10000').toFixed(); + } + } return createdTicks; }
- Add JSDoc comments explaining:
- The significance of each time point (twoDaysAgo, currentDay, etc.)
- The purpose of the PnL adjustment when a start date is provided
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1)
339-358: Consider refactoring duplicated logic in PnL ticks retrievalThe code for fetching and adjusting PnL ticks in
getVaultSubaccountPnlTicksandgetLatestPnlTickis similar. To adhere to the DRY principle, consider refactoring this logic into a shared helper function.Also applies to: 555-574
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.ts(2 hunks)indexer/packages/postgres/src/stores/pnl-ticks-table.ts(4 hunks)indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts(4 hunks)indexer/services/comlink/src/config.ts(1 hunks)indexer/services/comlink/src/controllers/api/v4/vault-controller.ts(5 hunks)
🔇 Additional comments (6)
indexer/services/comlink/src/config.ts (1)
65-65: Verify if future default date is intentional
The default value is set to '2024-01-01T00:00:00Z', which is a future date. This could affect PnL calculations by excluding all current data points. Please verify if this is the intended behavior.
✅ Verification successful
Based on the search results, I can see that:
- The
VAULT_PNL_START_DATEconfig is used invault-controller.tsto determine the start date for vault PnL calculations - The tests explicitly set this value to '2020-01-01T00:00:00Z' for testing purposes
- The default value of '2024-01-01T00:00:00Z' appears to be intentional as it's used in production code
Let me verify the vault PnL calculation logic to understand if this future date is indeed intentional.
Let me check the vault PnL query implementation to understand how the start date is used.
Let me try a different search pattern to find the PnL query implementation.
Let me try one final search pattern to understand the vault PnL implementation.
After analyzing the code, I can now provide a definitive response about the future default date.
Future default date is intentional and correct
The future default date of '2024-01-01T00:00:00Z' is intentionally set as it's used as a configuration parameter to control when vault PnL tracking begins. This is verified by:
- The code in
vault-controller.tsuses this date as a starting point for querying PnL data throughgetVaultPnlStartDate() - Tests explicitly override this value to '2020-01-01T00:00:00Z' for testing historical data
- The implementation adjusts PnL calculations by subtracting the PnL values from this start date, ensuring accurate PnL tracking from the configured date forward
- The date is used consistently across multiple functions including
getVaultSubaccountPnlTicks()andgetLatestPnlTick()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded references to this date in tests or related files
# that might provide context about the chosen default date
echo "Searching for date string references in vault-related files..."
rg -A 5 "2024-01-01|VAULT_PNL_START_DATE" --type ts
echo "Searching for PnL calculation logic that uses this config..."
ast-grep --pattern 'config.VAULT_PNL_START_DATE'
Length of output: 70551
Script:
#!/bin/bash
# Search for vault PnL calculation logic to understand how the start date is used
rg -A 10 "historicalPnl|totalPnl|calculatePnl" --type ts indexer/services/comlink/src/
Length of output: 15305
Script:
#!/bin/bash
# Search for the SQL query that fetches vault PnL data to see how the start date is used
rg -B 5 -A 10 "getVaultPnlTicks|getPnlTicks.*vault" --type ts indexer/services/comlink/src/
Length of output: 93
Script:
#!/bin/bash
# Search for PnL-related database queries and the usage of start date
echo "Searching for PnL database queries..."
rg -B 5 -A 10 "FROM pnl_ticks.*WHERE" --type sql indexer/services/comlink/src/
echo "Searching for start date usage in vault controller..."
rg -B 10 -A 5 "getVaultPnlStartDate|startDate.*vault" --type ts indexer/services/comlink/src/
Length of output: 378
Script:
#!/bin/bash
# Search for files containing both vault and PnL related code
echo "Searching for vault PnL implementation files..."
fd -e ts -e sql . indexer/services/comlink/src/ -x grep -l "vault.*pnl\|pnl.*vault" {} \; -x cat {}
echo "Searching for files referencing the start date..."
rg -l "VAULT_PNL_START_DATE" --type ts | xargs cat
Length of output: 79111
indexer/packages/postgres/src/stores/pnl-ticks-table.ts (1)
515-550: LGTM with optimization suggestions!
The new getLatestPnlTick function is well-implemented:
- Efficient use of DISTINCT ON for latest record retrieval
- Proper early return optimization
- Clear and type-safe parameters
Suggestions:
- Use parameterized queries for SQL injection prevention
- Ensure proper indexing for optimal query performance
Consider this safer implementation:
- "blockTime" <= '${beforeOrAt.toUTC().toISO()}'::timestamp
+ "blockTime" <= :beforeOrAt✅ Verification successful
Query is properly optimized with required indexes
The codebase has both required indexes for optimal query performance:
pnl_ticks_blocktime_indexonblockTimecolumnpnl_ticks_subaccountid_indexonsubaccountIdcolumn
These indexes will ensure efficient execution of the DISTINCT ON query with its ORDER BY clause. The only remaining suggestion is to use parameterized queries for SQL injection prevention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify indexes exist for the query's ORDER BY columns
# Check for indexes on blockTime and subaccountId columns
psql -c "\d+ pnl_ticks" | grep -E "blockTime|subaccountId"
Length of output: 132
Script:
#!/bin/bash
# Let's try to find the schema definition or migration files to check for indexes
fd -e sql -e ts migration | xargs rg -l "CREATE.*INDEX.*pnl_ticks"
# Also search for table creation to see column definitions
fd -e sql -e ts migration | xargs rg -l "CREATE.*TABLE.*pnl_ticks"
Length of output: 137
Script:
#!/bin/bash
# Let's try a broader search for any files containing pnl_ticks table definition
rg -A 10 "CREATE.*TABLE.*pnl_ticks"
# Also search for any index definitions related to pnl_ticks
rg "CREATE.*INDEX.*pnl_ticks"
# Search for knex migrations related to pnl_ticks
fd -e ts | xargs rg "knex.*createTable.*pnl_ticks"
Length of output: 552
indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.ts (2)
463-463: LGTM: Proper implementation of earliest date filtering
The addition of the earliestDate parameter correctly tests the filtering of PnL ticks based on a start date, which aligns with the PR's objective.
486-512: LGTM: Comprehensive test coverage for getLatestPnlTick
The new test cases provide thorough coverage of the getLatestPnlTick functionality:
- Testing successful retrieval of latest PnL ticks
- Handling dates before any PnL data exists
- Handling empty subaccount lists
The tests are well-structured and properly validate the edge cases.
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (1)
46-46: LGTM: Configuration management follows best practices.
The code properly manages the VAULT_PNL_START_DATE configuration by:
- Storing the previous value
- Setting a default value before tests
- Restoring the original value after tests
Also applies to: 62-63, 132-132
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1)
744-765: Implementation of adjustVaultPnlTicks is correct
The adjustVaultPnlTicks function accurately adjusts the totalPnl values for each subaccount. The logic is sound and the implementation is clean.
| getVautlPnlStartDate(), | ||
| ), |
There was a problem hiding this comment.
Fix typo in function name 'getVautlPnlStartDate'
The function name getVautlPnlStartDate is misspelled. It should be getVaultPnlStartDate. Please update the function name and all its usages accordingly.
Apply this diff to correct the function name and its usages:
-function getVautlPnlStartDate(): DateTime {
+function getVaultPnlStartDate(): DateTime {
const startDate: DateTime = DateTime.fromISO(config.VAULT_PNL_START_DATE).toUTC();
return startDate;
}
...
- getVautlPnlStartDate(),
+ getVaultPnlStartDate(),
...
- getVautlPnlStartDate().plus({ minutes: 10 }),
+ getVaultPnlStartDate().plus({ minutes: 10 }),
...
- getVautlPnlStartDate(),
+ getVaultPnlStartDate(),
...
- getVautlPnlStartDate().plus({ minutes: 10 }),
+ getVaultPnlStartDate().plus({ minutes: 10 }),Also applies to: 356-357, 566-567, 572-573, 799-802
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit 4633b85)
Co-authored-by: vincentwschau <[email protected]>
Changelist
Allow setting a config var to determine the day vault PnL should start from.
Only queries for PnL data points created past the specified date, and adjusts the total pnl of each point by subtracting the pnl of the start date.
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
Release Notes
New Features
Improvements
Bug Fixes