Implement gas refunds as an interaction. (gnosis/gp-v2-contracts#445)

Closes #442 The idea is to make gas refunds be encoded as interactions instead of being part of the settlement parameters. ### Gas Cost Analysis #### Native Order Refunds ![image](https://user-images.githubusercontent.com/4210206/109288257-e5884180-7824-11eb-9355-847d32908b1f.png) #### Interaction Based Order Refunds ![image](https://user-images.githubusercontent.com/4210206/109288348-05b80080-7825-11eb-9b49-2af960f88619.png) #### Results: So, as @fleupold mentioned, it would be smarter to accumulate refunds and then use them all at once. This reduces the overall gas cost per order. In the case of **native order refunds**, the optimal "refund batch" size to use is 7: - The "cold start" gas cost per order is `181490` (this is the average cost per order when doing 7 settlements without refunds, and then an 8th settlement with 7 refunds) - The "continuous" gas cost per order is `180467` (this is the average cost per order when doing 6 settlements without refunds, then a 7th settlement with 7 refunds) In the case of **interaction based order refunds**, the optimal "refund batch" size is 8: - The "cold start" gas cost per order is `181237` - The "continuous" gas cost per order is `180484` (only 17 gas more) This means that the gas cost per order is almost the same 🎉. One other bonus of keeping order refunds as interactions, is that if, for whatever reason, storage refunds get removed from the EVM, we don't pay additional costs per settlement for a dead feature if we use interaction based storage refunds. #### Full Settlements The full settlement benchmarks show a similar story, where settlements without gas costs become cheaper, and settlements without become more expensive. <details><summary>Benchmarks</summary> ``` === Settlement Gas Benchmarks === --------------+--------------+--------------+--------------+-------------- tokens | trades | interactions | refunds | gas --------------+--------------+--------------+--------------+-------------- 2 | 10 | 0 | 0 | 752558 (change: -137 / trade) 3 | 10 | 0 | 0 | 753102 (change: -136 / trade) 4 | 10 | 0 | 0 | 762034 (change: -139 / trade) 5 | 10 | 0 | 0 | 766766 (change: -137 / trade) 6 | 10 | 0 | 0 | 763086 (change: -139 / trade) 7 | 10 | 0 | 0 | 772018 (change: -134 / trade) 8 | 10 | 0 | 0 | 776690 (change: -136 / trade) 8 | 20 | 0 | 0 | 1463360 (change: -72 / trade) 8 | 30 | 0 | 0 | 2111933 (change: -47 / trade) 8 | 40 | 0 | 0 | 2764685 (change: -33 / trade) 8 | 50 | 0 | 0 | 3418082 (change: -28 / trade) 8 | 60 | 0 | 0 | 4067559 (change: -24 / trade) 8 | 70 | 0 | 0 | 4721274 (change: -19 / trade) 8 | 80 | 0 | 0 | 5374021 (change: -18 / trade) 2 | 10 | 1 | 0 | 823946 (change: -137 / trade) 2 | 10 | 2 | 0 | 870598 (change: -142 / trade) 2 | 10 | 3 | 0 | 917286 (change: -133 / trade) 2 | 10 | 4 | 0 | 963926 (change: -135 / trade) 2 | 10 | 5 | 0 | 1010614 (change: -136 / trade) 2 | 10 | 6 | 0 | 1057230 (change: -135 / trade) 2 | 10 | 7 | 0 | 1103870 (change: -139 / trade) 2 | 10 | 8 | 0 | 1150498 (change: -142 / trade) 2 | 50 | 0 | 10 | 3126161 (change: 196 / trade) 2 | 50 | 0 | 15 | 3085349 (change: 197 / trade) 2 | 50 | 0 | 20 | 3044429 (change: 200 / trade) 2 | 50 | 0 | 25 | 3003485 (change: 197 / trade) 2 | 50 | 0 | 30 | 2962745 (change: 202 / trade) 2 | 50 | 0 | 35 | 2921813 (change: 203 / trade) 2 | 50 | 0 | 40 | 2880953 (change: 204 / trade) 2 | 50 | 0 | 45 | 2839997 (change: 202 / trade) 2 | 50 | 0 | 50 | 2799233 (change: 207 / trade) 2 | 2 | 0 | 0 | 186781 (change: -683 / trade) 2 | 1 | 1 | 0 | 187309 (change: -1378 / trade) 10 | 100 | 10 | 20 | 6618975 (change: 100 / trade) ``` </details> ### Test Plan CI, added new unit tests for interaction-only methods. Coverage stays at 100%.
cowprotocol · Mar 2, 2021 · f2184c5 · f2184c5
1 parent 0f61e56
commit f2184c5
Show file tree

Hide file tree

Showing 9 changed files with 196 additions and 163 deletions.
diff --git a/.prettierignore b/.prettierignore
@@ -0,0 +1 @@
+src/contracts/test/vendor/
diff --git a/bench/single.ts b/bench/single.ts
@@ -7,24 +7,33 @@ async function main() {
 
   const pad = (x: unknown) => ` ${x} `.padStart(14);
   console.log(chalk.bold("=== Single Order Gas Benchmarks ==="));
-  console.log(chalk.gray("--------------+--------------"));
+  console.log(chalk.gray("--------------+--------------+--------------"));
   console.log(
-    ["gasToken", "gas"]
+    ["refunds", "gasToken", "gas"]
       .map((header) => chalk.cyan(pad(header)))
       .join(chalk.gray("|")),
   );
-  console.log(chalk.gray("--------------+--------------"));
-  for (const gasToken of [0, 5]) {
+  console.log(chalk.gray("--------------+--------------+--------------"));
+  for (const [refunds, gasToken] of [
+    [0, 0],
+    [2, 0],
+    [4, 0],
+    [8, 0],
+    [0, 2],
+    [2, 2],
+    [4, 2],
+    [0, 5],
+  ]) {
     const { gasUsed } = await fixture.settle({
       tokens: 2,
       trades: 1,
       interactions: 1,
-      refunds: 0,
+      refunds,
       gasToken,
     });
 
     console.log(
-      [pad(gasToken), chalk.yellow(pad(gasUsed.toString()))].join(
+      [pad(refunds), pad(gasToken), chalk.yellow(pad(gasUsed.toString()))].join(
         chalk.gray("|"),
       ),
     );

diff --git a/src/contracts/GPv2Settlement.sol b/src/contracts/GPv2Settlement.sol
@@ -22,12 +22,6 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible {
     using GPv2TradeExecution for GPv2TradeExecution.Data;
     using SafeMath for uint256;
 
-    /// @dev Data used for freeing storage and claiming a gas refund.
-    struct OrderRefunds {
-        bytes[] filledAmounts;
-        bytes[] preSignatures;
-    }
-
     /// @dev The authenticator is used to determine who can call the settle function.
     /// That is, only authorised solvers have the ability to invoke settlements.
     /// Any valid authenticator implements an isSolver method called by the onlySolver
@@ -88,6 +82,13 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible {
         _;
     }
 
+    /// @dev Modifier to ensure that an external function is only callable as a
+    /// settlement interaction.
+    modifier onlyInteraction {
+        require(address(this) == msg.sender, "GPv2: not an interaction");
+        _;
+    }
+
     /// @dev Settle the specified orders at a clearing price. Note that it is
     /// the responsibility of the caller to ensure that all GPv2 invariants are
     /// upheld for the input settlement, otherwise this call will revert.
@@ -110,14 +111,11 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible {
     /// @param interactions Smart contract interactions split into three
     /// separate lists to be run before the settlement, during the settlement
     /// and after the settlement respectively.
-    /// @param orderRefunds Order refunds for clearing storage related to
-    /// expired orders.
     function settle(
         IERC20[] calldata tokens,
         uint256[] calldata clearingPrices,
         GPv2Trade.Data[] calldata trades,
-        GPv2Interaction.Data[][3] calldata interactions,
-        OrderRefunds calldata orderRefunds
+        GPv2Interaction.Data[][3] calldata interactions
     ) external nonReentrant onlySolver {
         executeInteractions(interactions[0]);
 
@@ -132,8 +130,6 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible {
 
         executeInteractions(interactions[2]);
 
-        claimOrderRefunds(orderRefunds);
-
         emit Settlement(msg.sender);
     }
 
@@ -150,6 +146,30 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible {
         emit OrderInvalidated(owner, orderUid);
     }
 
+    /// @dev Free storage from the filled amounts of **expired** orders to claim
+    /// a gas refund. This method can only be called as an interaction.
+    ///
+    /// @param orderUids The unique identifiers of the expired order to free
+    /// storage for.
+    function freeFilledAmountStorage(bytes[] calldata orderUids)
+        external
+        onlyInteraction
+    {
+        freeOrderStorage(filledAmount, orderUids);
+    }
+
+    /// @dev Free storage from the pre signatures of **expired** orders to claim
+    /// a gas refund. This method can only be called as an interaction.
+    ///
+    /// @param orderUids The unique identifiers of the expired order to free
+    /// storage for.
+    function freePreSignatureStorage(bytes[] calldata orderUids)
+        external
+        onlyInteraction
+    {
+        freeOrderStorage(preSignature, orderUids);
+    }
+
     /// @dev Process all trades one at a time returning the computed net in and
     /// out transfers for the trades.
     ///
@@ -346,23 +366,15 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible {
         }
     }
 
-    /// @dev Claims order gas refunds.
-    ///
-    /// @param orderRefunds Order refund data for freeing storage.
-    function claimOrderRefunds(OrderRefunds calldata orderRefunds) internal {
-        freeOrderStorage(orderRefunds.filledAmounts, filledAmount);
-        freeOrderStorage(orderRefunds.preSignatures, preSignature);
-    }
-
     /// @dev Claims refund for the specified storage and order UIDs.
     ///
     /// This method reverts if any of the orders are still valid.
     ///
     /// @param orderUids Order refund data for freeing storage.
     /// @param orderStorage Order storage mapped on a UID.
     function freeOrderStorage(
-        bytes[] calldata orderUids,
-        mapping(bytes => uint256) storage orderStorage
+        mapping(bytes => uint256) storage orderStorage,
+        bytes[] calldata orderUids
     ) internal {
         for (uint256 i = 0; i < orderUids.length; i++) {
             bytes calldata orderUid = orderUids[i];

diff --git a/src/contracts/test/GPv2SettlementTestInterface.sol b/src/contracts/test/GPv2SettlementTestInterface.sol
@@ -55,9 +55,11 @@ contract GPv2SettlementTestInterface is GPv2Settlement {
         executeInteractions(interactions);
     }
 
-    function claimOrderRefundsTest(OrderRefunds calldata orderRefunds)
-        external
-    {
-        claimOrderRefunds(orderRefunds);
+    function freeFilledAmountStorageTest(bytes[] calldata orderUids) external {
+        this.freeFilledAmountStorage(orderUids);
+    }
+
+    function freePreSignatureStorageTest(bytes[] calldata orderUids) external {
+        this.freePreSignatureStorage(orderUids);
     }
 }
diff --git a/src/ts/settlement.ts b/src/ts/settlement.ts
@@ -110,11 +110,6 @@ export interface TradeExecution {
   feeDiscount: BigNumberish;
 }
 
-/**
- * Table mapping token addresses to their respective clearing prices.
- */
-export type Prices = Record<string, BigNumberish | undefined>;
-
 /**
  * Order refund data.
  */
@@ -125,6 +120,11 @@ export interface OrderRefunds {
   preSignatures: BytesLike[];
 }
 
+/**
+ * Table mapping token addresses to their respective clearing prices.
+ */
+export type Prices = Record<string, BigNumberish | undefined>;
+
 /**
  * Encoded settlement parameters.
  */
@@ -137,8 +137,6 @@ export type EncodedSettlement = [
   Trade[],
   /** Encoded interactions. */
   [Interaction[], Interaction[], Interaction[]],
-  /** Encoded order refunds. */
-  OrderRefunds,
 ];
 
 /**
@@ -260,25 +258,57 @@ export class SettlementEncoder {
   }
 
   /**
-   * Gets the encoded interactions for the specified stage as a hex-encoded
-   * string.
+   * Gets all encoded interactions for all stages.
+   *
+   * Note that order refund interactions are included as post-interactions.
    */
   public get interactions(): [Interaction[], Interaction[], Interaction[]] {
     return [
       this._interactions[InteractionStage.PRE].slice(),
       this._interactions[InteractionStage.INTRA].slice(),
-      this._interactions[InteractionStage.POST].slice(),
+      [
+        ...this._interactions[InteractionStage.POST],
+        ...this.encodedOrderRefunds,
+      ],
     ];
   }
 
   /**
-   * Gets the currently encoded order UIDs for gas refunds.
+   * Gets the order refunds encoded as interactions.
    */
-  public get orderRefunds(): OrderRefunds {
-    return {
-      filledAmounts: this._orderRefunds.filledAmounts.slice(),
-      preSignatures: this._orderRefunds.preSignatures.slice(),
-    };
+  public get encodedOrderRefunds(): Interaction[] {
+    const { filledAmounts, preSignatures } = this._orderRefunds;
+    if (filledAmounts.length + preSignatures.length === 0) {
+      return [];
+    }
+
+    const settlement = this.domain.verifyingContract;
+    if (settlement === undefined) {
+      throw new Error("domain missing settlement contract address");
+    }
+
+    // NOTE: Avoid importing the full GPv2Settlement contract artifact just for
+    // a tiny snippet of the ABI. Unit and integration tests will catch any
+    // issues that may arise from this definition becoming out of date.
+    const iface = new ethers.utils.Interface([
+      "function freeFilledAmountStorage(bytes[] orderUids)",
+      "function freePreSignatureStorage(bytes[] orderUids)",
+    ]);
+
+    const interactions = [];
+    for (const [functionName, orderUids] of [
+      ["freeFilledAmountStorage", filledAmounts] as const,
+      ["freePreSignatureStorage", preSignatures] as const,
+    ].filter(([, orderUids]) => orderUids.length > 0)) {
+      interactions.push(
+        normalizeInteraction({
+          target: settlement,
+          callData: iface.encodeFunctionData(functionName, [orderUids]),
+        }),
+      );
+    }
+
+    return interactions;
   }
 
   /**
@@ -381,9 +411,14 @@ export class SettlementEncoder {
   /**
    * Encodes order UIDs for gas refunds.
    *
+   * @param settlement The address of the settlement contract.
    * @param orderRefunds The order refunds to encode.
    */
   public encodeOrderRefunds(orderRefunds: Partial<OrderRefunds>): void {
+    if (this.domain.verifyingContract === undefined) {
+      throw new Error("domain missing settlement contract address");
+    }
+
     const filledAmounts = orderRefunds.filledAmounts ?? [];
     const preSignatures = orderRefunds.preSignatures ?? [];
 
@@ -408,7 +443,6 @@ export class SettlementEncoder {
       this.clearingPrices(prices),
       this.trades,
       this.interactions,
-      this.orderRefunds,
     ];
   }