Add Orderbook Mid Price Cache #2338
Conversation
WalkthroughThe changes introduce a new caching mechanism for orderbook mid prices using Redis, encapsulating functionality in a dedicated module. This includes the addition of two main functions, 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: 7
Outside diff range and nitpick comments (17)
indexer/packages/redis/src/scripts/add_market_price.lua (2)
1-8: LGTM! Consider adding a comment for the time window constant.The variable declarations and input processing look good. The use of
KEYSandARGVis correct, and converting inputs to numbers is a good practice.Consider adding a comment to explain the significance of the 5-second time window:
-- Time window (5 seconds) for maintaining recent price data local fiveSeconds = 5
13-15: LGTM! Consider using ZREMRANGEBYSCORE with ZADD for atomicity.The implementation for removing old entries is correct. However, we can optimize this operation by combining it with the ZADD command for atomicity.
Consider using a single EVAL call to perform both operations atomically:
redis.call("zadd", priceCacheKey, nowSeconds, price) redis.call("zremrangebyscore", priceCacheKey, "-inf", nowSeconds - fiveSeconds)This ensures that no other operations can occur between adding the new price and removing old entries, maintaining data consistency.
indexer/packages/redis/src/scripts/get_market_median_price.lua (3)
12-13: Consider adjusting the middle index calculation.The current calculation of the middle index is correct for even numbers of elements, but it might be off by one for odd numbers. To handle both cases correctly, consider using:
local middle = math.ceil(#prices / 2)This ensures that for odd numbers of elements, we get the correct middle index, and for even numbers, we get the upper middle index, which works with the subsequent logic.
15-22: LGTM: Correct median calculation logic with a minor optimization opportunity.The script correctly handles both even and odd number of elements, returning appropriate values for median calculation. The consistency in returning an array for both cases is good for external processing.
A minor optimization could be:
if #prices % 2 == 0 then return {prices[middle], prices[middle + 1]} else return {prices[middle]} endThis assumes the suggested change to the
middlecalculation is implemented. It simplifies the odd case and reduces the need for the+ 1adjustment.
1-22: Overall: Well-implemented script with minor improvement opportunities.The script effectively implements the core functionality for retrieving median prices from Redis, aligning well with the PR objectives. It handles different scenarios appropriately and leverages Redis's sorted set capabilities efficiently.
Consider the following suggestions for further improvement:
- Implement the suggested changes for middle index calculation and median retrieval for odd numbers of elements.
- Add error handling, particularly for cases where the key might not exist or other potential Redis errors.
- Consider adding comments explaining the external division for even number of elements, to improve maintainability.
To enhance the robustness of the system:
- Implement monitoring for the cache hit rate and latency of this script.
- Set up alerts for scenarios where the price cache is consistently empty or contains unexpected values.
- Consider implementing a fallback mechanism in case of Redis failures or unexpected errors.
These suggestions will help ensure the reliability and performance of the orderbook mid price cache in production.
indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts (1)
27-37: Enhance error handling and logging.
- For undefined prices, consider adding more context to help diagnose why the price might be missing:
logger.info({ at: 'cache-orderbook-mid-prices#runTask', message: 'Undefined price for market', ticker: market.ticker, marketId: market.id, // Add any other relevant market information });
- For error logging, include more details about the market being processed:
logger.error({ at: 'cache-orderbook-mid-prices#runTask', message: 'Failed to process market', ticker: market.ticker, marketId: market.id, error: error.message, stack: error.stack, });These changes will provide more context for debugging and monitoring purposes.
indexer/services/roundtable/__tests__/tasks/cache-orderbook-mid-prices.test.ts (2)
67-83: Consider adding more assertions for undefined price handling.The test case for handling undefined prices is good, but could be improved:
- Verify that the mock was called with the correct arguments.
- Check if the cache was attempted to be set even with an undefined price.
- Consider testing this scenario for multiple markets to ensure consistent behavior.
Here's a suggested improvement:
it('handles undefined prices', async () => { const market1 = testConstants.defaultPerpetualMarket; const market2 = testConstants.defaultPerpetualMarket2; const mockGetOrderBookMidPrice = jest.spyOn(OrderbookLevelsCache, 'getOrderBookMidPrice'); mockGetOrderBookMidPrice.mockResolvedValueOnce(undefined); mockGetOrderBookMidPrice.mockResolvedValueOnce(undefined); await runTask(); expect(mockGetOrderBookMidPrice).toHaveBeenCalledWith(market1.ticker, redisClient); expect(mockGetOrderBookMidPrice).toHaveBeenCalledWith(market2.ticker, redisClient); const price1 = await OrderbookMidPricesCache.getMedianPrice(redisClient, market1.ticker); const price2 = await OrderbookMidPricesCache.getMedianPrice(redisClient, market2.ticker); expect(price1).toBeNull(); expect(price2).toBeNull(); expect(jest.requireMock('@dydxprotocol-indexer/base').logger.info).toHaveBeenCalledWith({ at: 'cache-orderbook-mid-prices#runTask', message: `undefined price for ${market1.ticker}`, }); expect(jest.requireMock('@dydxprotocol-indexer/base').logger.info).toHaveBeenCalledWith({ at: 'cache-orderbook-mid-prices#runTask', message: `undefined price for ${market2.ticker}`, }); });
85-97: Enhance error handling test case.The error handling test case is good but could be improved:
- Verify that the mock was called with the correct arguments.
- Check if the cache remains unchanged after an error.
- Consider testing this scenario for multiple markets to ensure consistent behavior.
Here's a suggested improvement:
it('handles errors', async () => { const market1 = testConstants.defaultPerpetualMarket; const market2 = testConstants.defaultPerpetualMarket2; const mockGetOrderBookMidPrice = jest.spyOn(OrderbookLevelsCache, 'getOrderBookMidPrice'); mockGetOrderBookMidPrice.mockRejectedValueOnce(new Error('Test error for market1')); mockGetOrderBookMidPrice.mockRejectedValueOnce(new Error('Test error for market2')); await runTask(); expect(mockGetOrderBookMidPrice).toHaveBeenCalledWith(market1.ticker, redisClient); expect(mockGetOrderBookMidPrice).toHaveBeenCalledWith(market2.ticker, redisClient); const price1 = await OrderbookMidPricesCache.getMedianPrice(redisClient, market1.ticker); const price2 = await OrderbookMidPricesCache.getMedianPrice(redisClient, market2.ticker); expect(price1).toBeNull(); expect(price2).toBeNull(); expect(jest.requireMock('@dydxprotocol-indexer/base').logger.error).toHaveBeenCalledWith({ at: 'cache-orderbook-mid-prices#runTask', message: 'Test error for market1', error: expect.any(Error), }); expect(jest.requireMock('@dydxprotocol-indexer/base').logger.error).toHaveBeenCalledWith({ at: 'cache-orderbook-mid-prices#runTask', message: 'Test error for market2', error: expect.any(Error), }); });indexer/services/roundtable/src/config.ts (1)
141-143: LGTM: New configuration parameter for orderbook mid price caching interval.The addition of
LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICESis well-implemented and aligns with the PR objectives. The naming convention is consistent with other loop interval parameters, and the default value ofONE_SECOND_IN_MILLISECONDScorrectly sets the sampling interval to one second as specified in the PR summary.Consider adding a comment above this configuration to explicitly state its purpose, similar to comments present for some other configurations in this file. For example:
// Interval for caching orderbook mid prices LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES: parseInteger({ default: ONE_SECOND_IN_MILLISECONDS, }),This would enhance the readability and maintainability of the configuration file.
indexer/services/roundtable/src/index.ts (1)
276-282: LGTM: New conditional block for caching orderbook mid pricesThe implementation of the new conditional block for caching orderbook mid prices is well-structured and aligns with the existing patterns in the file. It correctly uses configuration values for enabling the feature and setting the interval, providing flexibility and ease of management.
This addition fulfills the PR objective of implementing a caching mechanism for orderbook mid prices, with the ability to sample the orderbook mid price every second.
For consistency with other similar blocks in this file, consider adding a newline before this block (i.e., between lines 275 and 276).
indexer/services/ender/src/lib/candles-generator.ts (1)
Action Required: Missing 5-Second Cache Duration Configuration
While the
OrderbookMidPricesCacheis implemented and utilized across various parts of the codebase, the specific configuration to maintain a 5-second sample of orderbook mid prices was not found. This configuration is crucial to ensure that the caching mechanism operates as intended and meets the PR objectives.Please address the following:
- Implement Cache Duration Configuration: Ensure that the cache duration is explicitly set to 5 seconds to maintain the required sample interval.
- Verify Interval Settings: Confirm that any interval-related settings, especially those pertaining to orderbook mid price sampling, are correctly configured to support the 5-second duration.
Analysis chain
Line range hint
1-554: Verify completeness of implementationThe changes implemented in this file focus on using the new
OrderbookMidPricesCacheto get median prices. However, the PR objectives mention implementing a 5-second sample of orderbook mid prices and a roundtable task to sample the orderbook mid price every second for each market.Could you please verify if additional changes are needed in other parts of the codebase to fully implement these features? Specifically:
- Is there a separate implementation for the roundtable task that samples the orderbook mid price every second?
- Are there any configurations or additional code needed to ensure the 5-second sample is maintained in the cache?
To help verify this, please run the following script:
This will help ensure that all aspects of the PR objectives have been addressed in the codebase.
Follow-Up: Correct Shell Script Execution Required
The previous shell script encountered a syntax error, preventing a complete verification of the implementation related to orderbook mid price sampling and caching. To ensure all aspects are thoroughly checked, please execute the following corrected shell scripts:
[/run_scripts]
Further Verification Required: Missing Cache Duration Configuration
While the
OrderbookMidPricesCacheand related roundtable tasks are implemented, the configuration for maintaining a 5-second sample of orderbook mid prices was not found. It's essential to ensure that the cache duration is correctly set to meet the PR objectives.Please execute the following shell scripts to search for cache duration configurations using alternative patterns and file types:
[/run_scripts]
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for implementations related to orderbook mid price sampling and caching echo "Checking for roundtable task implementations:" rg "roundtable.*orderbook.*mid.*price" --type ts echo "Checking for cache duration configurations:" rg "cache.*duration.*5.*second" --type ts echo "Checking for OrderbookMidPricesCache usage:" rg "OrderbookMidPricesCache" --type tsLength of output: 3641
Script:
#!/bin/bash # Description: Check for implementations related to orderbook mid price sampling and caching echo "Checking for roundtable task implementations:" rg "roundtable.*orderbook.*mid.*price" --type ts echo "Checking for cache duration configurations:" rg "cache.*duration.*5.*second" --type ts echo "Checking for OrderbookMidPricesCache usage:" rg "OrderbookMidPricesCache" --type tsLength of output: 3546
Script:
#!/bin/bash # Description: Search for cache duration configurations with varied patterns and in different file types echo "Checking for cache duration configurations with alternative patterns:" rg -i "duration[:=]\s*['\"]?5[sS]?[eE][cC][oO][nN][dD]['\"]?" --type ts --type json --type yaml echo "Checking for cache timeout settings related to orderbook mid prices:" rg -i "timeout[:=]\s*['\"]?5[sS]?[eE][cC][oO][nN][dD]['\"]?" --type ts --type json --type yaml echo "Checking for interval settings related to orderbook mid price sampling:" rg -i "interval[:=]\s*['\"]?1[sS]?" --type ts --type json --type yamlLength of output: 1014
indexer/services/ender/__tests__/lib/candles-generator.test.ts (6)
117-122: LGTM! Test case updated to use Redis cache for orderbook prices.The test case now uses
OrderbookMidPricesCache.setPriceto set multiple prices for 'BTC-USD' in parallel, which better simulates real-world scenarios. This change aligns with the new Redis caching strategy for orderbook mid prices.Consider extracting the ticker 'BTC-USD' into a constant at the top of the file, as it's used multiple times throughout the tests. This would improve maintainability and reduce the risk of typos.
162-167: LGTM! Test case updated to use Redis cache for orderbook prices.This test case has been updated to use
OrderbookMidPricesCache.setPricefor setting multiple prices for 'BTC-USD', consistent with the new Redis caching strategy. The change provides a more comprehensive test scenario by setting multiple prices.For consistency with the previous test case, consider using the same price values (100000, 105000, 110000) instead of the current values (80000, 81000, 80500). This would make it easier to compare the behavior across different test cases.
503-503: LGTM! Test case updated to use Redis cache for orderbook price.The test case has been updated to use
OrderbookMidPricesCache.setPricefor setting the orderbook mid price, which is consistent with the new Redis caching strategy.Consider setting multiple prices as done in previous test cases to maintain consistency and provide a more comprehensive test scenario. This could help ensure the behavior is correct when there are multiple price updates within the time frame.
601-601: LGTM! Test case updated to use Redis cache for orderbook price.The test case has been updated to use
OrderbookMidPricesCache.setPricefor setting the orderbook mid price, which aligns with the new Redis caching strategy.For consistency with earlier test cases, consider setting multiple prices using
Promise.all. This would provide a more comprehensive test scenario and ensure the behavior is correct when there are multiple price updates within the time frame.
665-677: LGTM! Test case updated to use Redis cache for multiple markets.The test case has been improved to set prices for multiple markets ('BTC-USD', 'ISO-USD', 'ETH-USD') using
OrderbookMidPricesCache.setPrice. This provides a more comprehensive test scenario. The expected output now correctly includes null values for markets without set prices ('ISO2-USD', 'SHIB-USD'), which is a good test for handling missing data.Consider adding a comment explaining why 'ISO2-USD' and 'SHIB-USD' are expected to have null values. This would improve the clarity of the test case for future readers.
Line range hint
1-824: Overall LGTM! Test suite updated to use Redis cache for orderbook mid prices.The test suite has been successfully updated to use
OrderbookMidPricesCache.setPricefor managing orderbook mid prices, reflecting the shift to a Redis-based caching strategy. This change improves the realism of the tests and aligns them with the new implementation.Consider standardizing the approach for setting prices across all test cases. Some tests set multiple prices using
Promise.all, while others set only a single price. Adopting a consistent approach (preferably setting multiple prices) across all tests would improve the comparability of test cases and ensure comprehensive coverage of scenarios involving multiple price updates.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- indexer/packages/redis/tests/caches/orderbook-mid-prices-cache.test.ts (1 hunks)
- indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts (1 hunks)
- indexer/packages/redis/src/caches/scripts.ts (2 hunks)
- indexer/packages/redis/src/index.ts (1 hunks)
- indexer/packages/redis/src/scripts/add_market_price.lua (1 hunks)
- indexer/packages/redis/src/scripts/get_market_median_price.lua (1 hunks)
- indexer/services/ender/tests/lib/candles-generator.test.ts (7 hunks)
- indexer/services/ender/src/lib/candles-generator.ts (2 hunks)
- indexer/services/roundtable/tests/tasks/cache-orderbook-mid-prices.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (2 hunks)
- indexer/services/roundtable/src/index.ts (2 hunks)
- indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts (1 hunks)
Additional comments not posted (19)
indexer/packages/redis/src/scripts/add_market_price.lua (2)
10-11: LGTM! Correct usage of ZADD command.The implementation for adding the price to the sorted set is correct. Using the timestamp as the score is a good approach for maintaining time-ordered data.
1-17: Overall, the script effectively implements the required functionality.The Lua script successfully implements a caching mechanism for orderbook mid prices using Redis, as described in the PR objectives. It maintains a 5-second sample of prices and efficiently manages the data using a sorted set.
Key points:
- The script correctly adds new prices to the sorted set.
- It removes entries older than 5 seconds, maintaining the desired time window.
- The implementation is concise and efficient.
Consider the suggested optimizations:
- Combining ZADD and ZREMRANGEBYSCORE for atomicity.
- Returning the number of elements in the sorted set for better monitoring.
These changes will further improve the script's robustness and usefulness.
indexer/packages/redis/src/scripts/get_market_median_price.lua (2)
1-5: LGTM: Correct key usage and price retrieval.The script correctly uses
KEYS[1]to get the sorted set key and retrieves all prices in ascending order, which is appropriate for median calculation.
7-10: LGTM: Proper handling of empty set.The script correctly checks for an empty price set and returns nil, which is an appropriate way to handle this edge case.
indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts (2)
1-13: LGTM: Imports are well-organized and relevant.The imports are appropriately structured, bringing in necessary components from different modules. This demonstrates good separation of concerns and modularity in the project structure.
18-40: LGTM: Well-structured implementation of the caching task.The
runTaskfunction is well-implemented, covering the main requirements:
- Fetches all perpetual markets.
- Retrieves and caches the mid price for each market.
- Handles errors gracefully, ensuring one market's failure doesn't affect others.
- Uses structured logging for better observability.
The error handling ensures robustness, and the use of async/await makes the code readable and maintainable.
indexer/packages/redis/src/index.ts (1)
15-15: LGTM! The new export aligns with the PR objectives.The addition of the
OrderbookMidPricesCacheexport is consistent with the existing file structure and supports the implementation of the orderbook mid prices caching mechanism mentioned in the PR objectives.To ensure the correct implementation of the
OrderbookMidPricesCachemodule, please run the following script:This script will help verify:
- The existence and structure of the
OrderbookMidPricesCachemodule.- The use of Redis for caching operations.
- The presence of unit tests for the new cache.
Verification successful
LGTM! The
OrderbookMidPricesCacheexport and implementation are correctly integrated.
- The
OrderbookMidPricesCacheis properly exported inindex.ts.- Redis is utilized for caching within the module as intended.
- Comprehensive unit tests are present and cover key functionalities.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the OrderbookMidPricesCache module # Test: Check if the OrderbookMidPricesCache module exists and contains expected functions ast-grep --lang typescript --pattern $'export const OrderbookMidPricesCache = { $$$ updateMidPrice: $_, getMidPrice: $_, $$$ }' indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts # Test: Verify that the cache is using Redis rg --type typescript 'redis\.hSet|redis\.hGet' indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts # Test: Check for the presence of unit tests fd -e test.ts orderbook-mid-prices-cache indexer/packages/redis/test/cachesLength of output: 548
Script:
#!/bin/bash # Description: Re-verify the implementation of the OrderbookMidPricesCache module with corrected commands # Test 1: Check if the OrderbookMidPricesCache module exists and contains expected functions with a more flexible pattern ast-grep --lang typescript --pattern $'export\s+\*\s+as\s+OrderbookMidPricesCache\s+from\s+\'\./caches/orderbook-mid-prices-cache\';' indexer/packages/redis/src/index.ts # Test 2: Verify that the cache is using Redis with the correct file type identifier rg -t ts 'redis\.hSet|redis\.hGet' indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts # Test 3: Locate unit tests for OrderbookMidPricesCache without assuming the directory structure fd -e test.ts 'orderbook-mid-prices-cache' indexer/packages/redis/Length of output: 417
Script:
#!/bin/bash # Description: Further verify the OrderbookMidPricesCache module implementation # Test 1: Display the contents of the OrderbookMidPricesCache module to verify exports and functions cat indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts # Test 2: Check for Redis-related imports in the OrderbookMidPricesCache module rg -t ts 'import\s+.*redis.*' indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts # Test 3: Display the contents of the test file to verify unit tests for OrderbookMidPricesCache cat indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.tsLength of output: 7763
Script:
#!/bin/bash # Description: Inspect the contents of index.ts to verify the OrderbookMidPricesCache export # Display the contents of index.ts cat indexer/packages/redis/src/index.tsLength of output: 1423
indexer/packages/redis/src/caches/scripts.ts (1)
80-81: LGTM! New scripts correctly added toallLuaScriptsarray.The
addMarketPriceScriptandgetMarketMedianScriptare properly included in theallLuaScriptsarray. This ensures they will be considered in any operations involving all Lua scripts.indexer/services/roundtable/__tests__/tasks/cache-orderbook-mid-prices.test.ts (3)
1-27: LGTM: Imports and mock setups are well-structured.The imports and mock setups are appropriate for the test file. The mocking of the logger and OrderbookLevelsCache is done correctly, which will allow for proper testing of the cache-orderbook-mid-prices task.
29-43: LGTM: Test suite setup and teardown are comprehensive.The test suite setup and teardown processes are well-implemented:
- Database migrations are run before all tests.
- Data is cleared, Redis cache is deleted, and test data is seeded before each test.
- Proper teardown of the database and resetting of mocks after all tests.
This ensures a clean and consistent test environment for each test case.
45-65: LGTM: Test case for caching mid prices is well-structured.This test case effectively verifies the caching of mid prices for multiple markets. It correctly mocks the
getOrderBookMidPricefunction, checks if it's called with the right arguments, and verifies that the prices are cached correctly.indexer/services/roundtable/src/config.ts (2)
63-63: LGTM: New configuration parameter for caching orderbook mid prices.The addition of
LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICESis well-implemented and aligns with the PR objectives. The naming convention is consistent with other loop enablement parameters, and the default value oftruecorrectly enables the new caching mechanism by default.
Line range hint
1-143: Overall assessment: Changes are well-implemented and align with PR objectives.The additions to the configuration file are focused and consistent with the existing structure. They provide the necessary parameters to control the new orderbook mid price caching feature, allowing for easy enablement/disablement and interval adjustment. These changes support the PR's goal of implementing a caching mechanism for orderbook mid prices using Redis.
To ensure these new configurations are properly utilized, please run the following verification script:
This script will help ensure that the new configuration parameters are being used appropriately in the codebase and that any necessary new files for implementing the caching mechanism have been created.
Verification successful
It appears there was an issue recognizing the
typescriptfile type in the previous search. Please run the following corrected verification scripts to ensure the new configuration parameters are properly utilized:
Verification Successful: New Configuration Parameters Are Properly Utilized
The new configuration parameters
LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICESandLOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICESare actively used inindex.tsandconfig.ts. Additionally, the implementation and corresponding test files for orderbook mid price caching have been correctly added within theredispackage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new configuration parameters in the codebase. # Test 1: Check for the usage of LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICES echo "Checking usage of LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICES:" rg "LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICES" --type typescript # Test 2: Check for the usage of LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES echo "Checking usage of LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES:" rg "LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES" --type typescript # Test 3: Look for any new files related to orderbook mid price caching echo "Checking for new files related to orderbook mid price caching:" fd -e ts -e js "orderbook.*mid.*price.*cache"Length of output: 789
Script:
#!/bin/bash # Description: Corrected verification of new configuration parameters usage. # Test 1: Check for the usage of LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICES echo "Checking usage of LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICES:" rg "LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICES" --type ts # Test 2: Check for the usage of LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES echo "Checking usage of LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES:" rg "LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES" --type ts # Test 3: Look for any new files related to orderbook mid price caching echo "Checking for new files related to orderbook mid price caching:" fd -e ts -e js "orderbook.*mid.*price.*cache"Length of output: 1224
indexer/services/roundtable/src/index.ts (2)
13-13: LGTM: New import statement for cacheOrderbookMidPricesThe import statement for
cacheOrderbookMidPricesis correctly placed and follows the existing pattern in the file. This import aligns with the PR objective of implementing a caching mechanism for orderbook mid prices.
Line range hint
1-287: Overall assessment: Well-implemented feature additionThe changes to this file successfully integrate the new orderbook mid price caching feature into the existing structure. The implementation follows established patterns, making it consistent with the rest of the codebase. The use of configuration values for enabling the feature and setting the interval provides flexibility and ease of management.
These changes align well with the PR objectives and appear to be a solid foundation for the new caching mechanism. No significant issues or inconsistencies were introduced by these changes.
indexer/services/ender/src/lib/candles-generator.ts (2)
23-23: Import statement updated to use OrderbookMidPricesCacheThe import statement has been correctly updated to use
OrderbookMidPricesCacheinstead ofOrderbookLevelsCache. This change aligns with the PR objective of implementing a caching mechanism for orderbook mid prices.
541-543: Updated to use OrderbookMidPricesCache.getMedianPriceThe function has been correctly updated to use
OrderbookMidPricesCache.getMedianPriceinstead ofOrderbookLevelsCache.getOrderBookMidPrice. This change aligns with the PR objective of implementing a caching mechanism for orderbook mid prices and using a 5-second median.To ensure consistency across the codebase, please run the following script:
#!/bin/bash # Description: Check for any remaining usage of OrderbookLevelsCache.getOrderBookMidPrice # and verify that OrderbookMidPricesCache.getMedianPrice is used consistently. echo "Checking for remaining usage of OrderbookLevelsCache.getOrderBookMidPrice:" rg "OrderbookLevelsCache\.getOrderBookMidPrice" --type ts echo "Verifying consistent usage of OrderbookMidPricesCache.getMedianPrice:" rg "OrderbookMidPricesCache\.getMedianPrice" --type tsindexer/services/ender/__tests__/lib/candles-generator.test.ts (1)
35-38: LGTM! Imports updated to reflect new Redis caching strategy.The changes in the import statements reflect the shift from using
updatePriceLeveltoOrderbookMidPricesCache.setPricefor managing orderbook prices. This suggests an improvement in how orderbook mid prices are cached and retrieved, potentially leading to better performance and consistency.indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.ts (1)
77-100: Verify compatibility of Jest fake timers withjest.setSystemTime()The use of
jest.setSystemTime()requires the modern fake timers in Jest. By default, Jest 27 and above use modern timers, but if your configuration specifies legacy timers, this could cause issues.Please ensure that modern timers are enabled. You can explicitly set them by modifying your test:
- jest.useFakeTimers(); + jest.useFakeTimers('modern');Alternatively, check your Jest configuration to confirm that modern timers are being used.
| local cutoffTime = nowSeconds - fiveSeconds | ||
| redis.call("zremrangebyscore", priceCacheKey, "-inf", cutoffTime) | ||
|
|
||
| return true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning more meaningful information.
While returning true indicates successful execution, it doesn't provide any actionable information about the operation.
Consider returning the number of elements remaining in the sorted set after the operations:
return redis.call("zcard", priceCacheKey)This provides more context about the state of the sorted set after the operation, which could be useful for monitoring or debugging purposes.
| export default async function runTask(): Promise<void> { | ||
| const markets: PerpetualMarketFromDatabase[] = await PerpetualMarketTable.findAll({}, []); | ||
|
|
||
| for (const market of markets) { | ||
| try { | ||
| const price = await OrderbookLevelsCache.getOrderBookMidPrice(market.ticker, redisClient); | ||
| if (price) { | ||
| await OrderbookMidPricesCache.setPrice(redisClient, market.ticker, price); | ||
| } else { | ||
| logger.info({ | ||
| at: 'cache-orderbook-mid-prices#runTask', | ||
| message: `undefined price for ${market.ticker}`, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| logger.error({ | ||
| at: 'cache-orderbook-mid-prices#runTask', | ||
| message: error.message, | ||
| error, | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider performance optimizations for scaling.
The current implementation processes markets sequentially, which might become a bottleneck as the number of markets grows. Consider the following optimizations:
- Process markets in parallel using
Promise.all()with a concurrency limit. - Batch Redis operations if possible to reduce network overhead.
Example of processing in parallel with concurrency limit:
import pLimit from 'p-limit';
const limit = pLimit(10); // Adjust the concurrency limit as needed
const tasks = markets.map(market => limit(async () => {
try {
const price = await OrderbookLevelsCache.getOrderBookMidPrice(market.ticker, redisClient);
if (price) {
await OrderbookMidPricesCache.setPrice(redisClient, market.ticker, price);
} else {
logger.info({
at: 'cache-orderbook-mid-prices#runTask',
message: `Undefined price for market`,
ticker: market.ticker,
});
}
} catch (error) {
logger.error({
at: 'cache-orderbook-mid-prices#runTask',
message: 'Failed to process market',
ticker: market.ticker,
error,
});
}
}));
await Promise.all(tasks);| export const addMarketPriceScript: LuaScript = newLuaScript('addMarketPrice', '../scripts/add_market_price.lua'); | ||
| export const getMarketMedianScript: LuaScript = newLuaScript('getMarketMedianPrice', '../scripts/get_market_median_price.lua'); |
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: get_market_median_price.lua Missing KEYS and ARGV
The get_market_median_price.lua script does not contain the required KEYS and ARGV declarations, which are necessary for handling input parameters in Redis Lua scripts.
- File Path:
indexer/packages/redis/src/scripts/get_market_median_price.lua
Analysis chain
LGTM! New Lua scripts added for market price operations.
The addition of addMarketPriceScript and getMarketMedianScript aligns with the PR objectives of implementing a caching mechanism for orderbook mid prices. The naming and structure are consistent with existing patterns in the file.
To ensure the Lua script files exist and contain the expected content, please run the following script:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of the new Lua script files
# Test: Check if the Lua script files exist
for script in add_market_price.lua get_market_median_price.lua; do
if [ ! -f "indexer/packages/redis/src/scripts/$script" ]; then
echo "Error: $script does not exist"
exit 1
fi
done
# Test: Check if the Lua scripts contain relevant content
for script in add_market_price.lua get_market_median_price.lua; do
if ! grep -q "KEYS" "indexer/packages/redis/src/scripts/$script" || ! grep -q "ARGV" "indexer/packages/redis/src/scripts/$script"; then
echo "Warning: $script may not contain expected Lua script content"
fi
done
echo "Verification complete"
Length of output: 934
| if (!prices || prices.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| if (prices.length === 1) { | ||
| return Big(prices[0]).toFixed(); | ||
| } | ||
|
|
||
| if (prices.length === 2) { | ||
| const [price1, price2] = prices.map((price) => { | ||
| return Big(price); | ||
| }); | ||
| return price1.plus(price2).div(2).toFixed(); | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Fix median calculation for more than two prices
The getMedianPrice function currently returns null when there are more than two prices in the cache. This might not be the intended behavior. To accurately compute the median for any number of prices, modify the function to handle cases where there are more than two prices.
Apply this diff to compute the median for any number of prices:
if (!prices || prices.length === 0) {
return null;
}
-if (prices.length === 1) {
- return Big(prices[0]).toFixed();
-}
-
-if (prices.length === 2) {
- const [price1, price2] = prices.map((price) => {
- return Big(price);
- });
- return price1.plus(price2).div(2).toFixed();
-}
-
-return null;
+const priceNumbers = prices.map((price) => Big(price));
+const sortedPrices = priceNumbers.sort((a, b) => a.minus(b).toNumber());
+const middle = Math.floor(sortedPrices.length / 2);
+
+if (sortedPrices.length % 2 === 0) {
+ // Even number of prices, average the two middle prices
+ const median = sortedPrices[middle - 1].plus(sortedPrices[middle]).div(2);
+ return median.toFixed();
+} else {
+ // Odd number of prices, take the middle price
+ return sortedPrices[middle].toFixed();
+}This adjustment ensures that the median is correctly calculated regardless of the number of prices in the cache.
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.
| if (!prices || prices.length === 0) { | |
| return null; | |
| } | |
| if (prices.length === 1) { | |
| return Big(prices[0]).toFixed(); | |
| } | |
| if (prices.length === 2) { | |
| const [price1, price2] = prices.map((price) => { | |
| return Big(price); | |
| }); | |
| return price1.plus(price2).div(2).toFixed(); | |
| } | |
| return null; | |
| if (!prices || prices.length === 0) { | |
| return null; | |
| } | |
| const priceNumbers = prices.map((price) => Big(price)); | |
| const sortedPrices = priceNumbers.sort((a, b) => a.minus(b).toNumber()); | |
| const middle = Math.floor(sortedPrices.length / 2); | |
| if (sortedPrices.length % 2 === 0) { | |
| // Even number of prices, average the two middle prices | |
| const median = sortedPrices[middle - 1].plus(sortedPrices[middle]).div(2); | |
| return median.toFixed(); | |
| } else { | |
| // Odd number of prices, take the middle price | |
| return sortedPrices[middle].toFixed(); | |
| } |
| let evalAsync: ( | ||
| marketCacheKey: string, | ||
| ) => Promise<string[]> = ( | ||
| marketCacheKey, | ||
| ) => { | ||
| return new Promise((resolve, reject) => { | ||
| const callback: Callback<string[]> = ( | ||
| err: Error | null, | ||
| results: string[], | ||
| ) => { | ||
| if (err) { | ||
| return reject(err); | ||
| } | ||
| return resolve(results); | ||
| }; | ||
|
|
||
| client.evalsha( | ||
| getMarketMedianScript.hash, | ||
| 1, | ||
| marketCacheKey, | ||
| callback, | ||
| ); | ||
| }); | ||
| }; | ||
| evalAsync = evalAsync.bind(client); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to simplify async function handling in getMedianPrice
Similar to setPrice, the evalAsync function within getMedianPrice can be simplified. The use of bind is unnecessary since client is accessible in the outer scope. Simplify the function by removing evalAsync and directly returning a Promise.
Apply this diff to simplify the code:
export async function getMedianPrice(client: RedisClient, ticker: string): Promise<string | null> {
- let evalAsync: (
- marketCacheKey: string,
- ) => Promise<string[]> = (
- marketCacheKey,
- ) => {
- return new Promise((resolve, reject) => {
- const callback: Callback<string[]> = (
- err: Error | null,
- results: string[],
- ) => {
- if (err) {
- return reject(err);
- }
- return resolve(results);
- };
-
- client.evalsha(
- getMarketMedianScript.hash,
- 1,
- marketCacheKey,
- callback,
- );
- });
- };
- evalAsync = evalAsync.bind(client);
-
- const prices = await evalAsync(
- getOrderbookMidPriceCacheKey(ticker),
- );
+ const marketCacheKey = getOrderbookMidPriceCacheKey(ticker);
+ const prices = await new Promise<string[]>((resolve, reject) => {
+ client.evalsha(
+ getMarketMedianScript.hash,
+ 1,
+ marketCacheKey,
+ (err: Error | null, results: string[]) => {
+ if (err) {
+ reject(err);
+ } else {
+ resolve(results);
+ }
+ },
+ );
+ });This refactoring removes unnecessary layers of abstraction, enhancing clarity and maintainability.
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.
| let evalAsync: ( | |
| marketCacheKey: string, | |
| ) => Promise<string[]> = ( | |
| marketCacheKey, | |
| ) => { | |
| return new Promise((resolve, reject) => { | |
| const callback: Callback<string[]> = ( | |
| err: Error | null, | |
| results: string[], | |
| ) => { | |
| if (err) { | |
| return reject(err); | |
| } | |
| return resolve(results); | |
| }; | |
| client.evalsha( | |
| getMarketMedianScript.hash, | |
| 1, | |
| marketCacheKey, | |
| callback, | |
| ); | |
| }); | |
| }; | |
| evalAsync = evalAsync.bind(client); | |
| const marketCacheKey = getOrderbookMidPriceCacheKey(ticker); | |
| const prices = await new Promise<string[]>((resolve, reject) => { | |
| client.evalsha( | |
| getMarketMedianScript.hash, | |
| 1, | |
| marketCacheKey, | |
| (err: Error | null, results: string[]) => { | |
| if (err) { | |
| reject(err); | |
| } else { | |
| resolve(results); | |
| } | |
| }, | |
| ); | |
| }); |
| let evalAsync: ( | ||
| marketCacheKey: string, | ||
| ) => Promise<void> = (marketCacheKey) => { | ||
|
|
||
| return new Promise<void>((resolve, reject) => { | ||
| const callback: Callback<void> = ( | ||
| err: Error | null, | ||
| ) => { | ||
| if (err) { | ||
| return reject(err); | ||
| } | ||
| return resolve(); | ||
| }; | ||
|
|
||
| const nowSeconds = Math.floor(Date.now() / 1000); // Current time in seconds | ||
| client.evalsha( | ||
| addMarketPriceScript.hash, | ||
| numKeys, | ||
| marketCacheKey, | ||
| price, | ||
| nowSeconds, | ||
| callback, | ||
| ); | ||
|
|
||
| }); | ||
| }; | ||
| evalAsync = evalAsync.bind(client); | ||
|
|
||
| return evalAsync( | ||
| getOrderbookMidPriceCacheKey(ticker), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to simplify async function handling in setPrice
The evalAsync function within setPrice is unnecessarily complex. Since client is accessible in the outer scope and this is not used, the use of bind is redundant. Consider simplifying the function by removing evalAsync and directly returning a Promise.
Apply this diff to simplify the code:
export async function setPrice(
client: RedisClient,
ticker: string,
price: string,
): Promise<void> {
- // Number of keys for the lua script.
- const numKeys: number = 1;
-
- let evalAsync: (
- marketCacheKey: string,
- ) => Promise<void> = (marketCacheKey) => {
-
- return new Promise<void>((resolve, reject) => {
- const callback: Callback<void> = (
- err: Error | null,
- ) => {
- if (err) {
- return reject(err);
- }
- return resolve();
- };
-
- const nowSeconds = Math.floor(Date.now() / 1000); // Current time in seconds
- client.evalsha(
- addMarketPriceScript.hash,
- numKeys,
- marketCacheKey,
- price,
- nowSeconds,
- callback,
- );
- });
- };
- evalAsync = evalAsync.bind(client);
-
- return evalAsync(
- getOrderbookMidPriceCacheKey(ticker),
- );
+ const marketCacheKey = getOrderbookMidPriceCacheKey(ticker);
+ const nowSeconds = Math.floor(Date.now() / 1000); // Current time in seconds
+ return new Promise<void>((resolve, reject) => {
+ client.evalsha(
+ addMarketPriceScript.hash,
+ 1,
+ marketCacheKey,
+ price,
+ nowSeconds,
+ (err: Error | null) => {
+ if (err) {
+ reject(err);
+ } else {
+ resolve();
+ }
+ },
+ );
+ });
}This refactoring streamlines the setPrice function by removing unnecessary complexity and improves readability.
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.
| let evalAsync: ( | |
| marketCacheKey: string, | |
| ) => Promise<void> = (marketCacheKey) => { | |
| return new Promise<void>((resolve, reject) => { | |
| const callback: Callback<void> = ( | |
| err: Error | null, | |
| ) => { | |
| if (err) { | |
| return reject(err); | |
| } | |
| return resolve(); | |
| }; | |
| const nowSeconds = Math.floor(Date.now() / 1000); // Current time in seconds | |
| client.evalsha( | |
| addMarketPriceScript.hash, | |
| numKeys, | |
| marketCacheKey, | |
| price, | |
| nowSeconds, | |
| callback, | |
| ); | |
| }); | |
| }; | |
| evalAsync = evalAsync.bind(client); | |
| return evalAsync( | |
| getOrderbookMidPriceCacheKey(ticker), | |
| ); | |
| } | |
| export async function setPrice( | |
| client: RedisClient, | |
| ticker: string, | |
| price: string, | |
| ): Promise<void> { | |
| const marketCacheKey = getOrderbookMidPriceCacheKey(ticker); | |
| const nowSeconds = Math.floor(Date.now() / 1000); // Current time in seconds | |
| return new Promise<void>((resolve, reject) => { | |
| client.evalsha( | |
| addMarketPriceScript.hash, | |
| 1, | |
| marketCacheKey, | |
| price, | |
| nowSeconds, | |
| (err: Error | null) => { | |
| if (err) { | |
| reject(err); | |
| } else { | |
| resolve(); | |
| } | |
| }, | |
| ); | |
| }); | |
| } |
| await client.zrange( | ||
| `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`, | ||
| 0, | ||
| -1, | ||
| (_: any, response: string[]) => { | ||
| expect(response[0]).toBe('50000'); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Redis client calls to use promises and async/await
The test code is currently using callback-based Redis client calls, which can be less readable and more prone to errors. Redis client methods support Promises, allowing you to use async/await syntax for better readability and error handling.
Apply this diff to refactor the Redis calls:
- await client.zrange(
- `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`,
- 0,
- -1,
- (_: any, response: string[]) => {
- expect(response[0]).toBe('50000');
- },
- );
+ const response = await client.zrange(
+ `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`,
+ 0,
+ -1,
+ );
+ expect(response[0]).toBe('50000');And similarly in the next test:
- await client.zrange(
- `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`,
- 0,
- -1,
- (_: any, response: string[]) => {
- expect(response).toEqual(['49000', '50000', '51000']);
- },
- );
+ const response = await client.zrange(
+ `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`,
+ 0,
+ -1,
+ );
+ expect(response).toEqual(['49000', '50000', '51000']);Also applies to: 37-45
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit f346663)
Co-authored-by: Adam Fraser <[email protected]>
Changelist
Test Plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests