[IND-481] Update handlers to use SQL function only removing the Knex option #811
Conversation
…option This is towards updating the block processor to be executed as a SQL function.
IND-481 Update ender block handler with a single SQL handler that delegates to sub-handlers
Update |
|
Warning Rate Limit Exceeded@lcwik has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 4 minutes and 34 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. 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 changes across various test and handler files in the indexer service indicate a significant refactoring effort. The primary focus seems to be the removal of configuration-based conditional logic for SQL function usage, simplification of test cases, and the elimination of redundant or unused code. This suggests a move towards a more streamlined codebase, possibly for improved maintainability and performance. Changes
Note: The ellipsis ( 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: 28
Configuration used: CodeRabbit UI
Files selected for processing (37)
- indexer/services/ender/tests/handlers/asset-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/funding-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/liquidity-tier-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/markets/market-create-handler.test.ts (2 hunks)
- indexer/services/ender/tests/handlers/markets/market-modify-handler.test.ts (2 hunks)
- indexer/services/ender/tests/handlers/markets/market-price-update-handler.test.ts (2 hunks)
- indexer/services/ender/tests/handlers/order-fills/liquidation-handler.test.ts (9 hunks)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (13 hunks)
- indexer/services/ender/tests/handlers/perpetual-market-handler.test.ts (3 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 (4 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 (5 hunks)
- indexer/services/ender/tests/handlers/subaccount-update-handler.test.ts (11 hunks)
- indexer/services/ender/tests/handlers/transfer-handler.test.ts (4 hunks)
- indexer/services/ender/tests/handlers/update-clob-pair-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/update-perpetual-handler.test.ts (3 hunks)
- indexer/services/ender/src/config.ts (1 hunks)
- indexer/services/ender/src/handlers/abstract-stateful-order-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/asset-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/funding-handler.ts (4 hunks)
- indexer/services/ender/src/handlers/liquidity-tier-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/markets/market-create-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/markets/market-modify-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/markets/market-price-update-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts (2 hunks)
- indexer/services/ender/src/handlers/order-fills/liquidation-handler.ts (4 hunks)
- indexer/services/ender/src/handlers/order-fills/order-handler.ts (4 hunks)
- indexer/services/ender/src/handlers/perpetual-market-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/stateful-order/conditional-order-placement-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/stateful-order/conditional-order-triggered-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts (4 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-removal-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/subaccount-update-handler.ts (4 hunks)
- indexer/services/ender/src/handlers/transfer-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/update-clob-pair-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/update-perpetual-handler.ts (3 hunks)
Files skipped from review due to trivial changes (4)
- indexer/services/ender/tests/handlers/asset-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
- indexer/services/ender/src/config.ts
Additional comments: 68
indexer/services/ender/__tests__/handlers/liquidity-tier-handler.test.ts (3)
173-175: The
expectLiquidityTierfunction is exported but not used within this file. If it's not used elsewhere, consider removing it to clean up the codebase.106-132: The test case "creates new liquidity tier" seems to be missing the definition of
createKafkaMessageFromLiquidityTiersEvent,expectNoExistingLiquidityTiers,validateLiquidityTierRefresher, andexpectKafkaMessages. These functions are called within the test but are not defined in the provided hunk. Ensure that these functions are defined elsewhere in the test file or imported from a helper module.134-170: Similar to the previous comment, the test case "updates existing liquidity tier" calls several functions that are not defined within the provided hunk. Verify that
createKafkaMessageFromLiquidityTiersEvent,expectLiquidityTier,validateLiquidityTierRefresher, andexpectKafkaMessagesare defined or imported correctly.indexer/services/ender/__tests__/handlers/markets/market-modify-handler.test.ts (2)
83-101: The test case for modifying an existing market appears to be correctly updated to reflect the removal of the 'config' object dependency. It now directly tests the functionality without any configuration-based conditional logic.
103-130: The test case for modifying a non-existent market is correctly expecting a
ParseMessageErrorto be thrown, which aligns with the updated logic that no longer relies on the 'config' object. The assertions for logging behavior and ensuring no messages are sent via the producer are also appropriate checks.indexer/services/ender/__tests__/handlers/markets/market-price-update-handler.test.ts (3)
98-126: The test case "fails when no market exists" correctly checks for the error handling when a market ID does not exist. It verifies that the appropriate errors are logged and no Kafka messages are sent.
129-162: The test case "successfully inserts new oracle price for existing market" correctly tests the insertion of a new oracle price and the sending of a Kafka message with the updated market information.
164-216: The test case "successfully inserts new oracle price for market created in the same block" correctly tests the handling of a new market and its associated price update within the same block.
indexer/services/ender/__tests__/handlers/order-fills/liquidation-handler.test.ts (7)
1-3: The removal of unused imports and references to the removed configuration option is consistent with the changes described in the pull request summary.
73-76: The removal of unused imports and helper functions is consistent with the changes described in the pull request summary.
205-226: The test cases appear to be correctly parameterized and should function as expected with the new SQL function logic.
416-432: The test cases appear to be correctly parameterized and should function as expected with the new SQL function logic.
635-651: The test cases appear to be correctly parameterized and should function as expected with the new SQL function logic.
840-983: The test case ensures that fills and orders are created with fixed-point notation quoteAmount, which is important for precision and consistency in the database.
835-987: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [986-1192]
The test case checks for the failure of
LiquidationOrderFillEventvalidation, which is important for ensuring data integrity and error handling.indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (13)
1-4: The removal of unused imports is a good practice for maintainability and clarity.
73-78: The addition of new imports seems necessary for the test cases.
203-223: The parameterized test case using
it.eachis a good practice for testing multiple scenarios efficiently.226-232: The setup for the test environment and creation of orders and fills is appropriate for the test case.
463-467: The test case correctly tests the order updates and removal for maker orders that are fully filled.
509-516: The parameterized test case using
it.eachis a good practice for testing multiple scenarios efficiently.777-781: The test case correctly tests the replacement of existing orders and upserting a new order with the same order id.
808-814: The parameterized test case using
it.eachis a good practice for testing multiple scenarios efficiently.1049-1053: The test case correctly tests the creation of fills and orders with fixed-point notation for the price.
1653-1672: The parameterized test case using
it.eachis a good practice for testing multiple scenarios efficiently.1653-1672: The test case correctly tests the setting of status for short term IOC orders.
1653-1672: The test case correctly tests the setting of status for short term limit and post-only orders.
1836-1837: The
createOrderFillEventfunction is correctly defined to create a mockOrderFillEventV1object for testing.indexer/services/ender/__tests__/handlers/perpetual-market-handler.test.ts (2)
42-42: Verify that
createPostgresFunctionsis properly imported from../../src/helpers/postgres/postgres-functionsas it's being used in thebeforeAllhook.135-170: The test case for creating a new perpetual market is well-structured and correctly checks for the absence of perpetual markets before the test and the presence of a new perpetual market after the message handling.
indexer/services/ender/__tests__/handlers/stateful-order/conditional-order-placement-handler.test.ts (6)
38-50: The
beforeAllhook has been updated to remove thestatsrelated mocks, which is consistent with the removal of performance metrics tracking for Knex. Ensure that no other part of the test suite relies on these mocks.125-126: The
getParallelizationIdstest case remains unchanged, which is appropriate as it does not seem to be related to the Knex or SQL function processing methods.128-131: The test case for 'successfully places order' has been updated to remove the reliance on configuration settings for Knex. Ensure that the test case now correctly tests the order placement using the SQL function logic.
154-161: The test case for 'successfully places order' includes a call to
expectOrderSubaccountKafkaMessage. Verify that this function's implementation is still valid in the context of the new SQL function logic and that it does not contain any remnants of the removed Knex-related code.164-167: The test case for 'successfully upserts order' has been updated to reflect the new SQL function logic. Ensure that the test case now correctly tests the upsert functionality using the SQL function logic and that it does not rely on any removed Knex-related code.
214-221: Similar to the previous test case, verify that the call to
expectOrderSubaccountKafkaMessagein the 'successfully upserts order' test case is still valid and does not rely on any Knex-related code that has been removed.indexer/services/ender/__tests__/handlers/stateful-order/conditional-order-triggered-handler.test.ts (2)
- 110-116: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [113-150]
The test case for successfully triggering an order and sending to Vulcan appears to be correctly updated to reflect the new logic that exclusively uses SQL functions.
- 147-155: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [152-160]
The test case for error handling when attempting to trigger a non-existent order appears to be correctly updated to reflect the new logic that exclusively uses SQL functions.
indexer/services/ender/__tests__/handlers/subaccount-update-handler.test.ts (9)
63-63: The test suite has been correctly updated to reflect the removal of the Knex-related code and configuration settings, focusing solely on SQL function logic.
143-149: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [146-183]
The test case for creating a subaccount appears to be correctly implemented and should verify the creation logic as expected.
- 180-195: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [185-262]
The use of
.eachto run parameterized test cases for upserting perpetual positions with different funding payment scenarios is a good practice for thorough testing.
- 322-330: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [327-407]
The test case for closing and creating a new position when the side is opposing is critical and appears to be correctly implemented.
- 406-412: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [409-454]
The test case for updating an existing asset position seems to be correctly implemented and should verify the update logic as expected.
- 454-464: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [456-522]
The use of
.eachto run parameterized test cases for creating new asset positions with different sizes is a good practice for thorough testing.
- 521-527: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [524-588]
The test case for closing a position when the size is 0 is an important edge case and appears to be correctly implemented.
- 587-593: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [590-680]
The test case for upserting perpetual and asset positions with fixed-point notation sizes is important for ensuring correct handling of small sizes and appears to be correctly implemented.
- 683-685: The helper function
createKafkaMessageFromSubaccountUpdateEventis correctly implemented and supports the test cases as expected.indexer/services/ender/__tests__/handlers/update-clob-pair-handler.test.ts (1)
- 94-123: The test case for 'updates an existing perpetual market' has been refactored to align with the new SQL function logic and the removal of the Knex option. The test now directly invokes the
onMessagefunction and checks the updated state of the perpetual market, which is the expected behavior after the refactor.indexer/services/ender/__tests__/handlers/update-perpetual-handler.test.ts (2)
93-120: The test case for 'updates an existing perpetual market' appears to be correctly implemented and should work as expected with the new SQL function-based logic.
123-123: The helper function
createKafkaMessageFromUpdatePerpetualEventis still being used and is correctly placed at the end of the file for better organization and readability.indexer/services/ender/src/handlers/abstract-stateful-order-handler.ts (1)
- 9-19: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [16-58]
The changes in the
AbstractStatefulOrderHandlerclass reflect the shift to using SQL functions for event handling. Ensure that the inputs to the SQL function are properly sanitized to prevent SQL injection vulnerabilities. The error handling withinhandleEventViaSqlFunctionis appropriate as it logs the error and rethrows it, maintaining the error context for upstream handling.indexer/services/ender/src/handlers/markets/market-create-handler.ts (1)
- 25-31: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [25-57]
The changes in the
MarketCreateHandlerclass reflect the removal of Knex and the adoption of direct SQL function calls for handling market creation events. The error handling and market update logic appear to be correctly implemented.indexer/services/ender/src/handlers/markets/market-modify-handler.ts (1)
- 22-24: The log message states 'Received MarketEvent with MarketCreate.' which might be misleading if this handler is supposed to handle modify events. Please verify that the log message accurately reflects the event being handled.
indexer/services/ender/src/handlers/markets/market-price-update-handler.ts (3)
30-32: The log message indicates that the handler received a
MarketEvent with MarketPriceUpdate. It's important to ensure that the log level and the information logged are appropriate and do not expose sensitive data.34-36: The SQL function
dydx_market_price_update_handleris being called with parameters from the block and the decoded event data. It's crucial to ensure that the SQL function exists and is defined with the correct parameters to avoid runtime errors.76-78: The
generateKafkaEventmethod is protected and takes anOraclePriceFromDatabaseand apairstring as arguments. It's important to verify that this method is only called from within the class or subclasses and that the arguments passed are always of the correct type.indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts (1)
- 1-33: The summary states that the
AbstractOrderFillHandlerclass and its associated methods were removed, but the provided hunk shows that the class and some methods are still present. Please verify if the class should indeed be removed or if the summary is incorrect.indexer/services/ender/src/handlers/order-fills/order-handler.ts (1)
- 58-70: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [67-164]
The changes in
internalHandlemethod seem to align with the summary's description of handling the total filled amount and generating Kafka events. However, ensure that the SQL functiondydx_order_fill_handler_per_orderis thoroughly tested, as it now encapsulates the core logic previously handled in the application layer. This is critical because moving logic to SQL functions can make debugging more difficult and errors in SQL functions can be harder to catch without extensive testing.indexer/services/ender/src/handlers/perpetual-market-handler.ts (2)
1-12: The import section has been correctly updated to remove unused imports, aligning with the refactoring to use SQL functions exclusively.
23-25: Ensure that the
PerpetualMarketCreateEventV1.decodeoutput is properly sanitized before being used in the SQL query to prevent SQL injection attacks.indexer/services/ender/src/handlers/stateful-order/conditional-order-triggered-handler.ts (1)
- 29-34: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [31-40]
The summary states that logic for converting orders to
IndexerOrderand creating Kafka events has been removed, but the code still contains this logic. Please verify if the summary is accurate or if the code needs to be updated to reflect the intended changes.indexer/services/ender/src/handlers/stateful-order/stateful-order-removal-handler.ts (1)
- 34-34: The code sets
removalStatustoOrderRemoveV1_OrderRemovalStatus.ORDER_REMOVAL_STATUS_CANCELED, but references toOrderStatus.CANCELEDwere supposed to be removed. Please verify if this is intentional or if it should be updated to reflect the removal ofOrderStatus.CANCELED.indexer/services/ender/src/handlers/subaccount-update-handler.ts (4)
1-7: The removal of these imports is consistent with the changes described in the summary, indicating the removal of Knex-related code and utility functions.
10-18: The removal of these imports is consistent with the changes described in the summary, indicating the removal of Knex-related code and utility functions.
42-45: The
internalHandlemethod has been refactored to use SQL functions exclusively, which aligns with the pull request's goal of removing Knex options and using SQL functions for processing.98-100: The
generateConsolidatedKafkaEventmethod's documentation has been updated, which is consistent with the refactoring of handlers to use SQL functions exclusively.indexer/services/ender/src/handlers/transfer-handler.ts (1)
- 27-30: The
internalHandlemethod is marked as async and correctly uses await when callingstoreHelpers.rawQuery. Ensure that the inputs to the SQL query are properly sanitized to prevent SQL injection vulnerabilities.indexer/services/ender/src/handlers/update-clob-pair-handler.ts (1)
- 21-26: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [23-50]
The
internalHandlemethod has been refactored to use a raw SQL query, which is a good practice for performance and maintainability. However, ensure that the SQL functiondydx_update_clob_pair_handleris properly defined in the database and that the input to the function is properly sanitized to prevent SQL injection. The use of parameterized queries is a good practice and should be maintained.
| @@ -1,4 +1,4 @@ | |||
| import { STATS_FUNCTION_NAME, stats } from '@dydxprotocol-indexer/base'; | |||
| import { stats } from '@dydxprotocol-indexer/base'; | |||
There was a problem hiding this comment.
The import of stats is no longer used and should be removed to keep the code clean and maintainable.
- import { stats } from '@dydxprotocol-indexer/base';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.
| import { stats } from '@dydxprotocol-indexer/base'; |
| @@ -1,4 +1,4 @@ | |||
| import { stats, STATS_FUNCTION_NAME } from '@dydxprotocol-indexer/base'; | |||
| import { stats } from '@dydxprotocol-indexer/base'; | |||
There was a problem hiding this comment.
The import of stats from @dydxprotocol-indexer/base should be removed as per the summary of changes.
- import { stats } from '@dydxprotocol-indexer/base';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.
| import { stats } from '@dydxprotocol-indexer/base'; |
| const result: pg.QueryResult = await storeHelpers.rawQuery( | ||
| `SELECT dydx_asset_create_handler( |
There was a problem hiding this comment.
Potential SQL injection vulnerability.
- `SELECT dydx_asset_create_handler('${JSON.stringify(AssetCreateEventV1.decode(eventDataBinary))}') AS result;`,
+ 'SELECT dydx_asset_create_handler($1::jsonb) AS result;',
+ [JSON.stringify(AssetCreateEventV1.decode(eventDataBinary))]Use parameterized queries to prevent SQL injection.
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.
| const result: pg.QueryResult = await storeHelpers.rawQuery( | |
| `SELECT dydx_asset_create_handler( | |
| const result: pg.QueryResult = await storeHelpers.rawQuery( | |
| 'SELECT dydx_asset_create_handler($1::jsonb) AS result;', | |
| [JSON.stringify(AssetCreateEventV1.decode(eventDataBinary))] |
| @@ -32,14 +32,13 @@ import { | |||
| createIndexerTendermintEvent, | |||
| expectVulcanKafkaMessage, | |||
| } from '../../helpers/indexer-proto-helpers'; | |||
| import { stats, STATS_FUNCTION_NAME } from '@dydxprotocol-indexer/base'; | |||
| import { stats } from '@dydxprotocol-indexer/base'; | |||
There was a problem hiding this comment.
The import of stats from @dydxprotocol-indexer/base can be removed as it is no longer used in the test file.
| @@ -23,7 +23,6 @@ import { | |||
| } from '../../helpers/indexer-proto-helpers'; | |||
| import Long from 'long'; | |||
There was a problem hiding this comment.
The import of Long from 'long' is not used in the provided code and should be removed if it's not used elsewhere in the file.
- import Long from 'long';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.
| import Long from 'long'; |
|
|
||
| // eslint-disable-next-line @typescript-eslint/require-await | ||
| public async internalHandle(): Promise<ConsolidatedKafkaEvent[]> { | ||
| if (config.USE_PERPETUAL_MARKET_HANDLER_SQL_FUNCTION) { | ||
| return this.handleViaSqlFunction(); | ||
| } | ||
| return this.handleViaKnex(); | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/require-await | ||
| private async handleViaSqlFunction(): Promise<ConsolidatedKafkaEvent[]> { | ||
| const eventDataBinary: Uint8Array = this.indexerTendermintEvent.dataBytes; | ||
| const result: pg.QueryResult = await storeHelpers.rawQuery( | ||
| `SELECT dydx_perpetual_market_handler( |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [22-48]
The error logging within internalHandle should be updated to reflect the new method name instead of the old handleViaSqlFunction.
- at: 'PerpetualMarketCreationHandler#handleViaSqlFunction',
+ at: 'PerpetualMarketCreationHandler#internalHandle',| message: 'Received MarketEvent with MarketCreate.', | ||
| event: this.event, | ||
| }); | ||
| if (config.USE_MARKET_MODIFY_HANDLER_SQL_FUNCTION) { | ||
| return this.handleViaSqlFunction(); | ||
| } | ||
| return this.handleViaKnexQueries(); | ||
| } | ||
|
|
||
| private async handleViaKnexQueries(): Promise<ConsolidatedKafkaEvent[]> { | ||
| logger.info({ | ||
| at: 'MarketModifyHandler#handle', | ||
| message: 'Received MarketEvent with MarketModify.', | ||
| event: this.event, | ||
| }); | ||
| // MarketHandler already makes sure the event has 'marketModify' as the oneofKind. | ||
| const castedMarketModifyMessage: | ||
| MarketModifyEventMessage = this.event as MarketModifyEventMessage; | ||
|
|
||
| await this.runFuncWithTimingStatAndErrorLogging( | ||
| this.updateMarketFromEvent(castedMarketModifyMessage), | ||
| this.generateTimingStatsOptions('update_market'), | ||
| ); | ||
| return []; | ||
| } | ||
|
|
||
| private async handleViaSqlFunction(): Promise<ConsolidatedKafkaEvent[]> { | ||
| const eventDataBinary: Uint8Array = this.indexerTendermintEvent.dataBytes; | ||
| const result: pg.QueryResult = await storeHelpers.rawQuery( | ||
| `SELECT dydx_market_modify_handler( |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [27-58]
The error handling in the internalHandle method could be improved by checking the type of error before casting the event and throwing a generic error. This would make the error handling more robust and maintainable.
| @@ -81,37 +57,4 @@ export class MarketModifyHandler extends Handler<MarketEventV1> { | |||
| marketRefresher.updateMarket(market); | |||
There was a problem hiding this comment.
There should be a check to ensure that the SQL function returned a valid market object before calling updateMarket. This would prevent potential runtime errors if the SQL function fails to return a valid market.
| : castedOrderFillEventMessage.totalFilledMaker; | ||
| } | ||
|
|
||
| public async handleViaSqlFunction(): Promise<ConsolidatedKafkaEvent[]> { | ||
| public async internalHandle(): Promise<ConsolidatedKafkaEvent[]> { | ||
| const eventDataBinary: Uint8Array = this.indexerTendermintEvent.dataBytes; | ||
| const transactionIndex: number = indexerTendermintEventToTransactionIndex( | ||
| this.indexerTendermintEvent, |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [87-184]
Consider refactoring the repeated access to result.rows[0].result to improve readability and maintainability. You could assign it to a variable at the beginning of the block and then use that variable throughout.
- const fill: FillFromDatabase = FillModel.fromJson(result.rows[0].result.fill) as FillFromDatabase;
- const perpetualMarket: PerpetualMarketFromDatabase = PerpetualMarketModel.fromJson(result.rows[0].result.perpetual_market) as PerpetualMarketFromDatabase;
- const position: PerpetualPositionFromDatabase = PerpetualPositionModel.fromJson(result.rows[0].result.perpetual_position) as PerpetualPositionFromDatabase;
+ const queryResult = result.rows[0].result;
+ const fill: FillFromDatabase = FillModel.fromJson(queryResult.fill) as FillFromDatabase;
+ const perpetualMarket: PerpetualMarketFromDatabase = PerpetualMarketModel.fromJson(queryResult.perpetual_market) as PerpetualMarketFromDatabase;
+ const position: PerpetualPositionFromDatabase = PerpetualPositionModel.fromJson(queryResult.perpetual_position) as PerpetualPositionFromDatabase;| @@ -1,4 +1,4 @@ | |||
| import { stats, STATS_FUNCTION_NAME } from '@dydxprotocol-indexer/base'; | |||
| import { stats } from '@dydxprotocol-indexer/base'; | |||
There was a problem hiding this comment.
The import of stats is no longer used and should be removed to keep the code clean and maintainable.
- import { stats } from '@dydxprotocol-indexer/base';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.
| import { stats } from '@dydxprotocol-indexer/base'; |
Changelist
[IND-481] Update handlers to use SQL function only removing the Knex option
This is towards updating the block processor to be executed as a SQL function.
Test Plan
Cleaned up existing tests.
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.