Improve vault start pnl query. #2664
Conversation
WalkthroughThis pull request introduces enhancements to the vault PnL (Profit and Loss) tracking system within the indexer services. Key changes include the addition of a new asynchronous function Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 1
🧹 Nitpick comments (3)
indexer/services/comlink/src/caches/vault-start-pnl.ts (1)
13-21: Clarify the Use of 10-Minute Buffer in PnL Tick RetrievalIn
startVaultStartPnlCache, a 10-minute buffer is added when retrieving the latest PnL tick:getVaultPnlStartDate().plus({ minutes: 10 }). Consider explaining the rationale behind the 10-minute buffer to improve code readability and maintainability. If the buffer duration may change in the future, consider making it a configurable parameter.indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts (1)
66-88: Optimize SQL Query ingetLatestVaultPnlfor PerformanceThe
getLatestVaultPnlfunction retrieves the latest PnL tick persubaccountIdusingDISTINCT ON. While this approach works, it may not be the most efficient with large datasets. Consider indexingsubaccountIdandblockTimecolumns if not already indexed, or refactoring the query to improve performance.indexer/services/comlink/src/lib/helpers.ts (1)
760-763: Add date string validation.The function should validate the date string format from config to prevent runtime errors.
Apply this diff:
export function getVaultPnlStartDate(): DateTime { + const dateStr = config.VAULT_PNL_START_DATE; + const parsedDate = DateTime.fromISO(dateStr); + if (!parsedDate.isValid) { + throw new Error(`Invalid date format in VAULT_PNL_START_DATE: ${dateStr}`); + } - const startDate: DateTime = DateTime.fromISO(config.VAULT_PNL_START_DATE).toUTC(); + const startDate: DateTime = parsedDate.toUTC(); return startDate; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts(1 hunks)indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts(3 hunks)indexer/services/comlink/src/caches/vault-start-pnl.ts(1 hunks)indexer/services/comlink/src/controllers/api/v4/vault-controller.ts(5 hunks)indexer/services/comlink/src/index.ts(2 hunks)indexer/services/comlink/src/lib/helpers.ts(3 hunks)indexer/services/comlink/src/types.ts(2 hunks)
🔇 Additional comments (8)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (4)
43-43: Importing getVaultStartPnl
The import of getVaultStartPnl from vault-start-pnl is correctly added to access the cached vault start PnL data.
352-358: Potential Issue with Empty Start PnL in adjustVaultPnlTicks
In the getVaultSubaccountPnlTicks function, when calling adjustVaultPnlTicks(pnlTicks, getVaultStartPnl()), if getVaultStartPnl() returns an empty array or undefined, this could lead to incorrect adjustments or runtime errors. Verify that adjustVaultPnlTicks can handle empty or missing start PnL data gracefully.
549-555: Consistency in Adjusting Latest PnL Ticks
In getLatestPnlTicks, the adjusted PnL ticks are obtained by calling adjustVaultPnlTicks(latestPnlTicks, getVaultStartPnl()). Ensure that getVaultStartPnl() returns the correct baseline PnL data and that adjustments are applied accurately across all vaults.
Line range hint 562-575: Handling Adjustments in getLatestPnlTick
The function getLatestPnlTick now adjusts PnL ticks using adjustVaultPnlTicks(pnlTicks, getVaultStartPnl()). Confirm that this adjustment is valid for the specific vault subaccounts and that any potential discrepancies are accounted for.
indexer/services/comlink/src/caches/vault-start-pnl.ts (1)
27-33: Restricting Cache Clearing to Test Environments
In clearVaultStartPnl, throwing an error when attempting to clear the vault start PnL cache outside of the test environment is appropriate. Ensure that this function is only invoked in testing contexts, and consider logging the error or providing more detailed feedback.
indexer/services/comlink/src/index.ts (1)
46-47: Initializing Vault Start PnL Cache During Startup
The addition of await startVaultStartPnlCache(); correctly initializes the vault start PnL cache at application startup. This ensures that the cached data is available before handling requests.
indexer/services/comlink/src/types.ts (1)
695-697: LGTM! Well-structured interface definition.
The VaultMapping interface is clear, concise, and follows TypeScript best practices for defining a dictionary type.
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (1)
27-27: LGTM! Proper test setup and teardown.
The changes correctly handle the vault start PnL cache state in tests:
- Cache is cleared in afterEach to prevent test pollution
- Cache is initialized with fresh data in createPnlTicks
Also applies to: 135-135, 658-658
| export async function getVaultMapping(): Promise<VaultMapping> { | ||
| const vaults: VaultFromDatabase[] = await VaultTable.findAll( | ||
| {}, | ||
| [], | ||
| {}, | ||
| ); | ||
| const vaultMapping: VaultMapping = _.zipObject( | ||
| vaults.map((vault: VaultFromDatabase): string => { | ||
| return SubaccountTable.uuid(vault.address, 0); | ||
| }), | ||
| vaults, | ||
| ); | ||
| const validVaultMapping: VaultMapping = {}; | ||
| for (const subaccountId of _.keys(vaultMapping)) { | ||
| const perpetual: PerpetualMarketFromDatabase | undefined = perpetualMarketRefresher | ||
| .getPerpetualMarketFromClobPairId( | ||
| vaultMapping[subaccountId].clobPairId, | ||
| ); | ||
| if (perpetual === undefined) { | ||
| logger.warning({ | ||
| at: 'VaultController#getVaultPositions', | ||
| message: `Vault clob pair id ${vaultMapping[subaccountId]} does not correspond to a ` + | ||
| 'perpetual market.', | ||
| subaccountId, | ||
| }); | ||
| continue; | ||
| } | ||
| validVaultMapping[subaccountId] = vaultMapping[subaccountId]; | ||
| } | ||
| return validVaultMapping; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and fix logging location.
The function should handle potential database errors. Also, there's a typo in the logging location.
Apply this diff:
export async function getVaultMapping(): Promise<VaultMapping> {
- const vaults: VaultFromDatabase[] = await VaultTable.findAll(
- {},
- [],
- {},
- );
+ try {
+ const vaults: VaultFromDatabase[] = await VaultTable.findAll(
+ {},
+ [],
+ {},
+ );
+ const vaultMapping: VaultMapping = _.zipObject(
+ vaults.map((vault: VaultFromDatabase): string => {
+ return SubaccountTable.uuid(vault.address, 0);
+ }),
+ vaults,
+ );
+ const validVaultMapping: VaultMapping = {};
+ for (const subaccountId of _.keys(vaultMapping)) {
+ const perpetual: PerpetualMarketFromDatabase | undefined = perpetualMarketRefresher
+ .getPerpetualMarketFromClobPairId(
+ vaultMapping[subaccountId].clobPairId,
+ );
+ if (perpetual === undefined) {
+ logger.warning({
- at: 'VaultController#getVaultPositions',
+ at: 'helpers#getVaultMapping',
+ message: `Vault clob pair id ${vaultMapping[subaccountId].clobPairId} does not correspond to a perpetual market.`,
+ subaccountId,
+ });
+ continue;
+ }
+ validVaultMapping[subaccountId] = vaultMapping[subaccountId];
+ }
+ return validVaultMapping;
+ } catch (error) {
+ logger.error({
+ at: 'helpers#getVaultMapping',
+ message: 'Failed to get vault mapping',
+ error,
+ });
+ throw error;
+ }
}📝 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.
| export async function getVaultMapping(): Promise<VaultMapping> { | |
| const vaults: VaultFromDatabase[] = await VaultTable.findAll( | |
| {}, | |
| [], | |
| {}, | |
| ); | |
| const vaultMapping: VaultMapping = _.zipObject( | |
| vaults.map((vault: VaultFromDatabase): string => { | |
| return SubaccountTable.uuid(vault.address, 0); | |
| }), | |
| vaults, | |
| ); | |
| const validVaultMapping: VaultMapping = {}; | |
| for (const subaccountId of _.keys(vaultMapping)) { | |
| const perpetual: PerpetualMarketFromDatabase | undefined = perpetualMarketRefresher | |
| .getPerpetualMarketFromClobPairId( | |
| vaultMapping[subaccountId].clobPairId, | |
| ); | |
| if (perpetual === undefined) { | |
| logger.warning({ | |
| at: 'VaultController#getVaultPositions', | |
| message: `Vault clob pair id ${vaultMapping[subaccountId]} does not correspond to a ` + | |
| 'perpetual market.', | |
| subaccountId, | |
| }); | |
| continue; | |
| } | |
| validVaultMapping[subaccountId] = vaultMapping[subaccountId]; | |
| } | |
| return validVaultMapping; | |
| } | |
| export async function getVaultMapping(): Promise<VaultMapping> { | |
| try { | |
| const vaults: VaultFromDatabase[] = await VaultTable.findAll( | |
| {}, | |
| [], | |
| {}, | |
| ); | |
| const vaultMapping: VaultMapping = _.zipObject( | |
| vaults.map((vault: VaultFromDatabase): string => { | |
| return SubaccountTable.uuid(vault.address, 0); | |
| }), | |
| vaults, | |
| ); | |
| const validVaultMapping: VaultMapping = {}; | |
| for (const subaccountId of _.keys(vaultMapping)) { | |
| const perpetual: PerpetualMarketFromDatabase | undefined = perpetualMarketRefresher | |
| .getPerpetualMarketFromClobPairId( | |
| vaultMapping[subaccountId].clobPairId, | |
| ); | |
| if (perpetual === undefined) { | |
| logger.warning({ | |
| at: 'helpers#getVaultMapping', | |
| message: `Vault clob pair id ${vaultMapping[subaccountId].clobPairId} does not correspond to a perpetual market.`, | |
| subaccountId, | |
| }); | |
| continue; | |
| } | |
| validVaultMapping[subaccountId] = vaultMapping[subaccountId]; | |
| } | |
| return validVaultMapping; | |
| } catch (error) { | |
| logger.error({ | |
| at: 'helpers#getVaultMapping', | |
| message: 'Failed to get vault mapping', | |
| error, | |
| }); | |
| throw error; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (2)
561-565: Consider extracting window configurationThe window configuration (
config.VAULT_LATEST_PNL_TICK_WINDOW_HOURS * 60 * 60) is used directly in the query. Consider extracting this to a constant for better maintainability.+const VAULT_LATEST_PNL_TICK_WINDOW_SECONDS = config.VAULT_LATEST_PNL_TICK_WINDOW_HOURS * 60 * 60; + export async function getLatestPnlTick( vaults: VaultFromDatabase[], ): Promise<PnlTicksFromDatabase | undefined> { const pnlTicks: PnlTicksFromDatabase[] = await VaultPnlTicksView.getVaultsPnl( PnlTickInterval.hour, - config.VAULT_LATEST_PNL_TICK_WINDOW_HOURS * 60 * 60, + VAULT_LATEST_PNL_TICK_WINDOW_SECONDS, getVaultPnlStartDate(), );
352-358: Consider consolidating window calculationsSimilar to the previous comment, the window calculations could be extracted and consolidated. This would improve maintainability and ensure consistent window calculations across the codebase.
+const VAULT_PNL_WINDOW_SECONDS = { + [PnlTickInterval.day]: config.VAULT_PNL_HISTORY_DAYS * 24 * 60 * 60, + [PnlTickInterval.hour]: config.VAULT_PNL_HISTORY_HOURS * 60 * 60, +}; + async function getVaultSubaccountPnlTicks( vaultSubaccountIds: string[], resolution: PnlTickInterval, ): Promise<PnlTicksFromDatabase[]> { if (vaultSubaccountIds.length === 0) { return []; } - let windowSeconds: number; - if (resolution === PnlTickInterval.day) { - windowSeconds = config.VAULT_PNL_HISTORY_DAYS * 24 * 60 * 60; - } else { - windowSeconds = config.VAULT_PNL_HISTORY_HOURS * 60 * 60; - } + const windowSeconds = VAULT_PNL_WINDOW_SECONDS[resolution];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts(6 hunks)
🔇 Additional comments (2)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (2)
549-555: Improved efficiency with cached start PnL
The implementation now uses VaultPnlTicksView.getLatestVaultPnl() and adjusts the results using cached start PnL, which aligns with the PR objective of optimizing PnL queries.
Line range hint 749-771: LGTM! Efficient implementation of PnL adjustment
The implementation uses a map-based approach for O(n) complexity and clearly handles the adjustment of PnL ticks. The code is well-structured and maintains good performance characteristics.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
indexer/services/comlink/src/lib/helpers.ts(3 hunks)
🔇 Additional comments (1)
indexer/services/comlink/src/lib/helpers.ts (1)
728-758: 🛠️ Refactor suggestion
Add error handling for database operations.
The function should handle potential database errors and validate inputs.
Apply this diff:
export async function getVaultMapping(): Promise<VaultMapping> {
+ try {
const vaults: VaultFromDatabase[] = await VaultTable.findAll(
{},
[],
{},
);
+ if (!Array.isArray(vaults)) {
+ throw new Error('Expected array of vaults from database');
+ }
const vaultMapping: VaultMapping = _.zipObject(
vaults.map((vault: VaultFromDatabase): string => {
return SubaccountTable.uuid(vault.address, 0);
}),
vaults,
);
const validVaultMapping: VaultMapping = {};
for (const subaccountId of _.keys(vaultMapping)) {
const perpetual: PerpetualMarketFromDatabase | undefined = perpetualMarketRefresher
.getPerpetualMarketFromClobPairId(
vaultMapping[subaccountId].clobPairId,
);
if (perpetual === undefined) {
logger.warning({
at: 'get-vault-mapping',
- message: `Vault clob pair id ${vaultMapping[subaccountId]} does not correspond to a ` +
- 'perpetual market.',
+ message: `Vault clob pair id ${vaultMapping[subaccountId].clobPairId} does not correspond to a perpetual market.`,
subaccountId,
});
continue;
}
validVaultMapping[subaccountId] = vaultMapping[subaccountId];
}
return validVaultMapping;
+ } catch (error) {
+ logger.error({
+ at: 'get-vault-mapping',
+ message: 'Failed to get vault mapping',
+ error,
+ });
+ throw error;
+ }
}Likely invalid or redundant comment.
| export function getVaultPnlStartDate(): DateTime { | ||
| const startDate: DateTime = DateTime.fromISO(config.VAULT_PNL_START_DATE).toUTC(); | ||
| return startDate; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for config value and date format.
The function should validate the config value exists and handle invalid date formats.
Apply this diff:
export function getVaultPnlStartDate(): DateTime {
+ if (!config.VAULT_PNL_START_DATE) {
+ throw new Error('VAULT_PNL_START_DATE is not configured');
+ }
const startDate: DateTime = DateTime.fromISO(config.VAULT_PNL_START_DATE).toUTC();
+ if (!startDate.isValid) {
+ throw new Error(`Invalid date format in VAULT_PNL_START_DATE: ${config.VAULT_PNL_START_DATE}`);
+ }
return startDate;
}📝 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.
| export function getVaultPnlStartDate(): DateTime { | |
| const startDate: DateTime = DateTime.fromISO(config.VAULT_PNL_START_DATE).toUTC(); | |
| return startDate; | |
| } | |
| export function getVaultPnlStartDate(): DateTime { | |
| if (!config.VAULT_PNL_START_DATE) { | |
| throw new Error('VAULT_PNL_START_DATE is not configured'); | |
| } | |
| const startDate: DateTime = DateTime.fromISO(config.VAULT_PNL_START_DATE).toUTC(); | |
| if (!startDate.isValid) { | |
| throw new Error(`Invalid date format in VAULT_PNL_START_DATE: ${config.VAULT_PNL_START_DATE}`); | |
| } | |
| return startDate; | |
| } |
(cherry picked from commit 81ec99c)
|
@Mergifyio backport release/indexer/v8.x |
❌ Command disallowed due to command restrictions in the Mergify configuration.
|
|
@Mergifyio backport release/indexer/v8.x |
✅ Backports have been created
|
(cherry picked from commit 81ec99c)
Changelist
Only fetch the starting vault pnl rows once when starting the service as this never changes. Use the hourly view to get the latest PnL ticks rather than the table as it contains less data.
Test Plan
Unit tests and deployed to a testing 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
Release Notes
New Features
Bug Fixes
Refactor
Documentation