[TRA-571] Add resolution argument to vault PnL endpoint. #2282
Conversation
WalkthroughThe changes involve enhancements to the vault controller's handling of historical Profit and Loss (PnL) data and the associated API endpoints. Key modifications include the introduction of a Changes
Possibly related PRs
Suggested labels
Poem
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
indexer/services/comlink/public/api-documentation.md (1)
5972-5973: Typo: Use "an" instead of "a" before words starting with a vowel sound.Please fix the typos in the schema definition comments:
- Line 5972: Change "a id" to "an id".
- Line 5973: Change "a id" to "an id".
Tools
LanguageTool
[misspelling] ~5972-~5972: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...val <a id=...(EN_A_VS_AN)
[misspelling] ~5973-~5973: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: .../a> <a id="to...(EN_A_VS_AN)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- indexer/services/comlink/tests/controllers/api/v4/vault-controller.test.ts (9 hunks)
- indexer/services/comlink/public/api-documentation.md (3 hunks)
- indexer/services/comlink/public/swagger.json (3 hunks)
- indexer/services/comlink/src/config.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (8 hunks)
- indexer/services/comlink/src/types.ts (2 hunks)
Additional context used
LanguageTool
indexer/services/comlink/public/api-documentation.md
[misspelling] ~5972-~5972: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...val <a id=...(EN_A_VS_AN)
[misspelling] ~5973-~5973: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: .../a> <a id="to...(EN_A_VS_AN)
Additional comments not posted (21)
indexer/services/comlink/src/config.ts (1)
66-68: Approve the addition of theVAULT_PNL_HISTORY_DAYSconfiguration parameter.The new configuration parameter
VAULT_PNL_HISTORY_DAYSwith a default value of 90 days is a useful addition for controlling the duration of historical PnL data maintenance for vaults.Verify that this parameter is used consistently across the codebase wherever historical PnL data handling is involved. Run the following script to check for usage:
Verification successful
Approve the addition and usage of the
VAULT_PNL_HISTORY_DAYSconfiguration parameterThe
VAULT_PNL_HISTORY_DAYSparameter has been correctly implemented in the config file and is being used appropriately in the vault-controller.ts file. The parameter is converted to seconds for practical application, which is a logical approach.While the usage is limited to a single controller file, this appears to be intentional and focused on its specific purpose related to vault PnL history. The config object is widely imported across the codebase, ensuring that the parameter can be easily accessed by other parts of the system if needed in the future.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of VAULT_PNL_HISTORY_DAYS parameter in the codebase. # Test: Search for the parameter usage. Expect: Relevant occurrences across the codebase. rg --type typescript $'VAULT_PNL_HISTORY_DAYS' -g '!config.ts'Length of output: 96
Script:
#!/bin/bash # Description: Verify usage of VAULT_PNL_HISTORY_DAYS parameter in the codebase. # Search for VAULT_PNL_HISTORY_DAYS in .ts and .tsx files, excluding config.ts echo "Searching for VAULT_PNL_HISTORY_DAYS in .ts and .tsx files:" rg 'VAULT_PNL_HISTORY_DAYS' --type-add 'ts:*.{ts,tsx}' -g '!config.ts' -g '*.{ts,tsx}' # Search for files that import or use the config object echo -e "\nSearching for files that import or use the config object:" rg 'import.*from.*config' --type-add 'ts:*.{ts,tsx}' -g '*.{ts,tsx}'Length of output: 16900
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3)
Line range hint
68-83: LGTM!The changes to introduce the optional
resolutionparameter and update thegetVaultSubaccountPnlTicksfunction to handle it look good. The aggregation logic and response mapping remain unchanged.
Line range hint
85-117: LGTM!The changes to introduce the optional
resolutionparameter and update thegetVaultSubaccountPnlTicksfunction to handle it look good. The grouping and mapping logic remain unchanged.
351-372: LGTM!The changes to introduce the optional
resolutionparameter and use the newPnlTicksTable.getPnlTicksAtIntervalsmethod to fetch PnL ticks at the specified interval look good. The function determines the PnL tick interval based on the providedresolutionor defaults to a daily interval.indexer/services/comlink/src/types.ts (2)
21-21: LGTM!The import statement for
PnlTickIntervaltype looks good.
683-687: Looks good!The new interfaces
MegavaultHistoricalPnlRequestandVaultsHistoricalPnlRequestare well-defined and will enable fetching historical PnL data at different resolutions. The usage ofPnlTickIntervaltype ensures type safety for theresolutionproperty.This change enhances the functionality and flexibility of the vault-related components.
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (10)
414-461: Excellent refactoring!The
createPnlTicksfunction consolidates the creation of PnL tick entries into a single asynchronous operation, improving the clarity and maintainability of the test setup. It generates multiple PnL ticks with varying timestamps and block heights, which enhances the flexibility of the tests.
Line range hint
77-85: LGTM!The test case correctly verifies the behavior of the
/megavault/historicalPnlendpoint when there are no vault subaccounts. It sets the relevant config values to empty strings and expects an empty array in the response.
86-109: Great use of parameterized tests!The test case effectively verifies the behavior of the
/megavault/historicalPnlendpoint with a single vault subaccount and different resolution query parameters. It uses a parameterized test to cover different scenarios and expects the response to contain the correct PnL ticks based on the resolution.
Line range hint
111-153: Excellent test coverage!The test case thoroughly verifies the behavior of the
/megavault/historicalPnlendpoint with two vault subaccounts and different resolution query parameters. It sets up the necessary config values, creates PnL ticks, and expects the response to contain the correct PnL ticks based on the resolution, with the values doubled due to having two vault subaccounts.
Line range hint
155-166: LGTM!The test case correctly verifies the behavior of the
/vaults/historicalPnlendpoint when there are no vault subaccounts. It sets the relevant config values to empty strings and expects an empty array in the response.
168-194: Great use of parameterized tests!The test case effectively verifies the behavior of the
/vaults/historicalPnlendpoint with a single vault subaccount and different resolution query parameters. It uses a parameterized test to cover different scenarios and expects the response to contain an array of length 1 with the correct PnL ticks based on the resolution.
Line range hint
196-245: Excellent test coverage!The test case thoroughly verifies the behavior of the
/vaults/historicalPnlendpoint with two vault subaccounts and different resolution query parameters. It sets up the necessary config values, creates PnL ticks, and expects the response to contain an array with the correct PnL ticks for each vault subaccount based on the resolution.
Line range hint
247-258: LGTM!The test case correctly verifies the behavior of the
/megavault/positionsendpoint when there are no vault subaccounts. It sets the relevant config values to empty strings and expects an empty array for thepositionsproperty in the response.
Line range hint
260-326: Comprehensive test case!The test case thoroughly verifies the behavior of the
/megavault/positionsendpoint with a single vault subaccount. It sets up the necessary data in the database, including a perpetual position, an asset position, and funding index updates. It then expects the response to contain the correct perpetual and asset positions with calculated values.
Line range hint
328-411: Excellent test coverage!The test case thoroughly verifies the behavior of the
/megavault/positionsendpoint with two vault subaccounts, one of which has no perpetual position. It sets up the necessary config values and data in the database, including a perpetual position, asset positions for both subaccounts, and funding index updates. It then expects the response to contain the correct perpetual and asset positions for the first subaccount and only the asset position for the second subaccount.indexer/services/comlink/public/swagger.json (2)
1499-1505: LGTM!The
PnlTickIntervalschema is correctly defined with appropriate enum values for specifying the resolution of PnL tick data.
3434-3443: Looks good!The
resolutionquery parameter is correctly added to the/vault/v1/megavault/historicalPnland/vault/v1/vaults/historicalPnlendpoints. It references thePnlTickIntervalschema to specify the allowed values for retrieving PnL data at different resolutions. This enhances the API's functionality and flexibility.Also applies to: 3462-3471
indexer/services/comlink/public/api-documentation.md (3)
3444-3456: LGTM!The addition of the optional
resolutionquery parameter to theGET /vault/v1/megavault/historicalPnlendpoint looks good. It aligns with the PR objective and maintains backward compatibility by being optional.
3536-3548: Looks good!The
resolutionquery parameter is added consistently to theGET /vault/v1/vaults/historicalPnlendpoint. The type and optionality match the other endpoint.
5970-5994: The newPnlTickIntervalschema definition looks good!The enumerated values cover the required resolutions of "hour" and "day". The naming is clear and it is used consistently in the
resolutionquery parameters added to the endpoints.Tools
LanguageTool
[misspelling] ~5972-~5972: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...val <a id=...(EN_A_VS_AN)
[misspelling] ~5973-~5973: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: .../a> <a id="to...(EN_A_VS_AN)
Changelist
Allow fetching for hourly or daily PnL points for vault endpoints.
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
resolutionparameter for historical PnL API endpoints, allowing users to specify data intervals ("hour" or "day").resolutionparameter for more flexible PnL data retrieval.Bug Fixes
Documentation
Configuration
VAULT_PNL_HISTORY_DAYS, to manage PnL history duration.