[TRA-890] Return nearest hour PnL as final data point for vaults pnl. #2616
Conversation
WalkthroughThe changes in this pull request involve modifications to the test cases for the 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 (4)
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (2)
342-344: Consider adding edge case testsWhile the test cases cover basic scenarios with different resolutions, consider adding tests for edge cases:
- When
currentTickIndexis out of bounds- When
currentTickIndexpoints to an invalid tickAlso applies to: 350-351
Based on the findings, I can now provide the final response:
Add test coverage for error scenarios in vault-controller endpoints
The test suite needs additional test cases to cover error scenarios, specifically:
- Invalid resolution parameter values (the API validates against
PnlTickIntervalenum values)- Error handling in
/v1/megavault/positionsendpoint- Request validation errors from the express-validator middleware
Example test cases to add:
- Test with invalid resolution value (e.g., '?resolution=invalid')
- Test error handling when database queries fail
- Test rate limiter behavior
🔗 Analysis chain
Line range hint
1-650: Verify test coverage for error scenariosThe test suite thoroughly covers successful scenarios but could benefit from additional error case testing:
- Invalid resolution parameter
- Missing or malformed query parameters
- Empty or invalid PNL data
Let me gather more information about the test file to verify error handling coverage.
Let me check the API implementation to understand what error cases should be tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check current test coverage for error handling rg -A 2 "it\(.*(error|invalid|fail)" indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.tsLength of output: 276
Script:
#!/bin/bash # Check for test cases with invalid/error scenarios in query parameters and responses rg -B 2 -A 5 "(resolution|queryParam|response.*status|response.*error)" indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.tsLength of output: 3862
Script:
#!/bin/bash # Check the API implementation for error handling rg -A 10 "router\.(get|post)" indexer/services/comlink/src/controllers/api/v4/vault-controller.tsLength of output: 1121
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (2)
Line range hint
174-175: Fix variable name typo: 'pnlTickFromDatabsae' should be 'pnlTickFromDatabase'The variable
pnlTickFromDatabsaein the filter function is misspelled. It should bepnlTickFromDatabaseto match the correct spelling.Apply the following diff to correct the variable name:
.filter((pnlTickFromDatabsae: PnlTicksFromDatabase): boolean => { - return vaultSubaccounts[pnlTickFromDatabsae.subaccountId] !== undefined; + return vaultSubaccounts[pnlTickFromDatabase.subaccountId] !== undefined;
Line range hint
764-768: Correct the logged value in the warning messageIn the warning log within
getVaultMapping, you are loggingvaultMapping[subaccountId], which is an object. You should logvaultMapping[subaccountId].clobPairIdto correctly display the Clob Pair ID that does not correspond to a perpetual market.Apply the following diff to fix the log message:
logger.warning({ at: 'VaultController#getVaultPositions', - message: `Vault clob pair id ${vaultMapping[subaccountId]} does not correspond to a ` + + message: `Vault clob pair id ${vaultMapping[subaccountId].clobPairId} does not correspond to a ` + 'perpetual market.', subaccountId, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts(4 hunks)indexer/services/comlink/src/controllers/api/v4/vault-controller.ts(4 hunks)
🔇 Additional comments (1)
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (1)
301-303: LGTM: Test case parameters properly updated
The addition of currentTickIndex parameter improves test clarity by explicitly specifying which tick should be used as the final data point.
Also applies to: 308-308
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts
Show resolved
Hide resolved
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: 0
🧹 Outside diff range and nitpick comments (1)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1)
562-589: Refactor to eliminate code duplication between 'getLatestPnlTicks' and 'getVaultSubaccountPnlTicks'Both
getLatestPnlTicksandgetVaultSubaccountPnlTicksfunctions perform similar operations in fetching and adjusting PnL ticks. Consider abstracting the common logic into a helper function to enhance maintainability and reduce code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts(4 hunks)indexer/services/comlink/src/controllers/api/v4/vault-controller.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- indexer/services/comlink/tests/controllers/api/v4/vault-controller.test.ts
🔇 Additional comments (1)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1)
563-564: Add a check for empty 'vaultSubaccountIds' array
The previous review comment about adding a check for an empty vaultSubaccountIds array in the getLatestPnlTicks function is still applicable.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts(7 hunks)indexer/services/comlink/src/controllers/api/v4/vault-controller.ts(7 hunks)
🔇 Additional comments (6)
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (3)
24-24: LGTM! Good test setup and cleanup.
The DateTime mock setup and cleanup ensures deterministic test behavior and prevents test pollution.
Also applies to: 127-127, 137-137
303-305: LGTM! Improved test case parameterization.
The addition of explicit currentTickIndex parameters makes the test cases more maintainable and clearer about which PnL tick should be used for assertions.
Also applies to: 310-310, 344-346, 352-353
319-319: Previous review comment is still applicable.
The fixes for the variable names and consistent usage of tick indices have been implemented as suggested.
Also applies to: 369-369, 376-376
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3)
38-38: LGTM!
The addition of the Dictionary type from lodash improves type safety for the latestTicksBySubaccountId mapping.
837-840: Function name correction looks good
The typo in the function name has been fixed as suggested in the previous review.
562-588: 🛠️ Refactor suggestion
Add input validation for empty vaultSubaccountIds array
The function should return early if vaultSubaccountIds is empty to prevent unnecessary database queries.
Add this check at the start of the function:
export async function getLatestPnlTicks(
vaultSubaccountIds: string[],
): Promise<PnlTicksFromDatabase[]> {
+ if (vaultSubaccountIds.length === 0) {
+ return [];
+ }
const [
latestPnlTicks,
adjustByPnlTicks,
] : [Likely invalid or redundant comment.
| latestTicks, | ||
| ] : [ | ||
| PnlTicksFromDatabase[], | ||
| Map<string, VaultPosition>, | ||
| BlockFromDatabase, | ||
| PnlTicksFromDatabase[], | ||
| ] = await Promise.all([ | ||
| getVaultSubaccountPnlTicks(_.keys(vaultSubaccounts), getResolution(resolution)), | ||
| getVaultPositions(vaultSubaccounts), | ||
| BlockTable.getLatest(), | ||
| getLatestPnlTicks(_.keys(vaultSubaccounts)), | ||
| ]); | ||
| const latestTicksBySubaccountId: Dictionary<PnlTicksFromDatabase> = _.keyBy( | ||
| latestTicks, | ||
| 'subaccountId', | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for undefined latest ticks
While the Dictionary organization is good, accessing latestTicksBySubaccountId[subaccountId] directly could be unsafe if the subaccountId doesn't exist in the dictionary.
Consider adding a null check or using optional chaining:
- latestTicksBySubaccountId[subaccountId],
+ latestTicksBySubaccountId[subaccountId] ?? undefined,Committable suggestion skipped: line range outside the PR's diff.
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
Changelist
Fetch the latest pnl ticks for all sub-vaults when returning historical pnl.
Test Plan
Unit tests, deployed onto test env.
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