-
Notifications
You must be signed in to change notification settings - Fork 115
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
Get latest hourly tick to compute final tick for megavault PnL. #2454
Conversation
WalkthroughThe pull request introduces modifications to the vault controller's test file, enhancing the handling of block heights and time representations. It adds new time-related variables and updates existing ones for clarity. Additionally, a new configuration parameter is introduced in the configuration schema. 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 (
|
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.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
indexer/services/comlink/src/config.ts (1)
65-65
: New configuration parameter added for vault PnL tick windowThe new configuration parameter
VAULT_LATEST_PNL_TICK_WINDOW_HOURS
has been added with a default value of 1 hour. This aligns with the PR objective of enhancing the computation of the final "current" tick by retrieving the latest hourly PnL tick.A few points to consider:
- The default value of 1 hour seems reasonable for retrieving the latest tick, but it might be worth documenting why this specific default was chosen.
- It may be beneficial to add a comment explaining the purpose and impact of this configuration parameter for future maintainers.
Consider adding a brief comment above this line to explain the purpose of this configuration, for example:
// Window in hours for retrieving the latest PnL tick for vault calculations VAULT_LATEST_PNL_TICK_WINDOW_HOURS: parseInteger({ default: 1 }),indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (4)
Line range hint
44-59
: New configuration for latest PnL tick windowThe introduction of
VAULT_LATEST_PNL_TICK_WINDOW_HOURS
set to 48 hours aligns with the PR objective of retrieving the latest hourly PnL tick. This change enhances the flexibility of the system.Consider adding a comment explaining the purpose and significance of the
VAULT_LATEST_PNL_TICK_WINDOW_HOURS
configuration.
140-147
: Updated test parameters for new PnL tick behaviorThe addition of
finalTickIndex
and the update to expected ticks for hourly resolution reflect the new behavior of including the latest hourly PnL tick in the results. This change aligns well with the PR objective.Consider adding a comment explaining the significance of
finalTickIndex
to improve test readability.
179-186
: Updated test assertions for different resolutionsThe changes to expected tick indices for different resolutions accurately reflect the new behavior of including or excluding certain ticks based on the resolution and the latest PnL tick window. The use of
undefined
values appropriately indicates scenarios where certain ticks should not be present.Consider adding comments explaining the significance of
undefined
values in the expected tick indices to improve test readability.
227-228
: Updated total PnL calculationThe comment explaining that total PnL should be fetched from the latest hourly PnL tick aligns with the PR objective. The update to multiply by 4 instead of 3 in the total PnL calculation likely accounts for an additional tick or data point.
Consider adding a brief explanation for why the multiplication factor is now 4 instead of 3 to improve code clarity.
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1)
489-505
: Add unit tests and documentation forgetLatestPnlTick
function.The new function
getLatestPnlTick
retrieves the latest hourly PnL tick. To ensure correctness and handle edge cases (e.g., no available ticks within the window), please add unit tests covering various scenarios. Additionally, include inline documentation to explain the function's purpose and logic for future maintainers.Would you like assistance in writing unit tests or adding documentation for this function?
📜 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 (14 hunks)
- indexer/services/comlink/src/config.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (4 hunks)
🔇 Additional comments (10)
indexer/services/comlink/src/config.ts (1)
65-65
: Verify usage of the new configuration parameterThe new configuration parameter
VAULT_LATEST_PNL_TICK_WINDOW_HOURS
has been added to the schema, but its usage is not visible in this file. To ensure it's properly integrated into the system:Please run the following script to check where and how this new configuration is being used:
This will help confirm that the new configuration is being utilized correctly in the relevant parts of the codebase for computing the final "current" tick as mentioned in the PR objectives.
✅ Verification successful
Retry searching for the usage of the new configuration parameter with correct file types
The previous search command failed due to an unrecognized file type. Please run the following script to search for
VAULT_LATEST_PNL_TICK_WINDOW_HOURS
in.ts
and.tsx
files:
Retry searching for the usage of the new configuration parameter with correct file patterns
The previous search command failed due to unrecognized file types. Please run the following script to search for
VAULT_LATEST_PNL_TICK_WINDOW_HOURS
in.ts
and.tsx
files using glob patterns:
Usage of
VAULT_LATEST_PNL_TICK_WINDOW_HOURS
VerifiedThe new configuration parameter
VAULT_LATEST_PNL_TICK_WINDOW_HOURS
is properly utilized in the codebase:
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of VAULT_LATEST_PNL_TICK_WINDOW_HOURS in the codebase # Test: Search for the usage of the new configuration parameter rg --type typescript --type javascript 'VAULT_LATEST_PNL_TICK_WINDOW_HOURS'Length of output: 110
Script:
#!/bin/bash # Description: Search for usage of VAULT_LATEST_PNL_TICK_WINDOW_HOURS in TypeScript files rg --type ts --type tsx 'VAULT_LATEST_PNL_TICK_WINDOW_HOURS'Length of output: 88
Script:
#!/bin/bash # Description: Search for usage of VAULT_LATEST_PNL_TICK_WINDOW_HOURS in TypeScript files rg --glob '*.ts' --glob '*.tsx' 'VAULT_LATEST_PNL_TICK_WINDOW_HOURS'Length of output: 755
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (7)
28-38
: Improved clarity in time and block height variablesThe renaming of
currentBlockHeight
tocurrentHourBlockHeight
and the addition ofcurrentDayBlockHeight
provide a clearer distinction between hourly and daily contexts. This change aligns well with the PR objective of enhancing the computation of the final "current" tick by retrieving the latest hourly PnL tick.
Line range hint
76-93
: Additional block creation for current day and hourThe addition of block creation for
currentDay
andcurrentHour
ensures that the test data includes blocks for both daily and hourly contexts. This change is necessary for thoroughly testing the new functionality related to retrieving the latest hourly PnL tick.
127-127
: Proper reset of new configurationResetting the
VAULT_LATEST_PNL_TICK_WINDOW_HOURS
configuration after each test ensures proper test isolation. This is a good practice to prevent unintended side effects between tests.
Line range hint
156-160
: Correct creation of final PnL tickThe creation of
finalTick
usingfinalTickIndex
and setting itsblockHeight
,blockTime
, andcreatedAt
to the latest values ensures that it accurately represents the most recent data. This is crucial for computing the final "current" tick as per the PR objective.
542-551
: Addition of current hour PnL tickThe creation of a new PnL tick for the current hour with a doubled total PnL value enhances the test coverage for the latest hourly PnL tick retrieval. This addition allows for more comprehensive testing of the new functionality, aligning well with the PR objective.
574-583
: Additional PnL ticks for vault subaccountThe creation of new PnL ticks for the current day and current hour for the vault subaccount ensures comprehensive test data. This addition is crucial for thoroughly testing the new functionality related to retrieving the latest hourly PnL tick across all subaccounts.
610-619
: Additional PnL ticks for main subaccountThe creation of new PnL ticks for the current day and current hour for the main subaccount completes the comprehensive test data set. This addition ensures that the new functionality for retrieving the latest hourly PnL tick is tested across all subaccount types, including the main subaccount.
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (2)
83-95
: Verify error handling and type safety forlatestPnlTick
inPromise.all
.The addition of
latestPnlTick
to the destructured array and thePromise.all
call introduces a new asynchronous operation. Please ensure thatgetLatestPnlTick
handles errors properly to prevent unhandled rejections, and that all promises inPromise.all
are reliable. Also, verify that the destructured types align correctly with the resolved values, consideringlatestPnlTick
can beundefined
.
111-111
: Confirm correct handling of optionallatestTick
in function call.The
getPnlTicksWithCurrentTick
function now accepts an optionallatestTick
parameter. The updated function call correctly includeslatestPnlTick
. This change is appropriate and ensures the latest tick is considered in the PnL calculation.
(cherry picked from commit deff758)
…port #2454) (#2466) Co-authored-by: vincentwschau <[email protected]>
Changelist
For all resolutions, get the latest hourly PnL tick to compute the final "current" tick. This ensures the final PnL tick when looking at daily resolution to have the most update to date PnL snapshot (computed hourly).
Test Plan
[Describe how this PR was tested (if applicable)]
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
.Summary by CodeRabbit