Add Orderbook Mid Price Cache #2289
Conversation
WalkthroughThe changes introduce a new caching mechanism for order book mid prices using Redis, including the implementation of two primary functions: Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (12)
Files skipped from review as they are similar to previous changes (12)
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-fix
branch
from
eabb5c0 to
48a133a
Compare
adamfraser
force-pushed
the
adam/candles-fix
branch
from
48a133a to
3aef283
Compare
adamfraser
force-pushed
the
adam/candles-fix
branch
from
3aef283 to
8ed6229
Compare
adamfraser
force-pushed
the
adam/candles-fix
branch
from
8ed6229 to
510dc80
Compare
adamfraser
force-pushed
the
adam/candles-fix
branch
2 times, most recently
from
a0ad34f to
af61f8e
Compare
adamfraser
force-pushed
the
adam/candles-fix
branch
from
af61f8e to
18e10e4
Compare
adamfraser
force-pushed
the
adam/candles-fix
branch
from
18e10e4 to
7e9675d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 13
Outside diff range and nitpick comments (11)
indexer/packages/redis/src/scripts/add_market_price.lua (2)
1-8: LGTM! Consider adding input validation.The variable declarations and initializations look good. The use of
KEYS[1]for the Redis key andARGV[1]andARGV[2]for the price and timestamp is correct. Converting the arguments to numbers usingtonumber()is a good practice.Consider adding input validation to ensure that both
priceandnowSecondsare valid numbers. For example:if not price or not nowSeconds then return redis.error_reply("Invalid input: price and timestamp must be numbers") end
1-17: Overall, the implementation looks solid. Consider adding documentation.The script effectively implements a 5-second cache for market prices using Redis sorted sets, aligning well with the PR objectives. The use of Redis commands is appropriate and should perform efficiently for high-frequency updates.
Consider adding a brief comment at the beginning of the script to explain its purpose and expected inputs. For example:
--[[ add_market_price.lua This script adds a new market price to a Redis sorted set and removes entries older than 5 seconds. Input: KEYS[1]: The Redis key for the sorted set storing price data ARGV[1]: The price to be added (number) ARGV[2]: The current timestamp in seconds (number) Returns: The number of old entries removed from the sorted set ]]This documentation would make the script more maintainable and easier for other developers to understand and use.
indexer/packages/redis/src/scripts/get_market_median_price.lua (1)
1-5: LGTM! Consider adding a comment for clarity.The input handling and data retrieval look correct. The script properly uses KEYS[1] for the sorted set key and retrieves all prices using 'zrange'.
Consider adding a comment explaining the expected format of the sorted set, e.g.:
-- Retrieve all prices from the sorted set (format: score = timestamp, member = price)This would provide clarity on the data structure being used.
indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts (1)
1-40: Unit tests are needed for this fileAs mentioned in a previous review comment, unit tests should be added to ensure the reliability and correctness of this implementation. Tests should cover:
- Successful price updates for multiple markets
- Handling of undefined prices
- Error cases (e.g., database errors, cache errors)
Would you like me to provide a skeleton for these unit tests or create a GitHub issue to track this task?
indexer/packages/redis/src/caches/scripts.ts (1)
66-67: LGTM! Consider adding comments for clarity.The new Lua scripts for adding market price and getting market median price are correctly implemented using the
newLuaScriptfunction, consistent with other script definitions in the file.Consider adding brief comments explaining the purpose of these new scripts, similar to the comments for other scripts in this file. For example:
// Lua Scripts for managing market prices export const addMarketPriceScript: LuaScript = newLuaScript('addMarketPrice', '../scripts/add_market_price.lua'); export const getMarketMedianScript: LuaScript = newLuaScript('getMarketMedianPrice', '../scripts/get_market_median_price.lua');indexer/services/roundtable/__tests__/tasks/cache-orderbook-mid-prices.test.ts (1)
29-43: LGTM: Test suite setup and teardown are comprehensive.The setup and teardown processes ensure a clean state for each test, which is crucial for reliable testing. The handling of database migrations, data clearing, Redis cache clearing, and mock resetting is well-implemented.
Consider adding a
afterEachblock to reset mocks after each test, in addition to theafterAllblock. This can help isolate tests further and prevent any potential inter-test dependencies. For example:afterEach(() => { jest.resetAllMocks(); });indexer/services/roundtable/src/index.ts (2)
269-275: LGTM: New loop for caching orderbook mid prices.The implementation follows the established pattern for starting loops and aligns with the PR objectives. The use of configuration variables allows for flexible deployment.
For consistency with other loops in this file, consider adding a lock multiplier configuration:
if (config.LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICES) { startLoop( cacheOrderbookMidPrices, 'cache_orderbook_mid_prices', config.LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES, + config.CACHE_ORDERBOOK_MID_PRICES_LOCK_MULTIPLIER, ); }This change would require adding the
CACHE_ORDERBOOK_MID_PRICES_LOCK_MULTIPLIERto the configuration file.
Missing Unit Tests for
cacheOrderbookMidPricesTaskThe
cacheOrderbookMidPricestask is implemented, and the relevant configuration variables are properly defined. However, no unit tests have been found for this task. It is recommended to add unit tests to ensure the functionality is correctly implemented and to prevent potential regressions.
- Add unit tests for
cacheOrderbookMidPricesin the test suite.Analysis chain
Line range hint
1-276: Summary: Changes align well with PR objectives.The modifications to this file successfully implement the caching of orderbook mid prices as described in the PR objectives. The changes are minimal, focused, and follow existing patterns, which reduces the risk of introducing bugs. The use of configuration variables allows for flexible deployment and easy adjustment of the new feature.
To fully validate these changes:
- Ensure that the
cacheOrderbookMidPricestask is implemented correctly in the./tasks/cache-orderbook-mid-pricesfile.- Verify that the necessary configuration variables (
LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICESandLOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES) are properly defined in the configuration file.- Consider adding unit tests for the new loop initialization if not already present.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of cacheOrderbookMidPrices task echo "Checking implementation of cacheOrderbookMidPrices task:" cat ./tasks/cache-orderbook-mid-prices.ts # Verify the configuration variables echo "Checking configuration variables:" grep -E "LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICES|LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES" ./config.ts # Check for existing unit tests echo "Checking for unit tests:" grep -r "cacheOrderbookMidPrices" ./testLength of output: 608
Script:
#!/bin/bash # Search for the cacheOrderbookMidPrices task implementation echo "Searching for 'cache-orderbook-mid-prices.ts' in the repository:" fd "cache-orderbook-mid-prices.ts" # Search for the config.ts file echo "Searching for 'config.ts' in the repository:" fd "config.ts" # Search for unit tests referencing 'cacheOrderbookMidPrices' echo "Searching for references to 'cacheOrderbookMidPrices' in test files:" rg "cacheOrderbookMidPrices" --type testLength of output: 1455
Script:
#!/bin/bash # Search for references to 'cacheOrderbookMidPrices' in test files with common test extensions echo "Searching for references to 'cacheOrderbookMidPrices' in test files (.test.ts and .spec.ts):" rg "cacheOrderbookMidPrices" --glob "*.test.ts" --glob "*.spec.ts" # Verify the relevant configuration variables in the correct config.ts echo "Verifying configuration variables in 'indexer/services/roundtable/src/config.ts':" grep -E "LOOPS_ENABLED_CACHE_ORDERBOOK_MID_PRICES|LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES" indexer/services/roundtable/src/config.tsLength of output: 726
indexer/services/roundtable/src/config.ts (1)
137-139: LGTM: New parameter for orderbook mid price caching intervalThe addition of
LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICESis appropriate and aligns with the PR objectives. The default value of one second matches the requirement to sample the orderbook mid price every second.For consistency with other similar parameters in the file, consider removing the line breaks:
- LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES: parseInteger({ - default: ONE_SECOND_IN_MILLISECONDS, - }), + LOOPS_INTERVAL_MS_CACHE_ORDERBOOK_MID_PRICES: parseInteger({ default: ONE_SECOND_IN_MILLISECONDS }),indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts (2)
10-10: Simplify constant declarationThe type annotation
: stringis redundant for aconstvariable initialized with a string literal. TypeScript can infer the type automatically. Removing the explicit type improves code readability.Apply this minor change:
-export const ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX: string = 'v4/orderbook_mid_prices/'; +export const ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX = 'v4/orderbook_mid_prices/';
17-19: Consider exportinggetOrderbookMidPriceCacheKeyif used externallyIf there's a need to generate the cache key outside of this module (e.g., for testing or other caches), you might want to export the
getOrderbookMidPriceCacheKeyfunction.Export the function as follows:
-function getOrderbookMidPriceCacheKey(ticker: string): string { +export function getOrderbookMidPriceCacheKey(ticker: string): string { return `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`; }
Comments failed to post (13)
indexer/packages/redis/src/scripts/add_market_price.lua (1)
13-17: LGTM! Consider returning more informative data.
The removal of old entries is implemented correctly using
ZREMRANGEBYSCORE. The cutoff time calculation is accurate.Consider returning the number of removed entries instead of just
true. This could provide useful information for monitoring and debugging. You can modify the last line as follows:-return true +return redis.call("zremrangebyscore", priceCacheKey, "-inf", cutoffTime)This change would return the number of elements removed from the sorted set.
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.-- 2. Remove any entries older than 5 seconds local cutoffTime = nowSeconds - fiveSeconds return redis.call("zremrangebyscore", priceCacheKey, "-inf", cutoffTime)indexer/packages/redis/src/scripts/get_market_median_price.lua (2)
1-23: LGTM! Consider adding error handling for robustness.
The script effectively calculates the median price from a Redis sorted set, aligning well with the PR objectives. It handles both even and odd number of prices correctly and deals with the empty data set case.
For increased robustness, consider adding error handling:
- Check if KEYS[1] is provided:
if #KEYS < 1 then return redis.error_reply("Missing required key") end
- Wrap the main logic in a pcall to catch any runtime errors:
local ok, result = pcall(function() -- Existing script logic here end) if not ok then return redis.error_reply("Error: " .. tostring(result)) end return resultThis will make the script more resilient to unexpected inputs or runtime errors.
12-23: LGTM! Consider ensuring compatibility with older Lua versions.
The median calculation logic is correct for both even and odd number of prices. The use of tonumber() and tostring() is appropriate for type conversion.
To ensure compatibility with Lua 5.2 and earlier versions, consider modifying the average calculation:
- local median = (tonumber(prices[middle]) + tonumber(prices[middle + 1])) / 2 + local median = (tonumber(prices[middle]) + tonumber(prices[middle + 1])) * 0.5This avoids potential issues with integer division in older Lua versions.
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.-- Calculate the middle index local middle = math.floor(#prices / 2) -- Calculate median if #prices % 2 == 0 then -- If even, return the average of the two middle elements local median = (tonumber(prices[middle]) + tonumber(prices[middle + 1])) * 0.5 return tostring(median) else -- If odd, return the middle element return prices[middle + 1] endindexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts (1)
19-31: Main logic looks good, but consider rate limiting
The implementation correctly fetches all markets and updates the cache for each. However, consider adding a small delay between iterations to prevent potential rate limiting or high load, especially if there are many markets.
Consider adding a delay between iterations:
import { sleep } from '@dydxprotocol-indexer/base'; // Inside the for loop await sleep(100); // 100ms delayindexer/services/roundtable/__tests__/tasks/cache-orderbook-mid-prices.test.ts (1)
45-98: LGTM: Test cases cover main scenarios, but could be improved.
The test cases effectively cover the main functionality, edge cases, and error scenarios for the
cache-orderbook-mid-pricestask. They verify caching behavior, handling of undefined prices, and error logging.Consider the following improvements to enhance test isolation and coverage:
Use
beforeEachto reset mocks instead of resetting them only inafterAll. This ensures each test starts with fresh mocks.In the "caches mid prices for all markets" test, consider adding assertions for the number of times
getOrderBookMidPricewas called to ensure it's not called more than expected.Add a test case for when
getOrderBookMidPricereturns a valid string that's not a number, to ensure proper error handling.In the "handles errors" test, consider adding an assertion to verify that the price was not cached in the error scenario.
Here's an example of how you might implement some of these suggestions:
beforeEach(() => { jest.resetAllMocks(); }); it('caches mid prices for all markets', async () => { // ... existing test code ... expect(mockGetOrderBookMidPrice).toHaveBeenCalledTimes(2); }); it('handles non-numeric prices', async () => { const market = testConstants.defaultPerpetualMarket; const mockGetOrderBookMidPrice = jest.spyOn(OrderbookLevelsCache, 'getOrderBookMidPrice'); mockGetOrderBookMidPrice.mockResolvedValueOnce('not a number'); await runTask(); const price = await OrderbookMidPricesCache.getMedianPrice(redisClient, market.ticker); expect(price).toBeNull(); expect(jest.requireMock('@dydxprotocol-indexer/base').logger.error).toHaveBeenCalled(); }); it('handles errors', async () => { // ... existing test code ... const price = await OrderbookMidPricesCache.getMedianPrice(redisClient, testConstants.defaultPerpetualMarket.ticker); expect(price).toBeNull(); });These changes will improve the robustness and coverage of your test suite.
indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts (5)
37-68: Refactor duplicated code for
evalAsyncinto a reusable helper functionThe
evalAsyncfunction in bothsetPriceandgetMedianPriceis nearly identical, leading to code duplication. This can make maintenance harder and increases the risk of inconsistencies. Consider abstracting this logic into a reusable helper function to improve modularity and maintainability.Apply the following refactor to create a generic helper function:
+async function evalAsync<T>( + client: RedisClient, + scriptHash: string, + numKeys: number, + keys: string[], + args: any[], +): Promise<T> { + return new Promise<T>((resolve, reject) => { + client.evalsha( + scriptHash, + numKeys, + ...keys, + ...args, + (err: Error | null, result: T) => { + if (err) { + return reject(err); + } + return resolve(result); + }, + ); + }); +}Update
setPriceto use the helper function:export async function setPrice( client: RedisClient, ticker: string, price: string, ): Promise<void> { - 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); - 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); + await evalAsync<void>( + client, + addMarketPriceScript.hash, + 1, + [marketCacheKey], + [price, nowSeconds], + ); }Similarly, update
getMedianPrice: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); - return evalAsync(getOrderbookMidPriceCacheKey(ticker)); + const marketCacheKey = getOrderbookMidPriceCacheKey(ticker); + const result = await evalAsync<string>( + client, + getMarketMedianScript.hash, + 1, + [marketCacheKey], + [], + ); + return result; }Also applies to: 77-107
95-99: Add fallback for missing Lua scripts in Redis
If Redis restarts, the cached Lua scripts might be cleared, causing
evalshato fail with aNOSCRIPTerror. Consider adding fallback logic to load the script usingevalifevalshafails.Implement a fallback mechanism:
client.evalsha( scriptHash, numKeys, ...keys, ...args, (err: Error | null, result: T) => { + if (err && err.message.includes('NOSCRIPT')) { + // Fallback to eval if script is not cached + client.eval( + scriptContent, // The Lua script content + numKeys, + ...keys, + ...args, + (evalErr: Error | null, evalResult: T) => { + if (evalErr) { + return reject(evalErr); + } + return resolve(evalResult); + }, + ); + } else if (err) { return reject(err); } return resolve(result); }, );Ensure that
scriptContentis accessible where needed.Also applies to: 53-59
77-107: Simplify the
getMedianPricefunction for claritySimilar to
setPrice, you can simplifygetMedianPriceby removing unnecessarybindusage and definingevalAsyncas a straightforward function.Simplify
getMedianPriceas follows:export async function getMedianPrice( client: RedisClient, ticker: string, ): Promise<string | null> { - let evalAsync: ( + const evalAsync = ( marketCacheKey: string, ): Promise<string> => { return new Promise((resolve, reject) => { client.evalsha( getMarketMedianScript.hash, 1, marketCacheKey, (err: Error | null, results: string) => { if (err) { return reject(err); } return resolve(results); }, ); }); }; - evalAsync = evalAsync.bind(client); return evalAsync(getOrderbookMidPriceCacheKey(ticker)); }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export async function getMedianPrice(client: RedisClient, ticker: string): Promise<string | null> { const evalAsync = ( marketCacheKey: string, ): Promise<string> => { 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, ); }); }; return evalAsync( getOrderbookMidPriceCacheKey(ticker), ); }
29-68: Simplify the
setPricefunction by removing unnecessarybindThe use of
bindonevalAsyncis unnecessary because theclientis already accessible within the scope. You can simplify the function by removingbindand redefiningevalAsyncas a regular function.Simplify
setPriceas follows:export async function setPrice( client: RedisClient, ticker: string, price: string, ): Promise<void> { const numKeys: number = 1; - let evalAsync: ( + const evalAsync = ( marketCacheKey: string, ): Promise<void> => { return new Promise<void>((resolve, reject) => { const nowSeconds = Math.floor(Date.now() / 1000); client.evalsha( addMarketPriceScript.hash, numKeys, marketCacheKey, price, nowSeconds, (err: Error | null) => { if (err) { return reject(err); } return resolve(); }, ); }); }; - evalAsync = evalAsync.bind(client); return evalAsync(getOrderbookMidPriceCacheKey(ticker)); }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export async function setPrice( client: RedisClient, ticker: string, price: string, ): Promise<void> { // Number of keys for the lua script. const numKeys: number = 1; const evalAsync = ( marketCacheKey: string, ): Promise<void> => { 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, ); }); }; return evalAsync( getOrderbookMidPriceCacheKey(ticker), ); }
85-87: Check for
nullinresultsbefore returningIn the callback of
getMedianPrice, theresultsvariable might benullif no median price is found. Ensure that this case is handled properly to prevent unexpected errors.Modify the callback as follows:
client.evalsha( getMarketMedianScript.hash, 1, marketCacheKey, (err: Error | null, results: string | null) => { if (err) { return reject(err); } + if (results === null) { + return resolve(null); + } return resolve(results); }, );Committable suggestion was skipped due to low confidence.
indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.ts (3)
24-31: Correct the usage of Redis client's
zrangemethodThe
client.zrangemethod returns a Promise and does not accept a callback. Using a callback here is incorrect and could lead to unexpected behavior or errors.Apply this diff to fix the issue:
-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');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 response = await client.zrange( `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`, 0, -1 ); expect(response[0]).toBe('50000');
41-49: Correct the usage of Redis client's
zrangemethodThe
client.zrangemethod returns a Promise and does not accept a callback. Using a callback here is incorrect and could lead to unexpected behavior or errors.Apply this diff to fix the issue:
-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']);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 response = await client.zrange( `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`, 0, -1 ); expect(response).toEqual(['49000', '50000', '51000']); });
81-104: Ensure timers are properly restored after using fake timers
When using
jest.useFakeTimers(), it's important to restore the real timers after the test completes, even if an error occurs, to avoid side effects on other tests.Consider wrapping the test code in a
try...finallyblock to ensurejest.useRealTimers()is always called:it('returns the correct median price after 5 seconds', async () => { + try { jest.useFakeTimers(); // ... rest of the test code ... const result = await getMedianPrice(client, ticker); expect(result).toBe('52500'); - jest.useRealTimers(); + } finally { + jest.useRealTimers(); + } });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.it('returns the correct median price after 5 seconds', async () => { try { jest.useFakeTimers(); const nowSeconds = Math.floor(Date.now() / 1000); jest.setSystemTime(nowSeconds * 1000); await Promise.all([ setPrice(client, ticker, '50000'), setPrice(client, ticker, '51000'), ]); jest.advanceTimersByTime(6000); // Advance time by 6 seconds await Promise.all([ setPrice(client, ticker, '49000'), setPrice(client, ticker, '48000'), setPrice(client, ticker, '52000'), setPrice(client, ticker, '53000'), ]); const result = await getMedianPrice(client, ticker); expect(result).toBe('52500'); } finally { jest.useRealTimers(); } });
This reverts commit 637c237.
Co-authored-by: Adam Fraser <[email protected]>
Changelist
Test Plan
Tested in dev
Added unit tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests