Skip to content

chore: Mark all potential chainId runtime errors #1089

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.

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

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 18 additions & 3 deletions src/clients/BundleDataClient/BundleDataClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,7 @@ export class BundleDataClient {
// and then query the FillStatus on-chain, but that might slow this function down too much. For now, we
// will live with this expected inaccuracy as it should be small. The pre-fill would have to precede the deposit
// by more than the caller's event lookback window which is expected to be unlikely.

const fillsToCount = this.spokePoolClients[chainId].getFills().filter((fill) => {
if (
fill.blockNumber < blockRanges[chainIndex][0] ||
Expand Down Expand Up @@ -790,24 +791,36 @@ export class BundleDataClient {
};

const _depositIsExpired = (deposit: DepositWithBlock): boolean => {
// @TODO: We should be defaulting to the mainnet time in case destination SpokePool deployment is not available.
// Something like this:
// const [, endTimestamp] =
// bundleBlockTimestamps[deposit.destinationChainId] ?? bundleBlockTimestamps[this.clients.hubPoolClient.chainId];
// return deposit.fillDeadline < endTimestamp;
return deposit.fillDeadline < bundleBlockTimestamps[deposit.destinationChainId][1];
};

const _getFillStatusForDeposit = (deposit: Deposit, queryBlock: number): Promise<FillStatus> => {
return spokePoolClients[deposit.destinationChainId].relayFillStatus(
const spokePoolClient = spokePoolClients[deposit.destinationChainId];

if (!isDefined(spokePoolClient)) {
return Promise.resolve(FillStatus.Unfilled);
}

return spokePoolClient.relayFillStatus(
deposit,
// We can assume that in production
// the block to query is not one that the spoke pool client
// hasn't queried. This is because this function will usually be called
// in production with block ranges that were validated by
// DataworkerUtils.blockRangesAreInvalidForSpokeClients.
Math.min(queryBlock, spokePoolClients[deposit.destinationChainId].latestHeightSearched)
Math.min(queryBlock, spokePoolClient.latestHeightSearched)
);
};

// Infer chain ID's to load from number of block ranges passed in.
const allChainIds = blockRangesForChains
.map((_blockRange, index) => chainIds[index])
// Because of this check, there is a good chance that _getFillStatusForDeposit will not throw an error.
.filter((chainId) => !_isChainDisabled(chainId) && spokePoolClients[chainId] !== undefined);
Comment on lines +823 to 824
Copy link
Contributor

Choose a reason for hiding this comment

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

I think _isChainDisabled() will end up throwing because it assumes it's supplied with a valid chainId. So we might consider updating _isChainDisabled or isChainDisabled to first verify that the chain exists in the ConfigStoreClient (CHAIN_ID_INDICES) first, and if not, to return false.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably err on adding an assert() to _isChainDisabled(), so we can explicitly specify that the error is: unsupported chainId (${chainId}).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, if the chain is not supported, we will throw this error anyway

if (indexForChain === -1) { throw new Error(Could not find chain ${chain} in chain ID list ${chainIdListForBundleEvaluationBlockNumbers}); }

I don't see the point of assert there bcs of this.

allChainIds.forEach((chainId) => {
const spokePoolClient = spokePoolClients[chainId];
Expand Down Expand Up @@ -1413,6 +1426,8 @@ export class BundleDataClient {
}
const deposit = deposits[index];
const { destinationChainId } = deposit;
// @TODO The function getBlockRangeForChain can throw an error if chainId is wrong.
// Look at getBlockRangeForChain implementation.
const destinationBlockRange = getBlockRangeForChain(blockRangesForChains, destinationChainId, chainIds);

// Only look for deposits that were mined before this bundle and that are newly expired.
Comment on lines +1429 to 1433
Copy link
Contributor

Choose a reason for hiding this comment

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

Here (and below) we use the bundle end block to resolve a timestamp, and then compare that against the fillDeadline. So if we are OK to fall back to the mainnet bundle timestamp then we might be able to replay that here. It looks like it'd require a little bit of refactoring, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this should be done in separate PR as well like this one #1089 (comment)

Expand Down Expand Up @@ -1563,7 +1578,7 @@ export class BundleDataClient {

bundleInvalidFillsV3.forEach((fill) => {
const originClient = spokePoolClients[fill.originChainId];
const fullyMatchedDeposit = originClient.getDepositForFill(fill);
const fullyMatchedDeposit = originClient?.getDepositForFill(fill);
if (!isDefined(fullyMatchedDeposit)) {
const partiallyMatchedDeposit = originClient.getDeposit(fill.depositId);
if (isDefined(partiallyMatchedDeposit)) {
Expand Down
2 changes: 2 additions & 0 deletions src/clients/HubPoolClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ export class HubPoolClient extends BaseAbstractClient {
.filter((l1Token) => this.l2TokenEnabledForL1Token(l1Token, destinationChainId))
.map((l1Token) => {
// Return all matching L2 token mappings that are equal to or earlier than the target block.

return this.l1TokensToDestinationTokensWithBlock[l1Token][destinationChainId].filter(
(mapping) => mapping.l2Token === l2Token && mapping.blockNumber <= latestHubBlock
);
Expand Down Expand Up @@ -1115,6 +1116,7 @@ export class HubPoolClient extends BaseAbstractClient {
if (chainIdIndex >= bundleEvaluationBlockNumbers.length) {
return 0;
}

return bundleEvaluationBlockNumbers[chainIdIndex].toNumber();
}
}