fix(pg): Improve error message when config is not defined

hujw77 · Feb 22, 2024 · 650a858 · 650a858
1 parent ea68964
commit 650a858
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 51 deletions.
diff --git a/.changeset/funny-suits-repair.md b/.changeset/funny-suits-repair.md
@@ -0,0 +1,5 @@
+---
+'@sphinx-labs/contracts': patch
+---
+
+Improve error message when config is not defined
diff --git a/packages/contracts/contracts/foundry/Sphinx.sol b/packages/contracts/contracts/foundry/Sphinx.sol
@@ -178,10 +178,7 @@ abstract contract Sphinx {
         deploymentInfo.moduleAddress = module;
         deploymentInfo.chainId = block.chainid;
         deploymentInfo.blockGasLimit = block.gaslimit;
-        deploymentInfo.safeInitData = sphinxUtils.getGnosisSafeInitializerData(
-            sphinxConfig.owners,
-            sphinxConfig.threshold
-        );
+        deploymentInfo.safeInitData = sphinxUtils.getGnosisSafeInitializerData(sphinxConfig);
         deploymentInfo.newConfig = SphinxConfig({
             projectName: sphinxConfig.projectName,
             owners: sphinxConfig.owners,
@@ -262,10 +259,7 @@ abstract contract Sphinx {
         );
         address singletonAddress = constants.safeSingletonAddress();
 
-        bytes memory safeInitializerData = sphinxUtils.getGnosisSafeInitializerData(
-            sphinxConfig.owners,
-            sphinxConfig.threshold
-        );
+        bytes memory safeInitializerData = sphinxUtils.getGnosisSafeInitializerData(sphinxConfig);
 
         // This is the transaction that deploys the Gnosis Safe, deploys the Sphinx Module,
         // and enables the Sphinx Module in the Gnosis Safe.
@@ -351,7 +345,8 @@ abstract contract Sphinx {
      *         off-chain. We ABI encode the config because it's difficult to decode complex
      *         data types that are returned by invoking Forge scripts.
      */
-    function sphinxConfigABIEncoded() external view returns (bytes memory) {
+    function sphinxConfigABIEncoded() public view returns (bytes memory) {
+        sphinxUtils.validate(sphinxConfig);
         return abi.encode(sphinxConfig, safeAddress(), sphinxModule());
     }
 

diff --git a/packages/contracts/contracts/foundry/SphinxUtils.sol b/packages/contracts/contracts/foundry/SphinxUtils.sol
@@ -526,7 +526,21 @@ contract SphinxUtils is SphinxConstants, StdUtils {
      *         configuration is valid. This validation occurs regardless of the `SphinxMode` (e.g.
      *         proposals, broadcasting, etc).
      */
-    function validate(SphinxConfig memory _config) external pure {
+    function validate(SphinxConfig memory _config) public pure {
+        if (
+            _config.owners.length == 0 &&
+            _config.threshold == 0 &&
+            bytes(_config.projectName).length == 0 &&
+            _config.mainnets.length == 0 &&
+            _config.testnets.length == 0 &&
+            _config.saltNonce == 0 &&
+            bytes(_config.orgId).length == 0
+        ) {
+            revert(
+                "Sphinx: Detected an empty 'sphinxConfig' struct. Did you forget to add fields to it in your script's\nsetUp function or constructor? If you've already added fields to it in your setUp function, have you\ncalled 'super.setUp()' in any contracts that inherit from your script?"
+            );
+        }
+
         require(
             _config.owners.length > 0,
             "Sphinx: You must have at least one owner in your 'sphinxConfig.owners' array before calling this function."
@@ -691,11 +705,7 @@ contract SphinxUtils is SphinxConstants, StdUtils {
     }
 
     function getGnosisSafeProxyAddress(SphinxConfig memory _config) public pure returns (address) {
-        address[] memory owners = _config.owners;
-        uint256 threshold = _config.threshold;
-
-        address[] memory sortedOwners = sortAddresses(owners);
-        bytes memory safeInitializerData = getGnosisSafeInitializerData(sortedOwners, threshold);
+        bytes memory safeInitializerData = getGnosisSafeInitializerData(_config);
         bytes32 salt = keccak256(
             abi.encodePacked(keccak256(safeInitializerData), _config.saltNonce)
         );
@@ -778,21 +788,13 @@ contract SphinxUtils is SphinxConstants, StdUtils {
      *         location.
      */
     function getGnosisSafeInitializerData(
-        address[] memory _owners,
-        uint _threshold
+        SphinxConfig memory _config
     ) public pure returns (bytes memory safeInitializerData) {
-        require(
-            _owners.length > 0,
-            "Sphinx: You must have at least one owner in your 'sphinxConfig.owners' array."
-        );
-        require(
-            _threshold > 0,
-            "Sphinx: You must set your 'sphinxConfig.threshold' to a value greater than 0."
-        );
+        validate(_config);
 
         // Sort the owner addresses. This provides a consistent ordering, which makes it easier
         // to calculate the `CREATE2` address of the Gnosis Safe off-chain.
-        address[] memory sortedOwners = sortAddresses(_owners);
+        address[] memory sortedOwners = sortAddresses(_config.owners);
 
         ISphinxModuleProxyFactory moduleProxyFactory = ISphinxModuleProxyFactory(
             sphinxModuleProxyFactoryAddress
@@ -846,7 +848,7 @@ contract SphinxUtils is SphinxConstants, StdUtils {
             IGnosisSafe.setup.selector,
             abi.encode(
                 sortedOwners,
-                _threshold,
+                _config.threshold,
                 multiSendAddress,
                 multiSendData,
                 // This is the default fallback handler used by Gnosis Safe during their

diff --git a/packages/contracts/test/Sphinx.t.sol b/packages/contracts/test/Sphinx.t.sol
@@ -6,7 +6,7 @@ pragma solidity ^0.8.0;
 // relied on a remapping in the wrong file.
 import "../lib/forge-std/src/Test.sol";
 
-import { Sphinx } from "../contracts/foundry/Sphinx.sol";
+import { Sphinx, Network } from "../contracts/foundry/Sphinx.sol";
 import { IGnosisSafe } from "../contracts/foundry/interfaces/IGnosisSafe.sol";
 import { SystemContractInfo } from "../contracts/foundry/SphinxPluginTypes.sol";
 import { SphinxTestUtils } from "./SphinxTestUtils.sol";

diff --git a/packages/contracts/test/SphinxInitCode.sol b/packages/contracts/test/SphinxInitCode.sol
diff --git a/packages/contracts/test/SphinxTestUtils.sol b/packages/contracts/test/SphinxTestUtils.sol
@@ -205,10 +205,7 @@ contract SphinxTestUtils is SphinxConstants, StdCheatsSafe, SphinxUtils, SphinxI
     ) public returns (IGnosisSafe) {
         IGnosisSafeProxyFactory safeProxyFactory = IGnosisSafeProxyFactory(safeFactoryAddress);
 
-        bytes memory safeInitializerData = getGnosisSafeInitializerData(
-            _config.owners,
-            _config.threshold
-        );
+        bytes memory safeInitializerData = getGnosisSafeInitializerData(_config);
 
         // This is the transaction that deploys the Gnosis Safe, deploys the Sphinx Module,
         // and enables the Sphinx Module in the Gnosis Safe.

diff --git a/packages/contracts/test/SphinxUtils.t.sol b/packages/contracts/test/SphinxUtils.t.sol
@@ -414,6 +414,14 @@ contract SphinxUtils_Test is Test, SphinxUtils {
         assertFalse(vm.keyExists(serialized, ".myKey"));
     }
 
+    function test_validate_revert_empty_config() external {
+        SphinxConfig memory sphinxConfig;
+        vm.expectRevert(
+            "Sphinx: Detected an empty 'sphinxConfig' struct. Did you forget to add fields to it in your script's\nsetUp function or constructor? If you've already added fields to it in your setUp function, have you\ncalled 'super.setUp()' in any contracts that inherit from your script?"
+        );
+        validate(sphinxConfig);
+    }
+
     /////////////////////////////////// Helpers //////////////////////////////////////
 
     function makeAccountAccess(