[IND-477, IND-478, IND-479, IND-480] Update all stateful order handlers to use a SQL function to perform reads & updates. #774
Conversation
…rs to use a SQL function to perform reads & updates.
IND-479 Ender stateful order placement handler SQL pushdown
Update IND-480 Ender stateful order removal handler SQL pushdown
Update IND-477 Ender conditional order placement handler SQL pushdown
Update IND-478 Ender conditional order trigger handler SQL pushdown
Update |
WalkthroughThe changes introduce a new configuration option Changes
TipsChat with CodeRabbit Bot (
|
![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: 6
Configuration used: CodeRabbit UI
Files selected for processing (16)
- indexer/packages/postgres/src/lib/order-translations.ts (2 hunks)
- indexer/services/ender/tests/handlers/stateful-order/conditional-order-placement-handler.test.ts (4 hunks)
- indexer/services/ender/tests/handlers/stateful-order/conditional-order-triggered-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-placement-handler.test.ts (5 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-removal-handler.test.ts (4 hunks)
- indexer/services/ender/tests/scripts/scripts.test.ts (2 hunks)
- indexer/services/ender/src/config.ts (1 hunks)
- indexer/services/ender/src/handlers/abstract-stateful-order-handler.ts (2 hunks)
- indexer/services/ender/src/handlers/stateful-order/conditional-order-placement-handler.ts (4 hunks)
- indexer/services/ender/src/handlers/stateful-order/conditional-order-triggered-handler.ts (5 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-removal-handler.ts (2 hunks)
- indexer/services/ender/src/helpers/postgres/postgres-functions.ts (1 hunks)
- indexer/services/ender/src/scripts/dydx_from_protocol_order_side.sql (1 hunks)
- indexer/services/ender/src/scripts/dydx_protocol_condition_type_to_order_type.sql (1 hunks)
- indexer/services/ender/src/scripts/dydx_stateful_order_handler.sql (1 hunks)
Files skipped from review due to trivial changes (3)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-removal-handler.test.ts
- indexer/services/ender/src/helpers/postgres/postgres-functions.ts
- indexer/services/ender/src/scripts/dydx_from_protocol_order_side.sql
Additional comments: 40
indexer/services/ender/src/config.ts (1)
- 50-55: The new configuration option
USE_STATEFUL_ORDER_HANDLER_SQL_FUNCTIONis introduced with a default value oftrue. Ensure that this change is reflected in the system's configuration files and that the system behaves as expected when this flag is toggled.indexer/services/ender/__tests__/scripts/scripts.test.ts (2)
7-11: The import statement has been updated to include
IndexerOrder_ConditionTypefrom@dydxprotocol-indexer/v4-protos. Ensure that this import is being used correctly throughout the file.178-186: A new test case has been added for the
dydx_protocol_condition_type_to_order_typefunction. This test case checks the function's behavior with various condition types. Ensure that the test case covers all possible condition types and that the function behaves as expected in each case.indexer/services/ender/src/scripts/dydx_protocol_condition_type_to_order_type.sql (1)
- 1-25: The SQL function
dydx_protocol_condition_type_to_order_typeis well written and handles all expected cases. It also throws an exception for unexpected condition types, which is a good practice for error handling. However, ensure that this exception is properly caught and handled in the application code to prevent unexpected crashes or disruptions.indexer/services/ender/__tests__/handlers/stateful-order/conditional-order-triggered-handler.test.ts (1)
- 111-124: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [111-160]
The test case 'successfully triggers order and sends to vulcan' has been parameterized to test both the Knex and SQL function-based order handling paths. Ensure that the
USE_STATEFUL_ORDER_HANDLER_SQL_FUNCTIONconfiguration flag is being set correctly in all environments where this code will run.indexer/services/ender/__tests__/handlers/stateful-order/conditional-order-placement-handler.test.ts (2)
- 162-185: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [129-173]
The test cases for both Knex and SQL function-based order placements are well-structured and cover the necessary scenarios. However, ensure that the SQL function and Knex queries are correctly implemented and that they handle edge cases and errors properly.
- 162-185: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [175-240]
The test cases for both Knex and SQL function-based order upserts are well-structured and cover the necessary scenarios. However, ensure that the SQL function and Knex queries are correctly implemented and that they handle edge cases and errors properly.
indexer/services/ender/src/handlers/stateful-order/stateful-order-removal-handler.ts (5)
24-25: Ensure that the
orderIdToUuidfunction handles all edge cases and errors properly. If it fails, it could lead to incorrect parallelization IDs.28-33: The switch between SQL function and Knex based on the configuration flag is clear and straightforward. However, ensure that the configuration flag is set correctly in all environments where this code will run.
35-39: Ensure that the
handleEventViaSqlFunctionmethod properly handles all possible errors and edge cases. If it fails, it could lead to incorrect Kafka events.41-49: Ensure that the
runFuncWithTimingStatAndErrorLoggingandupdateOrderStatusmethods properly handle all possible errors and edge cases. If they fail, it could lead to incorrect Kafka events.51-54: Ensure that the
OffChainUpdateV1.fromPartialmethod properly handles all possible errors and edge cases. If it fails, it could lead to incorrect Kafka events.indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts (5)
14-20: The import of
configis new. Ensure that the configuration is correctly set up and that theUSE_STATEFUL_ORDER_HANDLER_SQL_FUNCTIONflag is being properly read.39-44: The
internalHandlemethod now conditionally calls eitherhandleViaSqlFunctionorhandleViaKnexbased on theUSE_STATEFUL_ORDER_HANDLER_SQL_FUNCTIONconfiguration flag. This is a good approach to switch between different implementations.46-57: The
handleViaSqlFunctionmethod is new. It handles events via SQL function and creates Kafka events. Ensure that the SQL function is correctly implemented and that it handles all edge cases.82-91: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [59-86]
The
handleViaKnexmethod is new. It handles events via Knex and creates Kafka events. Ensure that the Knex queries are correctly implemented and that they handle all edge cases.
- 88-91: The
createKafkaEventsmethod is new. It creates Kafka events based on the provided order. Ensure that the Kafka events are correctly formed and that they are being sent to the correct topic.indexer/packages/postgres/src/lib/order-translations.ts (2)
22-32: The function
convertToIndexerOrderWithSubaccountis introduced to handle the conversion of an order to anIndexerOrderwith asubaccount. The function checks if the order is a long-term or conditional order and throws an error if it's not. This is a good practice as it ensures that the function is used correctly.86-101: The function
convertToIndexerOrderis updated to fetch thesubaccountusing theSubaccountTable.findByIdmethod and then call theconvertToIndexerOrderWithSubaccountfunction. It also handles the case where thesubaccountis not found and throws an error. This is a good practice as it ensures that the function fails early when the requiredsubaccountis not found.indexer/services/ender/src/handlers/stateful-order/conditional-order-placement-handler.ts (5)
14-20: The import of
configis new and is used to check theUSE_STATEFUL_ORDER_HANDLER_SQL_FUNCTIONflag. Ensure that the flag is correctly set in the configuration.33-40: The
internalHandlemethod now checks theUSE_STATEFUL_ORDER_HANDLER_SQL_FUNCTIONflag and calls eitherhandleViaSqlFunctionorhandleViaKnexaccordingly. This is a good use of feature flags to control the behavior of the system.42-51: The
handleViaSqlFunctionmethod is new and uses thehandleEventViaSqlFunctionmethod from the parent class to handle the event. It then creates Kafka events. Ensure that the SQL function is correctly implemented and that the Kafka events are correctly formed.77-90: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [53-81]
The
handleViaKnexmethod is essentially the oldinternalHandlemethod. It has been renamed and is now only called when theUSE_STATEFUL_ORDER_HANDLER_SQL_FUNCTIONflag is false. This maintains backward compatibility when the flag is not set.
- 83-90: The
createKafkaEventsmethod is new and generates Kafka events. Ensure that the Kafka events are correctly formed.indexer/services/ender/__tests__/handlers/stateful-order/stateful-order-placement-handler.test.ts (6)
139-150: The test cases have been updated to test both Knex and SQL function-based order placements. Ensure that the
USE_STATEFUL_ORDER_HANDLER_SQL_FUNCTIONconfiguration flag is being set correctly in the production environment.151-154: Ensure that the
createKafkaMessageFromStatefulOrderEventfunction is correctly creating Kafka messages from stateful order events.177-182: The
expectTimingStatsfunction is conditionally called based on the value ofuseSqlFunction. Ensure that this function is correctly measuring the timing stats whenuseSqlFunctionisfalse.196-207: The test cases have been updated to test both Knex and SQL function-based order placements. Ensure that the
USE_STATEFUL_ORDER_HANDLER_SQL_FUNCTIONconfiguration flag is being set correctly in the production environment.208-211: Ensure that the
SubaccountTable.subaccountIdToUuidfunction is correctly converting the subaccount ID to a UUID.258-263: The
expectTimingStatsfunction is conditionally called based on the value ofuseSqlFunction. Ensure that this function is correctly measuring the timing stats whenuseSqlFunctionisfalse.indexer/services/ender/src/scripts/dydx_stateful_order_handler.sql (4)
13-21: The use of constants and variable declarations at the beginning of the function improves readability and maintainability.
24-75: The function handles different types of order placement events and updates the order records accordingly. It's important to ensure that all possible event types are covered and that the function behaves as expected in each case.
77-97: The function uses the
INSERT INTO ... ON CONFLICT DO UPDATEconstruct to upsert order records. This is a good approach to handle both new and existing records.105-145: The function handles
conditionalOrderTriggeredandorderRemovalevents and updates the order status accordingly. It's important to ensure that the function behaves as expected in each case.indexer/services/ender/src/handlers/stateful-order/conditional-order-triggered-handler.ts (4)
34-41: The
internalHandlemethod now checks theUSE_STATEFUL_ORDER_HANDLER_SQL_FUNCTIONconfiguration flag to decide whether to handle events via SQL function or Knex. Ensure that this flag is set correctly in all environments where this code will run.43-52: The new
handleViaSqlFunctionmethod handles events via SQL function. It calls thehandleEventViaSqlFunctionmethod from theAbstractStatefulOrderHandlerclass, which executes a SQL function and handles the result. It then converts the result to anIndexerOrderand creates Kafka events. Ensure that the SQL function is correctly implemented and that it returns the expected result.76-85: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [54-80]
The new
handleViaKnexmethod handles events via Knex. It updates the order status toOPEN, fetches thePerpetualMarketFromDatabasefor theclobPairIdof the order, and throws an error if it cannot find the perpetual market. It then converts the order to anIndexerOrderand creates Kafka events. Ensure that theupdateOrderStatusmethod correctly updates the order status in the database and that thePerpetualMarketFromDatabaseis correctly fetched for theclobPairId.
- 89-95: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [82-95]
The new
createKafkaEventsmethod creates Kafka events. It creates anOffChainUpdateV1with theorderand aplacementStatusofORDER_PLACEMENT_STATUS_OPENED, and then generates a consolidated Vulcan Kafka event with theorderIdHashand theoffChainUpdate. Ensure that the Kafka events are correctly created and sent.indexer/services/ender/src/handlers/abstract-stateful-order-handler.ts (2)
1-29: The import statements have been reorganized and expanded. Ensure that all the imported modules are used in the code and that there are no unused imports.
42-71: A new method
handleEventViaSqlFunctionhas been added. This method executes a SQL function and handles the result. Ensure that the SQL functiondydx_stateful_order_handlerexists and works as expected. Also, make sure that the error handling is sufficient and that the function handles all possible edge cases.+ protected async handleEventViaSqlFunction(): + Promise<[OrderFromDatabase, + PerpetualMarketFromDatabase, + SubaccountFromDatabase | undefined]> { + const eventDataBinary: Uint8Array = this.indexerTendermintEvent.dataBytes; + const result: pg.QueryResult = await storeHelpers.rawQuery( + `SELECT dydx_stateful_order_handler( + ${this.block.height}, + '${this.block.time?.toISOString()}', + '${JSON.stringify(StatefulOrderEventV1.decode(eventDataBinary))}' + ) AS result;`, + { txId: this.txId }, + ).catch((error: Error) => { + logger.error({ + at: 'AbstractStatefulOrderHandler#handleEventViaSqlFunction', + message: 'Failed to handle StatefulOrderEventV1', + error, + }); + throw error; + }); + + return [ + OrderModel.fromJson(result.rows[0].result.order) as OrderFromDatabase, + PerpetualMarketModel.fromJson( + result.rows[0].result.perpetual_market) as PerpetualMarketFromDatabase, + result.rows[0].result.subaccount + ? SubaccountModel.fromJson(result.rows[0].result.subaccount) as SubaccountFromDatabase + : undefined, + ]; + }
| it.each([ | ||
| ['via knex', false], | ||
| ['via SQL function', true], | ||
| ])('throws error when attempting to trigger an order that does not exist (%s)', async ( | ||
| _name: string, | ||
| useSqlFunction: boolean, | ||
| ) => { | ||
| config.USE_STATEFUL_ORDER_HANDLER_SQL_FUNCTION = useSqlFunction; | ||
| const kafkaMessage: KafkaMessage = createKafkaMessageFromStatefulOrderEvent( | ||
| defaultStatefulOrderEvent, | ||
| ); | ||
|
|
||
| await expect(onMessage(kafkaMessage)).rejects.toThrowError( | ||
| new Error(`Unable to update order status with orderId: ${orderId}`), | ||
| `Unable to update order status with orderId: ${orderId}`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The test case 'throws error when attempting to trigger an order that does not exist' has been parameterized to test both the Knex and SQL function-based order handling paths. The error message has been updated to include the orderId which will help in debugging.
| @@ -45,6 +45,7 @@ import Long from 'long'; | |||
| 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'; | |||
| import config from '../../../src/config'; | |||
There was a problem hiding this comment.
The import of config from a specific path might cause issues if the path changes in the future. Consider using dependency injection or a configuration management library to handle configuration.
| order.subaccountId, | ||
| ); | ||
|
|
||
| if (!subaccount === undefined) { |
There was a problem hiding this comment.
The condition !subaccount === undefined is incorrect and will always evaluate to false because !subaccount will be a boolean and undefined is not a boolean. The correct condition should be subaccount === undefined.
- if (!subaccount === undefined) {
+ if (subaccount === undefined) {Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| if (!subaccount === undefined) { | |
| if (subaccount === undefined) { |
| /** | ||
| Parameters: | ||
| - block_height: the height of the block being processing. | ||
| - block_time: the time of the block being processed. | ||
| - event_data: The 'data' field of the IndexerTendermintEvent (https://github.com/dydxprotocol/v4-proto/blob/8d35c86/dydxprotocol/indexer/indexer_manager/event.proto#L25) | ||
| converted to JSON format. Conversion to JSON is expected to be done by JSON.stringify. | ||
| Returns: JSON object containing fields: | ||
| - order: The upserted order in order-model format (https://github.com/dydxprotocol/indexer/blob/cc70982/packages/postgres/src/models/order-model.ts). | ||
| */ |
There was a problem hiding this comment.
The function documentation is clear and provides a good overview of the function's purpose, parameters, and return value. However, it would be beneficial to include a brief description of the function's logic and behavior.
| BEGIN | ||
| SELECT * INTO STRICT perpetual_market_record FROM perpetual_markets WHERE "clobPairId" = clob_pair_id; | ||
| EXCEPTION | ||
| WHEN NO_DATA_FOUND THEN | ||
| RAISE EXCEPTION 'Unable to find perpetual market with clobPairId: %', clob_pair_id; | ||
| WHEN TOO_MANY_ROWS THEN | ||
| /** This should never happen and if it ever were to would indicate that the table has malformed data. */ | ||
| RAISE EXCEPTION 'Found multiple perpetual markets with clobPairId: %', clob_pair_id; | ||
| END; |
There was a problem hiding this comment.
The function queries the perpetual_markets table based on the clobPairId. It's good that the function handles NO_DATA_FOUND and TOO_MANY_ROWS exceptions. However, it's recommended to handle other potential exceptions as well, such as connection errors.
| ELSE | ||
| RAISE EXCEPTION 'Unkonwn sub-event type %', event_data; | ||
| END IF; |
There was a problem hiding this comment.
The function raises an exception for unknown sub-event types. This is a good practice to handle unexpected inputs.
Overall, the function seems to be well-structured and handles different types of events. However, it's recommended to add more comments to explain the logic and behavior of the function, especially for complex parts of the code. Also, it's important to handle potential exceptions that might occur during the execution of the function.
…rs to use a SQL function to perform reads & updates. (#774)
…rs to use a SQL function to perform reads & updates. (#774)
…rs to use a SQL function to perform reads & updates. (#774)
…rs to use a SQL function to perform reads & updates. (#774)
…rs to use a SQL function to perform reads & updates. (#774)
…rs to use a SQL function to perform reads & updates. (#774)
Changelist
[IND-477, IND-478, IND-479, IND-480] Update all stateful order handlers to use a SQL function to perform reads & updates.
Test Plan
Updated existing tests and added some new ones.
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.