Use vault table rather than placeholder config flags to fetch vaults. #2364
Conversation
WalkthroughThe changes involve the removal of configuration properties related to vaults and their markets, transitioning from a configuration-based approach to a database-driven method for managing vault subaccount mappings. Tests have been updated to create necessary vault entries directly in the database instead of relying on configurations. Additionally, several methods in the vault controller have been modified to utilize the new asynchronous function for retrieving vault mappings. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (5)
117-121: LGTM: Test setup now uses VaultTable.create() instead of config flags.This change aligns with the PR objective of using the vault table rather than placeholder config flags. It provides a more accurate representation of the database state for testing.
Consider extracting the vault creation into a separate helper function to improve test readability and reusability, especially if this pattern is repeated in other tests.
155-166: LGTM: Test setup for multiple vaults uses VaultTable.create().The change to use
VaultTable.create()for setting up multiple vaults is consistent with the PR objective and previous modifications. The use ofPromise.all()for creating multiple vaults concurrently is a good practice for efficiency.Consider extracting the vault creation logic into a helper function to reduce duplication and improve maintainability, especially if this pattern is used in multiple tests.
261-272: LGTM: Consistent use of VaultTable.create() for multiple vaults.The change to use
VaultTable.create()for setting up multiple vaults is consistent with previous modifications and the PR objective. The continued use ofPromise.all()for creating multiple vaults concurrently is good for efficiency.To improve consistency and reduce code duplication across all test cases, consider creating a helper function for vault creation. This function could accept an array of vault configurations and use
Promise.all()to create them. This would make the test setup more concise and easier to maintain.
332-336: LGTM: Consistent use of VaultTable.create() for test setup.The modification to use
VaultTable.create()for setting up the test data is consistent with previous changes and the PR objective. This approach provides a more accurate representation of the database state for testing.For consistency with the previous test cases and to improve code reusability, consider using the helper function suggested earlier for vault creation. This would make the test setup more uniform across all test cases.
Line range hint
1-500: Overall LGTM with suggestions for improvementThe changes in this file consistently implement the PR objective of using the vault table instead of placeholder config flags. All test cases have been updated to use
VaultTable.create()for setting up test data, which provides a more accurate representation of the database state for testing.To further improve the code:
- Create a helper function for vault creation that can be reused across all test cases. This function could accept an array of vault configurations and use
Promise.all()to create them concurrently.- Update all test cases to use this helper function, which will make the test setup more concise, consistent, and easier to maintain.
These refactoring steps will enhance code reusability and maintainability without changing the functionality or intent of the tests.
📜 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 (7 hunks)
- indexer/services/comlink/src/config.ts (0 hunks)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (8 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- indexer/services/comlink/src/config.ts
🔇 Additional comments (2)
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (2)
15-15: LGTM: Import of VaultTable is appropriate for the changes.The addition of
VaultTableto the imports is consistent with the new approach of directly creating vault entries in the database for test setup, aligning with the PR's objective of using the vault table instead of placeholder config flags.
220-224: LGTM: Consistent use of VaultTable.create() for test setup.The modification to use
VaultTable.create()for setting up the test data is consistent with previous changes and the PR objective. This approach provides a more accurate representation of the database state for testing.
| async function getVaultMapping(): Promise<VaultMapping> { | ||
| const vaults: VaultFromDatabase[] = await VaultTable.findAll( | ||
| {}, | ||
| [], | ||
| {} | ||
| ); | ||
| return _.zipObject( | ||
| vaults.map((vault: VaultFromDatabase): string => { | ||
| return SubaccountTable.uuid(vault.address, 0); | ||
| }), | ||
| vaults.map((vault: VaultFromDatabase): string => { | ||
| return vault.clobPairId; | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize database query in getVaultMapping()
In the getVaultMapping() function, consider specifying the fields to retrieve in VaultTable.findAll() to limit the data fetched from the database to only the necessary fields (address, clobPairId). This can improve performance by reducing unnecessary data transfer.
Apply this diff to select only the required fields:
async function getVaultMapping(): Promise<VaultMapping> {
const vaults: VaultFromDatabase[] = await VaultTable.findAll(
{},
- [],
+ ['address', 'clobPairId'],
{}
);
return _.zipObject(
vaults.map((vault: VaultFromDatabase): string => {
return SubaccountTable.uuid(vault.address, 0);
}),
vaults.map((vault: VaultFromDatabase): string => {
return vault.clobPairId;
}),
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function getVaultMapping(): Promise<VaultMapping> { | |
| const vaults: VaultFromDatabase[] = await VaultTable.findAll( | |
| {}, | |
| [], | |
| {} | |
| ); | |
| return _.zipObject( | |
| vaults.map((vault: VaultFromDatabase): string => { | |
| return SubaccountTable.uuid(vault.address, 0); | |
| }), | |
| vaults.map((vault: VaultFromDatabase): string => { | |
| return vault.clobPairId; | |
| }), | |
| async function getVaultMapping(): Promise<VaultMapping> { | |
| const vaults: VaultFromDatabase[] = await VaultTable.findAll( | |
| {}, | |
| ['address', 'clobPairId'], | |
| {} | |
| ); | |
| return _.zipObject( | |
| vaults.map((vault: VaultFromDatabase): string => { | |
| return SubaccountTable.uuid(vault.address, 0); | |
| }), | |
| vaults.map((vault: VaultFromDatabase): string => { | |
| return vault.clobPairId; | |
| }), |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (2)
Line range hint
72-82: LGTM: Updated to use database-driven vault mappingThe changes align with the PR objective of fetching vaults from the database instead of using placeholder config flags. The modification to
getVaultSubaccountPnlTickscall is consistent with its updated function signature.Consider caching the result of
getVaultMapping()to improve performance, as it's called multiple times in different methods (e.g., lines 72, 114, 166). If the vault mappings don't change frequently during the lifecycle of a request, you could cache the result at the class level or use a memoization technique.
458-471: LGTM with suggestions: New function to fetch vault mapping from databaseThe new
getVaultMappingfunction correctly implements fetching vault data from the database and creating the required mapping. This aligns with the PR objective.
- Consider optimizing the database query by specifying only the required fields:
const vaults: VaultFromDatabase[] = await VaultTable.findAll( {}, - [], + ['address', 'clobPairId'], {}, );
- Add error handling for the database query:
try { const vaults: VaultFromDatabase[] = await VaultTable.findAll(...); // ... rest of the function } catch (error) { console.error('Error fetching vault data:', error); throw new Error('Failed to fetch vault mapping from database'); }
📜 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 (8 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 (3)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3)
25-26: LGTM: New imports for database interactionThe addition of
VaultTableandVaultFromDatabaseimports aligns with the PR objective of fetching vaults from the database instead of using placeholder config flags.
Line range hint
114-124: LGTM: Consistent update to use database-driven vault mappingThe changes in this method are consistent with those in
getMegavaultHistoricalPnl, correctly implementing the transition to database-driven vault mapping.
166-166: LGTM: Consistent update to use database-driven vault mappingThe change in this method is consistent with the updates in other methods, correctly implementing the transition to database-driven vault mapping.
… (backport #2364) (#2367) Co-authored-by: vincentwschau <[email protected]>
Changelist
Fetch vaults from the database rather than using the placeholder flags.
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