Fix typo and add test for invalid vaults. #2535
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (3)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (2)
Line range hint
642-665: Consider enhancing error handling for invalid vaults.Currently, invalid vaults are silently filtered out. Consider one of these approaches:
- Throw an error if any invalid vaults are found (strict)
- Return both valid and invalid mappings (flexible)
- Add metrics to track invalid vault frequency (monitoring)
Example implementation for approach #2:
interface VaultMappingResult { validVaults: VaultMapping; invalidVaults: { subaccountId: string; clobPairId: string; }[]; } async function getVaultMapping(): Promise<VaultMappingResult> { const vaults: VaultFromDatabase[] = await VaultTable.findAll({}, [], {}); const vaultMapping: VaultMapping = _.zipObject( vaults.map((vault: VaultFromDatabase): string => SubaccountTable.uuid(vault.address, 0)), vaults.map((vault: VaultFromDatabase): string => vault.clobPairId), ); const validVaultMapping: VaultMapping = {}; const invalidVaults: {subaccountId: string; clobPairId: string}[] = []; for (const subaccountId of _.keys(vaultMapping)) { const perpetual = perpetualMarketRefresher.getPerpetualMarketFromClobPairId( vaultMapping[subaccountId], ); if (perpetual === undefined) { logger.warning({ at: 'VaultController#getVaultMapping', message: `Vault clob pair id ${vaultMapping[subaccountId]} does not correspond to a perpetual market.`, subaccountId, }); invalidVaults.push({ subaccountId, clobPairId: vaultMapping[subaccountId], }); continue; } validVaultMapping[subaccountId] = vaultMapping[subaccountId]; } return { validVaults: validVaultMapping, invalidVaults, }; }
Line range hint
642-665: Consider optimizing database query for vault retrieval.The current implementation loads all vaults and then filters them in memory. Consider adding a database-level filter to retrieve only vaults with valid clob pair IDs.
Example optimization:
async function getVaultMapping(): Promise<VaultMapping> { // Get all valid clob pair IDs first const validClobPairIds = new Set( Object.keys(perpetualMarketRefresher.getPerpetualMarketsMap()) .map(id => perpetualMarketRefresher.getPerpetualMarketFromId(id)?.clobPairId) .filter(Boolean) ); // Query only vaults with valid clob pair IDs const vaults: VaultFromDatabase[] = await VaultTable.findAll( { clobPairId: Array.from(validClobPairIds), }, [], {}, ); return _.zipObject( vaults.map((vault: VaultFromDatabase): string => SubaccountTable.uuid(vault.address, 0)), vaults.map((vault: VaultFromDatabase): string => vault.clobPairId), ); }indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (1)
Line range hint
454-470: LGTM! Consider improving the test name grammar.The test effectively validates the handling of invalid vaults while maintaining backward compatibility for valid vaults. The setup and assertions are comprehensive and well-structured.
Consider updating the test name to be more grammatically correct:
- it('Get /megavault/positions with 2 vault subaccount, 1 with no perpetual, 1 invalid', async () => { + it('Get /megavault/positions with 2 vault subaccounts, 1 with no perpetual and 1 invalid', async () => {
📜 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 (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (2)
665-665:⚠️ Potential issueFix incorrect function name in warning message.
The warning message references
VaultController#getVaultPositions, but it should beVaultController#getVaultMappingsince the warning is logged in thegetVaultMappingfunction.Apply this diff:
- at: 'VaultController#getVaultPositions', + at: 'VaultController#getVaultMapping',Likely invalid or redundant comment.
Line range hint
642-665: Verify test coverage for invalid vault handling.Let's ensure the test suite properly covers the invalid vault filtering logic.
✅ Verification successful
Let's examine the test file content to verify the test coverage for invalid vault handling.
Let's check the implementation of the vault controller to find where the invalid vault handling is tested.
Based on the test file content, I can see there is a test case "Get /megavault/positions with 2 vault subaccount, 1 with no perpetual, 1 invalid" that covers the invalid vault scenario. Let's verify the complete test case and its assertions.
Based on the evidence from the test file and implementation, I can now provide a final response about the test coverage for invalid vault handling.
Test coverage for invalid vault handling is sufficient
The code has proper test coverage for invalid vault handling:
There is a specific test case "Get /megavault/positions with 2 vault subaccount, 1 with no perpetual, 1 invalid" that verifies:
- A valid vault with a perpetual position
- A valid vault without a perpetual position
- An invalid vault with non-existent clob pair id ('999')
The implementation in
getVaultMappingproperly handles invalid vaults by:
- Logging a warning message when a vault's clob pair id doesn't correspond to a perpetual market
- Excluding invalid vaults from the
validVaultMappingreturn value- Continuing execution for valid vaults
The test coverage adequately verifies that the invalid vault filtering logic works as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files related to vault-controller echo "Searching for vault controller test files..." fd -e spec.ts -e test.ts vault-controller # Search for test cases covering invalid vault scenarios echo "Searching for invalid vault test cases..." rg -l "invalid.*vault|vault.*invalid" -g "*.spec.ts" -g "*.test.ts" # Search for specific test assertions related to invalid vaults echo "Searching for invalid vault assertions..." rg "expect.*vault.*invalid|invalid.*vault.*expect" -g "*.spec.ts" -g "*.test.ts"Length of output: 639
Script:
#!/bin/bash # Get the content of the vault controller test file echo "Content of vault controller test file:" cat "indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts" # Search for test cases specifically around getVaultMapping echo -e "\nTest cases for getVaultMapping:" rg -A 10 "describe.*getVaultMapping|test.*getVaultMapping" "indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts"Length of output: 24529
Script:
#!/bin/bash # Search for tests related to invalid vaults in the megavault/positions endpoint echo "Searching for invalid vault tests in positions endpoint..." rg -B 5 -A 10 "invalid.*vault|vault.*invalid" "indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts" # Search for the implementation of getVaultMapping echo -e "\nSearching for getVaultMapping implementation..." ast-grep --pattern 'getVaultMapping($$$) { $$$ }' "indexer/services/comlink/src/controllers/api/v4/vault-controller.ts"Length of output: 1049
Script:
#!/bin/bash # Get the complete test case for invalid vault handling echo "Getting complete test case for invalid vault handling..." rg -B 2 -A 50 "2 vault subaccount.*1 invalid" "indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts" # Get the implementation of getVaultPositions echo -e "\nGetting getVaultPositions implementation..." rg -B 10 -A 30 "getVaultPositions" "indexer/services/comlink/src/controllers/api/v4/vault-controller.ts"Length of output: 8524
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit 317a74e)
Co-authored-by: vincentwschau <[email protected]>
Changelist
Had a typo when returning valid/invalid vaults. Updated a test to catch this.
Test Plan
Unit test.
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
Tests