[CT-709] Add flag to send subaccount websocket message from Ender for long term orders #1223
Conversation
|
Warning Rate Limit Exceeded@dydxwill has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 1 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe updates primarily focus on enhancing functionality and organization within the indexer's Kafka and Redis components, along with adjustments in the Ender and Vulcan services. Key modifications include the division of a function for better clarity in Kafka, the introduction of a new conversion function in Redis, and the implementation of configuration-based logic for handling stateful order placements in Ender. Vulcan's changes streamline Redis order conversion by centralizing the functionality. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (10)
- indexer/packages/kafka/src/websocket-helper.ts (2 hunks)
- indexer/packages/redis/src/helpers/order-helper.ts (1 hunks)
- indexer/packages/redis/src/index.ts (1 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-placement-handler.test.ts (5 hunks)
- indexer/services/ender/tests/helpers/indexer-proto-helpers.ts (3 hunks)
- indexer/services/ender/src/config.ts (1 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts (2 hunks)
- indexer/services/vulcan/tests/handlers/order-place-handler.test.ts (3 hunks)
- indexer/services/vulcan/src/handlers/helpers.ts (1 hunks)
- indexer/services/vulcan/src/handlers/order-place-handler.ts (2 hunks)
Additional comments: 13
indexer/services/ender/src/config.ts (1)
- 26-28: The addition of the
SEND_SUBACCOUNT_WEBSOCKET_MESSAGE_FOR_STATEFUL_ORDERSconfiguration flag with a default value offalseis a good practice for introducing new features in a controlled manner.indexer/packages/redis/src/helpers/order-helper.ts (1)
- 10-27: The
convertToRedisOrderfunction correctly constructs aRedisOrderfrom anIndexerOrderand aPerpetualMarketFromDatabase. It's important to ensure that this function is always called with valid data to avoid potential runtime errors due to non-null assertions.indexer/services/vulcan/src/handlers/helpers.ts (1)
- 2-2: Adjustments to imports related to
IndexerOrderandRedisOrderseem appropriate. Ensure that the removal ofconvertToRedisOrder(mentioned in the summary but not shown in the provided code) does not impact other functionalities.indexer/packages/redis/src/index.ts (1)
- 18-18: Exporting all entities from
order-helpersimplifies imports in other parts of the application. Ensure that all exported entities are intended to be part of the public API.indexer/packages/kafka/src/websocket-helper.ts (2)
- 41-52: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [44-93]
Splitting
createSubaccountWebsocketMessageintogenerateSubaccountMessageContentsandcreateSubaccountWebsocketMessageis a good practice for separating concerns. Ensure that optional fields are handled carefully to avoid potential issues.
- 90-110: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [96-118]
The refactoring to use
generateSubaccountMessageContentswithincreateSubaccountWebsocketMessagemaintains clarity and modularity in the code. The approach to constructing and encoding protobuf messages is correctly implemented.indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts (1)
- 1-85: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [27-118]
The additions and modifications in
StatefulOrderPlacementHandler, including new methods for retrievingorderIdandsubaccountId, updates to theinternalHandlemethod, and the refactor ofcreateKafkaEvents, are well-implemented. Ensure thorough testing is conducted for the new functionality, especially with the use of configuration flags to toggle features.indexer/services/ender/__tests__/handlers/stateful-order/stateful-order-placement-handler.test.ts (1)
- 138-153: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [39-198]
The modifications to the test cases, including the addition of
expectOrderSubaccountKafkaMessageimport,configimport, and handling ofemitSubaccountMessagebased on a configuration flag, are well-implemented. Ensure that the configuration is reset after each test to avoid unintended side effects.indexer/services/vulcan/src/handlers/order-place-handler.ts (1)
- 17-17: Centralizing the
convertToRedisOrderfunctionality in@dydxprotocol-indexer/redisand removing the duplicate import is a good practice for promoting code reuse and maintainability. Ensure that the centralized functionality is correctly implemented and thoroughly tested.indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts (2)
- 32-32: The import of
OrderSubaccountMessageContentsandORDER_FLAG_CONDITIONALhas been added, which aligns with the PR's objective to enhance the handling of stateful orders and their messaging. Ensure these imports are utilized effectively in the subsequent code changes.- 709-732: The changes from lines 709 to 732 in the
expectOrderSubaccountKafkaMessagefunction introduce logic to handle conditional orders differently from other orders. This is a key part of the PR's objective to enhance messaging for stateful orders. The refactoring is clear and maintains readability. However, ensure that the removal of fields liketriggerPrice,totalFilled, andgoodTilBlockfromorderWithoutUnwantedFieldsis intentional and correctly aligns with the requirements for non-conditional orders.indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (2)
- 120-136: The changes from lines 120 to 136 update the conversion of
IndexerOrdertoRedisOrderby usingredisPackage.convertToRedisOrder. This modification ensures that the conversion logic is encapsulated within theredisPackage, promoting code reuse and maintainability. However, it's crucial to verify that theredisPackage.convertToRedisOrderfunction handles all the necessary fields and edge cases that might have been previously handled in the localconvertToRedisOrderfunction. Ensure that theredisPackageversion of the function has been thoroughly tested, especially for any custom logic that might have been present in the original implementation.- 281-281: The change on line 281 also applies the
redisPackage.convertToRedisOrderfor converting anIndexerOrderto aRedisOrder. This consistency in using theredisPackagefor conversions across the test file is good practice. It centralizes the conversion logic, making future updates or fixes easier to manage. As with the previous instances, ensure comprehensive testing of theredisPackage.convertToRedisOrderfunction to cover all scenarios.
| function isConditionalOrder(order: OrderFromDatabase): boolean { | ||
| return Number(order.orderFlags) === ORDER_FLAG_CONDITIONAL; | ||
| } |
There was a problem hiding this comment.
The isConditionalOrder function checks if an order is conditional based on its orderFlags. This is a straightforward and efficient way to determine the type of order. However, consider adding a brief comment explaining the significance of ORDER_FLAG_CONDITIONAL for future maintainability.
+ // Determines if an order is conditional based on its orderFlags
function isConditionalOrder(order: OrderFromDatabase): boolean {
return Number(order.orderFlags) === ORDER_FLAG_CONDITIONAL;
}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.
| function isConditionalOrder(order: OrderFromDatabase): boolean { | |
| return Number(order.orderFlags) === ORDER_FLAG_CONDITIONAL; | |
| } | |
| // Determines if an order is conditional based on its orderFlags | |
| function isConditionalOrder(order: OrderFromDatabase): boolean { | |
| return Number(order.orderFlags) === ORDER_FLAG_CONDITIONAL; | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts
dydxwill
changed the title
dydxwill
changed the title
dydxwill
changed the title
Changelist
Add flag to send subaccount websocket message from Ender for long term orders
Test Plan
Unit tested.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.