fix: MulticallerClient should ignore expected reverts for V3 fills (#…

…1217)

* fix: MulticallerClient should ignore expected reverts for V3 fills

This will filter out the barrage of duplicate fill issues

* Update MultiCallerClient.ts

* Update MultiCallerClient.ts

* add call static

* Revert "add call static"

This reverts commit ea994f4.

* Update MultiCallerClient.ts

package.json

@@ -12,7 +12,7 @@
   "dependencies": {
     "@across-protocol/constants-v2": "1.0.8",
     "@across-protocol/contracts-v2": "2.5.0-beta.7",
-    "@across-protocol/sdk-v2": "0.22.2",
+    "@across-protocol/sdk-v2": "0.22.4",
     "@arbitrum/sdk": "^3.1.3",
     "@defi-wonderland/smock": "^2.3.5",
     "@eth-optimism/sdk": "^3.1.0",

src/clients/MultiCallerClient.ts

@@ -24,18 +24,26 @@ import lodash from "lodash";
 // .includes() to partially match reason string in order to not ignore errors thrown by non-contract reverts.
 // For example, a NodeJS error might result in a reason string that includes more than just the contract r
 // evert reason.
-export const knownRevertReasons = new Set(["relay filled", "Already claimed"]);
+export const knownRevertReasons = new Set(["relay filled", "Already claimed", "RelayFilled", "ClaimedMerkleLeaf"]);
 
 // The following reason potentially includes false positives of reverts that we should be alerted on, however
 // there is something likely broken in how the provider is interpreting contract reverts. Currently, there are
 // a lot of duplicate transaction sends that are reverting with this reason, for example, sending a transaction
 // to execute a relayer refund leaf takes a while to execute and ends up reverting because a duplicate transaction
 // mines before it. This situation leads to this revert reason which is spamming the Logger currently.
-export const unknownRevertReason =
-  "missing revert data in call exception; Transaction reverted without a reason string";
+export const unknownRevertReasons = [
+  "missing revert data in call exception; Transaction reverted without a reason string",
+  // execution reverted is the error reason when a require statement with a custom error is thrown.
+  "execution reverted",
+];
 export const unknownRevertReasonMethodsToIgnore = new Set([
+  "multicall",
   "fillRelay",
   "fillRelayWithUpdatedFee",
+  "fillV3Relay",
+  "fillV3RelayWithUpdatedDeposit",
+  "requestV3SlowFill",
+  "executeV3SlowRelayLeaf",
   "executeSlowRelayLeaf",
   "executeRelayerRefundLeaf",
   "executeRootBundle",
@@ -438,7 +446,11 @@ export class MultiCallerClient {
   protected canIgnoreRevertReason(txn: TransactionSimulationResult): boolean {
     const { transaction: _txn, reason } = txn;
     const knownReason = [...knownRevertReasons].some((knownReason) => reason.includes(knownReason));
-    return knownReason || (unknownRevertReasonMethodsToIgnore.has(_txn.method) && reason.includes(unknownRevertReason));
+    return (
+      knownReason ||
+      (unknownRevertReasonMethodsToIgnore.has(_txn.method) &&
+        unknownRevertReasons.some((_reason) => reason.includes(_reason)))
+    );
   }
 
   // Filter out transactions that revert for non-critical, expected reasons. For example, the "relay filled" error may

src/utils/TransactionUtils.ts

@@ -169,9 +169,26 @@ export async function willSucceed(transaction: AugmentedTransaction): Promise<Tr
     return { transaction, succeed: true };
   }
 
+  const { contract, method } = transaction;
+  const args = transaction.value ? [...transaction.args, { value: transaction.value }] : transaction.args;
+
+  // First callStatic, which will surface a custom error if the transaction would fail.
+  // This is useful for surfacing custom error revert reasons like RelayFilled in the V3 SpokePool but
+  // it does incur an extra RPC call. We do this because estimateGas is a provider function that doesn't
+  // relay custom errors well: https://github.com/ethers-io/ethers.js/discussions/3291#discussion-4314795
+  try {
+    await contract.callStatic[method](...args);
+  } catch (err: any) {
+    if (err.errorName) {
+      return {
+        transaction,
+        succeed: false,
+        reason: err.errorName,
+      };
+    }
+  }
+
   try {
-    const { contract, method } = transaction;
-    const args = transaction.value ? [...transaction.args, { value: transaction.value }] : transaction.args;
     const gasLimit = await contract.estimateGas[method](...args);
     return { transaction: { ...transaction, gasLimit }, succeed: true };
   } catch (_error) {

test/MultiCallerClient.ts

@@ -4,6 +4,7 @@ import {
   knownRevertReasons,
   unknownRevertReason,
   unknownRevertReasonMethodsToIgnore,
+  unknownRevertReasons,
 } from "../src/clients";
 import { bnOne, BigNumber, TransactionSimulationResult } from "../src/utils";
 import { MockedTransactionClient, txnClientPassResult } from "./mocks/MockTransactionClient";
@@ -288,15 +289,17 @@ describe("MultiCallerClient", async function () {
     }
 
     // Verify that the defined "unknown" revert reason against known methods is ignored.
-    txn.args = [{ result: unknownRevertReason }];
-    for (const method of unknownRevertReasonMethodsToIgnore) {
-      txn.method = method;
-      txn.message = `${txn.method} simulation; expected to fail with: ${unknownRevertReason}.`;
+    for (const unknownRevertReason of unknownRevertReasons) {
+      txn.args = [{ result: unknownRevertReason }];
+      for (const method of unknownRevertReasonMethodsToIgnore) {
+        txn.method = method;
+        txn.message = `${txn.method} simulation; expected to fail with: ${unknownRevertReason}.`;
 
-      const result = await multiCaller.simulateTransactionQueue([txn]);
-      expect(result.length).to.equal(0);
-      expect(multiCaller.ignoredSimulationFailures.length).to.equal(1);
-      expect(multiCaller.loggedSimulationFailures.length).to.equal(0);
+        const result = await multiCaller.simulateTransactionQueue([txn]);
+        expect(result.length).to.equal(0);
+        expect(multiCaller.ignoredSimulationFailures.length).to.equal(1);
+        expect(multiCaller.loggedSimulationFailures.length).to.equal(0);
+      }
     }
 
     // Verify that unexpected revert reason against both known and "unknown" methods are logged.

yarn.lock

@@ -41,26 +41,6 @@
     axios "^1.6.2"
     zksync-web3 "^0.14.3"
 
-"@across-protocol/[email protected]":
-  version "2.5.0-beta.7"
-  resolved "https://registry.yarnpkg.com/@across-protocol/contracts-v2/-/contracts-v2-2.5.0-beta.7.tgz#9410efc9e39b8b1b6e814dcd416e6017f7d40abd"
-  integrity sha512-/u6VB5QZwi9i7EDRBYNGMne+/ZXTI1XbHzEEI4+Z1Bb6xKvKjTsJJsStz7wwBplGAWCkmRLVxCbJtfN2o0iUSw==
-  dependencies:
-    "@across-protocol/constants-v2" "^1.0.11"
-    "@defi-wonderland/smock" "^2.3.4"
-    "@eth-optimism/contracts" "^0.5.40"
-    "@ethersproject/abstract-provider" "5.7.0"
-    "@ethersproject/abstract-signer" "5.7.0"
-    "@ethersproject/bignumber" "5.7.0"
-    "@openzeppelin/contracts" "4.9.3"
-    "@openzeppelin/contracts-upgradeable" "4.9.3"
-    "@scroll-tech/contracts" "^0.1.0"
-    "@uma/common" "^2.34.0"
-    "@uma/contracts-node" "^0.4.17"
-    "@uma/core" "^2.56.0"
-    axios "^1.6.2"
-    zksync-web3 "^0.14.3"
-
 "@across-protocol/contracts@^0.1.4":
   version "0.1.4"
   resolved "https://registry.yarnpkg.com/@across-protocol/contracts/-/contracts-0.1.4.tgz#64b3d91e639d2bb120ea94ddef3d160967047fa5"
@@ -70,10 +50,10 @@
     "@openzeppelin/contracts" "4.1.0"
     "@uma/core" "^2.18.0"
 
-"@across-protocol/[email protected].2":
-  version "0.22.2"
-  resolved "https://registry.yarnpkg.com/@across-protocol/sdk-v2/-/sdk-v2-0.22.2.tgz#d07f39f5d7d54bb65ab6ffd752f191c57f16b020"
-  integrity sha512-K34JHw5gC0LVL/+1muyCYR4Char3QrPCNAR/phQiiy98PQzAvgJIi7ah7eD7QhBuNkY9vxD9N5oEC9ZNsjitUA==
+"@across-protocol/[email protected].4":
+  version "0.22.4"
+  resolved "https://registry.yarnpkg.com/@across-protocol/sdk-v2/-/sdk-v2-0.22.4.tgz#b20e41f45c5c3522482d7ad953caed45f48b1d5d"
+  integrity sha512-y6Yy66zrYsTHfHWpVYgMTgMlx1K9F/50LWRDUPOJGPM2Czv+iE2emdqnxz8Im90qSfgQ0akHJpD4zFVgqxEq5Q==
   dependencies:
     "@across-protocol/across-token" "^1.0.0"
     "@across-protocol/constants-v2" "^1.0.11"