Candles Cache Improvements #2590
Conversation
WalkthroughThe changes in this pull request primarily involve enhancements to the handling and caching of order book mid prices across various modules. Key modifications include the introduction of new functions and scripts for managing prices, updates to existing test suites to accommodate these changes, and the addition of an in-memory cache for better performance. Additionally, configuration options related to caching behavior have been updated. The overall structure of the code remains intact, while functionality is expanded to support multiple market tickers efficiently. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
adamfraser
force-pushed
the
adam/candles-cache-main
branch
2 times, most recently
from
dd7d0df to
40e67d0
Compare
adamfraser
force-pushed
the
adam/candles-cache-main
branch
from
40e67d0 to
a774971
Compare
adamfraser
force-pushed
the
adam/candles-cache-main
branch
from
a774971 to
354257b
Compare
adamfraser
force-pushed
the
adam/candles-cache-main
branch
from
3a158b7 to
77190e4
Compare
indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts
Outdated
Show resolved
Hide resolved
indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // Convert the prices to Big.js objects for precision | ||
| const bigPrices = prices.map((price) => Big(price)); |
indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts
Outdated
Show resolved
Hide resolved
indexer/services/roundtable/__tests__/tasks/cache-orderbook-mid-prices.test.ts
Outdated
Show resolved
Hide resolved
indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts
Outdated
Show resolved
Hide resolved
| jest.useFakeTimers(); | ||
| // Mock the getOrderBookMidPrice function for the ticker | ||
| const mockPrices = ['50000', '51000', '49000', '48000', '52000', '53000']; |
indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.ts
Outdated
Show resolved
Hide resolved
adamfraser
force-pushed
the
adam/candles-cache-main
branch
from
2254666 to
5212e1f
Compare
adamfraser
force-pushed
the
adam/candles-cache-main
branch
from
5212e1f to
70421a1
Compare
adamfraser
force-pushed
the
adam/candles-cache-main
branch
from
70421a1 to
7a1bcc9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (23)
indexer/packages/redis/src/scripts/get_orderbook_mid_prices.lua (2)
1-2: Enhance script documentationThe current documentation could be more comprehensive. Consider adding:
- Expected format of cache keys
- Description of the return value structure
- Example usage
-- KEYS is an array of cache keys for a market +-- Script to retrieve recent orderbook mid prices from Redis sorted sets +-- +-- Input: +-- KEYS: Array of cache keys for markets (e.g., "market:BTC-USD:mid_price") +-- +-- Returns: +-- Array of arrays, where each inner array contains up to 10 most recent prices +-- for the corresponding market +-- +-- Example: +-- EVAL get_orderbook_mid_prices 2 market:BTC-USD:mid_price market:ETH-USD:mid_price
6-6: Consider adding performance optimization for bulk operationsThe current implementation makes separate Redis calls for each key. For better performance with multiple keys, consider using
MULTI/EXECor pipelining if supported in your Redis setup.- local prices = redis.call("ZRANGE", key, 0, 9) + -- Pipeline multiple ZRANGE commands + redis.call("MULTI") + for _, key in ipairs(KEYS) do + redis.call("ZRANGE", key, 0, 9) + end + local all_prices = redis.call("EXEC")indexer/packages/redis/src/scripts/add_orderbook_mid_prices.lua (3)
10-19: Consider making the time window configurableThe 30-second window is hardcoded, which might limit flexibility for different markets or testing scenarios. Consider making this configurable by passing it as an additional argument.
Suggested implementation:
-local thirtySeconds = 30 +-- Get the time window from the second-to-last argument +local timeWindow = tonumber(ARGV[numArgs-1]) +if not timeWindow then + return redis.error_reply("Invalid time window") +end -local cutoffTime = timestamp - thirtySeconds +local cutoffTime = timestamp - timeWindow
21-36: Consider performance optimizations and error handlingThe current implementation makes multiple Redis calls per market and lacks error handling for Redis operations. Consider these improvements:
- Use Redis pipelining or MULTI to reduce round trips
- Add batch size limits to prevent long-running scripts
- Add error handling for Redis operations
Example implementation with error handling:
for i = 1, numKeys do + -- Add batch size limit + if i > 1000 then + return redis.error_reply("Too many markets to process at once") + end + local priceCacheKey = KEYS[i] local price = tonumber(ARGV[i]) -- Validate the price if not price then return redis.error_reply("Invalid price for key " .. priceCacheKey) end + -- Wrap Redis operations in pcall for error handling + local ok, err = pcall(function() -- Add the price to the sorted set with the current timestamp as the score redis.call("ZADD", priceCacheKey, timestamp, price) -- Remove entries older than the cutoff time redis.call("ZREMRANGEBYSCORE", priceCacheKey, "-inf", cutoffTime) + end) + if not ok then + return redis.error_reply("Redis operation failed for key " .. priceCacheKey .. ": " .. tostring(err)) + end end
38-38: Enhance return value with operation statisticsInstead of returning a simple boolean, consider returning statistics about the operation to aid in monitoring and debugging.
Example implementation:
-return true +-- Return operation statistics +local stats = { + processed_markets = numKeys, + timestamp = timestamp, + cutoff_time = cutoffTime +} +return cjson.encode(stats)Note: This requires adding
cjson = require "cjson"at the beginning of the script.indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts (2)
27-37: Consider enhancing the retry mechanismThe current retry logic might mask underlying issues with the perpetual market cache. Consider:
- Adding a maximum retry limit
- Implementing exponential backoff
- Adding more detailed error logging before retry
if (marketTickers.length === 0) { + logger.warn({ + at: 'cache-orderbook-mid-prices#runTask', + message: 'No markets found, attempting to refresh perpetual markets cache', + }); await perpetualMarketRefresher.updatePerpetualMarkets(); perpetualMarkets = Object.values(perpetualMarketRefresher.getPerpetualMarketsMap()); marketTickers = perpetualMarkets.map( (market: PerpetualMarketFromDatabase) => market.ticker, ); if (marketTickers.length === 0) { - throw new Error('perpetualMarketRefresher is empty'); + throw new Error('Failed to load perpetual markets after refresh attempt'); } }
14-16: Add documentation for timing requirementsThe PR objectives specify that this task runs every 5 seconds and cache entries older than 30 seconds should be removed. Consider documenting these timing requirements in the function's JSDoc comment.
/** * Updates OrderbookMidPricesCache with current orderbook mid price for each market + * + * This task is designed to run every 5 seconds to maintain fresh price data. + * Cache entries older than 30 seconds are automatically removed by the cache implementation. */indexer/services/ender/src/caches/orderbook-mid-price-memory-cache.ts (3)
16-16: Add type annotation for clarity.Consider adding the type annotation to make the variable's type explicit:
-let orderbookMidPriceCache: OrderbookMidPriceCache = {}; +let orderbookMidPriceCache: OrderbookMidPriceCache = {};
29-31: Add documentation and consider input validation.The function would benefit from JSDoc documentation and input validation.
+/** + * Retrieves the orderbook mid price for a given ticker from the cache. + * @param ticker - The market ticker to look up + * @returns The cached mid price if available, undefined otherwise + */ export function getOrderbookMidPrice(ticker: string): string | undefined { + if (!ticker) { + logger.error({ + at: 'orderbook-mid-price-cache#getOrderbookMidPrice', + message: 'Invalid ticker provided', + ticker, + }); + return undefined; + } return orderbookMidPriceCache[ticker]; }
1-69: Consider memory bounds and stale data handling.A few architectural considerations:
- The in-memory cache has no size limits. Consider adding a maximum size limit to prevent unbounded memory growth.
- There's no explicit handling of stale data. Consider adding a timestamp to each cache entry and implementing a cleanup mechanism for stale entries.
- The PR objectives mention a 30-second staleness policy, but this isn't implemented in the code.
Would you like assistance in implementing these improvements?
indexer/services/ender/src/index.ts (2)
8-8: Consider using specific imports instead of wildcard import.Using wildcard imports (
import * as) can make it harder to track which specific exports are being used. Consider importing only the required functions/types explicitly.-import * as OrderbookMidPriceMemoryCache from './caches/orderbook-mid-price-memory-cache'; +import { start as startOrderbookMidPriceCache } from './caches/orderbook-mid-price-memory-cache';
Line range hint
44-51: Consider adding cache cleanup in SIGTERM handler.The SIGTERM handler currently stops the consumer and quits Redis, but doesn't handle cleanup for the OrderbookMidPriceMemoryCache. Consider adding proper shutdown handling for the cache to prevent any resource leaks.
process.on('SIGTERM', async () => { logger.info({ at: 'index#SIGTERM', message: 'Received SIGTERM, shutting down', }); await stopConsumer(); + await OrderbookMidPriceMemoryCache.stop(); redisClient.quit(); });indexer/services/ender/__tests__/caches/orderbook-mid-price-memory-cache.test.ts (1)
37-82: Enhance test coverage for updateOrderbookMidPrices.While the current tests cover basic functionality, error handling, and timing stats, consider adding:
- Concurrent update handling
- Update frequency verification
- Price change validation over time
Example additional test cases:
it('should handle concurrent updates correctly', async () => { // Simulate multiple concurrent updates const updates = [ orderbookMidPriceMemoryCache.updateOrderbookMidPrices(), orderbookMidPriceMemoryCache.updateOrderbookMidPrices(), ]; await Promise.all(updates); // Verify cache consistency }); it('should maintain price history correctly', async () => { // Test multiple updates and verify price changes });indexer/services/roundtable/__tests__/tasks/cache-orderbook-mid-prices.test.ts (4)
28-31: Consider swapping the order of operations in beforeAllThe current order runs migration before clearing data. It's generally safer to clear data first to ensure a clean state before migration:
beforeAll(async () => { - await dbHelpers.migrate(); await dbHelpers.clearData(); + await dbHelpers.migrate(); });
55-58: Consider using constants for test price valuesThe mock prices ('200', '300', '400') should be defined as constants at the top of the file for better maintainability and clarity.
+const TEST_PRICES = { + MARKET_1: '200', + MARKET_2: '300', + MARKET_3: '400', +}; jest.spyOn(OrderbookLevelsCache, 'getOrderBookMidPrice') - .mockReturnValueOnce(Promise.resolve('200')) - .mockReturnValueOnce(Promise.resolve('300')) - .mockReturnValueOnce(Promise.resolve('400')); + .mockReturnValueOnce(Promise.resolve(TEST_PRICES.MARKET_1)) + .mockReturnValueOnce(Promise.resolve(TEST_PRICES.MARKET_2)) + .mockReturnValueOnce(Promise.resolve(TEST_PRICES.MARKET_3));
75-78: Add type annotation for consistencyAdd explicit type annotation to match the style used in the first test case:
- const market1 = await PerpetualMarketTable + const market1: PerpetualMarketFromDatabase | undefined = await PerpetualMarketTable
17-95: Consider adding test cases for error scenariosThe test suite would benefit from additional coverage:
- Redis operation failures
- Invalid market IDs
- Race conditions when updating prices
Would you like me to provide example test cases for these scenarios?
indexer/services/ender/src/lib/candles-generator.ts (1)
535-544: Consider optimizing map generationWhile the implementation is correct, consider using
reducefor better performance and readability:export function getOrderbookMidPriceMap(): { [ticker: string]: OrderbookMidPrice } { const start: number = Date.now(); const perpetualMarkets = Object.values(perpetualMarketRefresher.getPerpetualMarketsMap()); - const priceMap: { [ticker: string]: OrderbookMidPrice } = {}; - perpetualMarkets.forEach((perpetualMarket: PerpetualMarketFromDatabase) => { - priceMap[perpetualMarket.ticker] = getOrderbookMidPrice(perpetualMarket.ticker); - }); + const priceMap = perpetualMarkets.reduce((acc, perpetualMarket) => ({ + ...acc, + [perpetualMarket.ticker]: getOrderbookMidPrice(perpetualMarket.ticker), + }), {} as { [ticker: string]: OrderbookMidPrice }); stats.timing(`${config.SERVICE_NAME}.get_orderbook_mid_price_map.timing`, Date.now() - start); return priceMap; }indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts (2)
57-67: Separate logging from mapping to avoid side effectsIncluding side effects like logging within a
mapfunction can lead to unexpected behavior and reduce code readability. It's better to separate the logging from the mapping operation.Refactor the code as follows:
-const priceCacheKeys: string[] = validPairs.map((pair) => { - logger.info({ - at: 'orderbook-mid-prices-cache#fetchAndCacheOrderbookMidPrices', - message: 'Caching orderbook mid price', - cacheKey: pair.cacheKey, - midPrice: pair.midPrice, - }); - return pair.cacheKey; -}); +validPairs.forEach((pair) => { + logger.info({ + at: 'orderbook-mid-prices-cache#fetchAndCacheOrderbookMidPrices', + message: 'Caching orderbook mid price', + cacheKey: pair.cacheKey, + midPrice: pair.midPrice, + }); +}); +const priceCacheKeys: string[] = validPairs.map((pair) => pair.cacheKey);This approach improves code clarity by keeping the mapping function pure and handling side effects separately.
Line range hint
98-124: SimplifyevalAsyncfunction and remove unnecessary bindingThe current implementation of
evalAsyncincludes an unnecessarybindoperation and can be simplified for better readability. SinceevalAsyncdoesn't usethis, binding it toclienthas no effect.Consider refactoring as follows:
-let evalAsync: ( - marketCacheKeys: string[], -) => Promise<string[][]> = ( - marketCacheKeys, -) => { - return new Promise((resolve, reject) => { - const callback: Callback<string[][]> = ( - err: Error | null, - results: string[][], - ) => { - if (err) { - return reject(err); - } - return resolve(results); - }; - - client.evalsha( - getOrderbookMidPricesScript.hash, // The Lua script to get cached prices - marketCacheKeys.length, - ...marketCacheKeys, - callback, - ); - }); -}; -evalAsync = evalAsync.bind(client); + +async function evalAsync( + marketCacheKeys: string[], +): Promise<string[][]> { + return new Promise((resolve, reject) => { + client.evalsha( + getOrderbookMidPricesScript.hash, // The Lua script to get cached prices + marketCacheKeys.length, + ...marketCacheKeys, + (err: Error | null, results: string[][]) => { + if (err) { + return reject(err); + } + resolve(results); + }, + ); + }); +}This simplifies the function definition and removes unnecessary complexity, making the code more maintainable.
indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.ts (3)
30-31: Remove redundant mock reset inbeforeEachThe call to
jest.resetAllMocks()on line 30 already resets all mocks, making the explicitmockReset()on line 31 unnecessary.You can safely remove the redundant
mockReset():beforeEach(async () => { await deleteAllAsync(client); jest.resetAllMocks(); - (OrderbookLevelsCache.getOrderBookMidPrice as jest.Mock).mockReset(); });
57-61: UsemockResolvedValueOnceto sequence mock return valuesIn the loop, using
mockResolvedValueoverwrites the mock for all subsequent calls. To simulate sequential return values for each call offetchAndCacheOrderbookMidPrices, consider usingmockResolvedValueOnce.Refactor the loop as follows:
- for (const price of mockPrices) { - (OrderbookLevelsCache.getOrderBookMidPrice as jest.Mock).mockResolvedValue(price); - await fetchAndCacheOrderbookMidPrices(client, [defaultTicker]); - } + mockPrices.forEach((price) => { + (OrderbookLevelsCache.getOrderBookMidPrice as jest.Mock).mockResolvedValueOnce(price); + }); + await fetchAndCacheOrderbookMidPrices(client, [defaultTicker, defaultTicker, defaultTicker]);
63-71: Simplify asynchronous Redis calls using PromisesUsing callbacks with
client.zrangecan make the code less readable. Promisify the Redis method to leverageasync/awaitfor cleaner code.Implement the change as follows:
+ import { promisify } from 'util'; + const zrangeAsync = promisify(client.zrange).bind(client); ... - client.zrange( - `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${defaultTicker}`, - 0, - -1, - (_: any, response: string[]) => { - expect(response).toEqual(mockPrices); - }, - ); + const response = await zrangeAsync( + `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${defaultTicker}`, + 0, + -1, + ); + expect(response).toEqual(mockPrices);Apply the same pattern to other tests using
client.zrange.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
indexer/packages/postgres/src/index.ts(1 hunks)indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.ts(2 hunks)indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts(3 hunks)indexer/packages/redis/src/caches/scripts.ts(2 hunks)indexer/packages/redis/src/scripts/add_market_price.lua(0 hunks)indexer/packages/redis/src/scripts/add_orderbook_mid_prices.lua(1 hunks)indexer/packages/redis/src/scripts/get_market_median_price.lua(0 hunks)indexer/packages/redis/src/scripts/get_orderbook_mid_prices.lua(1 hunks)indexer/services/ender/__tests__/caches/orderbook-mid-price-memory-cache.test.ts(1 hunks)indexer/services/ender/__tests__/lib/candles-generator.test.ts(11 hunks)indexer/services/ender/src/caches/orderbook-mid-price-memory-cache.ts(1 hunks)indexer/services/ender/src/config.ts(2 hunks)indexer/services/ender/src/index.ts(2 hunks)indexer/services/ender/src/lib/candles-generator.ts(3 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)
💤 Files with no reviewable changes (2)
- indexer/packages/redis/src/scripts/add_market_price.lua
- indexer/packages/redis/src/scripts/get_market_median_price.lua
🔇 Additional comments (20)
indexer/packages/redis/src/scripts/get_orderbook_mid_prices.lua (1)
1-10: Consider implementing time-based filtering
According to the PR objectives, prices older than 30 seconds should be removed. The current implementation doesn't handle this requirement.
Let's verify if this filtering is handled elsewhere:
Consider two potential approaches:
- Add filtering in this script using current timestamp:
local current_time = redis.call("TIME")[1]
local cutoff_time = current_time - 30
local prices = redis.call("ZRANGEBYSCORE", key, cutoff_time, "+inf", "LIMIT", 0, 10)- Implement a separate cleanup job using
ZREMRANGEBYSCOREto periodically remove old prices.
Would you like me to provide a complete implementation of either approach?
indexer/services/ender/src/config.ts (2)
10-10: LGTM: Import statement is correctly placed
The parseInteger import is appropriately grouped with other parsing utilities and is necessary for the new configuration variable.
35-35: Consider adding environment variable documentation
While the 10-second default aligns with the PR objectives, this configuration would benefit from:
- Documentation about the corresponding environment variable name
- Acceptable value ranges or recommendations
- Impact on system performance and cache freshness
This helps operators understand how to tune this value in production environments.
Let's verify if this interval is documented elsewhere:
indexer/packages/redis/src/scripts/add_orderbook_mid_prices.lua (1)
1-38: Verify script performance under expected load
Given that this script will be called every 5 seconds as part of the roundtable job mentioned in the PR objectives, it's crucial to verify its performance under load.
Run the following script to check for any existing performance tests or benchmarks:
✅ Verification successful
Based on the comprehensive test coverage and implementation details found, I can now generate the final response:
Performance characteristics of the Lua script are well-suited for the 5-second update frequency
The script's performance characteristics are verified through the following evidence:
- The script maintains a 30-second rolling window for price data, automatically removing old entries
- Implementation includes efficient batch processing for multiple markets in a single Redis operation
- Test coverage demonstrates correct handling of:
- Multiple concurrent market updates
- Precision for both large and small numbers
- Memory management through automatic pruning of expired data
- Edge cases including empty states and different market combinations
The script's design aligns well with the 5-second update frequency requirement, utilizing Redis's sorted sets for efficient time-based operations and cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for performance tests related to the Redis script
# Search for performance/load tests
rg -g "**/*.{js,ts,lua}" -i "(benchmark|perf|load).*(test|spec)" indexer/packages/redis/
# Search for documentation about performance requirements
rg -g "**/*.{md,txt}" -i "(performance|scaling|load)" indexer/packages/redis/
Length of output: 465
Script:
#!/bin/bash
# Let's check the actual script loading and usage patterns
# Check how the Lua script is loaded and used
rg -A 10 "add_orderbook_mid_prices" indexer/packages/redis/
# Check the test files for this functionality
fd "test|spec" indexer/packages/redis/ --exec cat {}
# Look for any configuration or constants related to Redis performance
rg -g "**/*.{js,ts}" "(REDIS_|redis.*config)" indexer/packages/redis/
Length of output: 91504
Script:
#!/bin/bash
# Let's check the actual usage of this Lua script and its test coverage
# Check for test files related to the orderbook mid prices
rg -g "**/*.test.{js,ts}" "orderbook.*mid.*price" indexer/packages/redis/
# Check for the actual implementation and usage of the script
rg -g "**/*.{js,ts}" "addOrderbookMidPricesScript" indexer/packages/redis/
# Check for any rate limiting or batch processing configurations
rg -g "**/*.{js,ts}" "(batch|rate.*limit|interval)" indexer/packages/redis/
Length of output: 935
Script:
#!/bin/bash
# Let's examine the test file and implementation to understand the performance characteristics
# Check the test file content
cat indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.ts
# Check the implementation file
cat indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts
# Look for any performance-related configurations in the codebase
rg -g "**/*.{js,ts}" "TIMEOUT|INTERVAL|WINDOW" indexer/packages/redis/src/
Length of output: 16564
indexer/services/ender/src/caches/orderbook-mid-price-memory-cache.ts (1)
1-14: LGTM! Clean imports and well-defined interface.
The interface definition is appropriately typed for an in-memory cache mapping tickers to prices.
indexer/services/ender/src/index.ts (2)
32-33: LGTM! Clear documentation of cache refresh exceptions.
The comment clearly explains the exceptions to the cache refresh policy, which aligns with the PR objectives.
35-35: Verify cache update frequency implementation.
The PR objectives specify that the Ender cache should update every 10 seconds. Let's verify the implementation of this timing in the cache's start method.
✅ Verification successful
Cache update frequency implementation is correct
The implementation aligns with the 10-second requirement:
- The cache refresh interval is configured as 10,000ms (10 seconds) in
config.ts - The
start()method usesloopHelpers.startUpdateLoopwith this interval to periodically update the cache - Error handling and timing metrics are properly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the cache update frequency implementation
# Search for timing configuration in the cache implementation
rg -A 5 "start.*=.*async" ./indexer/services/ender/src/caches/orderbook-mid-price-memory-cache
# Look for any interval/timeout configurations
rg -A 2 "setInterval|setTimeout" ./indexer/services/ender/src/caches/orderbook-mid-price-memory-cache
Length of output: 418
Script:
#!/bin/bash
# Let's first find the correct path to the cache implementation
fd "orderbook.*cache" --type f
# Then search for the implementation details
rg -l "class.*OrderbookMidPriceMemoryCache"
Length of output: 499
Script:
#!/bin/bash
# Now that we have the correct path, let's examine the implementation
cat indexer/services/ender/src/caches/orderbook-mid-price-memory-cache.ts
# Also check the test file for any timing-related configurations
cat indexer/services/ender/__tests__/caches/orderbook-mid-price-memory-cache.test.ts
Length of output: 4944
Script:
#!/bin/bash
# Let's check the config file for the refresh interval value
cat indexer/services/ender/src/config.ts | grep -A 2 "ORDERBOOK_MID_PRICE_REFRESH"
Length of output: 178
indexer/services/ender/__tests__/caches/orderbook-mid-price-memory-cache.test.ts (3)
1-9: LGTM! Imports are well-organized and appropriate.
All necessary dependencies are properly imported for testing the cache functionality.
10-24: 🛠️ Refactor suggestion
Consider adding test coverage for timing-related requirements.
The PR objectives mention specific timing requirements that should be tested:
- 30-second expiry for cached prices
- 5-second roundtable job updates
- 10-second ender cache updates
Consider adding test cases using jest's timer mocks to verify these timing behaviors.
Example test structure:
describe('timing behaviors', () => {
beforeEach(() => {
jest.useFakeTimers();
});
afterEach(() => {
jest.useRealTimers();
});
it('should expire prices older than 30 seconds', () => {
// Test implementation
});
it('should update cache every 5 seconds via roundtable job', () => {
// Test implementation
});
it('should update ender cache every 10 seconds', () => {
// Test implementation
});
});1-83: Verify complete test coverage for cache improvements.
While the test file provides good basic coverage, consider adding integration tests to verify the complete cache improvement workflow:
- Roundtable job updating prices every 5 seconds
- Ender component retrieving median prices every 10 seconds
- Candles' orderbook mid prices using median prices from the cache
Run the following to check for related test coverage:
✅ Verification successful
Test coverage is comprehensive for cache improvements
The test coverage is complete and verifies all aspects of the cache improvements:
-
cache-orderbook-mid-prices.test.tsverifies the roundtable job updating prices:- Tests caching mid prices for all markets
- Handles undefined prices
- Verifies Redis cache updates
-
candles-generator.test.tsverifies the Ender component's cache usage:- Tests orderbook mid price updates every 10 seconds
- Verifies price retrieval from cache for candle generation
- Tests empty candle creation with cached prices
- Validates price updates for previous candles
-
orderbook-mid-price-memory-cache.test.tsverifies the memory cache:- Tests basic price retrieval
- Verifies cache updates
- Tests error handling and logging
- Validates timing stats
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related test files and their coverage
fd -e test.ts -e spec.ts . | rg -i "candles|orderbook|cache"
# Check for roundtable job tests
rg -A 5 "describe.*roundtable.*job"
# Check for ender cache tests
rg -A 5 "describe.*ender.*cache"
Length of output: 1988
Script:
#!/bin/bash
# Let's check the roundtable cache task implementation and tests
cat indexer/services/roundtable/__tests__/tasks/cache-orderbook-mid-prices.test.ts
# Let's also check the candles generator tests for cache usage
cat indexer/services/ender/__tests__/lib/candles-generator.test.ts
# And check the implementation of the memory cache
cat indexer/services/ender/src/caches/orderbook-mid-price-memory-cache.ts
Length of output: 32818
indexer/services/roundtable/__tests__/tasks/cache-orderbook-mid-prices.test.ts (1)
1-16: LGTM! Imports are well-organized.
The imports are properly grouped by package and all imported entities are utilized in the test file.
indexer/packages/redis/src/caches/scripts.ts (3)
80-81: LGTM! Consistent registration of new scripts.
The new scripts are properly registered in the allLuaScripts array, ensuring they'll be loaded and validated during initialization.
66-67: LGTM! New Redis Lua scripts for orderbook mid prices cache.
The addition of these scripts aligns with the PR objectives for implementing an orderbook mid price cache system.
Let's verify the existence and content of the new Lua scripts:
✅ Verification successful
✅ Verified: Redis Lua scripts properly implement orderbook mid prices cache
The Lua scripts correctly implement the requirements:
add_orderbook_mid_prices.lua:- Implements 30-second expiry using
ZREMRANGEBYSCOREwith cutoff time - Uses
ZADDfor timestamp-based ordering - Includes proper validation for timestamps and prices
- Implements 30-second expiry using
get_orderbook_mid_prices.lua:- Retrieves prices using
ZRANGEwith a limit of 10 entries - Returns an array of prices for each market key
- Retrieves prices using
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new Lua scripts exist and contain the expected cache timing logic
# Check if the scripts exist
echo "Checking for new Lua scripts..."
fd "add_orderbook_mid_prices.lua|get_orderbook_mid_prices.lua" --type f
# Verify the 30-second expiry logic in add_orderbook_mid_prices.lua
echo "Checking for 30-second expiry logic..."
rg -A 2 "30" "add_orderbook_mid_prices.lua"
# Verify the sorted set usage for timestamp-based ordering
echo "Checking for sorted set operations..."
rg "ZADD|ZREMRANGEBYSCORE" "add_orderbook_mid_prices.lua"
Length of output: 684
Script:
#!/bin/bash
# Let's read the content of both Lua scripts using their full paths
echo "Content of add_orderbook_mid_prices.lua:"
cat "indexer/packages/redis/src/scripts/add_orderbook_mid_prices.lua"
echo -e "\nContent of get_orderbook_mid_prices.lua:"
cat "indexer/packages/redis/src/scripts/get_orderbook_mid_prices.lua"
Length of output: 1643
66-67: Verify complete migration from old market price scripts.
Since addMarketPriceScript and getMarketMedianScript were removed, let's ensure there are no lingering references to these old scripts.
✅ Verification successful
Since no references to the old scripts (addMarketPriceScript and getMarketMedianScript) were found in the TypeScript files, and no old Lua script files were found in the codebase, it appears that the migration was complete. However, let's verify the existence and implementation of the new scripts to ensure the functionality is properly replaced.
Migration from old market price scripts is complete
The verification confirms that:
- The old scripts (
addMarketPriceScriptandgetMarketMedianScript) have been completely removed with no lingering references - The new scripts exist at the correct locations and are properly integrated:
indexer/packages/redis/src/scripts/add_orderbook_mid_prices.luaindexer/packages/redis/src/scripts/get_orderbook_mid_prices.lua
- The new scripts are properly imported and used in
orderbook-mid-prices-cache.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the removed scripts
echo "Checking for references to removed scripts..."
rg -i "addMarketPriceScript|getMarketMedianScript" --type ts
echo "Checking for old Lua script files..."
fd "add_market_price\.lua|get_market_median\.lua" --type f
Length of output: 307
Script:
#!/bin/bash
# Check for the new script files and their implementation
echo "Checking for new Lua script files..."
fd "add_orderbook_mid_prices\.lua|get_orderbook_mid_prices\.lua" --type f
echo "Checking implementation references of new scripts..."
rg -i "addOrderbookMidPricesScript|getOrderbookMidPricesScript" --type ts -A 2
Length of output: 2150
indexer/packages/postgres/src/index.ts (2)
74-74: LGTM! Export follows established patterns.
The new export for loopHelpers follows the established naming and organization patterns in the barrel file.
74-74: Verify usage in cache implementation.
Since this PR implements various timed operations (5s updates for roundtable job, 10s updates for ender cache), let's verify that the exported loopHelpers are being utilized appropriately in the cache implementation.
✅ Verification successful
Loop helpers are correctly utilized in the cache implementation
The verification confirms that loopHelpers are being used appropriately in the caching mechanism:
- The
orderbook-mid-price-memory-cache.tscorrectly usesloopHelpers.startUpdateLoopwith a configurable refresh interval - The loop helper implementation provides proper error handling and logging
- The cache implementation in roundtable service properly handles market data updates and error cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of loopHelpers in the new caching mechanism
# Expected: Find references to loopHelpers in the cache implementation files
# Search for imports of loopHelpers
rg -l "loopHelpers|from '\.\./postgres'" --type ts
# Search for specific cache-related files that might use loopHelpers
fd -e ts -e js "cache|orderbook.*price" --type f
# Look for files containing both cache and loopHelpers references
rg -l "cache.*loopHelpers|loopHelpers.*cache" --type ts
Length of output: 3014
Script:
#!/bin/bash
# Let's examine the content of the orderbook-mid-price-memory-cache.ts which imports loopHelpers
rg -A 10 "loopHelpers" indexer/services/ender/src/caches/orderbook-mid-price-memory-cache.ts
# Also check the cache-orderbook-mid-prices task implementation
cat indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts
# Let's also check the implementation of loopHelper itself
cat indexer/packages/postgres/src/loops/loopHelper.ts
Length of output: 2863
indexer/services/roundtable/src/index.ts (1)
13-13: LGTM: Import statement follows established patterns
The import statement is correctly placed and follows the existing code organization patterns.
indexer/services/roundtable/src/config.ts (2)
64-64: LGTM! Feature flag follows existing patterns.
The new configuration flag follows the established pattern and is correctly placed within the LOOPS_ENABLED section.
142-144: Verify interval alignment with requirements
The current implementation uses a 10-second interval (based on offline discussion), but this differs from the PR objectives which specify a 5-second interval for cache updates. Please clarify which interval should be used.
Additionally, consider adding a configuration for the cache expiry time (30 seconds as per PR objectives) to make it configurable rather than hardcoding it.
Example configuration to consider adding:
+ CACHE_ORDERBOOK_MID_PRICES_EXPIRY_MS: parseInteger({
+ default: THIRTY_SECONDS_IN_MILLISECONDS,
+ }),indexer/services/ender/src/lib/candles-generator.ts (1)
29-29: LGTM: Import aligns with cache improvements
The addition of getOrderbookMidPrice import supports the PR's objective to implement in-memory caching for orderbook mid prices.
| local results = {} | ||
| for i, key in ipairs(KEYS) do | ||
| -- Get the prices for each key, but limit to a maximum of 10 | ||
| local prices = redis.call("ZRANGE", key, 0, 9) | ||
| results[i] = prices | ||
| end |
There was a problem hiding this comment.
Add input validation and error handling
The script should validate inputs and handle potential Redis errors. Also, consider using ZREVRANGE to ensure we get the most recent prices first, as sorted sets typically store timestamps as scores.
+-- Validate input
+if #KEYS == 0 then
+ return redis.error_reply("No keys provided")
+end
+
local results = {}
for i, key in ipairs(KEYS) do
-- Get the prices for each key, but limit to a maximum of 10
- local prices = redis.call("ZRANGE", key, 0, 9)
+ -- Use ZREVRANGE to get most recent prices first (assuming scores are timestamps)
+ local prices = redis.call("ZREVRANGE", key, 0, 9)
+ if not prices then
+ return redis.error_reply("Failed to retrieve prices for key: " .. key)
+ end
results[i] = prices
end📝 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.
| local results = {} | |
| for i, key in ipairs(KEYS) do | |
| -- Get the prices for each key, but limit to a maximum of 10 | |
| local prices = redis.call("ZRANGE", key, 0, 9) | |
| results[i] = prices | |
| end | |
| -- Validate input | |
| if #KEYS == 0 then | |
| return redis.error_reply("No keys provided") | |
| end | |
| local results = {} | |
| for i, key in ipairs(KEYS) do | |
| -- Get the prices for each key, but limit to a maximum of 10 | |
| -- Use ZREVRANGE to get most recent prices first (assuming scores are timestamps) | |
| local prices = redis.call("ZREVRANGE", key, 0, 9) | |
| if not prices then | |
| return redis.error_reply("Failed to retrieve prices for key: " .. key) | |
| end | |
| results[i] = prices | |
| end |
| local numKeys = #KEYS | ||
| local numArgs = #ARGV | ||
|
|
||
| -- Get the timestamp from the last argument | ||
| local timestamp = tonumber(ARGV[numArgs]) | ||
|
|
||
| -- Time window (30 seconds) | ||
| local thirtySeconds = 30 | ||
|
|
||
| -- Validate the timestamp | ||
| if not timestamp then | ||
| return redis.error_reply("Invalid timestamp") | ||
| end |
There was a problem hiding this comment.
Add missing input validation checks
The script should validate that the number of prices matches the number of keys to prevent potential index out of bounds errors or silent data inconsistencies.
Add this validation after line 6:
local numKeys = #KEYS
local numArgs = #ARGV
+
+-- Validate that we have the correct number of arguments (one price per key plus timestamp)
+if numArgs ~= numKeys + 1 then
+ return redis.error_reply("Number of prices does not match number of keys")
+end📝 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.
| local numKeys = #KEYS | |
| local numArgs = #ARGV | |
| -- Get the timestamp from the last argument | |
| local timestamp = tonumber(ARGV[numArgs]) | |
| -- Time window (30 seconds) | |
| local thirtySeconds = 30 | |
| -- Validate the timestamp | |
| if not timestamp then | |
| return redis.error_reply("Invalid timestamp") | |
| end | |
| local numKeys = #KEYS | |
| local numArgs = #ARGV | |
| -- Validate that we have the correct number of arguments (one price per key plus timestamp) | |
| if numArgs ~= numKeys + 1 then | |
| return redis.error_reply("Number of prices does not match number of keys") | |
| end | |
| -- Get the timestamp from the last argument | |
| local timestamp = tonumber(ARGV[numArgs]) | |
| -- Time window (30 seconds) | |
| local thirtySeconds = 30 | |
| -- Validate the timestamp | |
| if not timestamp then | |
| return redis.error_reply("Invalid timestamp") | |
| end |
| } catch (error) { | ||
| logger.error({ | ||
| at: 'cache-orderbook-mid-prices#runTask', | ||
| message: (error as Error).message, | ||
| error, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and monitoring
The current error handling could be improved in several ways:
- Catch specific error types separately
- Consider adding metrics for monitoring cache operations
- Potentially propagate errors for critical failures
- } catch (error) {
+ } catch (error: unknown) {
+ // Track error metrics
+ if (error instanceof Error) {
logger.error({
at: 'cache-orderbook-mid-prices#runTask',
message: error.message,
error,
});
+ } else {
+ logger.error({
+ at: 'cache-orderbook-mid-prices#runTask',
+ message: 'Unknown error occurred',
+ error,
+ });
+ }
+ // Propagate error for critical failures
+ 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.
| } catch (error) { | |
| logger.error({ | |
| at: 'cache-orderbook-mid-prices#runTask', | |
| message: (error as Error).message, | |
| error, | |
| }); | |
| } | |
| } catch (error: unknown) { | |
| // Track error metrics | |
| if (error instanceof Error) { | |
| logger.error({ | |
| at: 'cache-orderbook-mid-prices#runTask', | |
| message: error.message, | |
| error, | |
| }); | |
| } else { | |
| logger.error({ | |
| at: 'cache-orderbook-mid-prices#runTask', | |
| message: 'Unknown error occurred', | |
| error, | |
| }); | |
| } | |
| // Propagate error for critical failures | |
| throw error; | |
| } |
| export async function updateOrderbookMidPrices(): Promise<void> { | ||
| const startTime: number = Date.now(); | ||
| try { | ||
| const perpetualMarkets: PerpetualMarketFromDatabase[] = Object.values( | ||
| perpetualMarketRefresher.getPerpetualMarketsMap(), | ||
| ); | ||
|
|
||
| const tickers: string[] = perpetualMarkets.map((market) => market.ticker); | ||
|
|
||
| orderbookMidPriceCache = await OrderbookMidPricesCache.getMedianPrices( | ||
| redisClient, | ||
| tickers, | ||
| ); | ||
|
|
||
| // Log out each median price for each market | ||
| Object.entries(orderbookMidPriceCache).forEach(([ticker, price]) => { | ||
| logger.info({ | ||
| at: 'orderbook-mid-price-cache#updateOrderbookMidPrices', | ||
| message: `Median price for market ${ticker}`, | ||
| ticker, | ||
| price, | ||
| }); | ||
| }); | ||
|
|
||
| } catch (error) { | ||
| logger.error({ | ||
| at: 'orderbook-mid-price-cache#updateOrderbookMidPrices', | ||
| message: 'Failed to fetch OrderbookMidPrices', | ||
| error, | ||
| }); | ||
| } finally { | ||
| stats.timing( | ||
| `${config.SERVICE_NAME}.update_orderbook_mid_prices_cache.timing`, | ||
| Date.now() - startTime, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider more specific error handling and memory management.
Two suggestions for improvement:
- The catch block could be more specific about the type of error being caught and handle different error scenarios appropriately.
- Reassigning the entire cache object might have memory implications. Consider updating individual entries instead.
export async function updateOrderbookMidPrices(): Promise<void> {
const startTime: number = Date.now();
try {
const perpetualMarkets: PerpetualMarketFromDatabase[] = Object.values(
perpetualMarketRefresher.getPerpetualMarketsMap(),
);
const tickers: string[] = perpetualMarkets.map((market) => market.ticker);
- orderbookMidPriceCache = await OrderbookMidPricesCache.getMedianPrices(
+ const newPrices = await OrderbookMidPricesCache.getMedianPrices(
redisClient,
tickers,
);
+
+ // Update individual entries to avoid memory churn
+ Object.entries(newPrices).forEach(([ticker, price]) => {
+ orderbookMidPriceCache[ticker] = price;
+ });
// Log out each median price for each market
Object.entries(orderbookMidPriceCache).forEach(([ticker, price]) => {
logger.info({
at: 'orderbook-mid-price-cache#updateOrderbookMidPrices',
message: `Median price for market ${ticker}`,
ticker,
price,
});
});
- } catch (error) {
+ } catch (error: unknown) {
+ // Handle specific error types
+ if (error instanceof Redis.ConnectionError) {
+ logger.error({
+ at: 'orderbook-mid-price-cache#updateOrderbookMidPrices',
+ message: 'Redis connection error while fetching OrderbookMidPrices',
+ error,
+ });
+ } else {
logger.error({
at: 'orderbook-mid-price-cache#updateOrderbookMidPrices',
message: 'Failed to fetch OrderbookMidPrices',
error,
});
+ }
} finally {
stats.timing(
`${config.SERVICE_NAME}.update_orderbook_mid_prices_cache.timing`,Committable suggestion skipped: line range outside the PR's diff.
| /** | ||
| * Refresh loop to cache the list of all perpetual markets from the database in-memory. | ||
| */ | ||
| export async function start(): Promise<void> { | ||
| await loopHelpers.startUpdateLoop( | ||
| updateOrderbookMidPrices, | ||
| config.ORDERBOOK_MID_PRICE_REFRESH_INTERVAL_MS, | ||
| 'updateOrderbookMidPrices', | ||
| ); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Refresh interval is set to 10 seconds instead of required 5 seconds
The configuration in indexer/services/ender/src/config.ts shows that ORDERBOOK_MID_PRICE_REFRESH_INTERVAL_MS is set to 10,000ms (10 seconds), but according to the PR objectives, it should be 5 seconds (5,000ms).
- Update
ORDERBOOK_MID_PRICE_REFRESH_INTERVAL_MSinindexer/services/ender/src/config.tsfrom 10,000ms to 5,000ms to meet the PR requirements.
🔗 Analysis chain
Verify the refresh interval configuration.
The PR objectives specify a 5-second refresh interval for the orderbook mid price cache. Let's verify this configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the refresh interval in config matches PR requirements (5 seconds)
rg "ORDERBOOK_MID_PRICE_REFRESH_INTERVAL_MS.*=.*5000" --type ts
Length of output: 65
Script:
#!/bin/bash
# Let's first find the config file that defines this constant
rg "ORDERBOOK_MID_PRICE_REFRESH_INTERVAL_MS" --type ts -l
# Then we'll look at its actual value
rg "ORDERBOOK_MID_PRICE_REFRESH_INTERVAL_MS" --type ts
Length of output: 470
| setCachePrice(ticker, '100000'); | ||
| setCachePrice(ticker, '105000'); | ||
| setCachePrice(ticker, '110000'); |
There was a problem hiding this comment.
Await calls to the asynchronous 'setCachePrice' function
Since setCachePrice is an asynchronous function, its invocations should be awaited to ensure that the Redis operations complete before proceeding.
Apply this diff to fix the issue:
-setCachePrice(ticker, '100000');
-setCachePrice(ticker, '105000');
-setCachePrice(ticker, '110000');
+await setCachePrice(ticker, '100000');
+await setCachePrice(ticker, '105000');
+await setCachePrice(ticker, '110000');📝 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.
| setCachePrice(ticker, '100000'); | |
| setCachePrice(ticker, '105000'); | |
| setCachePrice(ticker, '110000'); | |
| await setCachePrice(ticker, '100000'); | |
| await setCachePrice(ticker, '105000'); | |
| await setCachePrice(ticker, '110000'); |
| setCachePrice(ticker, '80000'); | ||
| setCachePrice(ticker, '81000'); | ||
| setCachePrice(ticker, '80500'); |
There was a problem hiding this comment.
Await calls to the asynchronous 'setCachePrice' function
Again, ensure that you await the calls to setCachePrice to handle the asynchronous nature of the function.
Apply this diff:
-setCachePrice(ticker, '80000');
-setCachePrice(ticker, '81000');
-setCachePrice(ticker, '80500');
+await setCachePrice(ticker, '80000');
+await setCachePrice(ticker, '81000');
+await setCachePrice(ticker, '80500');📝 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.
| setCachePrice(ticker, '80000'); | |
| setCachePrice(ticker, '81000'); | |
| setCachePrice(ticker, '80500'); | |
| await setCachePrice(ticker, '80000'); | |
| await setCachePrice(ticker, '81000'); | |
| await setCachePrice(ticker, '80500'); |
| const setCachePrice = (marketTicker: string, price: string) => { | ||
| const now = Date.now(); | ||
| redisClient.zadd(`${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${marketTicker}`, now, price); | ||
| }; |
There was a problem hiding this comment.
Missing 'await' in asynchronous Redis operation
The redisClient.zadd function is asynchronous and returns a Promise. Not handling the Promise may lead to unhandled rejections or unexpected behavior. The function setCachePrice should be declared as async, and the zadd operation should be awaited.
Apply this diff to fix the issue:
-const setCachePrice = (marketTicker: string, price: string) => {
+const setCachePrice = async (marketTicker: string, price: string) => {
const now = Date.now();
- redisClient.zadd(`${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${marketTicker}`, now, price);
+ await redisClient.zadd(`${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${marketTicker}`, now, price);
};📝 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.
| const setCachePrice = (marketTicker: string, price: string) => { | |
| const now = Date.now(); | |
| redisClient.zadd(`${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${marketTicker}`, now, price); | |
| }; | |
| const setCachePrice = async (marketTicker: string, price: string) => { | |
| const now = Date.now(); | |
| await redisClient.zadd(`${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${marketTicker}`, now, price); | |
| }; |
| @@ -435,7 +440,8 @@ describe('candleHelper', () => { | |||
| containsKafkaMessages: boolean = true, | |||
| orderbookMidPrice: number, | |||
| ) => { | |||
| await OrderbookMidPricesCache.setPrice(redisClient, 'BTC-USD', orderbookMidPrice.toFixed()); | |||
| setCachePrice('BTC-USD', orderbookMidPrice.toFixed()); | |||
There was a problem hiding this comment.
Await the asynchronous 'setCachePrice' function
Ensure to await the setCachePrice call to handle the asynchronous operation properly.
Apply this diff:
-setCachePrice('BTC-USD', orderbookMidPrice.toFixed());
+await setCachePrice('BTC-USD', orderbookMidPrice.toFixed());📝 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.
| setCachePrice('BTC-USD', orderbookMidPrice.toFixed()); | |
| await setCachePrice('BTC-USD', orderbookMidPrice.toFixed()); |
| } from '@dydxprotocol-indexer/redis'; | ||
| import { ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX } from '@dydxprotocol-indexer/redis/build/src/caches/orderbook-mid-prices-cache'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid importing from the 'build' directory
Importing from the 'build' directory directly can lead to issues with portability and maintainability. It is better to import from the source module or an established entry point to ensure consistent behavior across different environments.
Apply this diff to update the import statement:
-import { ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX } from '@dydxprotocol-indexer/redis/build/src/caches/orderbook-mid-prices-cache';
+import { ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX } from '@dydxprotocol-indexer/redis';Committable suggestion skipped: line range outside the PR's diff.
|
@mergify backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit 7454ced)
Co-authored-by: Adam Fraser <[email protected]>
Additions:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation