ensure we make progress even in manual exec (#1053)

We want to make sure that no progress is ever being reverted. This means that manual exec will not revert if the tx was untouched
smartcontractkit · Jun 20, 2024 · cf39242 · cf39242
1 parent a4b16c2
commit cf39242
Show file tree

Hide file tree

Showing 14 changed files with 320 additions and 261 deletions.
diff --git a/contracts/gas-snapshots/ccip.gas-snapshot b/contracts/gas-snapshots/ccip.gas-snapshot
diff --git a/contracts/scripts/native_solc_compile_all_ccip b/contracts/scripts/native_solc_compile_all_ccip
@@ -8,7 +8,7 @@ echo " └───────────────────────
 
 SOLC_VERSION="0.8.24"
 OPTIMIZE_RUNS=26000
-OPTIMIZE_RUNS_OFFRAMP=19500
+OPTIMIZE_RUNS_OFFRAMP=18000
 OPTIMIZE_RUNS_ONRAMP=3600
 
 

diff --git a/contracts/src/v0.8/ccip/offRamp/EVM2EVMMultiOffRamp.sol b/contracts/src/v0.8/ccip/offRamp/EVM2EVMMultiOffRamp.sol
@@ -500,8 +500,12 @@ contract EVM2EVMMultiOffRamp is IAny2EVMMultiOffRamp, ITypeAndVersion, MultiOCR3
       // Since it's hard to estimate whether manual execution will succeed, we
       // revert the entire transaction if it fails. This will show the user if
       // their manual exec will fail before they submit it.
-      if (manualExecution && newState == Internal.MessageExecutionState.FAILURE) {
-        // If manual execution fails, we revert the entire transaction.
+      if (
+        manualExecution && newState == Internal.MessageExecutionState.FAILURE
+          && originalState != Internal.MessageExecutionState.UNTOUCHED
+      ) {
+        // If manual execution fails, we revert the entire transaction, unless the originalState is UNTOUCHED as we
+        // would still be making progress by changing the state from UNTOUCHED to FAILURE.
         revert ExecutionError(message.messageId, returnData);
       }
 

diff --git a/contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol b/contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol
@@ -358,8 +358,12 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
       // Since it's hard to estimate whether manual execution will succeed, we
       // revert the entire transaction if it fails. This will show the user if
       // their manual exec will fail before they submit it.
-      if (manualExecution && newState == Internal.MessageExecutionState.FAILURE) {
-        // If manual execution fails, we revert the entire transaction.
+      if (
+        manualExecution && newState == Internal.MessageExecutionState.FAILURE
+          && originalState != Internal.MessageExecutionState.UNTOUCHED
+      ) {
+        // If manual execution fails, we revert the entire transaction, unless the originalState is UNTOUCHED as we
+        // would still be making progress by changing the state from UNTOUCHED to FAILURE.
         revert ExecutionError(returnData);
       }
 
@@ -427,10 +431,9 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
         // prepended by the 4 bytes of ReceiverError.selector, TokenHandlingError.selector or InvalidPoolAddress.selector.
         // Max length of revert data is Router.MAX_RET_BYTES, max length of err is 4 + Router.MAX_RET_BYTES
         return (Internal.MessageExecutionState.FAILURE, err);
-      } else {
-        // If revert is not caused by CCIP receiver, it is unexpected, bubble up the revert.
-        revert ExecutionError(err);
       }
+      // If revert is not caused by CCIP receiver, it is unexpected, bubble up the revert.
+      revert ExecutionError(err);
     }
     // If message execution succeeded, no CCIP receiver return data is expected, return with empty bytes.
     return (Internal.MessageExecutionState.SUCCESS, "");

diff --git a/contracts/src/v0.8/ccip/test/e2e/End2End.t.sol b/contracts/src/v0.8/ccip/test/e2e/End2End.t.sol
@@ -72,17 +72,17 @@ contract E2E is EVM2EVMOnRampSetup, CommitStoreSetup, EVM2EVMOffRampSetup {
     vm.warp(BLOCK_TIME + 2000);
 
     vm.expectEmit();
-    emit ExecutionStateChanged(
+    emit EVM2EVMOffRamp.ExecutionStateChanged(
       messages[0].sequenceNumber, messages[0].messageId, Internal.MessageExecutionState.SUCCESS, ""
     );
 
     vm.expectEmit();
-    emit ExecutionStateChanged(
+    emit EVM2EVMOffRamp.ExecutionStateChanged(
       messages[1].sequenceNumber, messages[1].messageId, Internal.MessageExecutionState.SUCCESS, ""
     );
 
     vm.expectEmit();
-    emit ExecutionStateChanged(
+    emit EVM2EVMOffRamp.ExecutionStateChanged(
       messages[2].sequenceNumber, messages[2].messageId, Internal.MessageExecutionState.SUCCESS, ""
     );
 

diff --git a/contracts/src/v0.8/ccip/test/offRamp/EVM2EVMMultiOffRamp.t.sol b/contracts/src/v0.8/ccip/test/offRamp/EVM2EVMMultiOffRamp.t.sol
diff --git a/contracts/src/v0.8/ccip/test/offRamp/EVM2EVMMultiOffRampSetup.t.sol b/contracts/src/v0.8/ccip/test/offRamp/EVM2EVMMultiOffRampSetup.t.sol
@@ -326,7 +326,7 @@ contract EVM2EVMMultiOffRampSetup is TokenSetup, PriceRegistrySetup, MultiOCR3Ba
     return message;
   }
 
-  function _generateBasicMessages(
+  function _generateSingleBasicMessage(
     uint64 sourceChainSelector,
     address onRamp
   ) internal view returns (Internal.EVM2EVMMessage[] memory) {