Revert "Add Orderbook Mid Price Cache (#2289)" #2333
Conversation
This reverts commit 637c237.
WalkthroughThe changes involve the removal of the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
indexer/services/ender/__tests__/lib/candles-generator.test.ts (2)
430-432: LGTM: Dynamic price level updates in parameterized test.The test now uses dynamic values to set buy and sell prices, allowing for more flexible testing of various scenarios. This approach enhances the test's ability to cover different market conditions.
Consider adding a test case with a larger spread between buy and sell prices to ensure the system handles extreme market conditions correctly. For example:
it('handles large price spreads correctly', async () => { const largeMidPriceSpread = 1000; await updatePriceLevel('BTC-USD', String(10000 + largeMidPriceSpread), OrderSide.SELL); await updatePriceLevel('BTC-USD', String(10000 - largeMidPriceSpread), OrderSide.BUY); // ... rest of the test });
663-678: LGTM: Comprehensive test for orderbook price map creation.The test now uses
updatePriceLevelto set price levels for multiple markets, providing a more comprehensive check of the orderbook price map creation. The expected result correctly includes undefined values for markets without set price levels.Consider adding a test case for markets with only one side (buy or sell) set to ensure the system handles incomplete orderbooks correctly. For example:
await updatePriceLevel('LINK-USD', '10000', OrderSide.BUY); // No sell price set for LINK-USD expect(map).toEqual({ // ... other markets 'LINK-USD': undefined, // or however your system handles this case });
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- indexer/packages/redis/tests/caches/orderbook-mid-prices-cache.test.ts (0 hunks)
- indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts (0 hunks)
- indexer/packages/redis/src/caches/scripts.ts (0 hunks)
- indexer/packages/redis/src/index.ts (0 hunks)
- indexer/packages/redis/src/scripts/add_market_price.lua (0 hunks)
- indexer/packages/redis/src/scripts/get_market_median_price.lua (0 hunks)
- indexer/services/ender/tests/lib/candles-generator.test.ts (8 hunks)
- indexer/services/ender/src/lib/candles-generator.ts (2 hunks)
- indexer/services/roundtable/tests/tasks/cache-orderbook-mid-prices.test.ts (0 hunks)
- indexer/services/roundtable/src/config.ts (0 hunks)
- indexer/services/roundtable/src/index.ts (0 hunks)
- indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts (0 hunks)
Files not reviewed due to no reviewable changes (10)
- indexer/packages/redis/tests/caches/orderbook-mid-prices-cache.test.ts
- indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts
- indexer/packages/redis/src/caches/scripts.ts
- indexer/packages/redis/src/index.ts
- indexer/packages/redis/src/scripts/add_market_price.lua
- indexer/packages/redis/src/scripts/get_market_median_price.lua
- indexer/services/roundtable/tests/tasks/cache-orderbook-mid-prices.test.ts
- indexer/services/roundtable/src/config.ts
- indexer/services/roundtable/src/index.ts
- indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts
Additional comments not posted (8)
indexer/services/ender/src/lib/candles-generator.ts (3)
23-23: Summary of changes: Orderbook mid price cache updateThe changes in this file revolve around updating the orderbook mid price cache mechanism:
- The import has been changed from
OrderbookMidPricesCachetoOrderbookLevelsCache.- The method call in
getOrderbookMidPriceMaphas been updated to useOrderbookLevelsCache.getOrderBookMidPrice.These changes appear to be part of a larger refactoring effort to improve the orderbook data caching system. The localized nature of these changes suggests a minimal impact on the overall functionality of the
CandlesGeneratorclass.To ensure a smooth transition:
- Verify that all references to the old cache mechanism have been updated throughout the codebase.
- Update any relevant documentation or comments related to orderbook mid price retrieval.
- Consider adding or updating unit tests to cover the new caching mechanism.
Also applies to: 541-543
541-543: Verify the behavior of the newgetOrderBookMidPricemethod.The method call has been updated from
OrderbookMidPricesCache.getMedianPricetoOrderbookLevelsCache.getOrderBookMidPrice. The new method now takesredisClientas an additional parameter.To ensure the new method behaves as expected:
- Verify the implementation of
OrderbookLevelsCache.getOrderBookMidPrice:#!/bin/bash # Search for the implementation of getOrderBookMidPrice rg -A 10 "getOrderBookMidPrice.*=" --type typescript
- Check if there are any other usages of the old method that need to be updated:
#!/bin/bash # Search for any remaining usage of getMedianPrice rg "getMedianPrice" --type typescript
23-23: Verify the impact of the import change throughout the file.The import has been updated from
OrderbookMidPricesCachetoOrderbookLevelsCache. This change suggests a shift in the caching mechanism for orderbook data.Let's verify if all usages of the old cache have been updated:
indexer/services/ender/__tests__/lib/candles-generator.test.ts (5)
21-21: LGTM: Import ofOrderSideadded.The addition of
OrderSideto the imports suggests that the tests now differentiate between buy and sell orders when updating price levels. This change likely reflects an update in the underlying implementation to more accurately represent the orderbook structure.
158-159: LGTM: Consistent use ofupdatePriceLevelin another test case.The test consistently uses the new
updatePriceLevelfunction to set both buy and sell prices for 'BTC-USD'. This change maintains consistency with the previous test case and aligns with the overall refactoring approach.
597-599: LGTM: Consistent use ofupdatePriceLevelfor updating previous candles.The test case for updating previous candles now uses the
updatePriceLevelfunction consistently with other test cases. This change ensures that the orderbook state is accurately represented when testing the update of previous candles.
Line range hint
1-878: Overall assessment: Consistent and beneficial refactoring.The changes in this test file consistently implement the new
updatePriceLevelfunction across all test cases. This refactoring improves the accuracy of orderbook representation in tests and aligns with the removal of directOrderbookMidPricesCache.setPricecalls mentioned in the summary. The modifications enhance the flexibility of the tests and provide a more comprehensive coverage of various market scenarios.Key improvements:
- Explicit setting of both buy and sell prices for each market.
- More accurate representation of orderbook states in different test scenarios.
- Improved handling of markets with undefined price levels.
These changes should result in more robust and maintainable tests for the candles generator functionality.
116-118: LGTM: Price level updates refactored usingupdatePriceLevel.The test now uses the new
updatePriceLevelfunction to set both buy and sell prices for 'BTC-USD'. This change provides a more accurate representation of the orderbook state and is consistent with the refactoring mentioned earlier.To ensure consistency, please verify if similar changes have been made across all test cases:
Verification successful
:
updatePriceLevelis used consistently across all test cases incandles-generator.test.ts. No instances of the old methodOrderbookMidPricesCache.setPricewere found, ensuring uniform refactoring and accurate orderbook state representation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of updatePriceLevel across test cases # Test: Search for all occurrences of updatePriceLevel in the test file rg "updatePriceLevel" indexer/services/ender/__tests__/lib/candles-generator.test.tsLength of output: 1239
| @@ -31,11 +32,9 @@ import { KafkaPublisher } from '../../src/lib/kafka-publisher'; | |||
| import { ConsolidatedKafkaEvent } from '../../src/lib/types'; | |||
| import { defaultTradeContent, defaultTradeKafkaEvent } from '../helpers/constants'; | |||
| import { contentToSingleTradeMessage, createConsolidatedKafkaEventFromTrade } from '../helpers/kafka-publisher-helpers'; | |||
| import { updatePriceLevel } from '../helpers/redis-helpers'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: updatePriceLevel function is missing in redis-helpers.ts.
The import statement references updatePriceLevel from ../helpers/redis-helpers, but the function does not exist in the specified file. Please verify the implementation of updatePriceLevel or update the import path accordingly.
Analysis chain
LGTM: New helper function updatePriceLevel imported.
The introduction of updatePriceLevel suggests a refactoring of the price level update logic, which should improve code reusability and maintainability. This change is consistent with the removal of direct calls to OrderbookMidPricesCache.setPrice mentioned in the summary.
To ensure the correctness of this refactoring, please verify the implementation of updatePriceLevel in the redis-helpers file:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of updatePriceLevel function
# Test: Check if the function exists and its basic structure
rg -A 10 "export function updatePriceLevel" ../helpers/redis-helpers.ts
Length of output: 141
Script:
#!/bin/bash
# Description: Search for redis-helpers.ts in the entire codebase
fd --type f --exact "redis-helpers.ts"
Length of output: 263
Script:
#!/bin/bash
# Description: Search for redis-helpers.ts in the entire codebase
fd --type f redis-helpers.ts
Length of output: 88
Script:
#!/bin/bash
# Description: Verify the implementation of updatePriceLevel function in redis-helpers.ts
# Test: Check if the function exists and review its basic structure
rg -A 10 "export function updatePriceLevel" indexer/services/ender/__tests__/helpers/redis-helpers.ts
Length of output: 103
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
Co-authored-by: Adam Fraser <[email protected]>
This reverts commit 637c237.
Summary by CodeRabbit
Bug Fixes
New Features
Chores
Tests