chore: Mark all potential chainId runtime errors #1089
Conversation
There was a problem hiding this comment.
Thanks @dijanin-brat. There seem to be a few ways we can mitigate most/all of these, though I'd love to find a way to ensure the caller is protected against this scenario without having to remember to check it manually.
| // @TODO This can throw an runtime error if chainId is wrong. | ||
| return deposit.fillDeadline < bundleBlockTimestamps[deposit.destinationChainId][1]; |
There was a problem hiding this comment.
@nicholaspai For this case, do we want to specify in the UMIP that we revert to mainnet time in case that a destination SpokePool deployment is not available? That would permit us to handle this pretty gracefully.
Perhaps something like this:
| // @TODO This can throw an runtime error if chainId is wrong. | |
| return deposit.fillDeadline < bundleBlockTimestamps[deposit.destinationChainId][1]; | |
| const [, endTimestamp] = bundleBlockTimestamps[deposit.destinationChainId] ?? bundleBlockTimestamps[this.hubChainId]; | |
| return deposit.fillDeadline < endTimestamp; |
| // Because of this check, there is a good chance that _getFillStatusForDeposit will not throw an error. | ||
| .filter((chainId) => !_isChainDisabled(chainId) && spokePoolClients[chainId] !== undefined); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'd probably err on adding an assert() to _isChainDisabled(), so we can explicitly specify that the error is: unsupported chainId (${chainId}).
There was a problem hiding this comment.
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.
| // @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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I guess this should be done in separate PR as well like this one #1089 (comment)
| return deposit.fillDeadline < bundleBlockTimestamps[deposit.destinationChainId][1]; | ||
| const [, endTimestamp] = | ||
| bundleBlockTimestamps[deposit.destinationChainId] ?? bundleBlockTimestamps[this.clients.hubPoolClient.chainId]; | ||
| return deposit.fillDeadline < endTimestamp; |
There was a problem hiding this comment.
@dijanin-brat OK to split this one out into a separate PR? It requires protocol clarification so we can't change it yet.
| @@ -1562,8 +1574,9 @@ export class BundleDataClient { | |||
| const unknownReasonInvalidFills: FillWithBlock[] = []; | |||
|
|
|||
| bundleInvalidFillsV3.forEach((fill) => { | |||
| // @TODO This can throw an runtime error if chainId is wrong. | |||
There was a problem hiding this comment.
Should be OK to drop this now.
…ll-potential-chain-id-runtime-errors
Mark all potential chainId runtime errors so we can fix them easier later.