feat: Propagate invalid fill reasons to caller (#486)

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.

The minor package version was bumped as part of this work because it's a
breaking change.

package.json

@@ -1,7 +1,7 @@
 {
   "name": "@across-protocol/sdk-v2",
   "author": "UMA Team",
-  "version": "0.19.0",
+  "version": "0.20.0",
   "license": "AGPL-3.0",
   "homepage": "https://docs.across.to/v/developer-docs/developers/across-sdk",
   "files": [

src/clients/SpokePoolClient.ts

@@ -318,6 +318,17 @@ export class SpokePoolClient extends BaseAbstractClient {
     };
   }
 
+  /**
+   * Find a deposit based on its deposit ID.
+   * @notice If evaluating a fill, be sure to verify it against the resulting deposit.
+   * @param depositId The unique ID of the deposit being queried.
+   * @returns The corresponding deposit if found, undefined otherwise.
+   */
+  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: isDefined(deposit) ? InvalidFill.FillMismatch : InvalidFill.DepositIdNotFound,
+      reason: `Deposit ID ${depositId} not found in SpokePoolClient event buffer.`,
+    };
   }
 
   let deposit: DepositWithBlock, cachedDeposit: Deposit | undefined;
@@ -82,7 +103,15 @@ 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

@@ -1,4 +1,14 @@
+import assert from "assert";
 import { RelayData } from "../src/interfaces";
+import { SpokePoolClient } from "../src/clients";
+import {
+  bnZero,
+  bnOne,
+  InvalidFill,
+  relayFilledAmount,
+  validateFillForDeposit,
+  queryHistoricalDepositForFill,
+} from "../src/utils";
 import {
   expect,
   toBNWei,
@@ -26,11 +36,8 @@ import {
   winston,
   lastSpyLogIncludes,
 } from "./utils";
-
-import { SpokePoolClient } from "../src/clients";
-import { MockConfigStoreClient, MockHubPoolClient, MockSpokePoolClient } from "./mocks";
-import { relayFilledAmount, validateFillForDeposit, queryHistoricalDepositForFill } from "../src/utils";
 import { CHAIN_ID_TEST_LIST, repaymentChainId } from "./constants";
+import { MockConfigStoreClient, MockHubPoolClient, MockSpokePoolClient } from "./mocks";
 
 let spokePool_1: Contract, erc20_1: Contract, spokePool_2: Contract, erc20_2: Contract, hubPool: Contract;
 let owner: SignerWithAddress, depositor: SignerWithAddress, relayer: SignerWithAddress;
@@ -412,8 +419,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);
+    assert(historicalDeposit.found === true, "Test is broken"); // Help tsc to narrow the discriminated union.
+    expect(historicalDeposit.deposit.depositId).to.deep.equal(deposit.depositId);
   });
 
   it("Can fetch younger deposit matching fill", async function () {
@@ -435,8 +444,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);
+    assert(historicalDeposit.found === true, "Test is broken"); // Help tsc to narrow the discriminated union.
+    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 () {
@@ -446,12 +457,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;
   });
 
@@ -464,12 +475,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;
   });
 
@@ -481,9 +492,12 @@ 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);
+
+    assert(search.found === false, "Test is broken"); // Help tsc to narrow the discriminated union.
+    expect(search.code).to.equal(InvalidFill.DepositIdInvalid);
     expect(lastSpyLogIncludes(spy, "Queried RPC for deposit")).is.not.true;
   });
 
@@ -506,10 +520,26 @@ 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);
+
+    assert(search.found === false, "Test is broken"); // Help tsc to narrow the discriminated union.
+    expect(search.code).to.equal(InvalidFill.DepositIdInvalid);
     expect(lastSpyLogIncludes(spy, "Queried RPC for deposit")).is.not.true;
   });
 
+  it("Ignores matching fills that mis-specify a deposit attribute", async function () {
+    const deposit = await buildDeposit(hubPoolClient, spokePool_1, erc20_1, depositor, destinationChainId);
+
+    deposit.realizedLpFeePct = (deposit.realizedLpFeePct ?? bnZero).add(bnOne);
+    const fill = await buildFill(spokePool_2, erc20_2, depositor, relayer, deposit, 1);
+
+    await Promise.all([spokePoolClient1.update(), spokePoolClient2.update()]);
+
+    const search = await queryHistoricalDepositForFill(spokePoolClient1, fill);
+    assert(search.found === false, "Test is broken"); // Help tsc to narrow the discriminated union.
+    expect(search.code).to.equal(InvalidFill.FillMismatch);
+  });
+
   it("Returns sped up deposit matched with fill", async function () {
     const deposit_1 = await buildDeposit(hubPoolClient, spokePool_1, erc20_1, depositor, destinationChainId);
     // Override the fill's realized LP fee % and destination token so that it matches the deposit's default zero'd