feat: Use custom errors when receiveApproval fails and set callbackgas to 200k (#331)

zguesmi · web-flow · commit 81778ee095ec · 2025-11-28T11:56:41.000+01:00
diff --git a/abis/contracts/IexecInterfaceToken.json b/abis/contracts/IexecInterfaceToken.json
@@ -1,4 +1,9 @@
 [
+  {
+    "inputs": [],
+    "name": "CallerIsNotTheRequester",
+    "type": "error"
+  },
   {
     "inputs": [
       {
@@ -10,6 +15,22 @@
     "name": "IncompatibleDatasetOrder",
     "type": "error"
   },
+  {
+    "inputs": [],
+    "name": "OperationFailed",
+    "type": "error"
+  },
+  {
+    "inputs": [
+      {
+        "internalType": "bytes4",
+        "name": "selector",
+        "type": "bytes4"
+      }
+    ],
+    "name": "UnsupportedOperation",
+    "type": "error"
+  },
   {
     "anonymous": false,
     "inputs": [
diff --git a/abis/contracts/facets/IexecEscrowTokenFacet.json b/abis/contracts/facets/IexecEscrowTokenFacet.json
@@ -1,4 +1,25 @@
 [
+  {
+    "inputs": [],
+    "name": "CallerIsNotTheRequester",
+    "type": "error"
+  },
+  {
+    "inputs": [],
+    "name": "OperationFailed",
+    "type": "error"
+  },
+  {
+    "inputs": [
+      {
+        "internalType": "bytes4",
+        "name": "selector",
+        "type": "bytes4"
+      }
+    ],
+    "name": "UnsupportedOperation",
+    "type": "error"
+  },
   {
     "anonymous": false,
     "inputs": [
diff --git a/abis/contracts/interfaces/IexecEscrowToken.json b/abis/contracts/interfaces/IexecEscrowToken.json
@@ -1,4 +1,25 @@
 [
+  {
+    "inputs": [],
+    "name": "CallerIsNotTheRequester",
+    "type": "error"
+  },
+  {
+    "inputs": [],
+    "name": "OperationFailed",
+    "type": "error"
+  },
+  {
+    "inputs": [
+      {
+        "internalType": "bytes4",
+        "name": "selector",
+        "type": "bytes4"
+      }
+    ],
+    "name": "UnsupportedOperation",
+    "type": "error"
+  },
   {
     "stateMutability": "payable",
     "type": "fallback"
diff --git a/abis/human-readable-abis/contracts/IexecInterfaceToken.sol/IexecInterfaceToken.json b/abis/human-readable-abis/contracts/IexecInterfaceToken.sol/IexecInterfaceToken.json
@@ -1,5 +1,8 @@
 [
+  "error CallerIsNotTheRequester()",
   "error IncompatibleDatasetOrder(string)",
+  "error OperationFailed()",
+  "error UnsupportedOperation(bytes4)",
   "event AccurateContribution(address indexed,bytes32 indexed)",
   "event Approval(address indexed,address indexed,uint256)",
   "event BroadcastAppOrder(tuple(address,uint256,uint256,bytes32,address,address,address,bytes32,bytes))",
diff --git a/abis/human-readable-abis/contracts/facets/IexecEscrowTokenFacet.sol/IexecEscrowTokenFacet.json b/abis/human-readable-abis/contracts/facets/IexecEscrowTokenFacet.sol/IexecEscrowTokenFacet.json
@@ -1,4 +1,7 @@
 [
+  "error CallerIsNotTheRequester()",
+  "error OperationFailed()",
+  "error UnsupportedOperation(bytes4)",
   "event Approval(address indexed,address indexed,uint256)",
   "event Lock(address,uint256)",
   "event Reward(address,uint256,bytes32)",
diff --git a/abis/human-readable-abis/contracts/interfaces/IexecEscrowToken.sol/IexecEscrowToken.json b/abis/human-readable-abis/contracts/interfaces/IexecEscrowToken.sol/IexecEscrowToken.json
@@ -1,4 +1,7 @@
 [
+  "error CallerIsNotTheRequester()",
+  "error OperationFailed()",
+  "error UnsupportedOperation(bytes4)",
   "function deposit(uint256) returns (bool)",
   "function depositFor(uint256,address) returns (bool)",
   "function depositForArray(uint256[],address[]) returns (bool)",
diff --git a/contracts/facets/IexecConfigurationFacet.sol b/contracts/facets/IexecConfigurationFacet.sol
@@ -37,7 +37,7 @@ contract IexecConfigurationFacet is IexecConfiguration, FacetBase {
         $.m_datasetregistry = IRegistry(_datasetregistryAddress);
         $.m_workerpoolregistry = IRegistry(_workerpoolregistryAddress);
         $.m_v3_iexecHub = IexecHubV3Interface(_v3_iexecHubAddress);
-        $.m_callbackgas = 100000;
+        $.m_callbackgas = 200_000;
     }
 
     function domain() external view override returns (IexecLibOrders_v5.EIP712Domain memory) {
diff --git a/contracts/facets/IexecEscrowTokenFacet.sol b/contracts/facets/IexecEscrowTokenFacet.sol
@@ -66,11 +66,11 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2
     }
 
     /***************************************************************************
-     *            Token Spender: Atomic Deposit+Match                  *
+     *            Token Spender: Atomic Deposit + Match                        *
      ***************************************************************************/
 
     /**
-     * @notice Receives approval, deposit and optionally executes an operation in one transaction
+     * @notice Receives approval, deposit and optionally executes a supported operation in one transaction.
      *
      * Usage patterns:
      * 1. Simple deposit: RLC.approveAndCall(escrow, amount, "")
@@ -82,21 +82,21 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2
      *
      * @dev Implementation details:
      * - Deposits tokens first, then executes the operation if data is provided
-     * - Extracts function selector from data to determine which operation
-     * - Each operation has a validator (_validateMatchOrders, etc.) for preconditions
+     * - Extracts function selector from data to determine the operation
+     * - Each operation has a validator (_validateMatchOrders, etc.) to check preconditions
      * - After validation, _executeOperation performs the delegatecall
-     * - Error handling is generalized: bubbles up revert reasons or returns 'operation-failed'
+     * - Error handling is generalized: reverts are bubbled up with revert reasons or custom errors
      * - Future operations can be added by implementing a validator and adding a selector case
      *
      * @dev matchOrders specific notes:
-     * - Sponsoring is NOT supported. The requester (sender) always pays for the deal.
+     * - Sponsoring is NOT supported. The requester (specified in the request order) always pays for the deal.
      * - Clients must compute the exact deal cost and deposit the right amount.
      *   The deal cost = (appPrice + datasetPrice + workerpoolPrice) * volume.
      *
      * @param sender The address that approved tokens
      * @param amount Amount of tokens approved and to be deposited
      * @param token Address of the token (must be RLC)
-     * @param data Optional: Function selector + ABI-encoded parameters for operation
+     * @param data Optional: Function selector + ABI-encoded parameters
      * @return success True if operation succeeded
      *
      *
@@ -114,7 +114,7 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2
      *     requestOrder
      * );
      *
-     * // One transaction does it all: approve, deposit, and match
+     * // Call the RLC contract with the encoded data.
      * RLC(token).approveAndCall(iexecProxy, dealCost, data);
      * ```
      */
@@ -128,56 +128,52 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2
         require(token == address($.m_baseToken), "wrong-token");
         _deposit(sender, amount);
         _mint(sender, amount);
-
         if (data.length > 0) {
             _executeOperation(sender, data);
         }
         return true;
     }
 
+    /**
+     * Executes a supported operation after depositing tokens.
+     * @param sender The address that approved tokens and initiated the operation
+     * @param data ABI-encoded function selector and parameters of the operation
+     */
     function _executeOperation(address sender, bytes calldata data) internal {
         // Extract the function selector (first 4 bytes)
         bytes4 selector = bytes4(data[:4]);
-
         // Validate operation-specific preconditions before execution
         if (selector == IexecPoco1.matchOrders.selector) {
             _validateMatchOrders(sender, data);
         } else {
-            revert("unsupported-operation");
+            revert UnsupportedOperation(selector);
         }
-
         // Execute the operation via delegatecall
-        // This preserves msg.sender context and allows the operation to access
-        // the diamond's storage and functions
+        // This preserves `msg.sender` context and allows the operation to access
+        // the diamond's storage and functions.
+        // Note: here `msg.sender` is the RLC token contract.
         (bool success, bytes memory result) = address(this).delegatecall(data);
-
+        if (success) {
+            return;
+        }
         // Handle failure and bubble up revert reason
-        if (!success) {
-            if (result.length > 0) {
-                // Decode and revert with the original error
-                assembly {
-                    let returndata_size := mload(result)
-                    revert(add(result, 32), returndata_size)
-                }
-            } else {
-                revert("operation-failed");
-            }
+        if (result.length == 0) {
+            revert OperationFailed();
+        }
+        // Decode and revert with the original error
+        assembly {
+            let returndata_size := mload(result)
+            revert(add(result, 32), returndata_size)
         }
     }
 
-    /******************************************************************************
-     *        Token Spender: Atomic Deposit+Match if used with RLC.approveAndCall *
-     *****************************************************************************/
-
     /**
      * @dev Validates matchOrders preconditions
      * @param sender The user who deposited (must be the requester)
-     * @param data ABI-encoded matchOrders call with orders
+     * @param data matchOrders calldata
      */
     function _validateMatchOrders(address sender, bytes calldata data) internal pure {
-        // Decode only the request order to validate the requester
-        // Full decoding: (AppOrder, DatasetOrder, WorkerpoolOrder, RequestOrder)
-        // We only need to check requestorder.requester
+        // Decode orders and check that the sender is the requester.
         (, , , IexecLibOrders_v5.RequestOrder memory requestorder) = abi.decode(
             data[4:],
             (
@@ -187,10 +183,8 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2
                 IexecLibOrders_v5.RequestOrder
             )
         );
-        // Validate that sender is the requester
-        // This ensures the caller is authorized to create this deal
         if (requestorder.requester != sender) {
-            revert("caller-must-be-requester");
+            revert CallerIsNotTheRequester();
         }
     }
 
diff --git a/contracts/interfaces/IexecEscrowToken.sol b/contracts/interfaces/IexecEscrowToken.sol
@@ -4,6 +4,10 @@
 pragma solidity ^0.8.0;
 
 interface IexecEscrowToken {
+    error UnsupportedOperation(bytes4 selector);
+    error OperationFailed();
+    error CallerIsNotTheRequester();
+
     receive() external payable;
     fallback() external payable;
     function deposit(uint256) external returns (bool);
diff --git a/docs/solidity/index.md b/docs/solidity/index.md
@@ -252,7 +252,7 @@ function recover() external returns (uint256)
 function receiveApproval(address sender, uint256 amount, address token, bytes data) external returns (bool)
 ```
 
-Receives approval, deposit and optionally executes an operation in one transaction
+Receives approval, deposit and optionally executes a supported operation in one transaction.
 
 Usage patterns:
 1. Simple deposit: RLC.approveAndCall(escrow, amount, "")
@@ -264,14 +264,14 @@ the operation, followed by ABI-encoded parameters. Supported operations:
 
 _Implementation details:
 - Deposits tokens first, then executes the operation if data is provided
-- Extracts function selector from data to determine which operation
-- Each operation has a validator (_validateMatchOrders, etc.) for preconditions
+- Extracts function selector from data to determine the operation
+- Each operation has a validator (_validateMatchOrders, etc.) to check preconditions
 - After validation, _executeOperation performs the delegatecall
-- Error handling is generalized: bubbles up revert reasons or returns 'operation-failed'
+- Error handling is generalized: reverts are bubbled up with revert reasons or custom errors
 - Future operations can be added by implementing a validator and adding a selector case
 
 matchOrders specific notes:
-- Sponsoring is NOT supported. The requester (sender) always pays for the deal.
+- Sponsoring is NOT supported. The requester (specified in the request order) always pays for the deal.
 - Clients must compute the exact deal cost and deposit the right amount.
   The deal cost = (appPrice + datasetPrice + workerpoolPrice) * volume._
 
@@ -282,13 +282,13 @@ matchOrders specific notes:
 | sender | address | The address that approved tokens |
 | amount | uint256 | Amount of tokens approved and to be deposited |
 | token | address | Address of the token (must be RLC) |
-| data | bytes | Optional: Function selector + ABI-encoded parameters for operation |
+| data | bytes | Optional: Function selector + ABI-encoded parameters |
 
 #### Return Values
 
 | Name | Type | Description |
 | ---- | ---- | ----------- |
-| [0] | bool | success True if operation succeeded @custom:example ```solidity // Compute deal cost uint256 dealCost = (appPrice + datasetPrice + workerpoolPrice) * volume; // Encode matchOrders operation with selector bytes memory data = abi.encodeWithSelector(     IexecPoco1.matchOrders.selector,     appOrder,     datasetOrder,     workerpoolOrder,     requestOrder ); // One transaction does it all: approve, deposit, and match RLC(token).approveAndCall(iexecProxy, dealCost, data); ``` |
+| [0] | bool | success True if operation succeeded @custom:example ```solidity // Compute deal cost uint256 dealCost = (appPrice + datasetPrice + workerpoolPrice) * volume; // Encode matchOrders operation with selector bytes memory data = abi.encodeWithSelector(     IexecPoco1.matchOrders.selector,     appOrder,     datasetOrder,     workerpoolOrder,     requestOrder ); // Call the RLC contract with the encoded data. RLC(token).approveAndCall(iexecProxy, dealCost, data); ``` |
 
 ## IexecOrderManagementFacet
 
diff --git a/test/byContract/IexecConfiguration/IexecConfiguration.test.ts b/test/byContract/IexecConfiguration/IexecConfiguration.test.ts
@@ -73,7 +73,7 @@ describe('IexecConfiguration', async () => {
             expect(await iexecPoco.datasetregistry()).equal(configureParams.datasetregistry);
             expect(await iexecPoco.workerpoolregistry()).equal(configureParams.workerpoolregistry);
             // no getter for m_v3_iexecHub
-            expect(await iexecPoco.callbackgas()).equal(100000);
+            expect(await iexecPoco.callbackgas()).equal(200_000);
         });
         it('Should not configure when sender is not owner', async () => {
             await expect(iexecPoco.configure(...configureArgs)).to.be.revertedWith(
diff --git a/test/byContract/IexecEscrow/IexecEscrowToken-receiveApproval.test.ts b/test/byContract/IexecEscrow/IexecEscrowToken-receiveApproval.test.ts
diff --git a/test/byContract/IexecPocoBoost/IexecPocoBoost.test.ts b/test/byContract/IexecPocoBoost/IexecPocoBoost.test.ts
diff --git a/test/byContract/iexecPocoAccessors/IexecPocoAccessors.test.ts b/test/byContract/iexecPocoAccessors/IexecPocoAccessors.test.ts
diff --git a/utils/createOrders.ts b/utils/createOrders.ts
diff --git a/utils/odb-tools.ts b/utils/odb-tools.ts

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,8 @@`
`1`	`1`	`[`
	`2`	`+ "error CallerIsNotTheRequester()",`
`2`	`3`	`"error IncompatibleDatasetOrder(string)",`
	`4`	`+ "error OperationFailed()",`
	`5`	`+ "error UnsupportedOperation(bytes4)",`
`3`	`6`	`"event AccurateContribution(address indexed,bytes32 indexed)",`
`4`	`7`	`"event Approval(address indexed,address indexed,uint256)",`
`5`	`8`	`"event BroadcastAppOrder(tuple(address,uint256,uint256,bytes32,address,address,address,bytes32,bytes))",`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,7 @@`
`1`	`1`	`[`
	`2`	`+ "error CallerIsNotTheRequester()",`
	`3`	`+ "error OperationFailed()",`
	`4`	`+ "error UnsupportedOperation(bytes4)",`
`2`	`5`	`"event Approval(address indexed,address indexed,uint256)",`
`3`	`6`	`"event Lock(address,uint256)",`
`4`	`7`	`"event Reward(address,uint256,bytes32)",`
Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,7 @@ contract IexecConfigurationFacet is IexecConfiguration, FacetBase {`
`37`	`37`	`$.m_datasetregistry = IRegistry(_datasetregistryAddress);`
`38`	`38`	`$.m_workerpoolregistry = IRegistry(_workerpoolregistryAddress);`
`39`	`39`	`$.m_v3_iexecHub = IexecHubV3Interface(_v3_iexecHubAddress);`
`40`		`- $.m_callbackgas = 100000;`
	`40`	`+ $.m_callbackgas = 200_000;`
`41`	`41`	`}`
`42`	`42`
`43`	`43`	`function domain() external view override returns (IexecLibOrders_v5.EIP712Domain memory) {`