[IND-467] Push ender market create logic to use a single sql function #737
Conversation
WalkthroughThe changes introduce a new Changes
TipsChat with CodeRabbit Bot (
|
IND-467 Ender market create handler SQL pushdown
Update |
![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 (14)
- indexer/packages/postgres/src/constants.ts (2 hunks)
- indexer/packages/postgres/src/index.ts (1 hunks)
- indexer/packages/postgres/src/loops/market-refresher.ts (1 hunks)
- indexer/packages/postgres/src/models/market-model.ts (1 hunks)
- indexer/services/ender/tests/handlers/markets/market-create-handler.test.ts (2 hunks)
- indexer/services/ender/tests/scripts/scripts.test.ts (1 hunks)
- indexer/services/ender/src/config.ts (1 hunks)
- indexer/services/ender/src/handlers/markets/market-create-handler.ts (3 hunks)
- indexer/services/ender/src/handlers/order-fills/liquidation-handler.ts (1 hunks)
- indexer/services/ender/src/handlers/order-fills/order-handler.ts (1 hunks)
- indexer/services/ender/src/handlers/subaccount-update-handler.ts (1 hunks)
- indexer/services/ender/src/helpers/postgres/postgres-functions.ts (2 hunks)
- indexer/services/ender/src/lib/on-message.ts (1 hunks)
- indexer/services/ender/src/scripts/dydx_market_create_handler.sql (1 hunks)
Files skipped from review due to trivial changes (8)
- indexer/packages/postgres/src/constants.ts
- indexer/packages/postgres/src/index.ts
- indexer/services/ender/tests/scripts/scripts.test.ts
- indexer/services/ender/src/handlers/order-fills/liquidation-handler.ts
- indexer/services/ender/src/handlers/order-fills/order-handler.ts
- indexer/services/ender/src/handlers/subaccount-update-handler.ts
- indexer/services/ender/src/helpers/postgres/postgres-functions.ts
- indexer/services/ender/src/lib/on-message.ts
Additional comments: 10
indexer/services/ender/src/config.ts (1)
- 29-34: The new configuration option
USE_MARKET_CREATE_HANDLER_SQL_FUNCTIONis added and set totrueby default. This means the new SQL function for market creation will be used by default. Ensure that the new SQL function is thoroughly tested and ready for production use.indexer/services/ender/__tests__/handlers/markets/market-create-handler.test.ts (2)
90-134: The test case 'creates new market' is well written and covers both the scenarios of creating a new market via knex and via SQL function. It checks if the market created matches the event data.
136-180: The test case 'errors when attempting to create an existing market' is also well written and covers both the scenarios of creating an existing market via knex and via SQL function. It checks if the correct error is thrown and if the market in the database is the old market. It also checks if the error logs are correct and if no messages were sent to the producer.
indexer/packages/postgres/src/models/market-model.ts (1)
- 54-68: The addition of the
sqlToJsonConversionsstatic method is a good practice for mapping SQL data types to JSON data types. This will help in maintaining consistency when dealing with data conversions. However, ensure that the mappings are correct and that they match the actual data types in the database. Also, the TODO comment on line 58 indicates that there might be a need to ensure consistency betweenjsonSchema(),sqlToJsonConversions(), and model fields. This should be addressed to avoid potential issues in the future.indexer/services/ender/src/handlers/markets/market-create-handler.ts (5)
1-15: Imports are correctly added for the new functionality.
24-34: The
internalHandle()method has been updated to conditionally call eitherhandleViaSqlFunction()orhandleViaKnexQueries(). Ensure that theUSE_MARKET_CREATE_HANDLER_SQL_FUNCTIONconfiguration option is correctly set in all environments where this code will run.37-40: The
handleViaKnexQueries()method has been added to handle market creation via Knex queries. This is a good approach for maintaining backward compatibility.57-86: The
handleViaSqlFunction()method has been added to handle market creation via a SQL function. This is a good approach for improving performance by pushing logic to the database. However, be aware that using raw SQL queries can increase the risk of SQL injection attacks. Make sure that the input to the SQL function is properly sanitized and parameterized.- `SELECT dydx_market_create_handler( - '${JSON.stringify(MarketEventV1.decode(eventDataBinary))}' - ) AS result;`, + `SELECT dydx_market_create_handler($1) AS result;`, + [JSON.stringify(MarketEventV1.decode(eventDataBinary))],
- 88-90: The
createMarket()method has been updated to useMarketTable.create(). This is a good approach for improving code readability and maintainability.indexer/services/ender/src/scripts/dydx_market_create_handler.sql (1)
- 1-25: The SQL function
dydx_market_create_handlerseems to be well written and follows good practices. It checks for an existing market before creating a new one, which is a good way to prevent duplicate entries. However, it's important to ensure that thedydx_to_jsonbfunction used in line 22 is defined and works as expected. Also, make sure that themarketstable and its columns exist and match the types used in this function.
| stats.timing(`${config.SERVICE_NAME}.loops.update_markets`, Date.now() - startTime); | ||
| } | ||
|
|
||
| /** | ||
| * Updates the markets map with the specified market. | ||
| */ | ||
| export function updateMarket(market: MarketFromDatabase): void { | ||
| idToMarket[market.id] = market; | ||
| } |
There was a problem hiding this comment.
The updateMarket function directly modifies the idToMarket map. This could potentially lead to data races if multiple threads are trying to update the map at the same time. Consider using a lock or other synchronization mechanism to ensure thread safety.
+ let updateLock = false;
export function updateMarket(market: MarketFromDatabase): void {
+ while(updateLock) {} // wait for lock to be released
+ updateLock = true; // acquire lock
idToMarket[market.id] = market;
+ updateLock = false; // release lock
}Commitable suggestion (Beta)
| stats.timing(`${config.SERVICE_NAME}.loops.update_markets`, Date.now() - startTime); | |
| } | |
| /** | |
| * Updates the markets map with the specified market. | |
| */ | |
| export function updateMarket(market: MarketFromDatabase): void { | |
| idToMarket[market.id] = market; | |
| } | |
| let updateLock = false; | |
| export function updateMarket(market: MarketFromDatabase): void { | |
| while(updateLock) {} // wait for lock to be released | |
| updateLock = true; // acquire lock | |
| idToMarket[market.id] = market; | |
| updateLock = false; // release lock | |
| } |
| IF FOUND THEN | ||
| RAISE EXCEPTION 'Market in MarketCreate already exists. Record: %', market_record; | ||
| END IF; |
There was a problem hiding this comment.
The error message in the exception is clear and provides useful information for debugging. However, consider adding more context to the error message, such as the event data that caused the exception. This can help with debugging if the error occurs.
- RAISE EXCEPTION 'Market in MarketCreate already exists. Record: %', market_record;
+ RAISE EXCEPTION 'Market in MarketCreate already exists. Event data: %, Record: %', event_data, market_record;Commitable suggestion (Beta)
| IF FOUND THEN | |
| RAISE EXCEPTION 'Market in MarketCreate already exists. Record: %', market_record; | |
| END IF; | |
| IF FOUND THEN | |
| RAISE EXCEPTION 'Market in MarketCreate already exists. Event data: %, Record: %', event_data, market_record; | |
| END IF; |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/ender/src/scripts/dydx_market_create_handler.sql (1 hunks)
Additional comments: 5
indexer/services/ender/src/scripts/dydx_market_create_handler.sql (5)
1-7: The function documentation is clear and provides a good overview of the function's purpose, parameters, and return value. It also provides links to the relevant data structures, which is helpful for understanding the context.
13-14: The code correctly retrieves the
marketIdfrom theevent_dataand checks if a market with the same ID already exists in themarketstable.16-18: The error handling here is appropriate. If a market with the same ID already exists, an exception is raised with a clear error message.
20-23: The
market_recordis populated with the necessary fields from theevent_data. The code correctly handles the conversion of JSON fields to the appropriate data types.27-30: The function correctly returns the created market as a JSON object in the market-model format.
| market_record."exponent" = (event_data->'marketCreate'->'exponent')::integer; | ||
| market_record."minPriceChangePpm" = (event_data->'marketCreate'->'base'->'minPriceChangePpm')::integer; | ||
|
|
||
| INSERT INTO markets VALUES (market_record.*); |
There was a problem hiding this comment.
The INSERT INTO statement doesn't specify the columns for which the values are being inserted. This can lead to errors if the structure of the markets table changes in the future. It's a good practice to always specify the column names in an INSERT INTO statement.
- INSERT INTO markets VALUES (market_record.*);
+ INSERT INTO markets (id, pair, exponent, minPriceChangePpm) VALUES (market_record.id, market_record.pair, market_record.exponent, market_record.minPriceChangePpm);Commitable suggestion (Beta)
| INSERT INTO markets VALUES (market_record.*); | |
| INSERT INTO markets (id, pair, exponent, minPriceChangePpm) VALUES (market_record.id, market_record.pair, market_record.exponent, market_record.minPriceChangePpm); |
There was a problem hiding this comment.
Seems like tests are failing because this test is missing a call to await createPostgresFunctions(); in beforeEach.
…clean up afterwards.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/ender/tests/lib/sync-handlers.test.ts (3 hunks)
- indexer/services/ender/src/handlers/markets/market-create-handler.ts (3 hunks)
Additional comments: 6
indexer/services/ender/__tests__/lib/sync-handlers.test.ts (3)
30-33: The import of
createPostgresFunctionsis fine. Ensure that the function is defined and exported correctly in the'../../src/helpers/postgres/postgres-functions'module.67-69: The
beforeAllblock is correctly used to set up the Postgres functions before running the tests.94-96: The
afterAllblock is correctly used to tear down the database after running the tests.indexer/services/ender/src/handlers/markets/market-create-handler.ts (3)
24-34: The
internalHandle()method now checks the configuration to decide whether to use the SQL function or Knex queries for market creation. This is a good approach for gradually transitioning between two different implementations and allows for easy rollback if issues arise with the new implementation.57-80: The
handleViaSqlFunction()method executes a raw SQL query and handles any errors that occur. It's important to note that raw SQL queries can be vulnerable to SQL injection attacks. However, in this case, the query does not directly incorporate user-provided data, so the risk is mitigated. Still, it's a good practice to use parameterized queries or prepared statements whenever possible to further reduce the risk.- `SELECT dydx_market_create_handler( - '${JSON.stringify(MarketEventV1.decode(eventDataBinary))}' - ) AS result;`, + `SELECT dydx_market_create_handler($1) AS result;`, + [JSON.stringify(MarketEventV1.decode(eventDataBinary))],
- 88-90: The
createMarket()method now usesMarketTable.create()instead of directly inserting the market data. This is a good practice as it abstracts the database operations and makes the code more maintainable.
…#737) * [IND-467] Push ender market create logic to use a single sql function
…#737) * [IND-467] Push ender market create logic to use a single sql function
…#737) * [IND-467] Push ender market create logic to use a single sql function
…#737) * [IND-467] Push ender market create logic to use a single sql function
Changelist
[IND-467] Push ender market create logic to use a single 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.