takeOrders4 skips an order whose calculate fails

crates/bindings/abis/RaindexV6.json

@@ -1593,6 +1593,31 @@
       ],
       "anonymous": false
     },
+    {
+      "type": "event",
+      "name": "OrderCalculateReverted",
+      "inputs": [
+        {
+          "name": "sender",
+          "type": "address",
+          "indexed": false,
+          "internalType": "address"
+        },
+        {
+          "name": "owner",
+          "type": "address",
+          "indexed": false,
+          "internalType": "address"
+        },
+        {
+          "name": "orderHash",
+          "type": "bytes32",
+          "indexed": false,
+          "internalType": "bytes32"
+        }
+      ],
+      "anonymous": false
+    },
     {
       "type": "event",
       "name": "OrderExceedsMaxRatio",

src/concrete/raindex/RaindexV6.sol

@@ -97,6 +97,11 @@ contract RaindexV6 is IRaindexV6, IMetaV1_2, ReentrancyGuard, Multicall, Raindex
     using LibDecimalFloat for Float;
     using LibBytes32Array for bytes32;
 
+    /// The order's calculate eval reverted, or returned fewer than
+    /// `CALCULATE_ORDER_MIN_OUTPUTS` stack items. The order is skipped rather
+    /// than reverting the batch.
+    event OrderCalculateReverted(address sender, address owner, bytes32 orderHash);
+
     /// All hashes of all active orders. There's nothing interesting in the value
     /// it's just nonzero if the order is live. The key is the hash of the order.
     /// Removing an order sets the value back to zero so it is identical to the
@@ -332,13 +337,14 @@ contract RaindexV6 is IRaindexV6, IMetaV1_2, ReentrancyGuard, Multicall, Raindex
 
         checkTokenSelfTrade(quoteConfig.order, quoteConfig.inputIOIndex, quoteConfig.outputIOIndex);
 
-        OrderIOCalculationV4 memory orderIOCalculation = calculateOrderIO(
+        (, OrderIOCalculationV4 memory orderIOCalculation) = calculateOrderIO(
             quoteConfig.order,
             quoteConfig.inputIOIndex,
             quoteConfig.outputIOIndex,
             msg.sender,
             quoteConfig.signedContext,
-            new bytes32[](0)
+            new bytes32[](0),
+            false
         );
         // true is quote2's "order is live" result required by IRaindexV6.
         // forge-lint: disable-next-line(boolean-cst)
@@ -421,13 +427,14 @@ contract RaindexV6 is IRaindexV6, IMetaV1_2, ReentrancyGuard, Multicall, Raindex
                     // calculates, so a later same-owner order observes earlier
                     // TAKEN orders' writes through the store. An untaken order
                     // runs no `handleIO`, so it makes no state change at all.
-                    OrderIOCalculationV4 memory orderIOCalculation = calculateOrderIO(
+                    (bool calculated, OrderIOCalculationV4 memory orderIOCalculation) = calculateOrderIO(
                         order,
                         takeOrderConfig.inputIOIndex,
                         takeOrderConfig.outputIOIndex,
                         msg.sender,
                         takeOrderConfig.signedContext,
-                        new bytes32[](0)
+                        new bytes32[](0),
+                        true
                     );
 
                     // Skip orders that are too expensive rather than revert as we have
@@ -441,7 +448,9 @@ contract RaindexV6 is IRaindexV6, IMetaV1_2, ReentrancyGuard, Multicall, Raindex
                     // nothing. The explicit `taken` flag is the order's outcome
                     // for the batch.
                     bool taken;
-                    if (orderIOCalculation.IORatio.gt(config.maximumIORatio)) {
+                    if (!calculated) {
+                        emit OrderCalculateReverted(msg.sender, order.owner, orderHash);
+                    } else if (orderIOCalculation.IORatio.gt(config.maximumIORatio)) {
                         emit OrderExceedsMaxRatio(msg.sender, order.owner, orderHash);
                     } else if (
                         !orderIOCalculation.outputMax.gt(LibDecimalFloat.FLOAT_ZERO)
@@ -612,21 +621,23 @@ contract RaindexV6 is IRaindexV6, IMetaV1_2, ReentrancyGuard, Multicall, Raindex
             // Emit the Clear event before `eval2`.
             emit ClearV3(msg.sender, aliceOrder, bobOrder, clearConfig);
         }
-        OrderIOCalculationV4 memory aliceOrderIOCalculation = calculateOrderIO(
+        (, OrderIOCalculationV4 memory aliceOrderIOCalculation) = calculateOrderIO(
             aliceOrder,
             clearConfig.aliceInputIOIndex,
             clearConfig.aliceOutputIOIndex,
             bobOrder.owner,
             bobSignedContext,
-            new bytes32[](0)
+            new bytes32[](0),
+            false
         );
-        OrderIOCalculationV4 memory bobOrderIOCalculation = calculateOrderIO(
+        (, OrderIOCalculationV4 memory bobOrderIOCalculation) = calculateOrderIO(
             bobOrder,
             clearConfig.bobInputIOIndex,
             clearConfig.bobOutputIOIndex,
             aliceOrder.owner,
             aliceSignedContext,
-            new bytes32[](0)
+            new bytes32[](0),
+            false
         );
 
         // Each side must be one `takeOrders4` would take. Given this, each
@@ -700,14 +711,20 @@ contract RaindexV6 is IRaindexV6, IMetaV1_2, ReentrancyGuard, Multicall, Raindex
     /// in-memory store before the calculate eval, making earlier same-owner,
     /// same-batch calculate writes visible to this evaluation. Empty for
     /// single-order callers (`quote`, `clear3`).
+    /// @param skipFailure If true, a reverting calculate eval or a stack shorter
+    /// than `CALCULATE_ORDER_MIN_OUTPUTS` returns false instead of reverting.
+    /// @return False if `skipFailure` caught a failure, in which case the
+    /// calculation is zeroed and MUST NOT be used.
+    /// @return The order IO calculation.
     function calculateOrderIO(
         OrderV4 memory order,
         uint256 inputIOIndex,
         uint256 outputIOIndex,
         address counterparty,
         SignedContextV1[] memory signedContext,
-        bytes32[] memory stateOverlay
-    ) internal view returns (OrderIOCalculationV4 memory) {
+        bytes32[] memory stateOverlay,
+        bool skipFailure
+    ) internal view returns (bool, OrderIOCalculationV4 memory) {
         unchecked {
             bytes32[][] memory context;
             {
@@ -764,12 +781,14 @@ contract RaindexV6 is IRaindexV6, IMetaV1_2, ReentrancyGuard, Multicall, Raindex
             // and recordVaultInput so that local storage writes happen before
             // writes on the interpreter.
             StateNamespace namespace = StateNamespace.wrap(uint256(uint160(order.owner)));
+            StackItem[] memory calculateOrderStack;
+            bytes32[] memory calculateOrderKVs;
             // Slither false positive. External calls within loops are fine if
             // the caller controls which orders are eval'd as they can drop
             // failing calls and resubmit a new transaction.
             // https://github.com/crytic/slither/issues/880
             //slither-disable-next-line calls-loop
-            (StackItem[] memory calculateOrderStack, bytes32[] memory calculateOrderKVs) = order.evaluable.interpreter
+            try order.evaluable.interpreter
                 .eval4(
                     EvalV4({
                         store: order.evaluable.store,
@@ -780,11 +799,29 @@ contract RaindexV6 is IRaindexV6, IMetaV1_2, ReentrancyGuard, Multicall, Raindex
                         inputs: new StackItem[](0),
                         stateOverlay: stateOverlay
                     })
-                );
+                ) returns (
+                StackItem[] memory stack, bytes32[] memory kvs
+            ) {
+                calculateOrderStack = stack;
+                calculateOrderKVs = kvs;
+            } catch {
+                if (!skipFailure) {
+                    assembly ("memory-safe") {
+                        let ptr := mload(0x40)
+                        returndatacopy(ptr, 0, returndatasize())
+                        revert(ptr, returndatasize())
+                    }
+                }
+            }
 
             // This is a much clearer error message and overall is more efficient
             // than solidity generic index out of bounds errors.
-            if (calculateOrderStack.length < CALCULATE_ORDER_MIN_OUTPUTS) {
+            bool calculated = calculateOrderStack.length >= CALCULATE_ORDER_MIN_OUTPUTS;
+            if (!calculated) {
+                if (skipFailure) {
+                    OrderIOCalculationV4 memory failed;
+                    return (calculated, failed);
+                }
                 revert UnsupportedCalculateOutputs(calculateOrderStack.length);
             }
 
@@ -806,15 +843,18 @@ contract RaindexV6 is IRaindexV6, IMetaV1_2, ReentrancyGuard, Multicall, Raindex
             context[CONTEXT_CALCULATIONS_COLUMN] =
                 LibBytes32Array.arrayFrom(Float.unwrap(orderOutputMax), Float.unwrap(orderIORatio));
 
-            return OrderIOCalculationV4({
-                order: order,
-                outputIOIndex: outputIOIndex,
-                outputMax: orderOutputMax,
-                IORatio: orderIORatio,
-                context: context,
-                namespace: namespace,
-                kvs: calculateOrderKVs
-            });
+            return (
+                calculated,
+                OrderIOCalculationV4({
+                    order: order,
+                    outputIOIndex: outputIOIndex,
+                    outputMax: orderOutputMax,
+                    IORatio: orderIORatio,
+                    context: context,
+                    namespace: namespace,
+                    kvs: calculateOrderKVs
+                })
+            );
         }
     }
 

subgraph/abis/Raindex.json

@@ -1593,6 +1593,31 @@
       ],
       "anonymous": false
     },
+    {
+      "type": "event",
+      "name": "OrderCalculateReverted",
+      "inputs": [
+        {
+          "name": "sender",
+          "type": "address",
+          "indexed": false,
+          "internalType": "address"
+        },
+        {
+          "name": "owner",
+          "type": "address",
+          "indexed": false,
+          "internalType": "address"
+        },
+        {
+          "name": "orderHash",
+          "type": "bytes32",
+          "indexed": false,
+          "internalType": "bytes32"
+        }
+      ],
+      "anonymous": false
+    },
     {
       "type": "event",
       "name": "OrderExceedsMaxRatio",

subgraph/generated/Raindex/Raindex.ts

@@ -388,6 +388,32 @@ export class MetaV1_2__Params {
   }
 }
 
+export class OrderCalculateReverted extends ethereum.Event {
+  get params(): OrderCalculateReverted__Params {
+    return new OrderCalculateReverted__Params(this);
+  }
+}
+
+export class OrderCalculateReverted__Params {
+  _event: OrderCalculateReverted;
+
+  constructor(event: OrderCalculateReverted) {
+    this._event = event;
+  }
+
+  get sender(): Address {
+    return this._event.parameters[0].value.toAddress();
+  }
+
+  get owner(): Address {
+    return this._event.parameters[1].value.toAddress();
+  }
+
+  get orderHash(): Bytes {
+    return this._event.parameters[2].value.toBytes();
+  }
+}
+
 export class OrderExceedsMaxRatio extends ethereum.Event {
   get params(): OrderExceedsMaxRatio__Params {
     return new OrderExceedsMaxRatio__Params(this);

test/concrete/raindex/RaindexV6.clear.calculateRevert.t.sol

@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: LicenseRef-DCL-1.0
+// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd
+pragma solidity =0.8.25;
+
+import {RaindexV6FreshTakeOrderTest} from "test/util/abstract/RaindexV6FreshTakeOrderTest.sol";
+import {LibTestTakeOrder} from "test/util/lib/LibTestTakeOrder.sol";
+import {OrderV4, ClearConfigV2, SignedContextV1} from "raindex-interface-0.1.8/src/interface/IRaindexV6.sol";
+
+/// A clear cannot drop one side, so a reverting calculate eval reverts
+/// `clear3` with the eval's own revert data.
+contract RaindexV6ClearCalculateRevertTest is RaindexV6FreshTakeOrderTest {
+    address internal alice = address(uint160(uint256(keccak256("alice.rain.test"))));
+    address internal bob = address(uint160(uint256(keccak256("bob.rain.test"))));
+
+    function checkClearReverts(bytes memory aliceExpression, bytes memory bobExpression, bytes memory err) internal {
+        OrderV4 memory aliceOrder = LibTestTakeOrder.addOrderWithExpression(
+            vm, alice, aliceExpression, address(iToken0), bytes32(uint256(1)), address(iToken1), bytes32(uint256(1))
+        );
+        OrderV4 memory bobOrder = LibTestTakeOrder.addOrderWithExpression(
+            vm, bob, bobExpression, address(iToken1), bytes32(uint256(1)), address(iToken0), bytes32(uint256(1))
+        );
+
+        vm.expectRevert(err);
+        iRaindex.clear3(
+            aliceOrder, bobOrder, ClearConfigV2(0, 0, 0, 0, 0, 0), new SignedContextV1[](0), new SignedContextV1[](0)
+        );
+    }
+
+    function testClearAliceCalculateRevertReverts() external {
+        checkClearReverts(":ensure(0 \"alice calc\"),_ _:1 1;:;", "_ _:1 1;:;", "alice calc");
+    }
+
+    function testClearBobCalculateRevertReverts() external {
+        checkClearReverts("_ _:1 1;:;", ":ensure(0 \"bob calc\"),_ _:1 1;:;", "bob calc");
+    }
+}

test/concrete/raindex/RaindexV6.quote.calculateRevert.t.sol

@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: LicenseRef-DCL-1.0
+// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd
+pragma solidity =0.8.25;
+
+import {RaindexV6FreshTakeOrderTest} from "test/util/abstract/RaindexV6FreshTakeOrderTest.sol";
+import {LibTestTakeOrder} from "test/util/lib/LibTestTakeOrder.sol";
+import {OrderV4, QuoteV2, SignedContextV1} from "raindex-interface-0.1.8/src/interface/IRaindexV6.sol";
+
+/// `quote2` surfaces a reverting calculate eval as the eval's own revert data.
+contract RaindexV6QuoteCalculateRevertTest is RaindexV6FreshTakeOrderTest {
+    function testQuoteCalculateRevertReverts() external {
+        OrderV4 memory order = LibTestTakeOrder.addOrderWithExpression(
+            vm,
+            address(uint160(uint256(keccak256("alice.rain.test")))),
+            ":ensure(0 \"calc\"),_ _:1 1;:;",
+            address(iToken0),
+            bytes32(uint256(1)),
+            address(iToken1),
+            bytes32(uint256(1))
+        );
+
+        vm.expectRevert(bytes("calc"));
+        iRaindex.quote2(QuoteV2(order, 0, 0, new SignedContextV1[](0)));
+    }
+}