Fix error conditions when warnings occur in validation output (#94)

OpenZeppelin · Jan 24, 2025 · c29dd49 · c29dd49
1 parent 453d9ff
commit c29dd49
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Changelog
 
+## 0.3.8 (2025-01-24)
+
+- Fix error conditions when warnings occur in validation output. ([#94](https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades/pull/94))
+
 ## 0.3.7 (2025-01-13)
 
 - Update documentation links. ([#88](https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades/pull/88))

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@openzeppelin/foundry-upgrades",
-  "version": "0.3.7",
+  "version": "0.3.8",
   "description": "Foundry library for deploying and managing upgradeable contracts",
   "license": "MIT",
   "files": [

diff --git a/src/internal/Core.sol b/src/internal/Core.sol
@@ -345,15 +345,29 @@ library Core {
         string memory stdout = string(result.stdout);
 
         // CLI validate command uses exit code to indicate if the validation passed or failed.
-        // As an extra precaution, we also check stdout for "SUCCESS" to ensure it actually ran.
-        if (result.exitCode == 0 && stdout.toSlice().contains("SUCCESS".toSlice())) {
-            return;
-        } else if (result.stderr.length > 0) {
-            // Validations failed to run
-            revert(string(abi.encodePacked("Failed to run upgrade safety validation: ", string(result.stderr))));
+        if (result.exitCode == 0) {
+            // As an extra precaution, we also check stdout for "SUCCESS" to ensure it actually ran.
+            if (stdout.toSlice().contains("SUCCESS".toSlice())) {
+                if (result.stderr.length > 0) {
+                    // Prints warnings from stderr
+                    console.log(string(result.stderr));
+                }
+                return;
+            } else {
+                revert(string(abi.encodePacked("Failed to run upgrade safety validation: ", stdout)));
+            }
         } else {
-            // Validations ran but some contracts were not upgrade safe
-            revert(string(abi.encodePacked("Upgrade safety validation failed:\n", stdout)));
+            if (stdout.toSlice().contains("FAILED".toSlice())) {
+                if (result.stderr.length > 0) {
+                    // Prints warnings from stderr
+                    console.log(string(result.stderr));
+                }
+                // Validations ran but some contracts were not upgrade safe
+                revert(string(abi.encodePacked("Upgrade safety validation failed:\n", stdout)));
+            } else {
+                // Validations failed to run
+                revert(string(abi.encodePacked("Failed to run upgrade safety validation: ", string(result.stderr))));
+            }
         }
     }
 

diff --git a/test/Upgrades.t.sol b/test/Upgrades.t.sol
@@ -333,6 +333,19 @@ contract UpgradesTest is Test {
             opts
         );
     }
+
+    function testWarningAndError() public {
+        Options memory opts;
+        opts.unsafeAllow = "state-variable-immutable";
+
+        Invoker i = new Invoker();
+        try i.validateImplementation("Validations.sol:HasWarningAndError", opts) {
+            fail();
+        } catch Error(string memory reason) {
+            strings.slice memory slice = reason.toSlice();
+            assertTrue(slice.contains("Use of delegatecall is not allowed".toSlice()));
+        }
+    }
 }
 
 contract Invoker {

diff --git a/test/contracts/Validations.sol b/test/contracts/Validations.sol
@@ -79,3 +79,12 @@ contract NamespacedV2_UpgradesFrom_Ok {
         uint256 c;
     }
 }
+
+contract HasWarningAndError {
+    uint256 immutable x = 1; // allow `state-variable-immutable` using option to turn into a warning
+
+    function unsafe() public {
+        (bool s, ) = msg.sender.delegatecall("");
+        s;
+    }
+}