cleanup checkupkeep and callback (#9651)

* cleanup checkupkeep and callback * cleanup * further cleanup * update comment * cleanup * add max revert data size struct * compile * regen wrappers and fix test * fix tests * add test for revert size limit * fix bug * add callback tests * more callback tests * more tests * regen wrappers * fix test * add clarifying comment * fix test * fix chaincli * regen wrappers --------- Co-authored-by: FelixFan1992 <fankejin@gmail.com>
smartcontractkit · Jul 6, 2023 · b0f6ac5 · b0f6ac5
1 parent 05c5843
commit b0f6ac5
Show file tree

Hide file tree

Showing 17 changed files with 281 additions and 52 deletions.
diff --git a/contracts/src/v0.7/tests/UpkeepMock.sol b/contracts/src/v0.7/tests/UpkeepMock.sol
@@ -10,6 +10,7 @@ contract UpkeepMock is KeeperCompatible {
   bytes public performData;
   uint256 public checkGasToBurn;
   uint256 public performGasToBurn;
+  string public checkRevertReason;
 
   uint256 constant checkGasBuffer = 6000; // use all but this amount in gas burn loops
   uint256 constant performGasBuffer = 1000; // use all but this amount in gas burn loops
@@ -32,6 +33,10 @@ contract UpkeepMock is KeeperCompatible {
     canPerform = value;
   }
 
+  function setCheckRevertReason(string calldata value) public {
+    checkRevertReason = value;
+  }
+
   function setCheckGasToBurn(uint256 value) public {
     require(value > checkGasBuffer || value == 0, "checkGasToBurn must be 0 (disabled) or greater than buffer");
     if (value > 0) {
@@ -50,13 +55,10 @@ contract UpkeepMock is KeeperCompatible {
     }
   }
 
-  function checkUpkeep(bytes calldata data)
-    external
-    override
-    cannotExecute
-    returns (bool callable, bytes memory executedata)
-  {
-    require(!shouldRevertCheck, "shouldRevertCheck should be false");
+  function checkUpkeep(
+    bytes calldata data
+  ) external override cannotExecute returns (bool callable, bytes memory executedata) {
+    require(!shouldRevertCheck, checkRevertReason);
     uint256 startGas = gasleft();
     bool couldCheck = canCheck;
 

diff --git a/contracts/src/v0.8/dev/automation/2_1/KeeperRegistry2_1.sol b/contracts/src/v0.8/dev/automation/2_1/KeeperRegistry2_1.sol
@@ -305,6 +305,7 @@ contract KeeperRegistry2_1 is KeeperRegistryBase2_1, OCR2Abstract, Chainable, ER
       transcoder: onchainConfig.transcoder,
       maxCheckDataSize: onchainConfig.maxCheckDataSize,
       maxPerformDataSize: onchainConfig.maxPerformDataSize,
+      maxRevertDataSize: onchainConfig.maxRevertDataSize,
       upkeepPrivilegeManager: onchainConfig.upkeepPrivilegeManager,
       nonce: s_storage.nonce,
       configCount: s_storage.configCount,

diff --git a/contracts/src/v0.8/dev/automation/2_1/KeeperRegistryBase2_1.sol b/contracts/src/v0.8/dev/automation/2_1/KeeperRegistryBase2_1.sol
@@ -172,7 +172,8 @@ abstract contract KeeperRegistryBase2_1 is ConfirmedOwner, ExecutionPrevention {
     UPKEEP_NOT_NEEDED,
     PERFORM_DATA_EXCEEDS_LIMIT,
     INSUFFICIENT_BALANCE,
-    CALLBACK_REVERTED
+    CALLBACK_REVERTED,
+    REVERT_DATA_EXCEEDS_LIMIT
   }
 
   /**
@@ -190,6 +191,9 @@ abstract contract KeeperRegistryBase2_1 is ConfirmedOwner, ExecutionPrevention {
    * when calculating the payment ceiling for keepers
    * @member minUpkeepSpend minimum LINK that an upkeep must spend before cancelling
    * @member maxPerformGas max executeGas allowed for an upkeep on this registry
+   * @member maxCheckDataSize max length of checkData bytes
+   * @member maxPerformDataSize max length of performData bytes
+   * @member maxRevertDataSize max length of revertData bytes
    * @member fallbackGasPrice gas price used if the gas price feed is stale
    * @member fallbackLinkPrice LINK price used if the LINK price feed is stale
    * @member transcoder address of the transcoder contract
@@ -206,6 +210,7 @@ abstract contract KeeperRegistryBase2_1 is ConfirmedOwner, ExecutionPrevention {
     uint32 maxPerformGas;
     uint32 maxCheckDataSize;
     uint32 maxPerformDataSize;
+    uint32 maxRevertDataSize;
     uint256 fallbackGasPrice;
     uint256 fallbackLinkPrice;
     address transcoder;
@@ -322,8 +327,9 @@ abstract contract KeeperRegistryBase2_1 is ConfirmedOwner, ExecutionPrevention {
     // 2 EVM word full
     uint32 maxCheckDataSize; // max length of checkData bytes
     uint32 maxPerformDataSize; // max length of performData bytes
-    // 4 bytes to 3rd EVM word
+    uint32 maxRevertDataSize; // max length of revertData bytes
     address upkeepPrivilegeManager; // address which can set privilege for upkeeps
+    // 3 EVM word full
   }
 
   // Report transmitted by OCR to transmit function

diff --git a/contracts/src/v0.8/dev/automation/2_1/KeeperRegistryLogicA2_1.sol b/contracts/src/v0.8/dev/automation/2_1/KeeperRegistryLogicA2_1.sol
@@ -104,33 +104,52 @@ contract KeeperRegistryLogicA2_1 is
       return (false, bytes(""), UpkeepFailureReason.INSUFFICIENT_BALANCE, 0, upkeep.executeGas, 0, 0);
     }
 
-    upkeepNeeded = true;
-
     bytes memory callData;
     if (triggerType == Trigger.CONDITION) {
       callData = abi.encodeWithSelector(CHECK_SELECTOR, checkData);
     } else {
       callData = abi.encodeWithSelector(CHECK_LOG_SELECTOR, checkData);
     }
     gasUsed = gasleft();
-    (upkeepNeeded, performData) = upkeep.target.call{gas: s_storage.checkGasLimit}(callData);
+    (bool success, bytes memory result) = upkeep.target.call{gas: s_storage.checkGasLimit}(callData);
     gasUsed = gasUsed - gasleft();
-    if (!upkeepNeeded) {
-      upkeepFailureReason = UpkeepFailureReason.TARGET_CHECK_REVERTED;
-    } else {
-      (upkeepNeeded, performData) = abi.decode(performData, (bool, bytes));
-      if (!upkeepNeeded)
+
+    if (!success) {
+      // User's target check reverted. We capture the revert data here and pass it within performData
+      if (result.length > s_storage.maxRevertDataSize) {
         return (
           false,
           bytes(""),
-          UpkeepFailureReason.UPKEEP_NOT_NEEDED,
+          UpkeepFailureReason.REVERT_DATA_EXCEEDS_LIMIT,
           gasUsed,
           upkeep.executeGas,
           fastGasWei,
           linkNative
         );
+      }
+      return (
+        upkeepNeeded,
+        result,
+        UpkeepFailureReason.TARGET_CHECK_REVERTED,
+        gasUsed,
+        upkeep.executeGas,
+        fastGasWei,
+        linkNative
+      );
     }
 
+    (upkeepNeeded, performData) = abi.decode(result, (bool, bytes));
+    if (!upkeepNeeded)
+      return (
+        false,
+        bytes(""),
+        UpkeepFailureReason.UPKEEP_NOT_NEEDED,
+        gasUsed,
+        upkeep.executeGas,
+        fastGasWei,
+        linkNative
+      );
+
     if (performData.length > s_storage.maxPerformDataSize)
       return (
         false,
@@ -180,13 +199,17 @@ contract KeeperRegistryLogicA2_1 is
     gasUsed = gasUsed - gasleft();
 
     if (!success) {
-      upkeepFailureReason = UpkeepFailureReason.CALLBACK_REVERTED;
-    } else {
-      (upkeepNeeded, performData) = abi.decode(result, (bool, bytes));
+      return (false, bytes(""), UpkeepFailureReason.CALLBACK_REVERTED, gasUsed);
     }
+    (upkeepNeeded, performData) = abi.decode(result, (bool, bytes));
+
     if (!upkeepNeeded) {
-      upkeepFailureReason = UpkeepFailureReason.UPKEEP_NOT_NEEDED;
+      return (false, bytes(""), UpkeepFailureReason.UPKEEP_NOT_NEEDED, gasUsed);
+    }
+    if (performData.length > s_storage.maxPerformDataSize) {
+      return (false, bytes(""), UpkeepFailureReason.PERFORM_DATA_EXCEEDS_LIMIT, gasUsed);
     }
+
     return (upkeepNeeded, performData, upkeepFailureReason, gasUsed);
   }
 

diff --git a/contracts/src/v0.8/dev/automation/2_1/KeeperRegistryLogicB2_1.sol b/contracts/src/v0.8/dev/automation/2_1/KeeperRegistryLogicB2_1.sol
@@ -348,6 +348,7 @@ contract KeeperRegistryLogicB2_1 is KeeperRegistryBase2_1 {
       maxPerformGas: s_storage.maxPerformGas,
       maxCheckDataSize: s_storage.maxCheckDataSize,
       maxPerformDataSize: s_storage.maxPerformDataSize,
+      maxRevertDataSize: s_storage.maxRevertDataSize,
       fallbackGasPrice: s_fallbackGasPrice,
       fallbackLinkPrice: s_fallbackLinkPrice,
       transcoder: s_storage.transcoder,

diff --git a/contracts/src/v0.8/dev/automation/2_1/interfaces/IKeeperRegistryMaster.sol b/contracts/src/v0.8/dev/automation/2_1/interfaces/IKeeperRegistryMaster.sol
diff --git a/contracts/src/v0.8/tests/MercuryUpkeep.sol b/contracts/src/v0.8/tests/MercuryUpkeep.sol
@@ -36,6 +36,8 @@ contract MercuryUpkeep is AutomationCompatibleInterface, FeedLookupCompatibleInt
   string public feedParamKey;
   string public timeParamKey;
   bool public immutable useL1BlockNumber;
+  bool public shouldRevertCallback;
+  bool public callbackReturnBool;
 
   constructor(uint256 _testRange, uint256 _interval, bool _useL1BlockNumber) {
     testRange = _testRange;
@@ -50,12 +52,22 @@ contract MercuryUpkeep is AutomationCompatibleInterface, FeedLookupCompatibleInt
     ];
     timeParamKey = "blockNumber"; // timestamp not supported yet
     useL1BlockNumber = _useL1BlockNumber;
+    callbackReturnBool = true;
   }
 
-  function checkCallback(bytes[] memory values, bytes memory extraData) external pure returns (bool, bytes memory) {
+  function setShouldRevertCallback(bool value) public {
+    shouldRevertCallback = value;
+  }
+
+  function setCallbackReturnBool(bool value) public {
+    callbackReturnBool = value;
+  }
+
+  function checkCallback(bytes[] memory values, bytes memory extraData) external view returns (bool, bytes memory) {
+    require(!shouldRevertCallback, "shouldRevertCallback is true");
     // do sth about the chainlinkBlob data in values and extraData
     bytes memory performData = abi.encode(values, extraData);
-    return (true, performData);
+    return (callbackReturnBool, performData);
   }
 
   function checkUpkeep(bytes calldata data) external view returns (bool, bytes memory) {