Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Orderbook Mid Price Cache #2338

Merged
merged 3 commits into from
Sep 25, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Return values from redis without division, perform division in javasc…
…ript
adamfraser committed Sep 24, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit 5a6ddd4a35dead6608518d81f35446590663a5ac
Original file line number Diff line number Diff line change
@@ -13,10 +13,6 @@ describe('orderbook-mid-prices-cache', () => {
await deleteAllAsync(client);
});

afterEach(async () => {
await deleteAllAsync(client);
});

describe('setPrice', () => {
it('sets a price for a ticker', async () => {
await setPrice(client, ticker, '50000');
@@ -102,5 +98,39 @@ describe('orderbook-mid-prices-cache', () => {

jest.useRealTimers();
});

it('returns the correct median price for small numbers with even number of prices', async () => {
await Promise.all([
setPrice(client, ticker, '0.00000000002345'),
setPrice(client, ticker, '0.00000000002346'),
]);

const midPrice1 = await getMedianPrice(client, ticker);
expect(midPrice1).toEqual('0.000000000023455');
});

it('returns the correct median price for small numbers with odd number of prices', async () => {
await Promise.all([
setPrice(client, ticker, '0.00000000001'),
setPrice(client, ticker, '0.00000000002'),
setPrice(client, ticker, '0.00000000003'),
setPrice(client, ticker, '0.00000000004'),
setPrice(client, ticker, '0.00000000005'),
]);

const midPrice1 = await getMedianPrice(client, ticker);
expect(midPrice1).toEqual('0.00000000003');

await deleteAllAsync(client);

await Promise.all([
setPrice(client, ticker, '0.00000847007'),
setPrice(client, ticker, '0.00000847006'),
setPrice(client, ticker, '0.00000847008'),
]);

const midPrice2 = await getMedianPrice(client, ticker);
expect(midPrice2).toEqual('0.00000847007');
});
});
});
30 changes: 25 additions & 5 deletions indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import Big from 'big.js';
import { Callback, RedisClient } from 'redis';

import {
@@ -69,21 +70,23 @@ export async function setPrice(

/**
* Retrieves the median price for a given ticker from the cache.
* Uses a Lua script to calculate the median price from the sorted set in Redis.
* Uses a Lua script to fetch either the middle element (for odd number of prices)
* or the two middle elements (for even number of prices) from a sorted set in Redis.
* If two middle elements are returned, their average is calculated in JavaScript.
* @param client The Redis client
* @param ticker The ticker symbol
* @returns A promise that resolves with the median price as a string, or null if not found
*/
export async function getMedianPrice(client: RedisClient, ticker: string): Promise<string | null> {
let evalAsync: (
marketCacheKey: string,
) => Promise<string> = (
) => Promise<string[]> = (
marketCacheKey,
) => {
return new Promise((resolve, reject) => {
const callback: Callback<string> = (
const callback: Callback<string[]> = (
err: Error | null,
results: string,
results: string[],
) => {
if (err) {
return reject(err);
@@ -101,7 +104,24 @@ export async function getMedianPrice(client: RedisClient, ticker: string): Promi
};
evalAsync = evalAsync.bind(client);

return evalAsync(
const prices = await evalAsync(
getOrderbookMidPriceCacheKey(ticker),
);

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;
Comment on lines +111 to +126
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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();
}

}
Original file line number Diff line number Diff line change
@@ -14,10 +14,9 @@ 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])) / 2
return tostring(median)
-- If even, return both prices, division will be handled in Javascript
return {prices[middle], prices[middle + 1]}
else
-- If odd, return the middle element
return prices[middle + 1]
return {prices[middle + 1]}
end