fix(Relayer): Correct deposit lookback (#787)

fix(Relayer): Correct deposit lookback

The MAX_RELAYER_DEPOSIT_LOOKBACK config item is currently intended to be
specified in units of seconds, but was previously configured with a
number of blocks. The Relayer class however passed
MAX_RELAYER_DEPOSIT_LOOKBACK directly in to the getUnfilledDeposits()
helper function, where it is compared directly against a block number.
This should to be translated from seconds to a block number first.

The effect of this misalignment in units is probably that the relayer
looks back further than anticipated on mainnet, where 1 block is ~12
seconds, and potentially less than anticipated on some L2s, where 
block production might occur multiple times per second. For larger
lookback configurations this potentially imposes a hefty time penalty
in looping mode when searching for unfilled deposits originating on
mainnet.

This may resolve an issue that we've received vague reports about in the
past from the relayer community. Feedback has been that the relayer is
less likely to capture fills if it runs in looping mode, so many from
the community choose to run in serverless mode and loop their bot
externally.

Thanks to James for the quick SDK update that was needed in this PR.

package.json

@@ -12,7 +12,7 @@
   "dependencies": {
     "@across-protocol/contracts-v2": "2.3.3",
     "@across-protocol/optimism-sdk": "2.1.3",
-    "@across-protocol/sdk-v2": "0.10.4",
+    "@across-protocol/sdk-v2": "0.11.4",
     "@arbitrum/sdk": "^3.1.3",
     "@defi-wonderland/smock": "^2.3.5",
     "@eth-optimism/sdk": "^2.1.0",

src/monitor/Monitor.ts

@@ -181,10 +181,10 @@ export class Monitor {
   }
 
   async reportUnfilledDeposits(): Promise<void> {
-    const unfilledDeposits = getUnfilledDeposits(
+    const unfilledDeposits = await getUnfilledDeposits(
       this.clients.spokePoolClients,
-      this.monitorConfig.maxRelayerLookBack,
-      this.clients.configStoreClient
+      this.clients.configStoreClient,
+      this.monitorConfig.maxRelayerLookBack
     );
 
     // Group unfilled amounts by chain id and token id.

src/relayer/Relayer.ts

@@ -42,7 +42,7 @@ export class Relayer {
       this.clients;
 
     const maxVersion = configStoreClient.configStoreVersion;
-    const unfilledDeposits = getUnfilledDeposits(spokePoolClients, config.maxRelayerLookBack, configStoreClient);
+    const unfilledDeposits = await getUnfilledDeposits(spokePoolClients, configStoreClient, config.maxRelayerLookBack);
     const { supportedDeposits = [], unsupportedDeposits = [] } = groupBy(unfilledDeposits, (deposit) =>
       deposit.version <= maxVersion ? "supportedDeposits" : "unsupportedDeposits"
     );

src/utils/BlockUtils.ts

@@ -24,7 +24,7 @@ export async function getBlockFinder(chainId: number): Promise<BlockFinder<Block
  * @param timestamp Approximate timestamp of the to requested block number.
  * @param blockFinder Caller can optionally pass in a block finder object to use instead of creating a new one
  * or loading from cache. This is useful for testing primarily.
- * @returns
+ * @returns Block number for the requested timestamp.
  */
 export async function getBlockForTimestamp(
   chainId: number,

src/utils/FillUtils.ts

@@ -1,11 +1,13 @@
+import assert from "assert";
 import { ConfigStoreClient, HubPoolClient } from "../clients";
 import { Deposit, DepositWithBlock, Fill, FillsToRefund, FillWithBlock, SpokePoolClientsByChain } from "../interfaces";
-import { queryHistoricalDepositForFill } from "../utils";
+import { getBlockForTimestamp, queryHistoricalDepositForFill } from "../utils";
 import {
   BigNumber,
   assign,
   getRealizedLpFeeForFills,
   getRefundForFills,
+  isDefined,
   sortEventsDescending,
   toBN,
   sortEventsAscending,
@@ -232,32 +234,46 @@ export type RelayerUnfilledDeposit = {
   invalidFills: Fill[];
 };
 
-// Returns all unfilled deposits over all spokePoolClients. Return values include the amount of the unfilled deposit.
-export function getUnfilledDeposits(
+// @description Returns an array of unfilled deposits over all spokePoolClients.
+// @param spokePoolClients  Mapping of chainIds to SpokePoolClient objects.
+// @param configStoreClient ConfigStoreClient instance.
+// @param depositLookBack   Deposit lookback (in seconds) since SpokePoolClient time as at last update.
+// @returns Array of unfilled deposits.
+export async function getUnfilledDeposits(
   spokePoolClients: SpokePoolClientsByChain,
-  maxUnfilledDepositLookBack: number,
-  configStoreClient: ConfigStoreClient
-): RelayerUnfilledDeposit[] {
+  configStoreClient: ConfigStoreClient,
+  depositLookBack?: number
+): Promise<RelayerUnfilledDeposit[]> {
   const unfilledDeposits: RelayerUnfilledDeposit[] = [];
+  const chainIds = Object.values(spokePoolClients).map(({ chainId }) => chainId);
+
+  let earliestBlockNumbers = Object.values(spokePoolClients).map(({ deploymentBlock }) => deploymentBlock);
+  if (isDefined(depositLookBack)) {
+    earliestBlockNumbers = await Promise.all(
+      Object.values(spokePoolClients).map((spokePoolClient) => {
+        const currentTime = spokePoolClient.getCurrentTime();
+        return getBlockForTimestamp(spokePoolClient.chainId, currentTime - depositLookBack);
+      })
+    );
+  }
+
   // Iterate over each chainId and check for unfilled deposits.
-  const chainIds = Object.keys(spokePoolClients);
-  for (const originChain of chainIds) {
-    const originClient = spokePoolClients[originChain];
-    for (const destinationChain of chainIds) {
-      if (originChain === destinationChain) {
-        continue;
-      }
+  for (const originClient of Object.values(spokePoolClients)) {
+    const { chainId: originChainId } = originClient;
+    const chainIdx = chainIds.indexOf(originChainId);
+    assert(chainIdx !== -1, `Invalid chain index for chainId ${originChainId} (${chainIdx})`);
+    const earliestBlockNumber = earliestBlockNumbers[chainIdx];
+
+    for (const destinationChain of chainIds.filter((chainId) => chainId !== originChainId)) {
       // Find all unfilled deposits for the current loops originChain -> destinationChain. Note that this also
       // validates that the deposit is filled "correctly" for the given deposit information. This includes validation
       // of the all deposit -> relay props, the realizedLpFeePct and the origin->destination token mapping.
       const destinationClient = spokePoolClients[destinationChain];
       const depositsForDestinationChain: DepositWithBlock[] =
         originClient.getDepositsForDestinationChain(destinationChain);
 
-      // If deposit is older than unfilled deposit lookback, ignore it.
-      const cutOff = originClient.latestBlockNumber - maxUnfilledDepositLookBack;
       const unfilledDepositsForDestinationChain = depositsForDestinationChain
-        .filter((deposit) => deposit.blockNumber >= cutOff)
+        .filter((deposit) => deposit.blockNumber >= earliestBlockNumber)
         .map((deposit) => {
           const version = configStoreClient.getConfigStoreVersionForTimestamp(deposit.quoteTimestamp);
           return { ...destinationClient.getValidUnfilledAmountForDeposit(deposit), deposit, version };

test/Monitor.ts

@@ -86,6 +86,11 @@ describe("Monitor", async function () {
     };
     const monitorConfig = new MonitorConfig(defaultMonitorEnvVars);
 
+    // @dev: Force maxRelayerLookBack to undefined to skip lookback block resolution.
+    // This overrules the readonly property for MonitorConfig.maxRelayerLookBack (...bodge!!).
+    // @todo: Relocate getUnfilledDeposits() into a class to permit it to be overridden in test.
+    monitorConfig["maxRelayerLookBack"] = undefined;
+
     // Set the config store version to 0 to match the default version in the ConfigStoreClient.
     process.env.CONFIG_STORE_VERSION = "0";
 

test/Relayer.BasicFill.ts

@@ -116,7 +116,6 @@ describe("Relayer: Check for Unfilled Deposits and Fill", async function () {
       {
         relayerTokens: [],
         relayerDestinationChains: [originChainId, destinationChainId],
-        maxRelayerLookBack: 24 * 60 * 60,
         minDepositConfirmations: defaultMinDepositConfirmations,
         quoteTimeBuffer: 0,
       } as unknown as RelayerConfig
@@ -402,7 +401,6 @@ describe("Relayer: Check for Unfilled Deposits and Fill", async function () {
       {
         relayerTokens: [],
         relayerDestinationChains: [originChainId],
-        maxRelayerLookBack: 24 * 60 * 60,
         minDepositConfirmations: defaultMinDepositConfirmations,
         quoteTimeBuffer: 0,
       } as unknown as RelayerConfig
@@ -452,7 +450,6 @@ describe("Relayer: Check for Unfilled Deposits and Fill", async function () {
       {
         relayerTokens: [],
         relayerDestinationChains: [originChainId, destinationChainId],
-        maxRelayerLookBack: 24 * 60 * 60,
         minDepositConfirmations: defaultMinDepositConfirmations,
         quoteTimeBuffer: 0,
       } as unknown as RelayerConfig

test/Relayer.SlowFill.ts

@@ -93,7 +93,6 @@ describe("Relayer: Zero sized fill for slow relay", async function () {
         relayerTokens: [],
         slowDepositors: [],
         relayerDestinationChains: [],
-        maxRelayerLookBack: 24 * 60 * 60,
         quoteTimeBuffer: 0,
         minDepositConfirmations: defaultMinDepositConfirmations,
       } as unknown as RelayerConfig

test/Relayer.TokenShortfall.ts

@@ -91,7 +91,6 @@ describe("Relayer: Token balance shortfall", async function () {
         relayerTokens: [],
         slowDepositors: [],
         relayerDestinationChains: [],
-        maxRelayerLookBack: 24 * 60 * 60,
         quoteTimeBuffer: 0,
         minDepositConfirmations: defaultMinDepositConfirmations,
       } as unknown as RelayerConfig