Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update BlockTable.getLatest to throw an error and simplify logic #915

Merged
merged 6 commits into from
Jan 2, 2024
+65 −57
Merged

Update BlockTable.getLatest to throw an error and simplify logic #915

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
612b34d
Update BlockTable.getLatest to throw an error and simplify logic
Christopher-Li Jan 2, 2024
fdc2d8a
nit
Christopher-Li Jan 2, 2024
bad492f
nits
Christopher-Li Jan 2, 2024
1c4384b
lint
Christopher-Li Jan 2, 2024
641abf5
fix
Christopher-Li Jan 2, 2024
47c8ffb
lint
Christopher-Li Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion indexer/packages/base/src/stats-util.ts
View file
Original file line number Diff line number Diff line change
@@ -11,7 +11,17 @@ export async function runFuncWithTimingStat(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
): Promise<any> {
const start: number = Date.now();
const result = await promise;
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,
Comment on lines +14 to 27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;

5 changes: 2 additions & 3 deletions indexer/packages/postgres/__tests__/stores/block-table.test.ts
View file
Original file line number Diff line number Diff line change
@@ -84,12 +84,11 @@ describe('Block store', () => {
BlockTable.create(defaultBlock2),
]);

const block: BlockFromDatabase | undefined = await BlockTable.getLatest();
const block: BlockFromDatabase = await BlockTable.getLatest();
expect(block).toEqual(expect.objectContaining(defaultBlock2));
});

it('Unable to find latest Block', async () => {
const block: BlockFromDatabase | undefined = await BlockTable.getLatest();
expect(block).toEqual(undefined);
await expect(BlockTable.getLatest()).rejects.toEqual(new Error('Unable to find latest block'));
});
});
13 changes: 11 additions & 2 deletions indexer/packages/postgres/src/stores/block-table.ts
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { logger } from '@dydxprotocol-indexer/base';
import { QueryBuilder } from 'objection';

import { DEFAULT_POSTGRES_OPTIONS } from '../constants';
@@ -91,7 +92,7 @@ export async function findByBlockHeight(

export async function getLatest(
options: Options = DEFAULT_POSTGRES_OPTIONS,
): Promise<BlockFromDatabase | undefined> {
): Promise<BlockFromDatabase> {
const baseQuery: QueryBuilder<BlockModel> = setupBaseQuery<BlockModel>(
BlockModel,
options,
@@ -102,5 +103,13 @@ export async function getLatest(
.limit(1)
.returning('*');

return results[0];
const latestBlock: BlockFromDatabase | undefined = results[0];
if (latestBlock === undefined) {
logger.error({
at: 'block-table#getLatest',
message: 'Unable to find latest block',
});
throw new Error('Unable to find latest block');
}
return latestBlock;
}
2 changes: 1 addition & 1 deletion indexer/services/comlink/__tests__/controllers/api/v4/height-controller.test.ts
View file
Original file line number Diff line number Diff line change
@@ -26,7 +26,7 @@ describe('height-controller#V4', () => {

it('Get /height gets latest block', async () => {
await testMocks.seedData();
const latestBlock: BlockFromDatabase | undefined = await BlockTable.getLatest();
const latestBlock: BlockFromDatabase = await BlockTable.getLatest();
const block: any = await sendRequest({
type: RequestMethod.GET,
path: '/v4/height',
2 changes: 1 addition & 1 deletion indexer/services/comlink/__tests__/lib/helpers.test.ts
View file
Original file line number Diff line number Diff line change
@@ -290,7 +290,7 @@ describe('helpers', () => {
latestBlock,
]: [
SubaccountFromDatabase | undefined,
BlockFromDatabase | undefined,
BlockFromDatabase,
] = await Promise.all([
SubaccountTable.findById(testConstants.defaultSubaccountId),
BlockTable.getLatest(),
Comment on lines 290 to 296
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts file contains a check for latestBlock === undefined, which is no longer necessary and should be replaced with error handling to align with the new behavior of BlockTable.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

8 changes: 4 additions & 4 deletions indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts
View file
Original file line number Diff line number Diff line change
@@ -82,7 +82,7 @@ class AddressesController extends Controller {
): 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(
{
Comment on lines 82 to 87
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The check for latestBlock being undefined in perpetual-positions-controller.ts is inconsistent with the PR's objectives and should be addressed.
  • The check within block-table.ts appears to be part of the getLatest method'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

address,
@@ -92,7 +92,7 @@ class AddressesController extends Controller {
BlockTable.getLatest(),
]);

if (subaccounts.length === 0 || latestBlock === undefined) {
if (subaccounts.length === 0) {
throw new NotFoundError(`No subaccounts found for address ${address}`);
}

@@ -175,7 +175,7 @@ class AddressesController extends Controller {
AssetPositionFromDatabase[],
AssetFromDatabase[],
MarketFromDatabase[],
BlockFromDatabase | undefined,
BlockFromDatabase,
] = await Promise.all([
SubaccountTable.findById(
subaccountId,
@@ -197,7 +197,7 @@ class AddressesController extends Controller {
BlockTable.getLatest(),
]);

if (subaccount === undefined || latestBlock === undefined) {
if (subaccount === undefined) {
throw new NotFoundError(
`No subaccount found with address ${address} and subaccountNumber ${subaccountNumber}`,
);
4 changes: 2 additions & 2 deletions indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts
View file
Original file line number Diff line number Diff line change
@@ -74,7 +74,7 @@ class AddressesController extends Controller {
AssetPositionFromDatabase[],
PerpetualPositionFromDatabase[],
AssetFromDatabase[],
BlockFromDatabase | undefined,
BlockFromDatabase,
] = await Promise.all([
SubaccountTable.findById(
subaccountUuid,
@@ -116,7 +116,7 @@ class AddressesController extends Controller {

// If a subaccount, latest block, and perpetual positions exist, calculate the unsettled funding
// for positions and adjust the returned USDC position
if (subaccount !== undefined && latestBlock !== undefined && perpetualPositions.length > 0) {
if (subaccount !== undefined && perpetualPositions.length > 0) {
const {
lastUpdatedFundingIndexMap,
latestFundingIndexMap,
15 changes: 7 additions & 8 deletions indexer/services/comlink/src/controllers/api/v4/height-controller.ts
View file
Original file line number Diff line number Diff line change
@@ -19,16 +19,15 @@ const controllerName: string = 'height-controller';
class HeightController extends Controller {
@Get('/')
async getHeight(): Promise<HeightResponse> {
const latestBlock: BlockFromDatabase | undefined = await BlockTable.getLatest();

if (latestBlock === undefined) {
try {
const latestBlock: BlockFromDatabase = await BlockTable.getLatest();
return {
height: latestBlock.blockHeight,
time: latestBlock.time,
};
} catch {
throw new NotFoundError('No blocks found');
}

return {
height: latestBlock.blockHeight,
time: latestBlock.time,
};
}
}

6 changes: 2 additions & 4 deletions indexer/services/ender/src/caches/block-cache.ts
View file
Original file line number Diff line number Diff line change
@@ -17,10 +17,8 @@ const INITIAL_BLOCK_HEIGHT: string = '-1';
let currentBlockHeight: string = INITIAL_BLOCK_HEIGHT;

export async function refreshBlockCache(txId?: number): Promise<void> {
const block: BlockFromDatabase | undefined = await BlockTable.getLatest({ txId });
if (block !== undefined) {
currentBlockHeight = block.blockHeight;
}
const block: BlockFromDatabase = await BlockTable.getLatest({ txId });
currentBlockHeight = block.blockHeight || INITIAL_BLOCK_HEIGHT;
}

export function getCurrentBlockHeight(): string {
11 changes: 1 addition & 10 deletions indexer/services/roundtable/src/tasks/cancel-stale-orders.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import {
logger,
stats,
} from '@dydxprotocol-indexer/base';
import {
@@ -17,18 +16,10 @@ import config from '../config';
// and the orders have their status correctly set
export default async function runTask(): Promise<void> {
const queryStart: number = Date.now();
const latestBlock: BlockFromDatabase | undefined = await BlockTable.getLatest({
const latestBlock: BlockFromDatabase = await BlockTable.getLatest({
readReplica: true,
});

if (latestBlock === undefined) {
logger.info({
at: 'close-stale-orders#getLatestBlockHeight',
message: 'No latest block height found. Skipping task',
});
return;
}

const latestBlockHeight: number = parseInt(latestBlock.blockHeight, 10);

const staleOpenOrders: OrderFromDatabase[] = await OrderTable.findAll(
7 changes: 3 additions & 4 deletions indexer/services/roundtable/src/tasks/create-pnl-ticks.ts
View file
Original file line number Diff line number Diff line change
@@ -8,7 +8,6 @@ import {
} from '@dydxprotocol-indexer/postgres';
import { LatestAccountPnlTicksCache } from '@dydxprotocol-indexer/redis';
import _ from 'lodash';
import { DateTime } from 'luxon';

import config from '../config';
import { getPnlTicksCreateObjects } from '../helpers/pnl-ticks-helper';
@@ -31,14 +30,14 @@ export default async function runTask(): Promise<void> {
block,
pnlTickLatestBlocktime,
]: [
BlockFromDatabase | undefined,
BlockFromDatabase,
string,
] = await Promise.all([
BlockTable.getLatest({ readReplica: true }),
PnlTicksTable.findLatestProcessedBlocktime(),
]);
const latestBlockTime: string = block !== undefined ? block.time : DateTime.utc(0).toISO();
const latestBlockHeight: string = block !== undefined ? block.blockHeight : '0';
const latestBlockTime: string = block.time;
const latestBlockHeight: string = block.blockHeight;
// Check that the latest block time is within PNL_TICK_UPDATE_INTERVAL_MS of the last computed
// PNL tick block time.
if (
9 changes: 1 addition & 8 deletions indexer/services/roundtable/src/tasks/remove-expired-orders.ts
View file
Original file line number Diff line number Diff line change
@@ -26,14 +26,7 @@ import { getExpiredOffChainUpdateMessage } from '../helpers/websocket';
*/
export default async function runTask(): Promise<void> {
const start: number = Date.now();
const block: BlockFromDatabase | undefined = await BlockTable.getLatest({ readReplica: true });
if (block === undefined) {
logger.error({
at: 'remove-expired-orders#runTask',
message: 'Unable to find latest block',
});
return;
}
const block: BlockFromDatabase = await BlockTable.getLatest({ readReplica: true });

try {
// Only need to expire short-term orders because long-term OrderRemoves will be on-chain.
6 changes: 1 addition & 5 deletions indexer/services/roundtable/src/tasks/track-lag.ts
View file
Original file line number Diff line number Diff line change
@@ -28,7 +28,7 @@ export default async function runTask(): Promise<void> {
validatorBlock,
otherFullNodeBlock,
]: [
BlockFromDatabase | undefined,
BlockFromDatabase,
BlockData,
BlockData,
BlockData,
@@ -39,10 +39,6 @@ export default async function runTask(): Promise<void> {
getValidatorBlockData(config.TRACK_LAG_OTHER_FULL_NODE_URL, 'other_full_node'),
]);

if (indexerBlockFromDatabase === undefined) {
return;
}

const indexerBlock: BlockData = {
block: indexerBlockFromDatabase.blockHeight,
timestamp: indexerBlockFromDatabase.time,
5 changes: 4 additions & 1 deletion indexer/services/vulcan/__tests__/handlers/order-remove-handler.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1705,7 +1705,10 @@ describe('OrderRemoveHandler', () => {
const removedRedisOrder: RedisOrder = redisTestConstants.defaultRedisOrder;
const expectedOrderUuid: string = redisTestConstants.defaultOrderUuid;

const tableSpy = jest.spyOn(BlockTable, 'getLatest').mockResolvedValueOnce(undefined);
// eslint-disable-next-line @typescript-eslint/require-await
const tableSpy = jest.spyOn(BlockTable, 'getLatest').mockImplementation(async () => {
throw new Error();
});

try {
const orderRemoveJson: OrderRemoveV1 = { ...indexerExpiredOrderRemoved, removedOrderId };
12 changes: 7 additions & 5 deletions indexer/services/vulcan/src/handlers/order-remove-handler.ts
View file
Original file line number Diff line number Diff line change
@@ -393,11 +393,13 @@ export class OrderRemoveHandler extends Handler {
* update since it occurred which would invalidate the message.
*/
protected async isOrderExpired(orderRemove: OrderRemoveV1): Promise<boolean> {
const block: BlockFromDatabase | undefined = await runFuncWithTimingStat(
BlockTable.getLatest({ readReplica: true }),
this.generateTimingStatsOptions('get_latest_block_for_indexer_expired_expiry_verification'),
);
if (block === undefined) {
let block: BlockFromDatabase;
try {
block = await runFuncWithTimingStat(
BlockTable.getLatest({ readReplica: true }),
this.generateTimingStatsOptions('get_latest_block_for_indexer_expired_expiry_verification'),
);
} catch {
logger.error({
at: 'orderRemoveHandler#isOrderExpired',
message: 'Unable to find latest block',