[IND-470] Update asset create handler to use SQL function. #748
Conversation
IND-470 Ender asset create handler SQL pushdown
Update |
WalkthroughThe changes introduce a new way to handle asset creation events in the indexer service. The 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: 3
Configuration used: CodeRabbit UI
Files selected for processing (8)
- indexer/packages/postgres/src/constants.ts (2 hunks)
- indexer/packages/postgres/src/index.ts (1 hunks)
- indexer/packages/postgres/src/models/asset-model.ts (1 hunks)
- indexer/services/ender/tests/handlers/asset-handler.test.ts (3 hunks)
- indexer/services/ender/src/config.ts (2 hunks)
- indexer/services/ender/src/handlers/asset-handler.ts (2 hunks)
- indexer/services/ender/src/helpers/postgres/postgres-functions.ts (1 hunks)
- indexer/services/ender/src/scripts/dydx_asset_create_handler.sql (1 hunks)
Files skipped from review due to trivial changes (2)
- indexer/packages/postgres/src/constants.ts
- indexer/packages/postgres/src/index.ts
Additional comments: 10
indexer/packages/postgres/src/models/asset-model.ts (1)
- 59-76: The new
sqlToJsonConversionsstatic method provides a mapping from column names to JSON conversion types. This is a good practice for ensuring data consistency when interacting with the database. However, it's important to ensure that these conversions align with the actual data types in the database. For instance, ifidis an integer in the database, converting it to a string might lead to unexpected behavior.indexer/services/ender/src/config.ts (2)
23-28: The new configuration
USE_ASSET_CREATE_HANDLER_SQL_FUNCTIONis introduced and set totrueby default. Ensure that this change is reflected in the environment configuration files and the deployment pipeline.41-43: The configuration
USE_ORDER_HANDLER_SQL_FUNCTIONis set totrueby default. Make sure that this change is also reflected in the environment configuration files and the deployment pipeline.indexer/services/ender/src/helpers/postgres/postgres-functions.ts (1)
- 29-35: The addition of the new SQL script
dydx_asset_create_handler.sqlto the list of scripts is appropriate given the context of the PR. Ensure that the script is correctly implemented and tested.indexer/services/ender/src/handlers/asset-handler.ts (4)
1-16: Imports look fine. Ensure that all the imported modules are being used in the code.
23-30: The
internalHandlemethod now checks theUSE_ASSET_CREATE_HANDLER_SQL_FUNCTIONconfig flag and calls the appropriate handler method. This is a good approach to switch between different methods of handling asset creation.32-53: The
handleViaSqlFunctionmethod executes a SQL query to handle the asset creation event and adds the asset toassetRefresher. Ensure that the SQL functiondydx_asset_create_handlerexists and works as expected. Also, make sure that the error handling is adequate and that the error message provides enough context for debugging.55-58: The
handleViaKnexmethod calls thecreateAssetmethod with timing stats and error logging. Ensure that therunFuncWithTimingStatAndErrorLoggingfunction and thecreateAssetmethod work as expected.indexer/services/ender/src/scripts/dydx_asset_create_handler.sql (1)
- 1-34: The SQL function
dydx_asset_create_handlerseems to be well written and follows good practices. However, there are a few points to consider:
Error handling: The function raises an exception when the market ID is not found. This is good, but it might be beneficial to also handle other potential errors, such as insertion failures.
Transaction handling: The function does not seem to use explicit transactions. If multiple operations need to be performed atomically, consider wrapping them in a transaction.
SQL Injection: The function seems to be safe from SQL injection as it uses parameterized queries.
Performance: The function performs a SELECT operation to retrieve the market ID. If the
marketstable is large, this could be a performance bottleneck. Consider indexing theidcolumn in themarketstable if it's not already indexed.Maintainability: The function is quite readable and maintainable. It uses clear variable names and is well-structured.
Please verify these points and make changes if necessary.
indexer/services/ender/__tests__/handlers/asset-handler.test.ts (1)
- 62-67: The addition of
assetRefresher.clear()in theafterEachblock is a good practice to ensure that the state from one test does not leak into another.
| } from '../helpers/constants'; | ||
| import { updateBlockCache } from '../../src/caches/block-cache'; | ||
| import { createPostgresFunctions } from '../../src/helpers/postgres/postgres-functions'; | ||
| import config from '../../src/config'; | ||
|
|
||
| describe('assetHandler', () => { | ||
| beforeAll(async () => { |
There was a problem hiding this comment.
The import of config and the use of config.USE_ASSET_CREATE_HANDLER_SQL_FUNCTION in the tests indicate that the tests are now dependent on the configuration. This could lead to unpredictable test results if the configuration changes. Consider mocking the configuration to ensure consistent test results.
| }); | ||
| }); | ||
|
|
||
| it('fails when market doesnt exist for asset', async () => { | ||
| const transactionIndex: number = 0; | ||
| const kafkaMessage: KafkaMessage = createKafkaMessageFromAssetEvent({ | ||
| assetEvent: defaultAssetCreateEvent, | ||
| transactionIndex, | ||
| height: defaultHeight, | ||
| time: defaultTime, | ||
| txHash: defaultTxHash, | ||
| }); | ||
|
|
||
| const message: string = 'Unable to find market with id: 0'; | ||
| await expect(onMessage(kafkaMessage)).rejects.toThrowError( | ||
| new Error(message), | ||
| ); | ||
| }); | ||
| it.each([ | ||
| [ | ||
| 'via knex', | ||
| false, | ||
| ], | ||
| [ | ||
| 'via SQL function', | ||
| true, | ||
| ], | ||
| ])( | ||
| 'fails when market doesnt exist for asset (%s)', | ||
| async ( | ||
| _name: string, | ||
| useSqlFunction: boolean, | ||
| ) => { | ||
| config.USE_ASSET_CREATE_HANDLER_SQL_FUNCTION = useSqlFunction; | ||
| const transactionIndex: number = 0; | ||
| const kafkaMessage: KafkaMessage = createKafkaMessageFromAssetEvent({ | ||
| assetEvent: defaultAssetCreateEvent, | ||
| transactionIndex, | ||
| height: defaultHeight, | ||
| time: defaultTime, | ||
| txHash: defaultTxHash, | ||
| }); | ||
|
|
||
| it('creates new asset', async () => { | ||
| await MarketTable.create(testConstants.defaultMarket); | ||
| await marketRefresher.updateMarkets(); | ||
| const transactionIndex: number = 0; | ||
|
|
||
| const assetEvent: AssetCreateEventV1 = defaultAssetCreateEvent; | ||
| const kafkaMessage: KafkaMessage = createKafkaMessageFromAssetEvent({ | ||
| assetEvent, | ||
| transactionIndex, | ||
| height: defaultHeight, | ||
| time: defaultTime, | ||
| txHash: defaultTxHash, | ||
| await expect(onMessage(kafkaMessage)).rejects.toThrowError( | ||
| 'Unable to find market with id: 0', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The parameterized tests are a good way to test the same logic with different configurations. However, the configuration is being set directly in the test. This could lead to unpredictable test results if the tests are run in parallel. Consider setting the configuration in a beforeEach block within the describe.each block to ensure that the configuration is set correctly for each test.
- it.each([
- [
- 'via knex',
- false,
- ],
- [
- 'via SQL function',
- true,
- ],
- ])(
- 'fails when market doesnt exist for asset (%s)',
- async (
- _name: string,
- useSqlFunction: boolean,
- ) => {
- config.USE_ASSET_CREATE_HANDLER_SQL_FUNCTION = useSqlFunction;
- // rest of the test
- });
+ describe.each([
+ [
+ 'via knex',
+ false,
+ ],
+ [
+ 'via SQL function',
+ true,
+ ],
+ ])('Asset creation %s', (_name: string, useSqlFunction: boolean) => {
+ beforeEach(() => {
+ config.USE_ASSET_CREATE_HANDLER_SQL_FUNCTION = useSqlFunction;
+ });
+
+ it('fails when market doesnt exist for asset', async () => {
+ // rest of the test
+ });
+ });Commitable suggestion (Beta)
| }); | |
| }); | |
| it('fails when market doesnt exist for asset', async () => { | |
| const transactionIndex: number = 0; | |
| const kafkaMessage: KafkaMessage = createKafkaMessageFromAssetEvent({ | |
| assetEvent: defaultAssetCreateEvent, | |
| transactionIndex, | |
| height: defaultHeight, | |
| time: defaultTime, | |
| txHash: defaultTxHash, | |
| }); | |
| const message: string = 'Unable to find market with id: 0'; | |
| await expect(onMessage(kafkaMessage)).rejects.toThrowError( | |
| new Error(message), | |
| ); | |
| }); | |
| it.each([ | |
| [ | |
| 'via knex', | |
| false, | |
| ], | |
| [ | |
| 'via SQL function', | |
| true, | |
| ], | |
| ])( | |
| 'fails when market doesnt exist for asset (%s)', | |
| async ( | |
| _name: string, | |
| useSqlFunction: boolean, | |
| ) => { | |
| config.USE_ASSET_CREATE_HANDLER_SQL_FUNCTION = useSqlFunction; | |
| const transactionIndex: number = 0; | |
| const kafkaMessage: KafkaMessage = createKafkaMessageFromAssetEvent({ | |
| assetEvent: defaultAssetCreateEvent, | |
| transactionIndex, | |
| height: defaultHeight, | |
| time: defaultTime, | |
| txHash: defaultTxHash, | |
| }); | |
| it('creates new asset', async () => { | |
| await MarketTable.create(testConstants.defaultMarket); | |
| await marketRefresher.updateMarkets(); | |
| const transactionIndex: number = 0; | |
| const assetEvent: AssetCreateEventV1 = defaultAssetCreateEvent; | |
| const kafkaMessage: KafkaMessage = createKafkaMessageFromAssetEvent({ | |
| assetEvent, | |
| transactionIndex, | |
| height: defaultHeight, | |
| time: defaultTime, | |
| txHash: defaultTxHash, | |
| await expect(onMessage(kafkaMessage)).rejects.toThrowError( | |
| 'Unable to find market with id: 0', | |
| ); | |
| }); | |
| describe.each([ | |
| [ | |
| 'via knex', | |
| false, | |
| ], | |
| [ | |
| 'via SQL function', | |
| true, | |
| ], | |
| ])('Asset creation %s', (_name: string, useSqlFunction: boolean) => { | |
| beforeEach(() => { | |
| config.USE_ASSET_CREATE_HANDLER_SQL_FUNCTION = useSqlFunction; | |
| }); | |
| it('fails when market doesnt exist for asset', async () => { | |
| // rest of the test | |
| }); | |
| }); |
| it.each([ | ||
| [ | ||
| 'via knex', | ||
| false, | ||
| ], | ||
| [ | ||
| 'via SQL function', | ||
| true, | ||
| ], | ||
| ])( | ||
| 'creates new asset (%s)', | ||
| async ( | ||
| _name: string, | ||
| useSqlFunction: boolean, | ||
| ) => { | ||
| config.USE_ASSET_CREATE_HANDLER_SQL_FUNCTION = useSqlFunction; | ||
| await MarketTable.create(testConstants.defaultMarket); | ||
| await marketRefresher.updateMarkets(); | ||
| const transactionIndex: number = 0; | ||
|
|
||
| const newAssets: AssetFromDatabase[] = await AssetTable.findAll( | ||
| {}, | ||
| [], { | ||
| orderBy: [[AssetColumns.id, Ordering.ASC]], | ||
| const assetEvent: AssetCreateEventV1 = defaultAssetCreateEvent; | ||
| const kafkaMessage: KafkaMessage = createKafkaMessageFromAssetEvent({ | ||
| assetEvent, | ||
| transactionIndex, | ||
| height: defaultHeight, | ||
| time: defaultTime, | ||
| txHash: defaultTxHash, | ||
| }); | ||
| expect(newAssets.length).toEqual(1); | ||
| expectAssetMatchesEvent(assetEvent, newAssets[0]); | ||
| expectTimingStats(); | ||
| const asset: AssetFromDatabase = assetRefresher.getAssetFromId('0'); | ||
| expect(asset).toBeDefined(); | ||
| }); | ||
| // Confirm there is no existing asset to or from the sender subaccount | ||
| await expectNoExistingAssets(); | ||
|
|
||
| await onMessage(kafkaMessage); | ||
|
|
||
| const newAssets: AssetFromDatabase[] = await AssetTable.findAll( | ||
| {}, | ||
| [], { | ||
| orderBy: [[AssetColumns.id, Ordering.ASC]], | ||
| }); | ||
| expect(newAssets.length).toEqual(1); | ||
| expectAssetMatchesEvent(assetEvent, newAssets[0]); | ||
| if (!useSqlFunction) { | ||
| expectTimingStats(); | ||
| } | ||
| const asset: AssetFromDatabase = assetRefresher.getAssetFromId('0'); | ||
| expect(asset).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
The same comment applies to these parameterized tests as well.
Changelist
[IND-470] Update asset create handler to use SQL function.
Test Plan
Updated existing tests.
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.