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

Show file tree
Hide file tree
Changes from 2 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
5 changes: 2 additions & 3 deletions indexer/packages/postgres/__tests__/stores/block-table.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Open in desktop
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';
Expand Down Expand Up @@ -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,
Expand All @@ -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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
2 changes: 1 addition & 1 deletion indexer/services/comlink/__tests__/lib/helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
8 changes: 4 additions & 4 deletions indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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}`);
}

Expand Down Expand Up @@ -175,7 +175,7 @@ class AddressesController extends Controller {
AssetPositionFromDatabase[],
AssetFromDatabase[],
MarketFromDatabase[],
BlockFromDatabase | undefined,
BlockFromDatabase,
] = await Promise.all([
SubaccountTable.findById(
subaccountId,
Expand All @@ -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}`,
);
Expand Down
4 changes: 2 additions & 2 deletions indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class AddressesController extends Controller {
AssetPositionFromDatabase[],
PerpetualPositionFromDatabase[],
AssetFromDatabase[],
BlockFromDatabase | undefined,
BlockFromDatabase,
] = await Promise.all([
SubaccountTable.findById(
subaccountUuid,
Expand Down Expand Up @@ -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,
Expand Down
15 changes: 7 additions & 8 deletions indexer/services/comlink/src/controllers/api/v4/height-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}
}

Expand Down
6 changes: 2 additions & 4 deletions indexer/services/ender/src/caches/block-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
11 changes: 1 addition & 10 deletions indexer/services/roundtable/src/tasks/cancel-stale-orders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import {
logger,
stats,
} from '@dydxprotocol-indexer/base';
import {
Expand All @@ -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(
Expand Down
7 changes: 3 additions & 4 deletions indexer/services/roundtable/src/tasks/create-pnl-ticks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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 (
Expand Down
9 changes: 1 addition & 8 deletions indexer/services/roundtable/src/tasks/remove-expired-orders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 1 addition & 5 deletions indexer/services/roundtable/src/tasks/track-lag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export default async function runTask(): Promise<void> {
validatorBlock,
otherFullNodeBlock,
]: [
BlockFromDatabase | undefined,
BlockFromDatabase,
BlockData,
BlockData,
BlockData,
Expand All @@ -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,
Expand Down
4 changes: 3 additions & 1 deletion indexer/services/vulcan/__tests__/handlers/order-remove-handler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1705,7 +1705,9 @@ describe('OrderRemoveHandler', () => {
const removedRedisOrder: RedisOrder = redisTestConstants.defaultRedisOrder;
const expectedOrderUuid: string = redisTestConstants.defaultOrderUuid;

const tableSpy = jest.spyOn(BlockTable, 'getLatest').mockResolvedValueOnce(undefined);
const tableSpy = jest.spyOn(BlockTable, 'getLatest').mockImplementation(() => {
throw new Error();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


try {
const orderRemoveJson: OrderRemoveV1 = { ...indexerExpiredOrderRemoved, removedOrderId };
Expand Down
12 changes: 7 additions & 5 deletions indexer/services/vulcan/src/handlers/order-remove-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Loading