-
Notifications
You must be signed in to change notification settings - Fork 115
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
[IND-402] Add cache for order updates for stateful orders. #682
Conversation
WalkthroughThe changes introduce a stateful order updates cache using Redis. The cache supports adding and removing order updates, and retrieving old updates based on a timestamp. The implementation includes new utility functions, Lua scripts for Redis operations, and tests to ensure the cache behaves as expected. Changes
TipsChat with CodeRabbit Bot (
|
IND-402 Fix Stateful order placement with no fill
Stateful orders that are not filled do not get added to the order book. Issue: Potential Solution:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (8)
- indexer/packages/base/src/utils.ts (1 hunks)
- indexer/packages/redis/tests/caches/stateful-order-updates-cache.test.ts (1 hunks)
- indexer/packages/redis/src/caches/scripts.ts (2 hunks)
- indexer/packages/redis/src/caches/stateful-order-updates-cache.ts (1 hunks)
- indexer/packages/redis/src/index.ts (1 hunks)
- indexer/packages/redis/src/scripts/add_stateful_order_update.lua (1 hunks)
- indexer/packages/redis/src/scripts/remove_stateful_order_update.lua (1 hunks)
- indexer/packages/redis/src/types.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/redis/src/types.ts
Additional comments (Suppressed): 11
indexer/packages/base/src/utils.ts (1)
- 1-14: The function
hasDefinedProperties
is well implemented and follows best practices. It checks if an object has any defined properties and returns a boolean value accordingly. This function can be useful in various scenarios where it's necessary to check if an object has any meaningful data before proceeding with further operations.indexer/packages/redis/src/index.ts (1)
- 12-12: The new
StatefulOrderUpdatesCache
is being exported. Ensure that the implementation of this cache is thread-safe and that it handles potential race conditions correctly. Also, verify that the cache eviction policy aligns with the application's requirements.indexer/packages/redis/src/scripts/add_stateful_order_update.lua (1)
- 1-16: The Lua script for adding a stateful order update to the Redis cache seems to be logically correct. It uses the
ZADD
command to add the order ID and timestamp to the ZSET and theHSET
command to add the order ID and the encoded order update to the HSET. However, it's important to ensure that thestatefulOrderId
,statefulOrderUpdate
, andtimestamp
are correctly provided and formatted before calling this script. Also, consider adding error handling to catch any potential Redis errors.indexer/packages/redis/src/scripts/remove_stateful_order_update.lua (3)
1-4: Ensure that the keys for the ZSET and HSET are unique and do not collide with other keys in the Redis database. This is to prevent accidental overwriting or deletion of data.
23-27: The script checks if the timestamp of the stateful order update is less than the maximum timestamp to remove. However, the comment on line 24 seems to contradict the condition on line 25. Please verify if the condition is correct.
39-41: The script deletes the order update from the HSET and returns the old order update. Ensure that this operation is atomic to prevent race conditions where another process might try to access the order update while it's being deleted.
indexer/packages/redis/src/caches/scripts.ts (2)
63-64: The addition of
addStatefulOrderUpdateScript
andremoveStatefulOrderUpdateScript
is consistent with the PR summary. Ensure that the corresponding Lua scriptsadd_stateful_order_update.lua
andremove_stateful_order_update.lua
are correctly implemented and tested.74-75: The newly added scripts
addStatefulOrderUpdateScript
andremoveStatefulOrderUpdateScript
are included in theallLuaScripts
array. This is necessary for the scripts to be loaded onto the Redis client. Ensure that these scripts are being used appropriately in the rest of the codebase.indexer/packages/redis/__tests__/caches/stateful-order-updates-cache.test.ts (2)
58-108: The test cases for
removeStatefulOrderUpdate
are well written and cover different scenarios. They test the removal of an existing order update, the case where the timestamp is lower, and the removal of a non-existing order.111-163: The test cases for
getOldOrderUpdates
are also well written and cover different scenarios. They test the retrieval of order updates older than a certain threshold and the retrieval of multiple order updates.indexer/packages/redis/src/caches/stateful-order-updates-cache.ts (1)
- 108-127: The
getOldOrderUpdates
function retrieves order updates that were added to the cache before a specified timestamp. It uses thezRangeByScoreAsync
helper function to retrieve the order IDs and timestamps from the ZSET. The function then maps the raw results to an array ofStatefulOrderUpdateInfo
objects. This function is well implemented and does not require any changes.
Changelist
First PR to track order updates for stateful orders.
Adds a cache to hold order update messages for stateful orders.
The cache consists of:
The ZSET is used to query the cache for the order ids for order updates stored in the cache along with the cached timestamp.
Lua scripts are added for both adding and removing order updates from the cache.
Test Plan
Unit tests added.
Author/Reviewer Checklist
state-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.