improve: Identify reason for deposit <-> fill mismatches (#493)

The combination of abiCoder.encode() and keccak256() when computing relayData hashes seems to be relatively expensive, and this can be a real performance suck when called repeatedly in a tight loop. Additionally, performing a per-field comparison exposes the underlying error when two RelayData objects don't match; this makes it easy to identify the root cause for invalid fills.
across-protocol · Oct 4, 2024 · b248382 · b248382
1 parent 3064ebd
commit b248382
Show file tree

Hide file tree

Showing 5 changed files with 127 additions and 50 deletions.
diff --git a/package.json b/package.json
@@ -1,7 +1,7 @@
 {
   "name": "@across-protocol/sdk",
   "author": "UMA Team",
-  "version": "3.2.3",
+  "version": "3.2.4",
   "license": "AGPL-3.0",
   "homepage": "https://docs.across.to/reference/sdk",
   "files": [

diff --git a/src/clients/SpokePoolClient.ts b/src/clients/SpokePoolClient.ts
@@ -305,8 +305,21 @@ export class SpokePoolClient extends BaseAbstractClient {
    * @returns The corresponding deposit if found, undefined otherwise.
    */
   public getDepositForFill(fill: Fill): DepositWithBlock | undefined {
-    const depositWithMatchingDepositId = this.depositHashes[this.getDepositHash(fill)];
-    return validateFillForDeposit(fill, depositWithMatchingDepositId) ? depositWithMatchingDepositId : undefined;
+    const deposit = this.depositHashes[this.getDepositHash(fill)];
+    const match = validateFillForDeposit(fill, deposit);
+    if (match.valid) {
+      return deposit;
+    }
+
+    this.logger.debug({
+      at: "SpokePoolClient::getDepositForFill",
+      message: `Rejected fill for ${getNetworkName(fill.originChainId)} deposit ${fill.depositId}.`,
+      reason: match.reason,
+      deposit,
+      fill,
+    });
+
+    return;
   }
 
   /**
@@ -341,7 +354,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);

diff --git a/src/utils/DepositUtils.ts b/src/utils/DepositUtils.ts
@@ -62,15 +62,25 @@ export async function queryHistoricalDepositForFill(
 
   ({ earliestDepositIdQueried: lowId, latestDepositIdQueried: highId } = spokePoolClient);
   if (depositId >= lowId && depositId <= highId) {
+    const originChain = getNetworkName(fill.originChainId);
     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 ${originChain} deposit ID ${depositId} is invalid (${match.reason}).`,
+      };
     }
 
     return {
       found: false,
-      code: isDefined(deposit) ? InvalidFill.FillMismatch : InvalidFill.DepositIdNotFound,
-      reason: `Deposit ID ${depositId} not found in SpokePoolClient event buffer.`,
+      code: InvalidFill.DepositIdNotFound,
+      reason: `${originChain} deposit ID ${depositId} not found in SpokePoolClient event buffer.`,
     };
   }
 
@@ -103,14 +113,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,
   };
 }
 

diff --git a/src/utils/FlowUtils.ts b/src/utils/FlowUtils.ts
@@ -1,48 +1,38 @@
-import { Deposit, Fill, RelayData, SlowFillRequest } from "../interfaces";
-import { getRelayDataHash } from "./SpokeUtils";
+import { isDefined } from "../utils";
+import { Deposit, RelayData } from "../interfaces";
 
-export const FILL_DEPOSIT_COMPARISON_KEYS = [
+export const RELAYDATA_KEYS = [
   "depositId",
   "originChainId",
   "destinationChainId",
   "depositor",
   "recipient",
-  "message",
-] as const;
-
-export const V3_DEPOSIT_COMPARISON_KEYS = [
-  ...FILL_DEPOSIT_COMPARISON_KEYS,
   "inputToken",
   "inputAmount",
   "outputToken",
   "outputAmount",
   "fillDeadline",
   "exclusivityDeadline",
   "exclusiveRelayer",
+  "message",
 ] as const;
 
-export function filledSameDeposit(fillA: Fill, fillB: Fill): boolean {
-  // Don't bother hashing obvious mismatches.
-  if (fillA.depositId !== fillB.depositId) {
-    return false;
-  }
-
-  const { destinationChainId: chainA } = fillA;
-  const { destinationChainId: chainB } = fillB;
-  return getRelayDataHash(fillA, chainA) === getRelayDataHash(fillB, chainB);
-}
-
+// 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 destinationToken, which are pulled from other clients.
 export function validateFillForDeposit(
-  relayData: RelayData & { destinationChainId: number }, // V3Fill, SlowFillRequest...
+  relayData: RelayData & { destinationChainId: number },
   deposit?: Deposit
-): boolean {
+): { valid: true } | { valid: false; reason: string } {
   if (deposit === undefined) {
-    return false;
+    return { valid: false, reason: "Deposit is undefined" };
   }
 
-  return validateV3FillForDeposit(relayData, deposit);
-}
+  // 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.
+  const invalidKey = RELAYDATA_KEYS.find((key) => relayData[key].toString() !== deposit[key].toString());
 
-function validateV3FillForDeposit(fill: Fill | SlowFillRequest, deposit: Deposit): boolean {
-  return getRelayDataHash(fill, fill.destinationChainId) === getRelayDataHash(deposit, deposit.destinationChainId);
+  return isDefined(invalidKey)
+    ? { valid: false, reason: `${invalidKey} mismatch (${relayData[invalidKey]} != ${deposit[invalidKey]})` }
+    : { valid: true };
 }
diff --git a/test/SpokePoolClient.ValidateFill.ts b/test/SpokePoolClient.ValidateFill.ts
@@ -116,6 +116,55 @@ describe("SpokePoolClient: Fill Validation", function () {
     outputAmount = inputAmount.sub(bnOne);
   });
 
+  it("Correctly matches fills with deposits", async function () {
+    const deposit_2 = await depositV3(
+      spokePool_1,
+      destinationChainId,
+      depositor,
+      inputToken,
+      inputAmount,
+      outputToken,
+      outputAmount
+    );
+    const fill = await fillV3Relay(spokePool_2, deposit_2, relayer);
+    await spokePoolClient2.update();
+
+    expect(validateFillForDeposit(fill, deposit_2)).to.deep.equal({ valid: true });
+
+    const ignoredFields = [
+      "fromLiteChain",
+      "toLiteChain",
+      "blockNumber",
+      "transactionHash",
+      "transactionIndex",
+      "logIndex",
+      "relayer",
+      "repaymentChainId",
+      "relayExecutionInfo",
+    ];
+
+    // For each RelayData field, toggle the value to produce an invalid fill. Verify that it's rejected.
+    const fields = Object.keys(fill).filter((field) => !ignoredFields.includes(field));
+    for (const field of fields) {
+      let val: BigNumber | string | number;
+      if (BigNumber.isBigNumber(fill[field])) {
+        val = fill[field].add(bnOne);
+      } else if (typeof fill[field] === "string") {
+        val = fill[field] + "xxx";
+      } else {
+        expect(typeof fill[field]).to.equal("number");
+        val = fill[field] + 1;
+      }
+
+      const result = validateFillForDeposit(fill, { ...deposit_2, [field]: val });
+      expect(result.valid).to.be.false;
+      expect((result as { reason: string }).reason.startsWith(`${field} mismatch`)).to.be.true;
+    }
+
+    // Verify that the underlying fill is untouched and still valid.
+    expect(validateFillForDeposit(fill, deposit_2)).to.deep.equal({ valid: true });
+  });
+
   it("Tracks v3 fill status", async function () {
     const deposit = await depositV3(
       spokePool_1,
@@ -206,7 +255,7 @@ 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 })).to.equal(true);
+    expect(validateFillForDeposit(fill_1, { ...deposit_1 })).to.deep.equal({ valid: true });
   });
 
   it("Returns deposit matched with fill", async function () {
@@ -221,11 +270,11 @@ describe("SpokePoolClient: Fill Validation", function () {
     );
 
     const fill = await fillV3Relay(spokePool_2, deposit, relayer);
-    // expect(spokePoolClientForDestinationChain.getDepositForFill(fill_1)).to.equal(undefined);
+    expect(spokePoolClient2.getDepositForFill(fill)).to.equal(undefined);
     await spokePoolClient1.update();
 
     expect(spokePoolClient1.getDepositForFill(fill))
-      .excludingEvery(["realizedLpFeePct", "quoteBlockNumber", "fromLiteChain", "toLiteChain"])
+      .excludingEvery(["quoteBlockNumber", "fromLiteChain", "toLiteChain"])
       .to.deep.equal(deposit);
   });
 
@@ -644,7 +693,7 @@ describe("SpokePoolClient: Fill Validation", function () {
     expect(fill_2.relayExecutionInfo.fillType === FillType.FastFill).to.be.true;
 
     expect(spokePoolClient1.getDepositForFill(fill_1))
-      .excludingEvery(["quoteBlockNumber", "realizedLpFeePct", "fromLiteChain", "toLiteChain"])
+      .excludingEvery(["quoteBlockNumber", "fromLiteChain", "toLiteChain"])
       .to.deep.equal(deposit_1);
     expect(spokePoolClient1.getDepositForFill(fill_2)).to.equal(undefined);
 
@@ -657,30 +706,44 @@ describe("SpokePoolClient: Fill Validation", function () {
       outputToken,
       outputAmount
     );
-    await fillV3Relay(spokePool_2, deposit_2, relayer);
+    const fill = await fillV3Relay(spokePool_2, deposit_2, relayer);
     await spokePoolClient2.update();
 
-    const validFill = spokePoolClient2.getFills().at(-1)!;
-    expect(validFill).to.exist;
+    expect(validateFillForDeposit(fill, deposit_2)).to.deep.equal({ valid: true });
 
-    expect(validateFillForDeposit(validFill, deposit_2)).to.be.true;
+    // Changed the input token.
+    let result = validateFillForDeposit(fill, { ...deposit_2, inputToken: owner.address });
+    expect(result.valid).to.be.false;
+    expect((result as { reason: string }).reason.startsWith("inputToken mismatch")).to.be.true;
 
     // Invalid input amount.
-    expect(validateFillForDeposit({ ...validFill, inputAmount: toBNWei(1337) }, deposit_2)).to.be.false;
+    result = validateFillForDeposit({ ...fill, inputAmount: toBNWei(1337) }, deposit_2);
+    expect(result.valid).to.be.false;
+    expect((result as { reason: string }).reason.startsWith("inputAmount mismatch")).to.be.true;
 
     // Changed the output token.
-    expect(validateFillForDeposit(validFill, { ...deposit_2, outputToken: owner.address })).to.be.false;
+    result = validateFillForDeposit(fill, { ...deposit_2, outputToken: owner.address });
+    expect(result.valid).to.be.false;
+    expect((result as { reason: string }).reason.startsWith("outputToken mismatch")).to.be.true;
 
     // Changed the output amount.
-    expect(validateFillForDeposit({ ...validFill, outputAmount: toBNWei(1337) }, deposit_2)).to.be.false;
+    result = validateFillForDeposit({ ...fill, outputAmount: toBNWei(1337) }, deposit_2);
+    expect(result.valid).to.be.false;
+    expect((result as { reason: string }).reason.startsWith("outputAmount mismatch")).to.be.true;
 
     // Invalid depositId.
-    expect(validateFillForDeposit({ ...validFill, depositId: 1337 }, deposit_2)).to.be.false;
+    result = validateFillForDeposit({ ...fill, depositId: 1337 }, deposit_2);
+    expect(result.valid).to.be.false;
+    expect((result as { reason: string }).reason.startsWith("depositId mismatch")).to.be.true;
 
     // Changed the depositor.
-    expect(validateFillForDeposit({ ...validFill, depositor: relayer.address }, deposit_2)).to.be.false;
+    result = validateFillForDeposit({ ...fill, depositor: relayer.address }, deposit_2);
+    expect(result.valid).to.be.false;
+    expect((result as { reason: string }).reason.startsWith("depositor mismatch")).to.be.true;
 
     // Changed the recipient.
-    expect(validateFillForDeposit({ ...validFill, recipient: relayer.address }, deposit_2)).to.be.false;
+    result = validateFillForDeposit({ ...fill, recipient: relayer.address }, deposit_2);
+    expect(result.valid).to.be.false;
+    expect((result as { reason: string }).reason.startsWith("recipient mismatch")).to.be.true;
   });
 });