feat: Propagate invalid fill reasons to caller

This change provides additional information to the caller of
queryHistoricalDepositForFill(). In the case that a fill is not found, a
cause code is associated:
- 0: The deposit ID was invalid for the SpokePool (i.e. cannot exist).
- 1: The deposit ID was valid, but it wasn't found within scraped data
  (i.e. maybe the RPCs aren't serving it in eth_getLogs() requests).
- 2: The deposit ID was valid, but one or more fields in the
  corresponding deposit were mismatched (i.e. the fill was invalid).

This is most interesting for case (1), where there's a reasonable
indication that the deposit should be held within the SpokePoolClient's
deposit map, but isn't. This indirectly acts like a qualitative measure
for RPC provider responses and will allow the upper layers to
dynamically revert to more conservative behaviour.

src/clients/SpokePoolClient.ts

@@ -318,6 +318,11 @@ export class SpokePoolClient extends BaseAbstractClient {
     };
   }
 
+  public getDeposit(depositId: number): DepositWithBlock | undefined {
+    const depositHash = this.getDepositHash({ depositId, originChainId: this.chainId });
+    return this.depositHashes[depositHash];
+  }
+
   /**
    * Find a corresponding deposit for a given fill.
    * @param fill The fill to find a corresponding deposit for.

src/utils/DepositUtils.ts

@@ -2,6 +2,7 @@ import assert from "assert";
 import { SpokePoolClient } from "../clients";
 import { DEFAULT_CACHING_TTL, EMPTY_MESSAGE } from "../constants";
 import { CachingMechanismInterface, Deposit, DepositWithBlock, Fill } from "../interfaces";
+import { getNetworkName } from "../utils";
 import { getDepositInCache, getDepositKey, setDepositInCache } from "./CachingUtils";
 import { validateFillForDeposit } from "./FlowUtils";
 import { getCurrentTime } from "./TimeUtils";
@@ -12,6 +13,16 @@ import { isDepositFormedCorrectly } from "./ValidatorUtils";
 // This can be used by the Dataworker to determine whether to give a relayer a refund for a fill
 // of a deposit older or younger than its fixed lookback.
 
+export enum InvalidFill {
+  DepositIdInvalid = 0, // Deposit ID seems invalid for origin SpokePool
+  DepositIdNotFound, // Deposit ID not found (bad RPC data?)
+  FillMismatch, // Fill does not match deposit parameters for deposit ID.
+};
+
+export type DepositSearchResult =
+  | { found: true; deposit: DepositWithBlock; }
+  | { found: false; code: InvalidFill; reason: string; };
+
 /**
  * Attempts to resolve a deposit for a fill. If the fill's deposit Id is within the spoke pool client's search range,
  * the deposit is returned immediately. Otherwise, the deposit is queried first from the provided cache, and if it is
@@ -28,7 +39,7 @@ export async function queryHistoricalDepositForFill(
   spokePoolClient: SpokePoolClient,
   fill: Fill,
   cache?: CachingMechanismInterface
-): Promise<DepositWithBlock | undefined> {
+): Promise<DepositSearchResult> {
   if (fill.originChainId !== spokePoolClient.chainId) {
     throw new Error(`OriginChainId mismatch (${fill.originChainId} != ${spokePoolClient.chainId})`);
   }
@@ -39,18 +50,28 @@ export async function queryHistoricalDepositForFill(
     throw new Error("SpokePoolClient must be updated before querying historical deposits");
   }
 
-  if (
-    fill.depositId < spokePoolClient.firstDepositIdForSpokePool ||
-    fill.depositId > spokePoolClient.lastDepositIdForSpokePool
-  ) {
-    return undefined;
+  const { depositId } = fill;
+  let { firstDepositIdForSpokePool: lowId, lastDepositIdForSpokePool: highId } = spokePoolClient;
+  if (depositId < lowId || depositId > highId)  {
+    return {
+      found: false,
+      code: InvalidFill.DepositIdInvalid,
+      reason: `Deposit ID ${depositId} is outside of SpokePool bounds [${lowId},${highId}].`,
+    };
   }
 
-  if (
-    fill.depositId >= spokePoolClient.earliestDepositIdQueried &&
-    fill.depositId <= spokePoolClient.latestDepositIdQueried
-  ) {
-    return spokePoolClient.getDepositForFill(fill);
+  ({ earliestDepositIdQueried: lowId, latestDepositIdQueried: highId } = spokePoolClient);
+  if (depositId >= lowId && depositId <= highId) {
+    const deposit = spokePoolClient.getDeposit(depositId);
+    if (isDefined(deposit) && validateFillForDeposit(fill, deposit)) {
+      return { found: true, deposit };
+    }
+
+    return {
+      found: false,
+      code: InvalidFill.DepositIdNotFound,
+      reason: `Deposit ID ${depositId} not found in SpokePoolClient event buffer.`
+    };
   }
 
   let deposit: DepositWithBlock, cachedDeposit: Deposit | undefined;
@@ -82,7 +103,18 @@ export async function queryHistoricalDepositForFill(
     }
   }
 
-  return validateFillForDeposit(fill, deposit) ? deposit : undefined;
+  if (validateFillForDeposit(fill, deposit)) {
+    return {
+      found: true,
+      deposit,
+    };
+  }
+
+  return {
+    found: false,
+    code: InvalidFill.FillMismatch,
+    reason: `Fill is not valid for ${getNetworkName(deposit.originChainId)} deposit ${depositId}`,
+  };
 }
 
 /**

test/SpokePoolClient.ValidateFill.ts

@@ -28,7 +28,7 @@ import {
 
 import { SpokePoolClient } from "../src/clients";
 import { MockConfigStoreClient, MockHubPoolClient, MockSpokePoolClient } from "./mocks";
-import { validateFillForDeposit, queryHistoricalDepositForFill } from "../src/utils";
+import { InvalidFill, validateFillForDeposit, queryHistoricalDepositForFill } from "../src/utils";
 import { CHAIN_ID_TEST_LIST, repaymentChainId } from "./constants";
 
 let spokePool_1: Contract, erc20_1: Contract, spokePool_2: Contract, erc20_2: Contract, hubPool: Contract;
@@ -401,7 +401,10 @@ describe("SpokePoolClient: Fill Validation", function () {
     // Client has 0 deposits in memory so querying historical deposit sends fresh RPC requests.
     expect(spokePoolClient1.getDeposits().length).to.equal(0);
     const historicalDeposit = await queryHistoricalDepositForFill(spokePoolClient1, fill);
-    expect(historicalDeposit?.depositId).to.deep.equal(deposit.depositId);
+    expect(historicalDeposit.found).is.true;
+    if (historicalDeposit.found) {
+      expect(historicalDeposit.deposit.depositId).to.deep.equal(deposit.depositId);
+    }
   });
 
   it("Can fetch younger deposit matching fill", async function () {
@@ -424,7 +427,10 @@ describe("SpokePoolClient: Fill Validation", function () {
     // Client has 0 deposits in memory so querying historical deposit sends fresh RPC requests.
     expect(spokePoolClient1.getDeposits().length).to.equal(0);
     const historicalDeposit = await queryHistoricalDepositForFill(spokePoolClient1, fill);
-    expect(historicalDeposit?.depositId).to.deep.equal(deposit.depositId);
+    expect(historicalDeposit.found).is.true;
+    if (historicalDeposit.found) {
+      expect(historicalDeposit.deposit.depositId).to.deep.equal(deposit.depositId);
+    }
   });
 
   it("Loads fills from memory with deposit ID > spoke pool client's earliest deposit ID queried", async function () {
@@ -434,12 +440,12 @@ describe("SpokePoolClient: Fill Validation", function () {
     expect(spokePoolClient1.earliestDepositIdQueried == 0).is.true;
 
     // Client should NOT send RPC requests to fetch this deposit, instead it should load from memory.
-    expect((await queryHistoricalDepositForFill(spokePoolClient1, fill)) !== undefined).is.true;
+    expect((await queryHistoricalDepositForFill(spokePoolClient1, fill)).found).is.true;
     expect(lastSpyLogIncludes(spy, "updated!")).is.true;
 
     // Now override earliest deposit ID queried so that its > deposit ID and check that client sends RPC requests.
     spokePoolClient1.earliestDepositIdQueried = 1;
-    expect((await queryHistoricalDepositForFill(spokePoolClient1, fill)) !== undefined).is.true;
+    expect((await queryHistoricalDepositForFill(spokePoolClient1, fill)).found).is.true;
     expect(lastSpyLogIncludes(spy, "Located deposit outside of SpokePoolClient's search range")).is.true;
   });
 
@@ -452,12 +458,12 @@ describe("SpokePoolClient: Fill Validation", function () {
     spokePoolClient1.latestDepositIdQueried = 1;
 
     // Client should NOT send RPC requests to fetch this deposit, instead it should load from memory.
-    expect((await queryHistoricalDepositForFill(spokePoolClient1, fill)) !== undefined).is.true;
+    expect((await queryHistoricalDepositForFill(spokePoolClient1, fill)).found).is.true;
     expect(lastSpyLogIncludes(spy, "updated!")).is.true;
 
     // Now override latest deposit ID queried so that its < deposit ID and check that client sends RPC requests.
     spokePoolClient1.latestDepositIdQueried = -1;
-    expect((await queryHistoricalDepositForFill(spokePoolClient1, fill)) !== undefined).is.true;
+    expect((await queryHistoricalDepositForFill(spokePoolClient1, fill)).found).is.true;
     expect(lastSpyLogIncludes(spy, "Located deposit outside of SpokePoolClient's search range")).is.true;
   });
 
@@ -469,9 +475,13 @@ describe("SpokePoolClient: Fill Validation", function () {
 
     // Override the first spoke pool deposit ID that the client thinks is available in the contract.
     await spokePoolClient1.update();
-    spokePoolClient1.firstDepositIdForSpokePool = 1;
+    spokePoolClient1.firstDepositIdForSpokePool = deposit.depositId + 1;
     expect(fill.depositId < spokePoolClient1.firstDepositIdForSpokePool).is.true;
-    await queryHistoricalDepositForFill(spokePoolClient1, fill);
+    const search = await queryHistoricalDepositForFill(spokePoolClient1, fill);
+    expect(search.found).is.false;
+    if (search.found === false) {
+      expect(search.code).to.equal(InvalidFill.DepositIdInvalid);
+    }
     expect(lastSpyLogIncludes(spy, "Queried RPC for deposit")).is.not.true;
   });
 
@@ -494,7 +504,11 @@ describe("SpokePoolClient: Fill Validation", function () {
 
     await spokePoolClient1.update();
     expect(fill.depositId > spokePoolClient1.lastDepositIdForSpokePool).is.true;
-    await queryHistoricalDepositForFill(spokePoolClient1, fill);
+    const search = await queryHistoricalDepositForFill(spokePoolClient1, fill);
+    expect(search.found).is.false;
+    if (search.found === false) {
+      expect(search.code).to.equal(InvalidFill.DepositIdInvalid);
+    }
     expect(lastSpyLogIncludes(spy, "Queried RPC for deposit")).is.not.true;
   });