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
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
import { deleteAllAsync } from '../../src/helpers/redis';
import { redis as client } from '../helpers/utils';
import {
setPrice,
getMedianPrice,
ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX,
} from '../../src/caches/orderbook-mid-prices-cache';

describe('orderbook-mid-prices-cache', () => {
const ticker: string = 'BTC-USD';

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

describe('setPrice', () => {
it('sets a price for a ticker', async () => {
await setPrice(client, ticker, '50000');

await client.zrange(
`${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`,
0,
-1,
(_: any, response: string[]) => {
expect(response[0]).toBe('50000');
},
);
Comment on lines +20 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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

});

it('sets multiple prices for a ticker', async () => {
await Promise.all([
setPrice(client, ticker, '50000'),
setPrice(client, ticker, '51000'),
setPrice(client, ticker, '49000'),
]);

await client.zrange(
`${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`,
0,
-1,
(_: any, response: string[]) => {
expect(response).toEqual(['49000', '50000', '51000']);
},
);
});
});

describe('getMedianPrice', () => {
it('returns null when no prices are set', async () => {
const result = await getMedianPrice(client, ticker);
expect(result).toBeNull();
});

it('returns the median price for odd number of prices', async () => {
await Promise.all([
setPrice(client, ticker, '50000'),
setPrice(client, ticker, '51000'),
setPrice(client, ticker, '49000'),
]);

const result = await getMedianPrice(client, ticker);
expect(result).toBe('50000');
});

it('returns the median price for even number of prices', async () => {
await Promise.all([
setPrice(client, ticker, '50000'),
setPrice(client, ticker, '51000'),
setPrice(client, ticker, '49000'),
setPrice(client, ticker, '52000'),
]);

const result = await getMedianPrice(client, ticker);
expect(result).toBe('50500');
});

it('returns the correct median price after 5 seconds', async () => {
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('50500');

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');
});
});
});
127 changes: 127 additions & 0 deletions indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
import Big from 'big.js';
import { Callback, RedisClient } from 'redis';

import {
addMarketPriceScript,
getMarketMedianScript,
} from './scripts';

// Cache of orderbook prices for each clob pair
// Each price is cached for a 5 second window and in a ZSET
export const ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX: string = 'v4/orderbook_mid_prices/';

/**
* Generates a cache key for a given ticker's orderbook mid price.
* @param ticker The ticker symbol
* @returns The cache key string
*/
function getOrderbookMidPriceCacheKey(ticker: string): string {
return `${ORDERBOOK_MID_PRICES_CACHE_KEY_PREFIX}${ticker}`;
}

/**
* Adds a price to the market prices cache for a given ticker.
* Uses a Lua script to add the price with a timestamp to a sorted set in Redis.
* @param client The Redis client
* @param ticker The ticker symbol
* @param price The price to be added
* @returns A promise that resolves when the operation is complete
*/
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),
);
}
Comment on lines +38 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

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


/**
* Retrieves the median price for a given ticker from the cache.
* 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[]> = (
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);

Comment on lines +81 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

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

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

}
4 changes: 4 additions & 0 deletions indexer/packages/redis/src/caches/scripts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ export const removeOrderScript: LuaScript = newLuaScript('removeOrder', '../scri
export const addCanceledOrderIdScript: LuaScript = newLuaScript('addCanceledOrderId', '../scripts/add_canceled_order_id.lua');
export const addStatefulOrderUpdateScript: LuaScript = newLuaScript('addStatefulOrderUpdate', '../scripts/add_stateful_order_update.lua');
export const removeStatefulOrderUpdateScript: LuaScript = newLuaScript('removeStatefulOrderUpdate', '../scripts/remove_stateful_order_update.lua');
export const addMarketPriceScript: LuaScript = newLuaScript('addMarketPrice', '../scripts/add_market_price.lua');
export const getMarketMedianScript: LuaScript = newLuaScript('getMarketMedianPrice', '../scripts/get_market_median_price.lua');
Comment on lines +66 to +67
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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


export const allLuaScripts: LuaScript[] = [
deleteZeroPriceLevelScript,
Expand All @@ -75,4 +77,6 @@ export const allLuaScripts: LuaScript[] = [
addCanceledOrderIdScript,
addStatefulOrderUpdateScript,
removeStatefulOrderUpdateScript,
addMarketPriceScript,
getMarketMedianScript,
];
1 change: 1 addition & 0 deletions indexer/packages/redis/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export * as CanceledOrdersCache from './caches/canceled-orders-cache';
export * as StatefulOrderUpdatesCache from './caches/stateful-order-updates-cache';
export * as StateFilledQuantumsCache from './caches/state-filled-quantums-cache';
export * as LeaderboardPnlProcessedCache from './caches/leaderboard-processed-cache';
export * as OrderbookMidPricesCache from './caches/orderbook-mid-prices-cache';
export { placeOrder } from './caches/place-order';
export { removeOrder } from './caches/remove-order';
export { updateOrder } from './caches/update-order';
Expand Down
17 changes: 17 additions & 0 deletions indexer/packages/redis/src/scripts/add_market_price.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
-- Key for the ZSET storing price data
local priceCacheKey = KEYS[1]
-- Price to be added
local price = tonumber(ARGV[1])
-- Current timestamp
local nowSeconds = tonumber(ARGV[2])
-- Time window (5 seconds)
local fiveSeconds = 5

-- 1. Add the price to the sorted set (score is the current timestamp)
redis.call("zadd", priceCacheKey, nowSeconds, price)

-- 2. Remove any entries older than 5 seconds
local cutoffTime = nowSeconds - fiveSeconds
redis.call("zremrangebyscore", priceCacheKey, "-inf", cutoffTime)

return true
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

22 changes: 22 additions & 0 deletions indexer/packages/redis/src/scripts/get_market_median_price.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
-- Key for the sorted set storing price data
local priceCacheKey = KEYS[1]

-- Get all the prices from the sorted set (ascending order)
local prices = redis.call('zrange', priceCacheKey, 0, -1)

-- If no prices are found, return nil
if #prices == 0 then
return nil
end

-- Calculate the middle index
local middle = math.floor(#prices / 2)

-- Calculate median
if #prices % 2 == 0 then
-- 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]}
end
Loading
Loading