-
Notifications
You must be signed in to change notification settings - Fork 117
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-489] Update order side when replacing. Fix liquidations side bug. #807
[IND-489] Update order side when replacing. Fix liquidations side bug. #807
Conversation
IND-489 BUG: Replaced order does not have all fields updated correctly
When an order is replaced (same order id), the Additionally, it may be possible to update the |
WalkthroughThe changes across the various files seem to focus on enhancing the testing and functionality of order handling, particularly for liquidation and order fill events. New test cases have been added to ensure the correct behavior when replacing orders, with attention to timing and order side details. The SQL scripts have been updated to accurately record the 'side' of orders in the database, ensuring that the order handling logic remains consistent with the new test scenarios. Changes
TipsChat with CodeRabbit Bot (
|
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: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- indexer/services/ender/tests/handlers/order-fills/liquidation-handler.test.ts (1 hunks)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (2 hunks)
- indexer/services/ender/src/scripts/dydx_liquidation_fill_handler_per_order.sql (3 hunks)
- indexer/services/ender/src/scripts/dydx_order_fill_handler_per_order.sql (2 hunks)
Additional comments: 6
indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (2)
758-758: The comment on line 758 seems to incorrectly state the expected quote amount as '2' when the code calculates it as '1'. Please verify and correct the comment to match the expected value from the code or update the code if the comment is correct.
856-1161: The changes from lines 856 to 1161 introduce a complex test case for replacing existing orders. Ensure that the test case is well-structured, covers all relevant scenarios, and has clear and accurate assertions. Given the complexity, it is recommended to review this test case thoroughly to ensure it meets the intended requirements and correctly validates the functionality.
indexer/services/ender/src/scripts/dydx_liquidation_fill_handler_per_order.sql (3)
80-94: The logic for determining
order_side
based on theside
field for 'makerOrder' and a conditional for other cases appears to be correctly implemented.99-105: The
order_record
is correctly updated with theside
information before theUPSERT
operation, ensuring that theorders
table will have the accurateside
data.117-123: The
UPSERT
operation correctly updates theorders
table with the newside
information from theorder_record
, which is essential for maintaining accurate order side information in the database.indexer/services/ender/src/scripts/dydx_order_fill_handler_per_order.sql (1)
- 86-90: The update to the 'side' field in the 'orders' table is correctly implemented to reflect the new value of 'order_side'. This change is critical for maintaining the consistency of order information after a fill event.
indexer/services/ender/src/scripts/dydx_order_fill_handler_per_order.sql
Show resolved
Hide resolved
indexer/services/ender/__tests__/handlers/order-fills/liquidation-handler.test.ts
Show resolved
Hide resolved
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: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/ender/src/scripts/dydx_liquidation_fill_handler_per_order.sql (3 hunks)
Additional comments: 3
indexer/services/ender/src/scripts/dydx_liquidation_fill_handler_per_order.sql (3)
80-95: The logic for determining the
order_side
based on the 'side' or 'isBuy' property is correctly implemented and aligns with the summary provided.100-106: The
order_record
is correctly updated with the neworder_side
and other order details before performing an UPSERT operation.118-124: The
orders
table is correctly updated with the neworder_record
values, ensuring that the order side and other details are consistent with the latest state.
Opened https://linear.app/dydx/issue/IND-493/backfill-incorrect-liquidated-fills-with-wrong-side for the backfill task to fix any incorrectly created liquidated fills due to the bug fixed in this PR. |
Changelist
SELL
Test Plan
Unit tests.
Author/Reviewer Checklist
state-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.