Filter out to single tick per interval. #2403
Conversation
WalkthroughThe changes implement enhancements to the vault controller's API, focusing on the handling of historical profit and loss (PnL) data for vault subaccounts. Key modifications include the introduction of a new configuration parameter for PnL history, updates to test cases to accommodate changes in block heights, and the implementation of new functions to standardize resolution handling and filter PnL ticks based on specified intervals. Changes
Possibly related PRs
Suggested labels
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: 3
🧹 Outside diff range and nitpick comments (1)
indexer/services/comlink/src/config.ts (1)
64-64: LGTM! Consider adding a comment for clarity.The addition of
VAULT_PNL_HISTORY_HOURSis a good enhancement that aligns with the PR objectives. It provides more granular control over the PnL history, complementing the existingVAULT_PNL_HISTORY_DAYSparameter.Consider adding a brief comment explaining the purpose and relationship between
VAULT_PNL_HISTORY_HOURSandVAULT_PNL_HISTORY_DAYS. This would improve code readability and maintainability. For example:VAULT_PNL_HISTORY_DAYS: parseInteger({ default: 90 }), + // VAULT_PNL_HISTORY_HOURS provides finer granularity for recent PnL history VAULT_PNL_HISTORY_HOURS: parseInteger({ default: 72 }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- indexer/services/comlink/tests/controllers/api/v4/vault-controller.test.ts (5 hunks)
- indexer/services/comlink/src/config.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (7 hunks)
🧰 Additional context used
🪛 Biome
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts
[error] 466-466: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
🔇 Additional comments (5)
indexer/services/comlink/src/config.ts (1)
64-64: Verify the usage of the new configuration parameter.The addition of
VAULT_PNL_HISTORY_HOURSis well-integrated into the existing configuration structure. It maintains consistency with other parameters and doesn't disrupt the overall configuration processing.To ensure proper implementation, please verify that this new configuration parameter is correctly utilized in the relevant parts of the codebase. Run the following script to check its usage:
This will help ensure that the new parameter is being used appropriately throughout the project.
✅ Verification successful
VAULT_PNL_HISTORY_HOURS usage verified
The
VAULT_PNL_HISTORY_HOURSparameter is correctly utilized inconfig.tsandvault-controller.ts. The integration maintains consistency with existing configuration parameters and functions as intended without disrupting the overall configuration processing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of VAULT_PNL_HISTORY_HOURS in the codebase # Search for usage of VAULT_PNL_HISTORY_HOURS echo "Searching for VAULT_PNL_HISTORY_HOURS usage:" rg --type typescript --type javascript "VAULT_PNL_HISTORY_HOURS" # Search for potential places where it should be used alongside VAULT_PNL_HISTORY_DAYS echo "\nSearching for potential usage alongside VAULT_PNL_HISTORY_DAYS:" rg --type typescript --type javascript "VAULT_PNL_HISTORY_DAYS"Length of output: 438
Script:
#!/bin/bash # Description: Verify the usage of VAULT_PNL_HISTORY_HOURS in the codebase # Search for usage of VAULT_PNL_HISTORY_HOURS echo "Searching for VAULT_PNL_HISTORY_HOURS usage:" rg --type=ts --type=js "VAULT_PNL_HISTORY_HOURS" # Search for potential places where it should be used alongside VAULT_PNL_HISTORY_DAYS echo "\nSearching for potential usage alongside VAULT_PNL_HISTORY_DAYS:" rg --type=ts --type=js "VAULT_PNL_HISTORY_DAYS"Length of output: 832
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (3)
162-170: Validate handling ofundefinedvalues in test casesIn the parameterized test cases starting at line 162, the arrays contain
undefinedvalues. Ensure that the test logic correctly handlesundefinedentries to prevent unexpected behaviors during execution.Please verify that the mapping functions and any iterations properly account for
undefinedvalues.
535-537: Consistency in timestamp and block height assignmentIn the creation of
PnlTicksTableentries, theblockTime,createdAt, andblockHeightare set usingalmostTwoDaysAgoandalmostTwoDayBlockHeight. Verify that these timestamps and block heights align correctly with the intended test scenarios.Ensure that
almostTwoDaysAgoandalmostTwoDayBlockHeightaccurately represent the desired time difference and are used consistently across the related test data.
27-29: Ensure consistent block height valuesThe block heights for
currentBlockHeight,twoHourBlockHeight, andalmostTwoDayBlockHeightare set to string values'9','7', and'5'respectively. Ensure that these values correctly represent the intended block heights and that any dependent logic or calculations align with these updated values.Run the following script to cross-verify block height usages:
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1)
33-33: Appropriate import of 'DateTime' from 'luxon'Importing
DateTimefrom 'luxon' is necessary for date and time manipulations in the new functionfilterOutIntervalTicks.
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts
Show resolved
Hide resolved
| function getResolution(resolution?: PnlTickInterval) { | ||
| if (resolution === undefined) { | ||
| return PnlTickInterval.day; | ||
| } | ||
|
|
||
| return resolution; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the getResolution function using default parameters
You can refactor the getResolution function to use default parameters, which makes the code cleaner and removes the need to check for undefined.
Apply this diff to refactor the function:
-function getResolution(resolution?: PnlTickInterval) {
- if (resolution === undefined) {
- return PnlTickInterval.day;
- }
-
- return resolution;
+function getResolution(resolution: PnlTickInterval = PnlTickInterval.day): PnlTickInterval {
+ return resolution;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getResolution(resolution?: PnlTickInterval) { | |
| if (resolution === undefined) { | |
| return PnlTickInterval.day; | |
| } | |
| return resolution; | |
| } | |
| function getResolution(resolution: PnlTickInterval = PnlTickInterval.day): PnlTickInterval { | |
| return resolution; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (5)
89-89: LGTM: Improved resolution handling and tick filteringThe changes in the
getMegavaultHistoricalPnlmethod effectively implement the new resolution handling and tick filtering:
- Using
getResolution(resolution)ensures consistent resolution handling.- The new
filterOutIntervalTicksfunction addresses the PR objective of filtering out single ticks per interval.These modifications should resolve the issue of extra PnL ticks disrupting the aggregation process.
Consider extracting the
getResolution(resolution)call to a variable at the beginning of the method to avoid repeated function calls:async getMegavaultHistoricalPnl( @Query() resolution?: PnlTickInterval, ): Promise<MegavaultHistoricalPnlResponse> { + const resolvedResolution = getResolution(resolution); // ... existing code ... const [ vaultPnlTicks, // ... other destructured variables ... ] : [ // ... type annotations ... ] = await Promise.all([ - getVaultSubaccountPnlTicks(vaultSubaccountIdsWithMainSubaccount, getResolution(resolution)), + getVaultSubaccountPnlTicks(vaultSubaccountIdsWithMainSubaccount, resolvedResolution), // ... other Promise.all elements ... ]); // ... existing code ... const pnlTicksWithCurrentTick: PnlTicksFromDatabase[] = getPnlTicksWithCurrentTick( currentEquity, - filterOutIntervalTicks(aggregatedPnlTicks, getResolution(resolution)), + filterOutIntervalTicks(aggregatedPnlTicks, resolvedResolution), latestBlock, ); // ... rest of the method ... }This change would slightly improve readability and reduce redundant function calls.
Also applies to: 106-106
132-132: LGTM: Consistent resolution handlingThe change in the
getVaultsHistoricalPnlmethod aligns with the modifications made ingetMegavaultHistoricalPnl, ensuring consistent resolution handling across different methods.Similar to the suggestion for
getMegavaultHistoricalPnl, consider extracting thegetResolution(resolution)call to a variable at the beginning of the method:async getVaultsHistoricalPnl( @Query() resolution?: PnlTickInterval, ): Promise<VaultsHistoricalPnlResponse> { + const resolvedResolution = getResolution(resolution); const vaultSubaccounts: VaultMapping = await getVaultMapping(); const [ vaultPnlTicks, vaultPositions, latestBlock, ] : [ PnlTicksFromDatabase[], Map<string, VaultPosition>, BlockFromDatabase, ] = await Promise.all([ - getVaultSubaccountPnlTicks(_.keys(vaultSubaccounts), getResolution(resolution)), + getVaultSubaccountPnlTicks(_.keys(vaultSubaccounts), resolvedResolution), getVaultPositions(vaultSubaccounts), BlockTable.getLatest(), ]); // ... rest of the method ... }This change would improve consistency with the
getMegavaultHistoricalPnlmethod and reduce redundant function calls.
298-313: LGTM: Improved resolution handling ingetVaultSubaccountPnlTicksThe changes in the
getVaultSubaccountPnlTicksfunction effectively implement resolution-based PnL history retrieval:
- The
resolutionparameter is now required, ensuring that a valid resolution is always provided.- The
windowSecondscalculation adapts based on the resolution (daily or hourly), using appropriate configuration values.- The
PnlTicksTable.getPnlTicksAtIntervalscall now uses the correct parameters for resolution-based querying.These modifications align well with the PR objectives and should provide more flexible and accurate PnL history retrieval.
Consider using a switch statement or object mapping for
windowSecondscalculation to improve readability and extensibility:const SECONDS_PER_DAY = 24 * 60 * 60; const SECONDS_PER_HOUR = 60 * 60; const windowSecondsMap = { [PnlTickInterval.day]: config.VAULT_PNL_HISTORY_DAYS * SECONDS_PER_DAY, [PnlTickInterval.hour]: config.VAULT_PNL_HISTORY_HOURS * SECONDS_PER_HOUR, }; const windowSeconds = windowSecondsMap[resolution]; if (windowSeconds === undefined) { throw new Error(`Unsupported resolution: ${resolution}`); }This approach would make it easier to add new resolutions in the future and provides a clear error if an unsupported resolution is passed.
466-466: LGTM with a minor suggestion: Use of_.maxByfor latest PnL tickThe change to use
_.maxBy(pnlTicks, 'blockTime')is a good improvement, ensuring that the most recent PnL tick is used as a base for the current tick. This aligns well with the PR objectives and should provide more accurate current tick information.Remove the unnecessary non-null assertion operator (
!) as suggested by the static analysis tool. ThepnlTicksarray is guaranteed to be non-empty due to the preceding check, so_.maxBywill always return a value.Apply this diff to fix the issue:
- ...(_.maxBy(pnlTicks, 'blockTime')!), + ...(_.maxBy(pnlTicks, 'blockTime')),This change will improve code safety and comply with the static analysis recommendation.
🧰 Tools
🪛 Biome
[error] 466-466: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
475-518: LGTM: Well-implementedfilterOutIntervalTicksfunctionThe new
filterOutIntervalTicksfunction effectively implements the core logic for filtering PnL ticks to one per interval, addressing the main objective of the PR. Key points:
- Proper use of Luxon's DateTime for date/time operations.
- Correct handling of both day and hour intervals.
- Logical approach to finding the closest tick to the start of each interval.
This implementation should resolve the issue of extra PnL ticks disrupting the aggregation process.
Consider adding some comments to explain the logic, especially for the comparison in the if statement on line 512. This will improve readability and maintainability. For example:
// If this block's time is closer to the start of the interval than the currently stored block, // update the block and tick for this interval if (blockTime.diff(startOfInterval) < startOfDayBlockTime.diff(startOfInterval)) { blocksPerInterval.set(startOfInterval.toISO(), block); ticksPerInterval.set(startOfInterval.toISO(), pnlTick); }Additionally, consider extracting the interval start calculation to a separate function for better readability:
function getIntervalStart(dateTime: DateTime, resolution: PnlTickInterval): DateTime { return dateTime.toUTC().startOf(resolution); } // Usage in the function: const startOfInterval: DateTime = getIntervalStart(blockTime, resolution);These changes would make the function easier to understand and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (7 hunks)
🧰 Additional context used
🪛 Biome
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts
[error] 466-466: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
🔇 Additional comments (3)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3)
33-33: LGTM: Appropriate import for date/time handlingThe import of
DateTimefrom the Luxon library is a good choice for handling date and time operations in the new filtering function.
520-522: LGTM: UsefulgetResolutionhelper functionThe new
getResolutionfunction is a simple yet effective helper that ensures a valid resolution is always returned, defaulting toPnlTickInterval.daywhen no resolution is provided. This function contributes to standardizing resolution handling across the controller, which aligns well with the PR objectives.
Line range hint
1-523: Overall assessment: Well-implemented changes addressing PR objectivesThe changes in this file effectively address the main objectives of the PR:
- Filtering out single ticks per interval to prevent disruption in the aggregation process.
- Handling different resolutions (daily and hourly) for PnL history.
- Standardizing resolution handling across the controller.
The new
filterOutIntervalTicksfunction, along with the modifications to existing methods, should resolve the issue of extra PnL ticks. The code is generally well-structured and follows good practices.Minor suggestions for improvement have been made, mainly focusing on:
- Extracting repeated function calls to variables.
- Improving readability with comments and helper functions.
- Removing an unnecessary non-null assertion.
These suggestions, if implemented, would further enhance the code quality and maintainability.
|
how are pnl ticks handled with generic subaccounts (what happens if a subaccount gets transferred to for the first time in the middle of a day/hour)? |
| pnlTickInterval = PnlTickInterval.day; | ||
|
|
||
| let windowSeconds: number; | ||
| if (resolution === PnlTickInterval.day) { |
There was a problem hiding this comment.
nit: maybe define a function that converts from resolution to seconds? thinking about the possibility of adding more resolution values in the future
There was a problem hiding this comment.
This doesn't work with the underlying SQL query.
| /** | ||
| * Takes in a map of block heights to PnlTicks and filters out the closest pnl tick per interval. | ||
| * @param pnlTicksByBlock | ||
| * @returns |
There was a problem hiding this comment.
mega nit: missing return value? not sure whether the convention is to have something here
| blockToBlockTime.set(block, blockTime); | ||
|
|
||
| const startOfInterval: DateTime = blockTime.toUTC().startOf(resolution); | ||
| const startOfIntervalBlock: number | undefined = blocksPerInterval.get(startOfInterval.toISO()); |
There was a problem hiding this comment.
nit: not sure how much work this call is. if notable, define startOfInterval.toISO() as a const so we don't call it many times?
A pnl tick only gets created at each hour. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3)
298-313: LGTM: Updated getVaultSubaccountPnlTicks functionThe changes to
getVaultSubaccountPnlTicksfunction improve the handling of resolution for PnL ticks. Makingresolutiona required parameter ensures consistent behavior.Consider extracting the
windowSecondscalculation into a separate function for better readability and maintainability:function getWindowSeconds(resolution: PnlTickInterval): number { return resolution === PnlTickInterval.day ? config.VAULT_PNL_HISTORY_DAYS * 24 * 60 * 60 : config.VAULT_PNL_HISTORY_HOURS * 60 * 60; }Then use it in the function:
const windowSeconds = getWindowSeconds(resolution);
466-466: Approve change and remove unnecessary non-null assertionThe use of
_.maxBy(pnlTicks, 'blockTime')is a good improvement, ensuring that the most recent PnL tick is used as the base for the current tick. This is more robust than the previous approach of using the last element of the array.However, the non-null assertion operator (
!) is unnecessary here. SincepnlTicksis guaranteed to be non-empty (checked at the beginning of the function),_.maxBywill always return a value. You can safely remove the assertion:- ...(_.maxBy(pnlTicks, 'blockTime')!), + ...(_.maxBy(pnlTicks, 'blockTime')),🧰 Tools
🪛 Biome
[error] 466-466: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
475-519: LGTM: New filterOutIntervalTicks functionThe
filterOutIntervalTicksfunction is well-implemented and aligns with the PR objectives. It effectively filters out additional PnL ticks, keeping only the closest tick per interval.Suggestions for minor improvements:
- Add a return type annotation to the function for better type safety:
function filterOutIntervalTicks( pnlTicksByBlock: Map<number, PnlTicksFromDatabase>, resolution: PnlTickInterval, ): PnlTicksFromDatabase[] { // ... function body ... }
- Consider using a more descriptive variable name instead of
startOfDayBlockTimewhen working with non-day resolutions:const closestIntervalBlockTime: DateTime | undefined = blockToBlockTime.get(startOfIntervalBlock);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (7 hunks)
🧰 Additional context used
🪛 Biome
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts
[error] 466-466: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
🔇 Additional comments (3)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3)
33-33: LGTM: Import of DateTime from luxonThe addition of the
DateTimeimport from 'luxon' is appropriate for the new functionality introduced in this file, particularly for thefilterOutIntervalTicksfunction.
Line range hint
89-106: LGTM: Updated getMegavaultHistoricalPnl methodThe changes to the
getMegavaultHistoricalPnlmethod are well-implemented and consistent with the PR objectives. The use ofgetResolutionensures proper handling of the resolution parameter, and the introduction offilterOutIntervalTicksaddresses the issue of having multiple PnL ticks per interval.These modifications effectively solve the problem described in the PR summary, where additional PnL ticks were being generated when a vault was instantiated mid-interval.
132-132: LGTM: Updated getVaultsHistoricalPnl methodThe change to the
getVaultsHistoricalPnlmethod, incorporating thegetResolutionfunction, is appropriate and consistent with the modifications made togetMegavaultHistoricalPnl. This ensures that the resolution is properly handled for individual vaults, maintaining consistency across the API.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (7 hunks)
🧰 Additional context used
🪛 Biome
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts
[error] 466-466: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
🔇 Additional comments (5)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (5)
89-89: LGTM: Improved handling of resolution and PNL tick filteringThe changes to
getMegavaultHistoricalPnlfunction now incorporate the newgetResolutionandfilterOutIntervalTicksfunctions. This implementation aligns well with the PR objectives of filtering out single ticks per interval, improving the accuracy of historical PNL data.Also applies to: 106-106
132-132: LGTM: Consistent resolution handlingThe
getVaultsHistoricalPnlfunction now uses thegetResolutionfunction, ensuring consistent resolution handling across different methods in the controller. This change improves the overall coherence of the code.
298-313: LGTM: Improved resolution handling in PNL tick retrievalThe
getVaultSubaccountPnlTicksfunction has been updated to handle different resolutions (day or hour) and calculate the appropriate window size. This change supports the new filtering functionality and aligns well with the PR objectives. The use of configuration values (config.VAULT_PNL_HISTORY_DAYSandconfig.VAULT_PNL_HISTORY_HOURS) for determining the window size is a good practice for maintainability.
475-520: LGTM: Well-implemented interval filtering logicThe new
filterOutIntervalTicksfunction effectively implements the core logic for filtering PNL ticks to ensure only one tick per interval is retained. The use of Luxon for date/time operations and the algorithm to find the closest tick to the start of each interval are well-implemented. This function directly addresses the PR objectives and should resolve the issue of extra PNL ticks being generated.
466-466:⚠️ Potential issueRemove unnecessary non-null assertion operator
The non-null assertion operator
!in_.maxBy(pnlTicks, 'blockTime')!is unnecessary sincepnlTicksis guaranteed to be non-empty (validated by the preceding check). Removing it will improve code safety and comply with the static analysis recommendation.Apply this diff to fix the issue:
- ...(_.maxBy(pnlTicks, 'blockTime')!), + ...(_.maxBy(pnlTicks, 'blockTime')),Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 466-466: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3)
89-89: LGTM: Improved resolution handling and tick filteringThe changes in the
getMegavaultHistoricalPnlmethod enhance the handling of PnL data:
- Using
getResolutionensures a consistent approach to resolution handling.- The introduction of
filterOutIntervalTicksaligns with the PR objective of filtering out single ticks per interval.These modifications should effectively address the issue of extra PnL ticks being generated when a vault is instantiated mid-interval.
Consider extracting the
getResolution(resolution)call into a variable at the beginning of the method to avoid calling it twice.Also applies to: 106-106
298-298: LGTM: Improved handling of resolution and time windowsThe changes in the
getVaultSubaccountPnlTicksfunction enhance its functionality and flexibility:
- Making
resolutiona required parameter improves type safety.- The new logic for
windowSecondsallows for different time windows based on the resolution.- Using
resolutionin thegetPnlTicksAtIntervalscall ensures correct interval usage.These modifications align well with the PR objectives and provide more precise control over PnL data retrieval.
Consider using a switch statement or an object lookup for determining
windowSecondsto make it easier to add new resolution types in the future:const windowSecondsMap = { [PnlTickInterval.day]: config.VAULT_PNL_HISTORY_DAYS * 24 * 60 * 60, [PnlTickInterval.hour]: config.VAULT_PNL_HISTORY_HOURS * 60 * 60, }; const windowSeconds = windowSecondsMap[resolution];Also applies to: 304-308, 312-313
481-520: LGTM: Well-implemented interval tick filteringThe new
filterOutIntervalTicksfunction effectively addresses the PR objective of filtering out single ticks per interval. Key points:
- Efficient use of maps to track blocks and ticks per interval.
- Proper handling of edge cases, such as when no block is set for the start of an interval.
- Correct use of the
DateTimeclass for date and time operations.This implementation should successfully resolve the issue of extra PnL ticks being generated when a vault is instantiated mid-interval.
Consider adding a comment explaining the logic for choosing the closest block to the start of the interval (lines 514-517). This will make the code more maintainable for future developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (7 hunks)
🧰 Additional context used
🪛 Biome
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts
[error] 466-466: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
🔇 Additional comments (4)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (4)
33-33: LGTM: New import for DateTimeThe addition of the
DateTimeimport from the 'luxon' library is appropriate for the new date and time operations introduced in this file.
132-132: LGTM: Consistent resolution handlingThe use of
getResolutionin thegetVaultsHistoricalPnlmethod ensures consistent handling of the resolution parameter across different methods in the controller. This change aligns well with the modifications made in thegetMegavaultHistoricalPnlmethod.
522-524: LGTM: Concise resolution handlingThe new
getResolutionfunction is a simple yet effective way to handle the resolution parameter:
- It provides a default resolution of
PnlTickInterval.day.- The implementation is concise and easy to understand.
- It ensures consistent handling of the resolution parameter throughout the file.
This function contributes to the overall improvement in PnL data handling in the controller.
Line range hint
1-525: Overall assessment: Well-implemented PnL tick filteringThe changes in this file effectively address the PR objectives:
- Introduction of
filterOutIntervalTicksfunction resolves the issue of extra PnL ticks.- Consistent handling of resolution parameter across different methods.
- Improved flexibility in determining time windows for PnL data retrieval.
The code is well-structured and maintainable. Minor suggestions have been made for further improvements, but overall, these changes should significantly enhance the accuracy of PnL data for vaults.
(cherry picked from commit ce94992)
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
Changelist
It's possible for a vault to be instantiated in the middle of the day/hour, and start having PnL ticks in the middle of the day/hour. This breaks the aggregation logic for PnL ticks as it introduces an extra PnL tick in the middle of the day / hour for the megavault.
This change adds a filtering function that only takes the aggregated PnL ticks with block times closest to the start of the day / 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