improve: Identify reason for deposit <-> fill mismatches

This makes it easier to debug the pending v3 changes, and in production
it should reduce the time required to identify problems.

src/clients/SpokePoolClient.ts

@@ -335,10 +335,21 @@ export class SpokePoolClient extends BaseAbstractClient {
    * @returns The corresponding deposit if found, undefined otherwise.
    */
   public getDepositForFill(fill: Fill, fillFieldsToIgnore: string[] = []): DepositWithBlock | undefined {
-    const depositWithMatchingDepositId = this.depositHashes[this.getDepositHash(fill)];
-    return validateFillForDeposit(fill, depositWithMatchingDepositId, fillFieldsToIgnore)
-      ? depositWithMatchingDepositId
-      : undefined;
+    const deposit = this.depositHashes[this.getDepositHash(fill)];
+    const match = validateFillForDeposit(fill, deposit, fillFieldsToIgnore);
+    if (match.valid) {
+      return deposit;
+    }
+
+    const originChain = getNetworkName(fill.originChainId);
+    this.logger.debug({
+      at: "SpokePoolClient::getDepositForFill",
+      message: `Rejected fill for ${originChain} deposit ${fill.depositId}.`,
+      reason: match.reason,
+      deposit,
+      fill
+    });
+    return undefined;
   }
 
   /**
@@ -376,7 +387,7 @@ export class SpokePoolClient extends BaseAbstractClient {
 
     const { validFills, invalidFills } = fillsForDeposit.reduce(
       (groupedFills: { validFills: Fill[]; invalidFills: Fill[] }, fill: Fill) => {
-        if (validateFillForDeposit(fill, deposit)) {
+        if (validateFillForDeposit(fill, deposit).valid) {
           groupedFills.validFills.push(fill);
         } else {
           groupedFills.invalidFills.push(fill);
@@ -481,7 +492,7 @@ export class SpokePoolClient extends BaseAbstractClient {
       maxBlockLookBack: this.eventSearchConfig.maxBlockLookBack,
     };
     return (await this.queryFillsInBlockRange(fill, searchConfig)).filter((_fill) =>
-      validateFillForDeposit(_fill, deposit)
+      validateFillForDeposit(_fill, deposit).valid
     );
   }
 

src/utils/DepositUtils.ts

@@ -63,13 +63,22 @@ export async function queryHistoricalDepositForFill(
   ({ 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 };
+    if (isDefined(deposit)) {
+      const match = validateFillForDeposit(fill, deposit);
+      if (match.valid) {
+        return { found: true, deposit };
+      }
+
+      return {
+        found: false,
+        code: InvalidFill.FillMismatch,
+        reason: `Fill for deposit ID ${depositId} buffer is invalid (${match.reason}).`,
+      };
     }
 
     return {
       found: false,
-      code: isDefined(deposit) ? InvalidFill.FillMismatch : InvalidFill.DepositIdNotFound,
+      code: InvalidFill.DepositIdNotFound,
       reason: `Deposit ID ${depositId} not found in SpokePoolClient event buffer.`,
     };
   }
@@ -103,14 +112,15 @@ export async function queryHistoricalDepositForFill(
     }
   }
 
-  if (validateFillForDeposit(fill, deposit)) {
+  const match = validateFillForDeposit(fill, deposit)
+  if (match.valid) {
     return { found: true, deposit };
   }
 
   return {
     found: false,
     code: InvalidFill.FillMismatch,
-    reason: `Fill is not valid for ${getNetworkName(deposit.originChainId)} deposit ${depositId}`,
+    reason: match.reason,
   };
 }
 

src/utils/FlowUtils.ts

@@ -69,18 +69,25 @@ export function filledSameDeposit(fillA: Fill, fillB: Fill): boolean {
 
 // Ensure that each deposit element is included with the same value in the fill. This includes all elements defined
 // by the depositor as well as the realizedLpFeePct and the destinationToken, which are pulled from other clients.
-export function validateFillForDeposit(fill: Fill, deposit?: Deposit, fillFieldsToIgnore: string[] = []): boolean {
+export function validateFillForDeposit(
+  fill: Fill,
+  deposit?: Deposit,
+  fillFieldsToIgnore: string[] = []
+): { valid: true } | { valid: false; reason: string } {
   if (deposit === undefined) {
-    return false;
+    return { valid: false, reason: "Deposit is undefined" };
   }
 
   // Note: this short circuits when a key is found where the comparison doesn't match.
   // TODO: if we turn on "strict" in the tsconfig, the elements of FILL_DEPOSIT_COMPARISON_KEYS will be automatically
   // validated against the fields in Fill and Deposit, generating an error if there is a discrepency.
-  return FILL_DEPOSIT_COMPARISON_KEYS.every((key) => {
-    if (fillFieldsToIgnore.includes(key)) {
-      return true;
+  for (const key of FILL_DEPOSIT_COMPARISON_KEYS) {
+    if (fill[key]?.toString() === deposit[key]?.toString() || fillFieldsToIgnore.includes(key)) {
+      continue;
     }
-    return fill[key] !== undefined && fill[key].toString() === deposit[key]?.toString();
-  });
+
+    return { valid: false, reason: `${key} mismatch (${fill[key]} != ${deposit[key]}` };
+  }
+
+  return { valid: true };
 }

test/SpokePoolClient.ValidateFill.ts

@@ -134,13 +134,12 @@ describe("SpokePoolClient: Fill Validation", function () {
 
     // Some fields are expected to be dynamically populated by the client, but aren't in this environment.
     // Fill them in manually from the fill struct to get a valid comparison.
-    expect(
-      validateFillForDeposit(fill_1, {
-        ...deposit_1,
-        realizedLpFeePct: fill_1.realizedLpFeePct,
-        destinationToken: fill_1.destinationToken,
-      })
-    ).to.equal(true);
+    const fill_2 = {
+      ...deposit_1,
+      realizedLpFeePct: fill_1.realizedLpFeePct,
+      destinationToken: fill_1.destinationToken,
+    };
+    expect(validateFillForDeposit(fill_1, fill_2).valid).to.equal(true);
   });
 
   it("Returns deposit matched with fill", async function () {
@@ -608,19 +607,19 @@ describe("SpokePoolClient: Fill Validation", function () {
     };
 
     // Invalid Amount.
-    expect(validateFillForDeposit({ ...validFill, amount: toBNWei(1337) }, validDeposit)).to.be.false;
+    expect(validateFillForDeposit({ ...validFill, amount: toBNWei(1337) }, validDeposit).valid).to.be.false;
 
     // Invalid depositId.
-    expect(validateFillForDeposit({ ...validFill, depositId: 1337 }, validDeposit)).to.be.false;
+    expect(validateFillForDeposit({ ...validFill, depositId: 1337 }, validDeposit).valid).to.be.false;
 
     // Changed the depositor.
-    expect(validateFillForDeposit({ ...validFill, depositor: relayer.address }, validDeposit)).to.be.false;
+    expect(validateFillForDeposit({ ...validFill, depositor: relayer.address }, validDeposit).valid).to.be.false;
 
     // Changed the recipient.
-    expect(validateFillForDeposit({ ...validFill, recipient: relayer.address }, validDeposit)).to.be.false;
+    expect(validateFillForDeposit({ ...validFill, recipient: relayer.address }, validDeposit).valid).to.be.false;
 
     // Changed the relayerFeePct.
-    expect(validateFillForDeposit({ ...validFill, relayerFeePct: toBNWei(1337) }, validDeposit)).to.be.false;
+    expect(validateFillForDeposit({ ...validFill, relayerFeePct: toBNWei(1337) }, validDeposit).valid).to.be.false;
 
     // Validate the realizedLPFeePct and destinationToken matches. These values are optional in the deposit object and
     // are assigned during the update method, which is not polled in this set of tests.
@@ -631,19 +630,19 @@ describe("SpokePoolClient: Fill Validation", function () {
       validateFillForDeposit(validFill, {
         ...validDeposit,
         realizedLpFeePct: validFill.realizedLpFeePct,
-      })
+      }).valid
     ).to.be.true;
 
     expect(
       validateFillForDeposit(validFill, {
         ...validDeposit,
         realizedLpFeePct: validFill.realizedLpFeePct.add(toBNWei("0.1")),
-      })
+      }).valid
     ).to.be.false;
 
     // Assign a destinationToken to the deposit and ensure it is validated correctly. erc20_2 from the fillRelay method
     // above is the destination token. After, try changing this to something that is clearly wrong.
-    expect(validateFillForDeposit(validFill, { ...validDeposit, destinationToken: erc20_2.address })).to.be.true;
-    expect(validateFillForDeposit(validFill, { ...validDeposit, destinationToken: owner.address })).to.be.false;
+    expect(validateFillForDeposit(validFill, { ...validDeposit, destinationToken: erc20_2.address }).valid).to.be.true;
+    expect(validateFillForDeposit(validFill, { ...validDeposit, destinationToken: owner.address }).valid).to.be.false;
   });
 });