[IND-481] Remove market cache. #818
Conversation
More clean-up in preparation of migration to SQL based block processor.
IND-481 Update ender block handler with a single SQL handler that delegates to sub-handlers
Update |
WalkthroughThe recent updates involve the removal of 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: 8
Configuration used: CodeRabbit UI
Files selected for processing (13)
- indexer/packages/postgres/src/index.ts (1 hunks)
- indexer/services/ender/tests/caches/block-cache.test.ts (5 hunks)
- indexer/services/ender/tests/handlers/asset-handler.test.ts (2 hunks)
- indexer/services/ender/tests/handlers/perpetual-market-handler.test.ts (3 hunks)
- indexer/services/ender/tests/lib/on-message.test.ts (3 hunks)
- indexer/services/ender/tests/validators/asset-validator.test.ts (2 hunks)
- indexer/services/ender/tests/validators/perpetual-market-validator.test.ts (2 hunks)
- indexer/services/ender/src/caches/block-cache.ts (2 hunks)
- indexer/services/ender/src/handlers/markets/market-create-handler.ts (2 hunks)
- indexer/services/ender/src/handlers/markets/market-modify-handler.ts (2 hunks)
- indexer/services/ender/src/handlers/markets/market-price-update-handler.ts (2 hunks)
- indexer/services/ender/src/index.ts (2 hunks)
- indexer/services/ender/src/lib/cache-manager.ts (2 hunks)
Files skipped from review due to trivial changes (3)
- indexer/packages/postgres/src/index.ts
- indexer/services/ender/tests/handlers/asset-handler.test.ts
- indexer/services/ender/tests/handlers/perpetual-market-handler.test.ts
Additional comments: 14
indexer/services/ender/__tests__/caches/block-cache.test.ts (1)
- 35-36: The summary states that calls to
marketRefreshermethods have been removed, but the hunks show calls toperpetualMarketRefresher.clear()andassetRefresher.clear(). Please verify if these are part of the intended changes and update the summary or the code accordingly.indexer/services/ender/__tests__/lib/on-message.test.ts (2)
- 892-902: The code still contains calls to
perpetualMarketRefresher.updatePerpetualMarkets()andperpetualMarketRefresher.clear(), which should have been removed according to the summary and pull request details.- await perpetualMarketRefresher.updatePerpetualMarkets(); - assetRefresher.clear(); - perpetualMarketRefresher.clear();
- 906-909: The code still contains calls to
perpetualMarketRefresher.getPerpetualMarketsMap(), which should have been removed according to the summary and pull request details.- expect(perpetualMarketRefresher.getPerpetualMarketsMap()).not.toEqual({});indexer/services/ender/__tests__/validators/asset-validator.test.ts (2)
1-6:
The removal ofmarketRefresherfrom the import statement is consistent with the summary and pull request description.15-19:
ThebeforeEachhook has been correctly updated to remove the call tomarketRefresher.updateMarkets(), aligning with the changes described in the summary and pull request.indexer/services/ender/src/caches/block-cache.ts (2)
5-9: The import statements for
perpetualMarketRefresherandassetRefresherare still present, which is consistent with the pull request summary that does not mention their removal.110-113: The calls to
perpetualMarketRefresher.updatePerpetualMarkets({ txId })andassetRefresher.updateAssets({ txId })are still present, which aligns with the pull request summary that does not state their removal.indexer/services/ender/src/handlers/markets/market-create-handler.ts (2)
2-6: The removal of
marketRefresheris consistent with the summary and pull request description. Ensure that the removal of this functionality does not leave any orphaned code or logic that depends on the removed functionality elsewhere in the codebase.51-56: The logic for updating the market cache has been removed from the
MarketCreateHandler. Verify that this change aligns with the intended system behavior and that no other parts of the system are adversely affected by the removal of this functionality.indexer/services/ender/src/handlers/markets/market-modify-handler.ts (1)
- 55-56: The method
internalHandle()is now returning an empty array, which is consistent with the summary stating that the return type has been changed to void. However, the method signature still indicates that it returns an array. Please verify if the method signature should be updated to reflect that it now returns void, or if the implementation should be changed to match the expected return type.indexer/services/ender/src/handlers/markets/market-price-update-handler.ts (2)
3-9: The removal of
marketRefresherfrom the imports is consistent with the summary and pull request description.62-67: The code within
MarketPriceUpdateHandlerhas been updated to remove the market cache update logic, which is consistent with the summary and pull request description.indexer/services/ender/src/index.ts (2)
3-5: The import statement for
marketRefresheris still present, which might be an oversight if it's no longer used in the file. Please verify ifmarketRefresheris used elsewhere in the file or if the import should be removed.24-28: The changes to the
startKafkafunction reflect the removal of themarketRefresher.updateMarkets()call, which aligns with the intent to remove the market cache functionality. This change appears to be correct and consistent with the summary and pull request.
| @@ -13,7 +12,6 @@ export async function refreshDataCaches(): Promise<void> { | |||
| await Promise.all([ | |||
| perpetualMarketRefresher.updatePerpetualMarkets({ txId, readReplica: true }), | |||
| assetRefresher.updateAssets({ txId, readReplica: true }), | |||
| marketRefresher.updateMarkets({ txId, readReplica: true }), | |||
| ]); | |||
|
|
|||
| await Transaction.rollback(txId); | |||
There was a problem hiding this comment.
The refreshDataCaches function ends with a rollback, which undoes any changes made within the transaction. This could be an error unless intentionally designed to simulate the updates without persisting them. Please verify the intended behavior.
- await Transaction.rollback(txId);
+ await Transaction.commit(txId);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.
| await Transaction.rollback(txId); | |
| await Transaction.commit(txId); |
| @@ -1,7 +1,7 @@ | |||
| import { logger, ParseMessageError } from '@dydxprotocol-indexer/base'; | |||
| import { PerpetualMarketCreateEventV1, IndexerTendermintBlock, IndexerTendermintEvent } from '@dydxprotocol-indexer/v4-protos'; | |||
| import { | |||
| dbHelpers, marketRefresher, testMocks, perpetualMarketRefresher, | |||
| dbHelpers, testMocks, perpetualMarketRefresher, | |||
There was a problem hiding this comment.
The import of perpetualMarketRefresher should be removed as per the summary of changes.
- dbHelpers, testMocks, perpetualMarketRefresher,
+ dbHelpers, testMocks,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.
| dbHelpers, testMocks, perpetualMarketRefresher, | |
| dbHelpers, testMocks, |
| import { logger, ParseMessageError } from '@dydxprotocol-indexer/base'; | ||
| import { PerpetualMarketCreateEventV1, IndexerTendermintBlock, IndexerTendermintEvent } from '@dydxprotocol-indexer/v4-protos'; | ||
| import { | ||
| dbHelpers, marketRefresher, testMocks, perpetualMarketRefresher, | ||
| dbHelpers, testMocks, perpetualMarketRefresher, | ||
| } from '@dydxprotocol-indexer/postgres'; | ||
| import { DydxIndexerSubtypes } from '../../src/lib/types'; | ||
| import { |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [24-24]
The call to perpetualMarketRefresher.clear() in the afterEach hook should be removed as it is no longer necessary with the removal of the market cache functionality.
- await perpetualMarketRefresher.clear();Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [34-34]
The call to perpetualMarketRefresher.updatePerpetualMarkets() in the test case for 'throws error on existing perpetual market' should be removed as it is no longer necessary with the removal of the market cache functionality.
- await perpetualMarketRefresher.updatePerpetualMarkets();| expect(perpetualMarketRefresher.getPerpetualMarketsMap()).not.toEqual({}); | ||
| expect(assetRefresher.getAssetsMap()).not.toEqual({}); |
There was a problem hiding this comment.
The summary indicates that references to marketRefresher have been removed, but the test assertions still check the state of perpetualMarketRefresher.getPerpetualMarketsMap() and assetRefresher.getAssetsMap(). These assertions should be removed if marketRefresher and its methods are no longer used.
| expect(perpetualMarketRefresher.getPerpetualMarketsMap()).toEqual({}); | ||
| expect(assetRefresher.getAssetsMap()).toEqual({}); |
There was a problem hiding this comment.
The test setup still contains calls to perpetualMarketRefresher.getPerpetualMarketsMap() and assetRefresher.getAssetsMap(), which should be removed according to the summary that states all references to marketRefresher have been removed.
| expect(perpetualMarketRefresher.getPerpetualMarketsMap()).not.toEqual({}); | ||
| expect(assetRefresher.getAssetsMap()).not.toEqual({}); |
There was a problem hiding this comment.
The test assertions still check the state of perpetualMarketRefresher.getPerpetualMarketsMap() and assetRefresher.getAssetsMap(). If these are related to the marketRefresher that was removed, these assertions should also be removed.
| @@ -111,7 +110,6 @@ export async function initializeAllCaches(): Promise<void> { | |||
| startCandleCache(txId), | |||
| perpetualMarketRefresher.updatePerpetualMarkets({ txId }), | |||
| assetRefresher.updateAssets({ txId }), | |||
| marketRefresher.updateMarkets({ txId }), | |||
| ]); | |||
|
|
|||
| await Transaction.rollback(txId); | |||
There was a problem hiding this comment.
The transaction in initializeAllCaches is being rolled back at the end of the function. This would undo any updates made by the refresh functions. If this is not intentional, it should be replaced with a commit.
- await Transaction.rollback(txId);
+ await Transaction.commit(txId);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.
| await Transaction.rollback(txId); | |
| await Transaction.commit(txId); |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- 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 (2 hunks)
Additional comments: 5
indexer/services/ender/src/handlers/markets/market-create-handler.ts (3)
25-29: The changes from lines 25 to 29 show the updated
rawQuerycall without assigning the result to a variable, which is consistent with the summary. However, ensure that the lack of assignment does not affect any downstream logic that may have depended on the result of this query.46-46: The error handling logic on line 46 rethrows the error if it does not match the specific condition checked. Ensure that this behavior is intended and that there are no additional error cases that need to be handled here.
49-49: The function now returns an empty array on line 49. Confirm that this is the intended behavior and that the consumers of this function are expecting an empty array as a valid response.
indexer/services/ender/src/handlers/markets/market-modify-handler.ts (1)
- 52-53: The function
internalHandleis still returning an empty array, which contradicts the summary stating that the return type has changed to void. Please verify if the return type should indeed be void or if the summary needs to be updated.- return []; + return; // If the function should return voidindexer/services/ender/src/handlers/markets/market-price-update-handler.ts (1)
- 3-9: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [3-67]
The changes in the
MarketPriceUpdateHandlerclass reflect the removal of themarketRefresherutility and its associated calls. The class now handles market price updates by directly querying the database and generating Kafka events, which aligns with the new architecture described in the pull request summary. The error handling and logging within theinternalHandlemethod are appropriate, and thegenerateKafkaEventmethod is correctly implemented to create the necessary Kafka event. Ensure that all dependent services and components are compatible with these changes and that the removal of themarketRefresherdoes not introduce any regressions or unexpected behavior.
Changelist
[IND-481] Remove market cache.
More clean-up in preparation of migration to SQL based block processor.
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.