Improve vault endpoint performance. #2475
Conversation
WalkthroughThe changes in this pull request introduce enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
indexer/packages/postgres/__tests__/stores/funding-index-updates-table.test.ts (1)
246-278: LGTM! Comprehensive test case forfindFundingIndexMaps.This new test case thoroughly covers the functionality of the
findFundingIndexMapsmethod, aligning well with the PR's objective of improving vault endpoint performance. It tests multiple scenarios including different block heights, multiple perpetual IDs, and updates at various effective heights.To improve readability, consider extracting the expected values into constants at the beginning of the test case. This would make the assertions more clear and easier to maintain.
Here's a suggestion for improving readability:
const expectedFundingIndices = { '3': { [defaultFundingIndexUpdate.perpetualId]: Big(defaultFundingIndexUpdate.fundingIndex), [fundingIndexUpdates3.perpetualId]: Big(fundingIndexUpdates3.fundingIndex), }, '6': { [defaultFundingIndexUpdate.perpetualId]: Big(fundingIndexUpdates2.fundingIndex), [fundingIndexUpdates3.perpetualId]: Big(fundingIndexUpdates3.fundingIndex), }, }; // ... (rest of the test case) Object.entries(expectedFundingIndices).forEach(([blockHeight, indices]) => { Object.entries(indices).forEach(([perpetualId, expectedIndex]) => { expect(fundingIndexMaps[blockHeight][perpetualId]).toEqual(expectedIndex); }); });This approach centralizes the expected values and makes the assertions more concise and easier to update if needed.
indexer/packages/postgres/src/stores/funding-index-updates-table.ts (2)
243-244: Optimize retrieval ofminHeightCurrently,
minHeightis obtained after sorting the entire array of heights, which has a time complexity of O(n log n). Since only the minimum value is needed, consider usingMath.minto improve performance.Apply this diff to optimize the retrieval:
const heightNumbers: number[] = effectiveBeforeOrAtHeights .map((height: string) => { const parsedHeight = parseInt(height, 10); if (isNaN(parsedHeight)) { throw new Error(`Invalid block height: ${height}`); } return parsedHeight; - }) - .sort(); + }); -// Get the min height to limit the search to blocks 4 hours or before the min height. -const minHeight: number = heightNumbers[0]; +// Get the min height without sorting the entire array. +const minHeight: number = Math.min(...heightNumbers);
283-285: Add a semicolon at the end of the statementIn line 284, the statement is missing a semicolon, which could lead to linting errors or unintended behavior.
Apply this diff to add the missing semicolon:
for (const funding of result.rows) { fundingIndexMaps[funding.searchHeight][funding.perpetualId] = Big(funding.fundingIndex) -} +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- indexer/packages/postgres/tests/stores/funding-index-updates-table.test.ts (1 hunks)
- indexer/packages/postgres/src/stores/funding-index-updates-table.ts (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3 hunks)
🔇 Additional comments (2)
indexer/packages/postgres/__tests__/stores/funding-index-updates-table.test.ts (1)
Line range hint
1-278: Overall, excellent test coverage and alignment with PR objectives.The test suite in this file provides comprehensive coverage for the
FundingIndexUpdatesTable, including various scenarios and edge cases. The newly added test case forfindFundingIndexMapscomplements the existing tests well and directly supports the PR's goal of improving vault endpoint performance through bulk fetching of funding index update maps.The structure of the tests is consistent, making the suite easy to understand and maintain. This thorough testing approach will help ensure the reliability of the new functionality and reduce the risk of regressions.
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1)
378-394: Performance Improvement with Bulk Fetching of Funding Index MapsFetching funding index maps in bulk using
findFundingIndexMapsenhances performance by reducing the number of individual database calls. This optimization is beneficial when dealing with multiple subaccounts and aligns with the PR's objective to improve endpoint performance.
indexer/packages/postgres/src/stores/funding-index-updates-table.ts
Outdated
Show resolved
Hide resolved
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts
Outdated
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- indexer/packages/postgres/src/stores/funding-index-updates-table.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
indexer/packages/postgres/src/stores/funding-index-updates-table.ts (2)
228-289: Approved: Efficient implementation offindFundingIndexMapsThe
findFundingIndexMapsfunction effectively retrieves funding index maps for multiple heights using bulk fetching, which should enhance performance of the vault endpoint as intended. The logic and SQL query appear correct, and the implementation aligns with the PR objectives.
243-243:⚠️ Potential issueEnsure numerical sorting of block heights
The default
Array.prototype.sort()method sorts elements as strings, which can lead to incorrect ordering when dealing with numbers. For instance,[1, 2, 10]would be sorted as[1, 10, 2]. To correctly sort theheightNumbersarray numerically, please provide a compare function.Apply this diff to use a numerical comparator:
- .sort(); + .sort((a, b) => a - b);Likely invalid or redundant comment.
(cherry picked from commit bee57fe)
Co-authored-by: vincentwschau <[email protected]>
Changelist
Fetch funding index update maps in bulk rather than one at a time per vault.
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
getVaultPositionsfunction for improved performance in fetching funding index maps.Documentation
Refactor
VaultControllerto enhance efficiency and readability.