From 76e4fbd5c8ceb253a825c03c203d64ac0a19e2e6 Mon Sep 17 00:00:00 2001
From: Paul <108695806+pxrl@users.noreply.github.com>
Date: Mon, 25 Mar 2024 13:24:57 +0100
Subject: [PATCH] refactor(relayer): Remove filledRelays mapping

v3 has removed partial fills and the relayer now knows the onchain fill
status of each candidate deposit prior to considering possible fills, so
there's no need for additional internal tracking of the filled relays.
---
 src/relayer/Relayer.ts    | 23 +----------------------
 test/Relayer.BasicFill.ts | 33 +++++++++++++++------------------
 2 files changed, 16 insertions(+), 40 deletions(-)

diff --git a/src/relayer/Relayer.ts b/src/relayer/Relayer.ts
index b84981f0d..a3481a0a5 100644
--- a/src/relayer/Relayer.ts
+++ b/src/relayer/Relayer.ts
@@ -26,10 +26,6 @@ const UNPROFITABLE_DEPOSIT_NOTICE_PERIOD = 60 * 60; // 1 hour
 export class Relayer {
   public readonly relayerAddress: string;
 
-  // Track by originChainId since depositId is issued on the origin chain.
-  // Key is in the form of "chainId-depositId".
-  private fullyFilledDeposits: { [key: string]: boolean } = {};
-
   constructor(
     relayerAddress: string,
     readonly logger: winston.Logger,
@@ -393,21 +389,7 @@ export class Relayer {
   }
 
   fillRelay(deposit: V3Deposit, repaymentChainId: number, realizedLpFeePct: BigNumber, gasLimit?: BigNumber): void {
-    const { originChainId, depositId, outputToken, outputAmount } = deposit;
-    // Skip deposits that this relayer has already filled completely before to prevent double filling (which is a waste
-    // of gas as the second fill would fail).
-    // TODO: Handle the edge case scenario where the first fill failed due to transient errors and needs to be retried.
-    const fillKey = `${originChainId}-${depositId}`;
-    if (this.fullyFilledDeposits[fillKey]) {
-      this.logger.debug({
-        at: "Relayer",
-        message: "Skipping deposit already filled by this relayer.",
-        originChainId: deposit.originChainId,
-        depositId: deposit.depositId,
-      });
-      return;
-    }
-
+    const { outputToken, outputAmount } = deposit;
     const { spokePoolClients, multiCallerClient } = this.clients;
     this.logger.debug({ at: "Relayer", message: "Filling v3 deposit.", deposit, repaymentChainId, realizedLpFeePct });
 
@@ -434,9 +416,6 @@ export class Relayer {
 
     // Decrement tokens in token client used in the fill. This ensures that we dont try and fill more than we have.
     this.clients.tokenClient.decrementLocalBalance(deposit.destinationChainId, outputToken, outputAmount);
-
-    // All fills routed through `fillRelay()` will complete the relay.
-    this.fullyFilledDeposits[fillKey] = true;
   }
 
   protected async resolveRepaymentChain(
diff --git a/test/Relayer.BasicFill.ts b/test/Relayer.BasicFill.ts
index 32f9e8f65..86516b01b 100644
--- a/test/Relayer.BasicFill.ts
+++ b/test/Relayer.BasicFill.ts
@@ -229,6 +229,7 @@ describe("Relayer: Check for Unfilled Deposits and Fill", async function () {
       await relayerInstance.checkForUnfilledDepositsAndFill();
       expect(lastSpyLogIncludes(spy, "Filling v3 deposit")).to.be.true;
       expect(multiCallerClient.transactionCount()).to.equal(1); // One transaction, filling the one deposit.
+      await multiCallerClient.executeTxnQueues();
 
       // The first fill is still pending but if we rerun the relayer loop, it shouldn't try to fill a second time.
       await Promise.all([spokePoolClient_1.update(), spokePoolClient_2.update(), hubPoolClient.update()]);
@@ -343,10 +344,12 @@ describe("Relayer: Check for Unfilled Deposits and Fill", async function () {
     });
 
     it("Ignores exclusive deposits", async function () {
-      for (const exclusiveRelayer of [randomAddress(), relayerInstance.relayerAddress]) {
-        const currentTime = (await spokePool_2.getCurrentTime()).toNumber();
-        const exclusivityDeadline = currentTime + 60;
+      const currentTime = (await spokePool_2.getCurrentTime()).toNumber();
+      const exclusivityDeadline = currentTime + 7200;
 
+      // Make two deposits - one with the relayer as exclusiveRelayer, and one with a random address.
+      // Verify that the relayer can immediately fill the first deposit, and both after the exclusivity window.
+      for (const exclusiveRelayer of [randomAddress(), relayerInstance.relayerAddress]) {
         await depositV3(
           spokePool_1,
           destinationChainId,
@@ -357,24 +360,18 @@ describe("Relayer: Check for Unfilled Deposits and Fill", async function () {
           outputAmount.div(2),
           { exclusivityDeadline, exclusiveRelayer }
         );
-        const relayerIsExclusive = relayerInstance.relayerAddress === exclusiveRelayer;
-        await updateAllClients();
-
-        await relayerInstance.checkForUnfilledDepositsAndFill();
-        expect(multiCallerClient.transactionCount()).to.equal(relayerIsExclusive ? 1 : 0);
+      }
 
-        if (relayerIsExclusive) {
-          continue;
-        }
+      await updateAllClients();
+      await relayerInstance.checkForUnfilledDepositsAndFill();
+      expect(multiCallerClient.transactionCount()).to.equal(1);
 
-        await spokePool_2.setCurrentTime(exclusivityDeadline + 1);
-        await updateAllClients();
+      await spokePool_2.setCurrentTime(exclusivityDeadline + 1);
+      await updateAllClients();
 
-        // Relayer can unconditionally fill after the exclusivityDeadline.
-        expect(multiCallerClient.transactionCount()).to.equal(0);
-        await relayerInstance.checkForUnfilledDepositsAndFill();
-        expect(multiCallerClient.transactionCount()).to.equal(1);
-      }
+      // Relayer can unconditionally fill after the exclusivityDeadline.
+      await relayerInstance.checkForUnfilledDepositsAndFill();
+      expect(multiCallerClient.transactionCount()).to.equal(2);
     });
 
     it("Ignores deposits older than min deposit confirmation threshold", async function () {