-
Notifications
You must be signed in to change notification settings - Fork 125
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
Handle stateful order replacement in Ender #1667
Handle stateful order replacement in Ender #1667
Conversation
Warning Rate limit exceeded@chenyaoy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 8 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. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes introduce new functionalities and refinements to the stateful order handling within the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StatefulOrderReplacementHandler
participant Database
participant Kafka
Client->>StatefulOrderReplacementHandler: Replace Order
StatefulOrderReplacementHandler->>Database: Retrieve Order Details
Database-->>StatefulOrderReplacementHandler: Order Details
StatefulOrderReplacementHandler->>Database: Update Order Status
StatefulOrderReplacementHandler->>Kafka: Produce Kafka Event
Kafka-->>StatefulOrderReplacementHandler: Acknowledgment
StatefulOrderReplacementHandler-->>Client: Order Replacement Confirmation
Poem
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 (
|
@@ -45,7 +45,7 @@ import { producer } from '@dydxprotocol-indexer/kafka'; | |||
import { ConditionalOrderPlacementHandler } from '../../../src/handlers/stateful-order/conditional-order-placement-handler'; | |||
import { createPostgresFunctions } from '../../../src/helpers/postgres/postgres-functions'; | |||
|
|||
describe('conditionalOrderPlacementHandler', () => { | |||
describe('conditional-order-placement-handler', () => { |
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.
pnpm test -- {test_name}
won't find the test if it's not the same as the file name
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- indexer/services/ender/tests/handlers/stateful-order/conditional-order-placement-handler.test.ts (1 hunks)
- indexer/services/ender/tests/handlers/stateful-order/conditional-order-triggered-handler.test.ts (1 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-placement-handler.test.ts (1 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-removal-handler.test.ts (1 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-replacement-handler.test.ts (1 hunks)
- indexer/services/ender/tests/helpers/constants.ts (1 hunks)
- indexer/services/ender/tests/validators/stateful-order-validator.test.ts (3 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-replacement-handler.ts (1 hunks)
- indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql (2 hunks)
- indexer/services/ender/src/validators/stateful-order-validator.ts (6 hunks)
Files not summarized due to errors (1)
- indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql: Error: Server error. Please try again later.
Files skipped from review due to trivial changes (4)
- indexer/services/ender/tests/handlers/stateful-order/conditional-order-placement-handler.test.ts
- indexer/services/ender/tests/handlers/stateful-order/conditional-order-triggered-handler.test.ts
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-placement-handler.test.ts
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-removal-handler.test.ts
Additional context used
Biome
indexer/services/ender/src/validators/stateful-order-validator.ts
[error] 37-41: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 61-64: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 68-71: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 80-83: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 86-86: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 93-96: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 100-103: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 110-113: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 122-125: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 131-134: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 138-141: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 145-148: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 156-159: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 163-167: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 174-177: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 186-189: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 195-198: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 209-212: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 218-221: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 225-228: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
Additional comments not posted (7)
indexer/services/ender/src/handlers/stateful-order/stateful-order-replacement-handler.ts (1)
28-104
: The implementation ofStatefulOrderReplacementHandler
is robust and well-structured.The class methods are well-defined and adhere to the expected functionalities of handling stateful order replacements. The use of async patterns and Kafka event generation is correctly implemented.
indexer/services/ender/__tests__/handlers/stateful-order/stateful-order-replacement-handler.test.ts (1)
42-221
: Comprehensive test coverage forStatefulOrderReplacementHandler
.The tests adequately cover various scenarios including transaction and block events, ensuring that the handler behaves as expected under different conditions. The use of mocks and assertions is appropriate and helps validate the functionality effectively.
indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql (1)
Line range hint
26-182
: SQL script handles stateful order events effectively, including the neworderReplacement
.The script correctly handles different types of order events, ensuring that orders are processed based on their specific characteristics. The inclusion of
orderReplacement
is well-integrated into the existing logic.indexer/services/ender/src/validators/stateful-order-validator.ts (1)
Line range hint
1-240
: TheStatefulOrderValidator
class is well-implemented and covers all necessary validations.The class methods are correctly structured to handle various stateful order events, ensuring that all necessary validations are performed. The use of detailed error messages enhances the clarity of the validation process.
Tools
Biome
[error] 209-212: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 218-221: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
[error] 225-228: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
indexer/services/ender/__tests__/validators/stateful-order-validator.test.ts (2)
21-21
: The addition ofdefaultStatefulOrderReplacementEvent
is correctly implemented to support new test scenarios.
46-46
: The addition of the test case for 'stateful order replacement' is well-implemented to ensure the new functionality behaves as expected under normal conditions.indexer/services/ender/__tests__/helpers/constants.ts (1)
451-463
: The definition ofdefaultStatefulOrderReplacementEvent
is correctly implemented to facilitate testing of the stateful order replacement functionality. It includes necessary details like the old order ID and the new order configuration.
extends AbstractStatefulOrderHandler<StatefulOrderEventV1> { | ||
eventType: string = 'StatefulOrderEvent'; | ||
|
||
public getOrderId(): string { |
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.
private?
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.
updated
}, | ||
)); | ||
|
||
if (config.SEND_SUBACCOUNT_WEBSOCKET_MESSAGE_FOR_STATEFUL_ORDERS) { |
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.
I think you can remove this config check - there's a bug with it anyways & it's not being used anywhere.
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.
Got it, thanks for the context. Removed this if statement
@@ -141,5 +141,44 @@ BEGIN | |||
ELSE | |||
RAISE EXCEPTION 'Unkonwn sub-event type %', event_data; | |||
END IF; | |||
|
|||
/* For order replacement, remove old order */ | |||
IF event_data->'orderReplacement' IS NOT NULL THEN |
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.
this code won't ever execute because of the earlier return statement on line 93.
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.
Thank you for catching! I fixed and adding the check for the old order tests that this is now working properly
.../services/ender/__tests__/handlers/stateful-order/stateful-order-replacement-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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-replacement-handler.test.ts (1 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-replacement-handler.ts (1 hunks)
- indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-replacement-handler.test.ts
- indexer/services/ender/src/handlers/stateful-order/stateful-order-replacement-handler.ts
Additional comments not posted (3)
indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql (3)
25-53
: Handle order replacement with careful state management.The addition of handling for
orderReplacement
is well-implemented. The logic to cancel the old order and update its status to 'CANCELED' before processing the new order ensures data consistency. However, ensure that theorder_id
andsubaccount_id
are validated before use to prevent potential issues from malformed input.
54-54
: Resetorder_record
to prevent data carryover.Resetting
order_record
toNULL
after handling the order replacement is a good practice to ensure that subsequent operations do not inadvertently use stale data. This is crucial for maintaining the integrity of the function's operations.
Line range hint
57-84
: Enhance handling of various order placements.The expanded handling for different types of order placements (
orderPlace
,longTermOrderPlacement
,conditionalOrderPlacement
,orderReplacement
) is comprehensive. The use ofcoalesce
to select the first non-null order object is efficient. However, consider adding explicit null checks or validations for the fields accessed fromorder_
to ensure they are not null before use, which could lead to runtime errors.
indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql
Show resolved
Hide resolved
indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql
Show resolved
Hide resolved
indexer/services/ender/src/handlers/stateful-order/stateful-order-replacement-handler.ts
Outdated
Show resolved
Hide resolved
let producerSendMock: jest.SpyInstance; | ||
|
||
it.each([ | ||
['stateful order placement as txn event', defaultStatefulOrderReplacementEvent, false, 0], |
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.
stateful orders can be both txn & block events?
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.
I copied this part from stateful-order-place-handler.test.ts
, i can remove one if you think not needed to test it. i don't have full context
.../services/ender/__tests__/handlers/stateful-order/stateful-order-replacement-handler.test.ts
Outdated
Show resolved
Hide resolved
RETURNING * INTO order_record; | ||
|
||
IF NOT FOUND THEN | ||
RAISE NOTICE 'Unable to cancel replaced order because not found with orderId: %', dydx_uuid_from_order_id(order_id); |
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.
what's the diff between notice and exception?
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.
raise notice doesn't abort the transaction. I basically just want to continue here if not found, do you know of a better way to do that?
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.
can we surface this logging/add metrics somewhere since we don't expect this to happen?
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.
done!
.../services/ender/__tests__/handlers/stateful-order/stateful-order-replacement-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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-placement-handler.test.ts (1 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-replacement-handler.test.ts (1 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-replacement-handler.ts (1 hunks)
- indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-placement-handler.test.ts
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-replacement-handler.test.ts
- indexer/services/ender/src/handlers/stateful-order/stateful-order-replacement-handler.ts
- indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql
const oldOrderId = this.event.orderReplacement!.oldOrderId!; | ||
const order = this.event.orderReplacement!.order!; | ||
if (resultRow.errors != null) { | ||
logger.error({ |
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.
nit: the next time an error is added to that postgres script, this can break. Add a check indicating this error is actually an order replacement related error
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.
Added a comment and the test should catch if the error at index 0 is no longer the expected error
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.
Updated to check if error message is related to order replacement!
indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql
Outdated
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-replacement-handler.ts (1 hunks)
- indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-replacement-handler.ts
- indexer/services/ender/src/scripts/handlers/dydx_stateful_order_handler.sql
Changelist
dydx_stateful_order_handler.sql
to handle order replacement messageTest Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Refactor