Update BlockTable.getLatest to throw an error and simplify logic #915
Conversation
|
Warning Rate Limit Exceeded@Christopher-Li has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 5 minutes and 56 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 overall change across various files in the project focuses on the handling of the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 (14)
- indexer/packages/postgres/tests/stores/block-table.test.ts (1 hunks)
- indexer/packages/postgres/src/stores/block-table.ts (3 hunks)
- indexer/services/comlink/tests/controllers/api/v4/height-controller.test.ts (1 hunks)
- indexer/services/comlink/tests/lib/helpers.test.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts (4 hunks)
- indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/height-controller.ts (1 hunks)
- indexer/services/ender/src/caches/block-cache.ts (1 hunks)
- indexer/services/roundtable/src/tasks/cancel-stale-orders.ts (2 hunks)
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (2 hunks)
- indexer/services/roundtable/src/tasks/remove-expired-orders.ts (1 hunks)
- indexer/services/roundtable/src/tasks/track-lag.ts (2 hunks)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts (1 hunks)
- indexer/services/vulcan/src/handlers/order-remove-handler.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts
Additional comments: 15
indexer/services/comlink/__tests__/controllers/api/v4/height-controller.test.ts (1)
- 29-29: The update to the type declaration of
latestBlockis consistent with the new behavior ofBlockTable.getLatestalways returning aBlockFromDatabaseobject.indexer/services/roundtable/src/tasks/cancel-stale-orders.ts (1)
- 19-19: The update to the type declaration of
latestBlockis consistent with the new behavior ofBlockTable.getLatestalways returning aBlockFromDatabaseobject.indexer/services/comlink/src/controllers/api/v4/height-controller.ts (1)
- 22-31: The refactoring to use a
try...catchblock ingetHeightmethod is consistent with the new behavior ofBlockTable.getLatestand correctly throws aNotFoundErrorwhen no block is found.indexer/packages/postgres/__tests__/stores/block-table.test.ts (2)
87-87: The update to the type declaration of
blockis consistent with the new behavior ofBlockTable.getLatestalways returning aBlockFromDatabaseobject.92-92: The test now correctly expects a rejection when
BlockTable.getLatestis unable to find the latest block, aligning with the updated behavior of the method.indexer/packages/postgres/src/stores/block-table.ts (1)
- 103-115: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [95-114]
The
getLatestfunction has been correctly updated to throw an error when no latest block is found, which is consistent with the new expected behavior.indexer/services/roundtable/src/tasks/track-lag.ts (1)
- 31-31: The update to the assignment of
indexerBlockFromDatabaseis consistent with the new behavior ofBlockTable.getLatestalways returning aBlockFromDatabaseobject.indexer/services/ender/src/caches/block-cache.ts (1)
- 20-21: The update to the
refreshBlockCachefunction is consistent with the new behavior ofBlockTable.getLatestalways returning aBlockFromDatabaseobject.indexer/services/roundtable/src/tasks/remove-expired-orders.ts (1)
- 29-29: The update to the
runTaskfunction is consistent with the new behavior ofBlockTable.getLatestalways returning aBlockFromDatabaseobject.indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts (2)
- 74-80: The change in the
Promise.allcall reflects the updated behavior ofBlockTable.getLatestto no longer returnundefined. Ensure that all error handling and fallbacks related tolatestBlockbeingundefinedhave been removed throughout the codebase.Verification successful
Given that the search for any remaining checks for
latestBlockbeingundefineddid not produce any output, it suggests that there are no instances in the TypeScript files wherelatestBlockis compared toundefined. This aligns with the changes made in the PR, whereBlockTable.getLatestno longer returnsundefined, and thus such checks should have been removed.Based on the information provided:
- The PR's objective was to update
BlockTable.getLatestto throw an error instead of returningundefined.- The AI-generated summary confirms that the method's return type no longer includes
undefinedand that the codebase has been updated accordingly.- The executed script did not find any instances of
latestBlockbeing compared toundefined.Therefore, the review comment is consistent with the changes made in the PR, and the verification script supports the conclusion that the necessary codebase updates have been made.
* 119-122: The removal of the check for `latestBlock` being `undefined` is consistent with the new behavior of `BlockTable.getLatest`. Ensure that the error handling for the `BlockTable.getLatest` method is robust and that it properly propagates errors so that this assumption remains valid.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining checks for `latestBlock` being undefined. rg --type ts "latestBlock\s*?==\s*?undefined"Length of output: 47
indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts (3)
95-97: The removal of the check for
latestBlockbeingundefinedis consistent with the new behavior ofBlockTable.getLatest. The addition of theNotFoundErrorthrow is appropriate for the case where no subaccounts are found for the given address.175-181: The change in the
Promise.allcall reflects the updated behavior ofBlockTable.getLatestto no longer returnundefined. Ensure that all error handling and fallbacks related tolatestBlockbeingundefinedhave been removed throughout the codebase.Verification successful
The executed script did not find any instances of
latestBlockbeing checked againstundefinedin TypeScript files, which aligns with the changes made in the PR to ensure thatBlockTable.getLatestno longer returnsundefined. This suggests that the necessary removals of error handling and fallbacks related tolatestBlockbeingundefinedhave been carried out throughout the TypeScript codebase.However, since the script only checked TypeScript files and the repository could contain JavaScript files or other relevant code that wasn't covered, it might be prudent to run an additional check without the TypeScript filter to ensure that no instances were missed.
The additional script also did not find any instances of
latestBlockbeing checked againstundefined. This further supports the conclusion that the necessary updates to remove checks forlatestBlockbeingundefinedhave been successfully made throughout the codebase.Given the information from the executed scripts, it appears that the changes related to the handling of
latestBlockare consistent with the PR's objectives and the review comment is accurate.* 200-203: The removal of the check for `latestBlock` being `undefined` is consistent with the new behavior of `BlockTable.getLatest`. The addition of the `NotFoundError` throw is appropriate for the case where no subaccount is found with the given address and subaccount number.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining checks for `latestBlock` being undefined. rg --type ts "latestBlock\s*?==\s*?undefined"Length of output: 47
Script:
#!/bin/bash # Search for any remaining checks for `latestBlock` being undefined without TypeScript filter. rg "latestBlock\s*?==\s*?undefined"Length of output: 37
indexer/services/vulcan/src/handlers/order-remove-handler.ts (1)
- 393-399: The method
isOrderExpiredhas been updated to useBlockTable.getLatestwhich now throws an error instead of returningundefined. Ensure that the calling code is prepared to handle this exception.
| const tableSpy = jest.spyOn(BlockTable, 'getLatest').mockImplementation(() => { | ||
| throw new Error(); | ||
| }); |
There was a problem hiding this comment.
The mock implementation of BlockTable.getLatest is set to throw an error unconditionally. Ensure that this behavior aligns with the intended test scenario, as it may not accurately reflect the behavior of the actual method under test conditions where it should succeed.
| latestBlock, | ||
| ]: [ | ||
| SubaccountFromDatabase | undefined, | ||
| BlockFromDatabase | undefined, | ||
| BlockFromDatabase, | ||
| ] = await Promise.all([ | ||
| SubaccountTable.findById(testConstants.defaultSubaccountId), | ||
| BlockTable.getLatest(), |
There was a problem hiding this comment.
The script output indicates that there is still a check for latestBlock === undefined in the perpetual-positions-controller.ts file. This check should be removed or replaced with appropriate error handling, given that BlockTable.getLatest() will now throw an error instead of returning undefined.
No output was produced for the asset-positions-controller.ts and addresses-controller.ts files, which suggests that there are no checks for latestBlock === undefined in these files, or the files do not exist or are not accessible. However, since we only have evidence of the check in perpetual-positions-controller.ts, we will focus on that.
Based on the provided information, the following issue has been identified:
- The
perpetual-positions-controller.tsfile contains a check forlatestBlock === undefined, which is no longer necessary and should be replaced with error handling to align with the new behavior ofBlockTable.getLatest().
Analysis chain
The change to the latestBlock variable type from BlockFromDatabase | undefined to BlockFromDatabase aligns with the PR's objective to ensure that BlockTable.getLatest always returns a BlockFromDatabase object. This change will require that all usages of latestBlock in the codebase are updated to handle a thrown error instead of an undefined check.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all usages of `latestBlock` handle the new error-throwing behavior.
rg --type ts "latestBlock" --context 5
Length of output: 40307
Script:
#!/bin/bash
# Check for the presence of 'undefined' checks related to 'latestBlock' in the specified files
# and ensure they have been replaced with error handling.
# Check in perpetual-positions-controller.ts
echo "Checking perpetual-positions-controller.ts"
rg "latestBlock === undefined" "indexer/services/comlink/src/controllers/api/v4/perpetual-positions-controller.ts"
# Check in asset-positions-controller.ts
echo "Checking asset-positions-controller.ts"
rg "latestBlock === undefined" "indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts"
# Check in addresses-controller.ts
echo "Checking addresses-controller.ts"
rg "latestBlock === undefined" "indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts"
Length of output: 654
| ): Promise<SubaccountResponseObject[]> { | ||
| // TODO(IND-189): Use a transaction across all the DB queries | ||
| const [subaccounts, latestBlock]: | ||
| [SubaccountFromDatabase[], BlockFromDatabase | undefined] = await Promise.all([ | ||
| [SubaccountFromDatabase[], BlockFromDatabase] = await Promise.all([ | ||
| SubaccountTable.findAll( | ||
| { |
There was a problem hiding this comment.
- The check for
latestBlockbeingundefinedinperpetual-positions-controller.tsis inconsistent with the PR's objectives and should be addressed. - The check within
block-table.tsappears to be part of thegetLatestmethod's logic to throw an error and is likely correct as per the PR's description.
Analysis chain
The change in the Promise.all call reflects the updated behavior of BlockTable.getLatest to no longer return undefined. Ensure that all error handling and fallbacks related to latestBlock being undefined have been removed throughout the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining checks for `latestBlock` being undefined.
rg --type ts "latestBlock\s*?==\s*?undefined"
Length of output: 47
Script:
#!/bin/bash
# Search for a broader pattern of checks involving `latestBlock` and `undefined`.
rg --type ts "latestBlock\s*?[=!]==?\s*?undefined"
Length of output: 288
Script:
#!/bin/bash
# Check the context of the lines where `latestBlock` is being checked against `undefined`.
rg --type ts -C 3 "latestBlock\s*?[=!]==?\s*?undefined" indexer/services/comlink/src/controllers/api/v4/perpetual-positions-controller.ts
rg --type ts -C 3 "latestBlock\s*?[=!]==?\s*?undefined" indexer/packages/postgres/src/stores/block-table.ts
Length of output: 746
Christopher-Li
force-pushed
the
cl_get_latest_block_error
branch
from
5e768f6 to
fdc2d8a
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (14)
- indexer/packages/postgres/tests/stores/block-table.test.ts (1 hunks)
- indexer/packages/postgres/src/stores/block-table.ts (3 hunks)
- indexer/services/comlink/tests/controllers/api/v4/height-controller.test.ts (1 hunks)
- indexer/services/comlink/tests/lib/helpers.test.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts (4 hunks)
- indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/height-controller.ts (1 hunks)
- indexer/services/ender/src/caches/block-cache.ts (1 hunks)
- indexer/services/roundtable/src/tasks/cancel-stale-orders.ts (2 hunks)
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (2 hunks)
- indexer/services/roundtable/src/tasks/remove-expired-orders.ts (1 hunks)
- indexer/services/roundtable/src/tasks/track-lag.ts (2 hunks)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts (1 hunks)
- indexer/services/vulcan/src/handlers/order-remove-handler.ts (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- indexer/packages/postgres/tests/stores/block-table.test.ts
- indexer/packages/postgres/src/stores/block-table.ts
- indexer/services/comlink/tests/controllers/api/v4/height-controller.test.ts
- indexer/services/comlink/tests/lib/helpers.test.ts
- indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts
- indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts
- indexer/services/comlink/src/controllers/api/v4/height-controller.ts
- indexer/services/ender/src/caches/block-cache.ts
- indexer/services/roundtable/src/tasks/cancel-stale-orders.ts
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts
- indexer/services/roundtable/src/tasks/remove-expired-orders.ts
- indexer/services/roundtable/src/tasks/track-lag.ts
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts
- indexer/services/vulcan/src/handlers/order-remove-handler.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/packages/base/src/stats-util.ts (1 hunks)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts
| let result; | ||
| try { | ||
| result = await promise; | ||
| } catch (error) { | ||
| stats.timing( | ||
| `${config.SERVICE_NAME}.${functionName ?? STATS_FUNCTION_NAME}.timing`, | ||
| Date.now() - start, | ||
| options, | ||
| ); | ||
| throw error; | ||
| } | ||
| stats.timing( | ||
| `${config.SERVICE_NAME}.${functionName ?? STATS_FUNCTION_NAME}.timing`, | ||
| Date.now() - start, |
There was a problem hiding this comment.
The addition of a try-catch block for error handling in runFuncWithTimingStat is a good practice for capturing errors during the execution of the promise. However, the stats.timing call is duplicated in both the catch block and after the try-catch block. Consider refactoring to avoid this duplication and ensure that timing stats are recorded whether an error occurs or not.
const start: number = Date.now();
let result;
try {
result = await promise;
} catch (error) {
stats.timing(
`${config.SERVICE_NAME}.${functionName ?? STATS_FUNCTION_NAME}.timing`,
Date.now() - start,
options,
);
throw error;
}
+ stats.timing(
+ `${config.SERVICE_NAME}.${functionName ?? STATS_FUNCTION_NAME}.timing`,
+ Date.now() - start,
+ options,
+ );
- stats.timing(
- `${config.SERVICE_NAME}.${functionName ?? STATS_FUNCTION_NAME}.timing`,
- Date.now() - start,
- options,
- );
return result;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.
| let result; | |
| try { | |
| result = await promise; | |
| } catch (error) { | |
| stats.timing( | |
| `${config.SERVICE_NAME}.${functionName ?? STATS_FUNCTION_NAME}.timing`, | |
| Date.now() - start, | |
| options, | |
| ); | |
| throw error; | |
| } | |
| stats.timing( | |
| `${config.SERVICE_NAME}.${functionName ?? STATS_FUNCTION_NAME}.timing`, | |
| Date.now() - start, | |
| const start: number = Date.now(); | |
| let result; | |
| try { | |
| result = await promise; | |
| } catch (error) { | |
| stats.timing( | |
| `${config.SERVICE_NAME}.${functionName ?? STATS_FUNCTION_NAME}.timing`, | |
| Date.now() - start, | |
| options, | |
| ); | |
| throw error; | |
| } | |
| stats.timing( | |
| `${config.SERVICE_NAME}.${functionName ?? STATS_FUNCTION_NAME}.timing`, | |
| Date.now() - start, | |
| options, | |
| ); | |
| return result; |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/ender/src/caches/block-cache.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/ender/src/caches/block-cache.ts
Changelist
Update BlockTable.getLatest to throw an error and simplify logic
Test Plan
Update unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.