Use materialized view for vault pnl data. #2598
Conversation
WalkthroughThis pull request introduces a comprehensive testing framework for the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (12)
indexer/services/roundtable/src/tasks/refresh-vault-pnl.ts (1)
30-44: Refactor to reduce duplication and improve consistency.The daily view refresh implementation duplicates logic from the hourly view refresh and has inconsistent variable naming.
Consider extracting a common function for both refresh operations:
+async function refreshView( + viewType: 'hourly' | 'daily', + currentTime: DateTime, +): Promise<void> { + const timeUnit = viewType === 'hourly' ? 'hour' : 'day'; + const timeFromStart = currentTime.diff( + currentTime.startOf(timeUnit) + ).toMillis(); + + if (timeFromStart < config.TIME_WINDOW_FOR_REFRESH_MS) { + logger.info({ + at: 'refresh-vault-pnl#runTask', + message: `Refreshing vault ${viewType} pnl view`, + currentTime, + timeFromStart, + }); + + const refreshStart: number = Date.now(); + try { + await (viewType === 'hourly' + ? VaultPnlTicksView.refreshHourlyView() + : VaultPnlTicksView.refreshDailyView()); + logger.info({ + at: 'refresh-vault-pnl#runTask', + message: `Successfully refreshed ${viewType} view`, + }); + } catch (error) { + logger.error({ + at: 'refresh-vault-pnl#runTask', + message: `Failed to refresh ${viewType} view`, + error, + }); + throw error; + } + stats.timing( + `${config.SERVICE_NAME}.refresh-vault-pnl.${viewType}-view.timing`, + Date.now() - refreshStart, + ); + } +} export default async function runTask(): Promise<void> { const currentTime: DateTime = DateTime.utc(); - // ... existing hourly and daily refresh code ... + await refreshView('hourly', currentTime); + await refreshView('daily', currentTime); }indexer/packages/postgres/src/db/migrations/migration_files/20241119163402_create_vault_daily_view.ts (1)
45-47: Enhance down migration to clean up all artifactsThe down migration should clean up all created database objects, including the index.
Consider this implementation:
export async function down(knex: Knex): Promise<void> { + return knex.transaction(async (trx) => { + try { + // Drop the index first to avoid dependency issues + await knex.raw('DROP INDEX IF EXISTS vaults_daily_pnl_id_idx;'); await knex.raw('DROP MATERIALIZED VIEW IF EXISTS vaults_daily_pnl;'); + } catch (error) { + console.error('Rollback failed:', error); + throw error; + } + }); }indexer/packages/postgres/src/db/migrations/migration_files/20241119162238_create_vault_hourly_view.ts (3)
35-35: Consider making the time window configurable.The 7-day window (604800 seconds) is hardcoded. Consider making this configurable to allow for different retention periods based on environment needs.
3-38: Consider performance optimizations for the materialized view.A few suggestions to optimize the view:
- Add an index on
blockTimeto improve the window function performance- Consider adding indexes on frequently queried columns
- Define a concurrent refresh strategy to avoid blocking reads
Example index creation:
CREATE INDEX ON vaults_hourly_pnl (blockTime); CREATE INDEX ON vaults_hourly_pnl (subaccountId, blockTime);
40-43: Consider adding error handling and logging.While the migration functions are well-structured, they could benefit from try-catch blocks and logging for better operational visibility and error recovery.
export async function up(knex: Knex): Promise<void> { + try { await knex.raw(RAW_VAULTS_PNL_HOURLY_QUERY); await knex.raw(`CREATE UNIQUE INDEX ON vaults_hourly_pnl (id);`) + console.log('Successfully created vaults_hourly_pnl materialized view'); + } catch (error) { + console.error('Failed to create materialized view:', error); + throw error; + } }indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts (2)
14-21: Consider consolidating the refresh functions to reduce duplication.While the current implementation correctly uses CONCURRENT refresh and proper write connection, there's unnecessary code duplication between the two refresh functions.
Consider consolidating into a single function:
-export async function refreshHourlyView(): Promise<void> { - return await rawQuery( - `REFRESH MATERIALIZED VIEW CONCURRENTLY ${VAULT_HOURLY_PNL_VIEW}`, - { - readReplica: false, - } - ); -} - -export async function refreshDailyView(): Promise<void> { - return await rawQuery( - `REFRESH MATERIALIZED VIEW CONCURRENTLY ${VAULT_DAILY_PNL_VIEW}`, - { - readReplica: false, - } - ); -} +export async function refreshView(interval: PnlTickInterval): Promise<void> { + const viewName = interval === PnlTickInterval.hour + ? VAULT_HOURLY_PNL_VIEW + : VAULT_DAILY_PNL_VIEW; + return await rawQuery( + `REFRESH MATERIALIZED VIEW CONCURRENTLY ${viewName}`, + { + readReplica: false, + } + ); +}Also applies to: 23-30
32-65: Consider adding a caching layer for better performance.Since the materialized views are only refreshed every 5 minutes, implementing a caching layer (e.g., Redis) with a TTL matching the refresh interval could further reduce database load and improve response times.
Example implementation approach:
- Check cache first
- If cache miss, query database
- Store results in cache with 5-minute TTL
- Return results
Would you like me to provide a detailed implementation example?
indexer/packages/postgres/__tests__/stores/vault-pnl-ticks-view.test.ts (3)
23-50: Consider adding error handling to database operations.The test setup is well-structured with proper isolation between tests. However, consider adding try-catch blocks around database operations in the lifecycle hooks to handle potential database errors gracefully.
beforeEach(async () => { + try { await seedData(); await WalletTable.create(defaultWallet2); await SubaccountTable.create(defaultSubaccountWithAlternateAddress); await Promise.all([ VaultTable.create({ ...defaultVault, address: defaultSubaccount.address, }), VaultTable.create({ ...defaultVault, address: defaultSubaccountWithAlternateAddress.address, }), ]); + } catch (error) { + console.error('Failed to setup test data:', error); + throw error; + } });
52-93: Consider adding more test cases for edge scenarios.While the current test cases cover the basic functionality, consider adding tests for:
- Empty result sets
- Edge cases around interval boundaries
- Error cases (e.g., invalid intervals)
- Cases with missing block data
Example test case for empty results:
it('should return empty array when no data exists', async () => { await clearData(); // Clear all data await VaultPnlTicksView.refreshDailyView(); await VaultPnlTicksView.refreshHourlyView(); const pnlTicks = await VaultPnlTicksView.getVaultsPnl( PnlTickInterval.hour, 7 * 24 * 60 * 60, DateTime.utc(), ); expect(pnlTicks).toEqual([]); });
95-208: Improve maintainability of the test data setup.Consider the following improvements to make the setup function more maintainable:
- Extract magic numbers into named constants
- Use TypeScript enums or const objects for block heights
- Create a type-safe factory function for PnL tick creation
Example refactor:
const TIME_OFFSETS = { TEN_MINUTES: { minute: 10 }, TWO_HOURS: { hour: 2 }, TWO_DAYS: { day: 2 }, ONE_MONTH: { day: 30 }, } as const; const BLOCK_HEIGHTS = { MONTH_AGO: '3', TWO_DAYS_AGO: '4', TWO_HOURS_AGO: '6', TEN_MIN_AGO: '8', ALMOST_TEN_MIN_AGO: '10', } as const; interface PnlTickCreationParams { subaccountId: string; equity: string; blockHeight: string; blockTime: string; totalPnl: string; netTransfers?: string; } function createPnlTick(params: PnlTickCreationParams): Partial<PnlTicksFromDatabase> { return { ...params, createdAt: params.blockTime, netTransfers: params.netTransfers ?? '50', }; }indexer/services/roundtable/src/config.ts (1)
222-224: Consider grouping related configurationThe
TIME_WINDOW_FOR_REFRESH_MSconfig should be grouped with other vault PnL related configurations for better maintainability. Consider moving it nearLOOPS_INTERVAL_MS_REFRESH_VAULT_PNL.LOOPS_INTERVAL_MS_REFRESH_VAULT_PNL: parseInteger({ default: 5 * ONE_MINUTE_IN_MILLISECONDS, }), + + // Refresh vault pnl view + TIME_WINDOW_FOR_REFRESH_MS: parseInteger({ default: 15 * ONE_MINUTE_IN_MILLISECONDS }), // Start delay START_DELAY_ENABLED: parseBoolean({ default: true }), - - // Refresh vault pnl view - TIME_WINDOW_FOR_REFRESH_MS: parseInteger({ default: 15 * ONE_MINUTE_IN_MILLISECONDS }),indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (1)
Line range hint
1-661: Consider adding tests for edge cases related to materialized views.While the test coverage is comprehensive for the happy path and basic error cases, consider adding tests for:
- Behavior when view refresh fails
- Concurrent view refresh scenarios
- Performance with large datasets
Example test case structure:
it('handles view refresh failures gracefully', async () => { // Mock view refresh to fail jest.spyOn(VaultPnlTicksView, 'refreshDailyView').mockRejectedValueOnce(new Error('Refresh failed')); // Test that the system handles the failure appropriately await expect(createPnlTicks()).rejects.toThrow('Refresh failed'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
indexer/packages/postgres/__tests__/stores/vault-pnl-ticks-view.test.ts(1 hunks)indexer/packages/postgres/src/db/migrations/migration_files/20241119162238_create_vault_hourly_view.ts(1 hunks)indexer/packages/postgres/src/db/migrations/migration_files/20241119163402_create_vault_daily_view.ts(1 hunks)indexer/packages/postgres/src/index.ts(1 hunks)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/controllers/api/v4/vault-controller.ts(3 hunks)indexer/services/roundtable/src/config.ts(3 hunks)indexer/services/roundtable/src/tasks/refresh-vault-pnl.ts(1 hunks)
🔇 Additional comments (16)
indexer/services/roundtable/src/tasks/refresh-vault-pnl.ts (1)
11-45: Verify refresh interval configuration and concurrent execution handling.
Please ensure:
- The refresh interval configuration (
LOOPS_INTERVAL_MS_REFRESH_VAULT_PNL) aligns with theTIME_WINDOW_FOR_REFRESH_MSto avoid missing refresh windows. - The materialized view refresh operations handle concurrent executions gracefully.
Let's verify the configuration and implementation:
✅ Verification successful
Based on the code analysis, I can now generate the final response:
Configuration and concurrent execution are properly handled
-
The refresh interval configuration is correctly set:
LOOPS_INTERVAL_MS_REFRESH_VAULT_PNLis set to 5 minutes by defaultTIME_WINDOW_FOR_REFRESH_MSis set to 15 minutes by default- This provides a sufficient window to avoid missing refreshes
-
Concurrent execution is properly handled:
- The materialized views are refreshed using
REFRESH MATERIALIZED VIEW CONCURRENTLY - Both hourly and daily views use concurrent refresh to prevent blocking reads
- The materialized views are refreshed using
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check refresh interval configuration and concurrent execution handling
# Check refresh interval configuration
echo "Checking refresh interval configuration..."
rg -A 5 "LOOPS_INTERVAL_MS_REFRESH_VAULT_PNL|TIME_WINDOW_FOR_REFRESH_MS" --type ts
# Check if materialized view refresh uses CONCURRENTLY option
echo "Checking materialized view refresh implementation..."
rg -A 10 "REFRESH MATERIALIZED VIEW" --type sql
Length of output: 2177
Script:
#!/bin/bash
# Let's check the materialized view implementation in VaultPnlTicksView
echo "Checking VaultPnlTicksView implementation..."
rg -p "class VaultPnlTicksView" -A 20 --type ts
# Check SQL queries for refresh operations
echo "Checking SQL queries for materialized view refresh..."
fd -e sql . | xargs cat
# Check if there are any locks or transaction handling
echo "Checking transaction and lock handling..."
rg -p "BEGIN|LOCK" --type sql
Length of output: 101158
Script:
#!/bin/bash
# Let's check the VaultPnlTicksView implementation specifically
echo "Checking VaultPnlTicksView implementation..."
rg -p "VaultPnlTicksView" -A 30 --type ts
# Check materialized view refresh queries
echo "Checking materialized view refresh queries..."
fd -e sql -e ts -g "*vault*pnl*" | xargs cat
Length of output: 21182
indexer/packages/postgres/src/db/migrations/migration_files/20241119163402_create_vault_daily_view.ts (1)
3-38: Verify the impact on database performance
The materialized view creation might impact database performance during refresh operations. Let's verify the potential impact:
✅ Verification successful
Materialized view implementation follows best practices for performance
The implementation shows careful consideration for performance:
- Uses
REFRESH MATERIALIZED VIEW CONCURRENTLYin vault-pnl-ticks-view.ts, allowing reads during refresh - Time-bounds data with a 90-day window (
7776000 seconds), limiting data volume - Shares similar structure with hourly view, suggesting consistent query patterns
- Includes only necessary columns and appropriate filtering in the base query
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing indexes and table sizes
# Look for similar materialized views or tables
rg -l "CREATE MATERIALIZED VIEW" --type ts
# Check for other PnL-related queries that might be affected
rg "FROM\s+pnl_ticks" --type ts -A 5
# Look for refresh schedules or cron jobs
rg "REFRESH MATERIALIZED VIEW" --type ts
Length of output: 3078
indexer/packages/postgres/src/db/migrations/migration_files/20241119162238_create_vault_hourly_view.ts (1)
3-38: Verify refresh strategy implementation.
The PR objectives mention refreshing the view every 5 minutes during the first 15 minutes of each hour/day, but this refresh strategy isn't defined in the migration. Ensure this is implemented in the application layer.
✅ Verification successful
Refresh strategy is properly implemented
The refresh strategy is correctly implemented in the codebase:
- The materialized view refresh is executed every 5 minutes (LOOPS_INTERVAL_MS_REFRESH_VAULT_PNL = 5 minutes)
- The refresh only occurs within the first 15 minutes of each hour (TIME_WINDOW_FOR_REFRESH_MS = 15 minutes)
- The refresh is performed concurrently to avoid blocking reads
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for refresh implementation in the codebase
rg -l "vaults_hourly_pnl" | xargs rg "REFRESH MATERIALIZED VIEW"
Length of output: 327
Script:
#!/bin/bash
# Check the refresh implementation details in vault-pnl-ticks-view.ts
cat indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts
Length of output: 1674
Script:
#!/bin/bash
# Search for scheduler or cron implementation that calls the refresh functions
rg -l "refreshHourlyView" | xargs rg -A 10 "refreshHourlyView"
Length of output: 5135
Script:
#!/bin/bash
# Check the refresh task implementation in roundtable service
cat indexer/services/roundtable/src/tasks/refresh-vault-pnl.ts
Length of output: 1381
Script:
#!/bin/bash
# Check the configuration for refresh window
cat indexer/services/roundtable/src/config.ts
Length of output: 9588
indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts (2)
1-12: LGTM! Imports and constants are well-organized.
The imports are appropriate for the functionality, and view names are properly defined as constants.
11-12: Verify materialized view performance and indexing.
Let's ensure the materialized views are properly indexed for optimal query performance.
✅ Verification successful
Let me generate another verification script to check for refresh strategies and view definitions since we need to examine the materialized view performance more thoroughly.
This will help us understand:
- How the views are refreshed
- Complete view definitions to analyze query complexity
- Any scheduled refresh mechanisms
Let me try a different approach to find the view definitions and refresh mechanisms.
This broader search should help us locate:
- Migration files containing view definitions
- Any TypeScript code handling view refreshes
- Task scheduler implementations that might handle view maintenance
Let me check the specific content of these identified files to understand the view implementations and refresh mechanisms.
This will show us:
- The view store implementation which likely contains refresh logic
- The complete view definitions from migrations
- Any refresh strategies defined in the migrations
Materialized views are properly optimized for performance
The implementation shows good performance optimization practices:
- Concurrent refresh support using
REFRESH MATERIALIZED VIEW CONCURRENTLY - Unique indexes on
idcolumn for both views - Efficient time-based filtering with
blockTimewindow conditions - Row partitioning with
ROW_NUMBER()to get single records per time interval - Views are scoped to vault subaccounts only
- Time windows are appropriately sized (7 days for hourly, 90 days for daily)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check materialized view definitions and indexes
# Check materialized view definitions
echo "Checking materialized view definitions..."
rg -A 20 "CREATE MATERIALIZED VIEW.*${VAULT_HOURLY_PNL_VIEW}"
rg -A 20 "CREATE MATERIALIZED VIEW.*${VAULT_DAILY_PNL_VIEW}"
# Check indexes on the views
echo "Checking indexes..."
rg -A 5 "CREATE INDEX.*${VAULT_HOURLY_PNL_VIEW}"
rg -A 5 "CREATE INDEX.*${VAULT_DAILY_PNL_VIEW}"
Length of output: 41719
Script:
#!/bin/bash
# Check for materialized view refresh strategies and definitions
# Search for refresh related configurations
echo "Checking refresh configurations..."
rg -A 10 "REFRESH MATERIALIZED VIEW.*vaults_(hourly|daily)_pnl"
# Search for view definitions and dependencies
echo "Checking view definitions..."
rg -A 30 "CREATE MATERIALIZED VIEW.*vaults_(hourly|daily)_pnl" --type sql
# Check for any scheduled tasks or triggers
echo "Checking for scheduled refreshes..."
rg -A 10 "RefreshMaterializedViewTask|refreshMaterializedView"
Length of output: 433
Script:
#!/bin/bash
# Search for materialized view related code with broader patterns
echo "Checking view definitions in migrations..."
fd -e ts -e sql . indexer/packages/postgres/src/db/migrations | xargs rg -l "vaults_.*_pnl"
echo "Checking for refresh mechanisms in TypeScript files..."
fd -e ts . indexer/packages/postgres/src | xargs rg -l "refresh.*view|materialized.*view" --ignore-case
echo "Checking task scheduler implementations..."
fd -e ts . indexer/packages/postgres/src | xargs rg -A 5 "class.*Task" --ignore-case
Length of output: 1066
Script:
#!/bin/bash
echo "Checking vault-pnl-ticks-view.ts implementation..."
cat indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts
echo -e "\nChecking hourly view migration..."
cat indexer/packages/postgres/src/db/migrations/migration_files/20241119162238_create_vault_hourly_view.ts
echo -e "\nChecking daily view migration..."
cat indexer/packages/postgres/src/db/migrations/migration_files/20241119163402_create_vault_daily_view.ts
Length of output: 5337
indexer/packages/postgres/src/index.ts (2)
54-54: LGTM! Export follows established patterns
The new export for VaultPnlTicksView follows the consistent naming and organization patterns used throughout the file, and aligns with the PR objective of implementing materialized views for vault PnL data.
54-54: Verify the existence of the vault-pnl-ticks-view module
Let's ensure the referenced module exists to prevent potential runtime errors.
✅ Verification successful
The vault-pnl-ticks-view module exists and exports the expected functions
The verification confirms that:
- The file exists at the expected location:
indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts - The module exports three key functions:
refreshHourlyViewrefreshDailyViewgetVaultsPnl
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the vault-pnl-ticks-view module
# and its exported functions mentioned in the AI summary
# Check if the file exists
fd -t f "vault-pnl-ticks-view.ts" indexer/packages/postgres/src/stores
# Verify the module exports the expected functions
rg -A 5 "export (async )?function (refreshHourlyView|refreshDailyView|getVaultsPnl)" indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts
Length of output: 873
indexer/packages/postgres/__tests__/stores/vault-pnl-ticks-view.test.ts (1)
1-22: LGTM!
The imports are well-organized and include all necessary dependencies for the test suite.
indexer/services/roundtable/src/config.ts (2)
63-63: LGTM: Loop enablement flag follows existing patterns
The LOOPS_ENABLED_REFRESH_VAULT_PNL flag follows the established pattern of other loop enablement flags and has a sensible default value.
144-146: Verify refresh interval timing
The 5-minute refresh interval might be too frequent since PnL ticks are generated hourly (as mentioned in PR objectives). Consider if this could lead to unnecessary database load.
Let's check the PnL tick generation interval in the codebase:
Consider adjusting the refresh interval based on:
- The actual PnL tick generation frequency
- The trade-off between data freshness and database load
✅ Verification successful
Let me gather more information about the vault PnL refresh implementation to understand its relationship with PnL ticks.
Let me search for the actual implementation of vault PnL refresh to understand its purpose and dependencies.
Let me search for the actual implementation of the refresh vault PnL task to understand how the data is used.
Let me check the task implementation in the roundtable service.
Based on the gathered information, I can now provide a final response:
5-minute refresh interval for vault PnL materialized views is appropriate
The 5-minute refresh interval for vault PnL views is justified because:
- The materialized views (
vaults_hourly_pnlandvaults_daily_pnl) are used to serve historical PnL data through API endpoints - While PnL ticks are generated hourly (
PNL_TICK_UPDATE_INTERVAL_MS: ONE_HOUR_IN_MILLISECONDS), the views need to be refreshed more frequently to ensure timely data availability for API consumers - The refresh operation uses
REFRESH MATERIALIZED VIEW CONCURRENTLYwhich doesn't block reads, minimizing database impact - The views are used by
/vault/v1/megavault/historicalPnland/vault/v1/vaults/historicalPnlendpoints which need fresh data for UI display
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PnL tick generation interval
# Look for PNL_TICK_UPDATE_INTERVAL_MS usage and other timing configurations
rg -A 2 "PNL_TICK_UPDATE_INTERVAL_MS"
Length of output: 5749
Script:
#!/bin/bash
# Search for vault PnL refresh implementation and its dependencies
rg -A 5 "LOOPS_INTERVAL_MS_REFRESH_VAULT_PNL"
# Also search for any vault PnL related code
ast-grep --pattern 'function $FUNC_NAME($$$) {
$$$
refreshVaultPnl
$$$
}'
Length of output: 558
Script:
#!/bin/bash
# Search for vault PnL refresh implementation
rg -A 10 "refreshVaultPnl"
# Search for any loops or tasks related to vault PnL
rg -A 5 "vault.*pnl" -i
Length of output: 70945
Script:
#!/bin/bash
# Search for the refresh vault PnL task implementation
rg -A 10 "refresh-vault-pnl" src/tasks/
# Also search for any materialized view refresh scheduling
rg -A 5 "REFRESH MATERIALIZED VIEW"
Length of output: 1094
Script:
#!/bin/bash
# Search for refresh vault PnL task in roundtable service
rg -A 15 "refresh-vault-pnl" indexer/services/roundtable/src/
# Also search for any PNL tick generation related code
rg -A 10 "PNL_TICK_UPDATE_INTERVAL_MS" indexer/services/roundtable/src/
Length of output: 8664
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3)
32-32: LGTM: Import of VaultPnlTicksView
The addition of VaultPnlTicksView import aligns with the PR objective to implement materialized views for vault PnL data.
562-565: LGTM: Consistent use of materialized view for latest PnL tick
The implementation correctly uses the materialized view with appropriate interval and window configuration.
Verify the configuration for latest PnL tick window:
✅ Verification successful
Configuration for latest PnL tick window is properly defined
The configuration VAULT_LATEST_PNL_TICK_WINDOW_HOURS is properly defined in the config schema with a default value of 1 hour, which is a reasonable window size for latest PnL tick calculation. The test files demonstrate that this configuration can be overridden when needed (e.g., set to 48 hours in tests).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the configuration for PnL tick windows
rg "VAULT_LATEST_PNL_TICK_WINDOW_HOURS.*=.*"
Length of output: 335
Script:
#!/bin/bash
# Check the configuration file for PNL tick window definition
fd "config.ts|config.js" indexer/services/comlink/ --exec cat {}
Length of output: 3482
347-350: LGTM: Migration to materialized view for PnL data
The implementation correctly uses the new materialized view while maintaining the adjustment logic. This change should improve query performance as intended.
Verify that the materialized view refresh timing aligns with the PnL tick generation:
✅ Verification successful
LGTM: Materialized view refresh is properly configured
The materialized views for vault PnL data are refreshed through a dedicated service (roundtable) with appropriate timing:
- Hourly view refresh interval: 5 minutes (LOOPS_INTERVAL_MS_REFRESH_VAULT_PNL)
- Views are refreshed concurrently to avoid blocking reads
- Both hourly and daily views are refreshed within a 15-minute window (TIME_WINDOW_FOR_REFRESH_MS)
- Views have unique indices for efficient querying
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the materialized view refresh configuration
rg -A 5 "CREATE MATERIALIZED VIEW.*vault_pnl"
Length of output: 47
Script:
#!/bin/bash
# Let's try searching for SQL files and view definitions with different patterns
fd -e sql | xargs rg -l "vault.*pnl"
# Also search for view definitions in TypeScript files
rg -g "*.ts" "class.*VaultPnlTicksView" -A 10
Length of output: 86
Script:
#!/bin/bash
# Let's try a broader search for any PnL view or materialized view related code
rg -g "*.{ts,sql}" -i "materialized.*view|pnl.*view|vault.*pnl" -B 2 -A 2
# Also search for the specific class implementation
ast-grep --pattern 'class VaultPnlTicksView {
$$$
getVaultsPnl($_) {
$$$
}
$$$
}'
Length of output: 51064
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (3)
19-19: LGTM!
The import of VaultPnlTicksView is correctly placed and necessary for the materialized view functionality.
131-132: LGTM! Good test cleanup practice.
The refresh calls for both daily and hourly views in the afterEach block ensure proper test isolation and data consistency between test runs.
658-659: LGTM! Proper view refresh timing.
The refresh calls are correctly placed at the end of createPnlTicks to ensure the materialized views reflect all the created test data.
...ackages/postgres/src/db/migrations/migration_files/20241119163402_create_vault_daily_view.ts
Show resolved
Hide resolved
...ackages/postgres/src/db/migrations/migration_files/20241119163402_create_vault_daily_view.ts
Show resolved
Hide resolved
...ackages/postgres/src/db/migrations/migration_files/20241119163402_create_vault_daily_view.ts
Outdated
Show resolved
Hide resolved
...ckages/postgres/src/db/migrations/migration_files/20241119162238_create_vault_hourly_view.ts
Show resolved
Hide resolved
indexer/packages/postgres/__tests__/stores/vault-pnl-ticks-view.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
indexer/services/roundtable/__tests__/tasks/refresh-vault-pnl.test.ts (3)
41-47: Consider improving test isolation.Refreshing views in
afterEachmight mask issues where tests depend on stale view data. Consider:
- Moving view refresh to
beforeEachto ensure fresh state for each test- Adding explicit assertions about view state where relevant
109-129: Consider parameterizing test data setup.The helper function uses hardcoded values that could be made more meaningful or parameterized:
- Block height ('6') should be a named constant or parameter
- PnL values (equity: '1080', totalPnl: '1180') could be parameterized for different test scenarios
Example refactor:
- async function setupPnlTick(): Promise<PnlTicksFromDatabase> { + async function setupPnlTick( + params: { + blockHeight?: string, + equity?: string, + totalPnl?: string, + netTransfers?: string, + } = {}, + ): Promise<PnlTicksFromDatabase> { + const { + blockHeight = '6', + equity = '1080', + totalPnl = '1180', + netTransfers = '50', + } = params;
18-130: Consider adding test cases for error conditions.The test suite thoroughly covers the happy path and time window boundaries, but could benefit from additional test cases:
- Database errors during view refresh
- Invalid PnL data scenarios
- Concurrent refresh attempts
indexer/packages/postgres/__tests__/stores/vault-pnl-ticks-view.test.ts (2)
52-93: Enhance test coverage with edge cases and clearer descriptionsWhile the parameterized tests cover basic functionality, consider:
- Adding edge cases for time boundary conditions
- Making test descriptions more explicit about what's being verified
- Adding test cases for empty results and error conditions
Example improvements:
it.each([ { description: 'returns hourly PnL ticks excluding same-hour ticks except first', interval: PnlTickInterval.hour, // ... test params }, { description: 'returns daily PnL ticks excluding same-day ticks except first', interval: PnlTickInterval.day, // ... test params }, { description: 'returns empty array when no ticks exist', interval: PnlTickInterval.hour, // ... test params }, { description: 'handles ticks exactly at hour/day boundaries', interval: PnlTickInterval.hour, // ... test params } ])
95-209: Improve test data setup with explicit time offsets and validationThe setup function creates comprehensive test data, but could be improved by:
- Using constants for time offsets to make the test data structure more explicit
- Adding validation to ensure test data was created correctly
- Using more descriptive variable names for time offsets
Example improvements:
const TIME_OFFSETS = { RECENT: { minutes: 10 }, SAME_HOUR: { minutes: 50 }, DIFFERENT_HOUR: { hours: 2 }, DIFFERENT_DAY: { days: 2 }, HISTORICAL: { days: 30 }, } as const; async function setupIntervalPnlTicks(): Promise<PnlTicksFromDatabase[]> { const currentTime: DateTime = DateTime.utc().startOf('day'); const timePoints = Object.entries(TIME_OFFSETS).reduce((acc, [key, offset]) => ({ ...acc, [key]: currentTime.minus(offset).toISO(), }), {}); // Create blocks and ticks using timePoints // Add validation const createdTicks = await PnlTicksTable.createMany([...]); expect(createdTicks).toHaveLength(9); // Validate creation return createdTicks; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
indexer/packages/postgres/__tests__/stores/vault-pnl-ticks-view.test.ts(1 hunks)indexer/services/roundtable/__tests__/tasks/refresh-vault-pnl.test.ts(1 hunks)indexer/services/roundtable/src/tasks/refresh-vault-pnl.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/src/tasks/refresh-vault-pnl.ts
🔇 Additional comments (2)
indexer/services/roundtable/__tests__/tasks/refresh-vault-pnl.test.ts (1)
1-17: LGTM! Import statements and setup are well-organized.
The imports are properly structured, and AWS helpers are correctly mocked for unit testing.
indexer/packages/postgres/__tests__/stores/vault-pnl-ticks-view.test.ts (1)
1-22: LGTM: Imports are well organized
The imports are logically grouped and all necessary dependencies are included for database operations, types, and test helpers.
indexer/services/roundtable/__tests__/tasks/refresh-vault-pnl.test.ts
Outdated
Show resolved
Hide resolved
| const result: { | ||
| rows: PnlTicksFromDatabase[], | ||
| } = await knexReadReplica.getConnection().raw( | ||
| ` |
indexer/services/roundtable/__tests__/tasks/refresh-vault-pnl.test.ts
Outdated
Show resolved
Hide resolved
| import * as Knex from "knex"; | ||
|
|
||
| const RAW_VAULTS_PNL_HOURLY_QUERY: string = ` | ||
| CREATE MATERIALIZED VIEW IF NOT EXISTS vaults_hourly_pnl AS WITH vault_subaccounts AS |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
indexer/services/roundtable/src/tasks/refresh-vault-pnl.ts (2)
9-11: Enhance the JSDoc documentation with more details.The current documentation could be more descriptive about the task's purpose, timing windows, and behavior.
Consider expanding the documentation:
/** - * Refresh the vault pnl ticks views. + * Refreshes the vault PnL materialized views (hourly and daily) within a configured time window + * after the start of each hour/day. This task is designed to optimize query performance by + * maintaining pre-computed PnL data that updates periodically instead of computing it on-demand. + * + * The refresh occurs only if the current time is within TIME_WINDOW_FOR_REFRESH_VAULT_PNL_MS + * milliseconds from the start of the hour (for hourly view) or day (for daily view). */
31-53: Refactor to reduce code duplication and improve maintainability.The hourly and daily refresh logic shares similar structure, which could be abstracted into a reusable function.
Consider refactoring to reduce duplication:
+async function refreshView( + viewType: 'hourly' | 'daily', + currentTime: DateTime, + refreshFn: () => Promise<void>, +): Promise<void> { + const timeStart = currentTime.startOf(viewType === 'hourly' ? 'hour' : 'day'); + const timeFromStart = currentTime.diff(timeStart).toMillis(); + + if (timeFromStart < config.TIME_WINDOW_FOR_REFRESH_VAULT_PNL_MS) { + logger.info({ + at: 'refresh-vault-pnl#runTask', + message: `Refreshing vault ${viewType} pnl view`, + currentTime, + timeFromStart, + }); + const refreshStart: number = Date.now(); + try { + await refreshFn(); + logger.info({ + at: 'refresh-vault-pnl#runTask', + message: `Successfully refreshed ${viewType} view`, + }); + stats.timing( + `${config.SERVICE_NAME}.refresh-vault-pnl.${viewType}-view.timing`, + Date.now() - refreshStart, + ); + } catch (error) { + logger.error({ + at: 'refresh-vault-pnl#runTask', + message: `Failed to refresh ${viewType} view`, + error, + }); + throw error; + } + } +} export default async function runTask(): Promise<void> { try { const currentTime: DateTime = DateTime.utc(); - if (currentTime.diff(...) - // ... hourly refresh logic - } - - if (currentTime.diff(...) - // ... daily refresh logic - } + await refreshView('hourly', currentTime, VaultPnlTicksView.refreshHourlyView); + await refreshView('daily', currentTime, VaultPnlTicksView.refreshDailyView); } catch (error) { logger.error({ at: 'refresh-vault-pnl#runTask', message: 'Failed to refresh vault pnl views', error, }); + throw error; // Re-throw to ensure task failure is properly handled } }indexer/services/roundtable/src/config.ts (1)
222-224: Improve configuration organization and naming consistencyConsider these improvements to better align with the codebase's established patterns:
- Move the configuration under an existing section (e.g., "Loop Timing") instead of creating a new section
- Rename the configuration to follow the established naming pattern
- Remove the unnecessary blank line
- - // Refresh vault pnl view - TIME_WINDOW_FOR_REFRESH_VAULT_PNL_MS: parseInteger({ default: 15 * ONE_MINUTE_IN_MILLISECONDS }), + // Loop Timing (continued) + LOOPS_WINDOW_MS_REFRESH_VAULT_PNL: parseInteger({ default: 15 * ONE_MINUTE_IN_MILLISECONDS }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
indexer/packages/postgres/src/db/migrations/migration_files/20241119162238_create_vault_hourly_view.ts(1 hunks)indexer/packages/postgres/src/db/migrations/migration_files/20241119163402_create_vault_daily_view.ts(1 hunks)indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts(1 hunks)indexer/services/roundtable/__tests__/tasks/refresh-vault-pnl.test.ts(1 hunks)indexer/services/roundtable/src/config.ts(3 hunks)indexer/services/roundtable/src/tasks/refresh-vault-pnl.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- indexer/packages/postgres/src/db/migrations/migration_files/20241119162238_create_vault_hourly_view.ts
- indexer/packages/postgres/src/db/migrations/migration_files/20241119163402_create_vault_daily_view.ts
- indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts
- indexer/services/roundtable/tests/tasks/refresh-vault-pnl.test.ts
🔇 Additional comments (3)
indexer/services/roundtable/src/tasks/refresh-vault-pnl.ts (1)
46-52: Verify error handling in the task scheduler.
The error is logged but not re-thrown, which might prevent the task scheduler from properly handling task failures.
Let's verify how task failures are handled:
indexer/services/roundtable/src/config.ts (2)
63-63: LGTM!
The loop enablement flag follows the established naming convention and its default value of true is appropriate for this core functionality.
144-146: LGTM!
The loop interval configuration follows the established pattern and its default value of 5 minutes aligns with the PR objectives for refreshing the materialized view.
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit 92733ca)
Changelist
Vault PnL queries will scale with the number of vaults and the amount of data to query from the database. These queries fetch information that does not have to be updated often (pnl ticks are created once an hour). Instead of querying and computing this data each time, use a materialized view that is periodically refreshed (once every 5 minutes within the first 15 minutes of each hour / day).
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
Release Notes
New Features
Tests
Configuration