safe-tools: Reorder functions in SafeTestLib

Moving the multiple overriden execTransaction calls to the end is more readable.
The other write operations are also now grouped together.

packages/contracts-bedrock/test/safe-tools/SafeTestTools.sol

@@ -151,10 +151,10 @@ library SafeTestLib {
         return sortedPKs;
     }
 
-    /// @dev A wrapper for the full execTransaction method, if no signatures are provided it will
-    ///         generate them for all owners.
-    function execTransaction(
+    /// @dev Sign a transaction as a safe owner with a private key.
+    function signTransaction(
         SafeInstance memory instance,
+        uint256 pk,
         address to,
         uint256 value,
         bytes memory data,
@@ -163,20 +163,16 @@ library SafeTestLib {
         uint256 baseGas,
         uint256 gasPrice,
         address gasToken,
-        address refundReceiver,
-        bytes memory signatures
+        address refundReceiver
     )
         internal
-        returns (bool)
+        view
+        returns (uint8 v, bytes32 r, bytes32 s)
     {
-        if (instance.owners.length == 0) {
-            revert("SAFETEST: Instance not initialized. Call _setupSafe() to initialize a test safe");
-        }
-
-        bytes32 safeTxHash;
+        bytes32 txDataHash;
         {
             uint256 _nonce = instance.safe.nonce();
-            safeTxHash = instance.safe.getTransactionHash({
+            txDataHash = instance.safe.getTransactionHash({
                 to: to,
                 value: value,
                 data: data,
@@ -190,61 +186,47 @@ library SafeTestLib {
             });
         }
 
-        if (signatures.length == 0) {
-            for (uint256 i; i < instance.ownerPKs.length; ++i) {
-                uint256 pk = instance.ownerPKs[i];
-                (uint8 v, bytes32 r, bytes32 s) = Vm(VM_ADDR).sign(pk, safeTxHash);
-                if (isSmartContractPK(pk)) {
-                    v = 0;
-                    address addr = decodeSmartContractWalletAsAddress(pk);
-                    assembly {
-                        r := addr
-                    }
-                    console.logBytes32(r);
-                }
-                signatures = bytes.concat(signatures, abi.encodePacked(r, s, v));
+        (v, r, s) = Vm(VM_ADDR).sign(pk, txDataHash);
+    }
+
+    /// @notice Get the previous owner in the linked list of owners
+    /// @param _owner The owner whose previous owner we want to find
+    /// @param _owners The list of owners
+    function getPrevOwner(address _owner, address[] memory _owners) internal pure returns (address prevOwner_) {
+        for (uint256 i = 0; i < _owners.length; i++) {
+            if (_owners[i] != _owner) continue;
+            if (i == 0) {
+                prevOwner_ = SENTINEL_OWNERS;
+                break;
             }
+            prevOwner_ = _owners[i - 1];
         }
-
-        return instance.safe.execTransaction({
-            to: to,
-            value: value,
-            data: data,
-            operation: operation,
-            safeTxGas: safeTxGas,
-            baseGas: baseGas,
-            gasPrice: gasPrice,
-            gasToken: gasToken,
-            refundReceiver: payable(refundReceiver),
-            signatures: signatures
-        });
     }
 
-    /// @dev Executes either a CALL or DELEGATECALL transaction.
-    function execTransaction(
+    /// @dev Given an array of owners to remove, this function will return an array of the previous owners
+    ///         in the order that they must be provided to the LivenessMoules's removeOwners() function.
+    ///         Because owners are removed one at a time, and not necessarily in order, we need to simulate
+    ///         the owners list after each removal, in order to identify the correct previous owner.
+    /// @param _ownersToRemove The owners to remove
+    /// @return prevOwners_ The previous owners in the linked list
+    function getPrevOwners(
         SafeInstance memory instance,
-        address to,
-        uint256 value,
-        bytes memory data,
-        Enum.Operation operation
+        address[] memory _ownersToRemove
     )
         internal
-        returns (bool)
+        returns (address[] memory prevOwners_)
     {
-        return execTransaction(instance, to, value, data, operation, 0, 0, 0, address(0), address(0), "");
-    }
+        OwnerSimulator ownerSimulator = new OwnerSimulator(instance.owners, 1);
+        prevOwners_ = new address[](_ownersToRemove.length);
+        address[] memory currentOwners;
+        for (uint256 i = 0; i < _ownersToRemove.length; i++) {
+            currentOwners = ownerSimulator.getOwners();
+            prevOwners_[i] = SafeTestLib.getPrevOwner(instance.owners[i], currentOwners);
 
-    /// @dev Executes a CALL transaction.
-    function execTransaction(
-        SafeInstance memory instance,
-        address to,
-        uint256 value,
-        bytes memory data
-    )
-        internal
-        returns (bool)
-    {
-        return execTransaction(instance, to, value, data, Enum.Operation.Call, 0, 0, 0, address(0), address(0), "");
+            // Don't try to remove the last owner
+            if (currentOwners.length == 1) break;
+            ownerSimulator.removeOwnerWrapped(prevOwners_[i], _ownersToRemove[i], 1);
+        }
     }
 
     /// @dev Enables a module on the Safe.
@@ -335,10 +317,16 @@ library SafeTestLib {
         EIP1271Sign(instance, abi.encodePacked(digest));
     }
 
-    /// @dev Sign a transaction as a safe owner with a private key.
-    function signTransaction(
+    /// @dev Increments the nonce of the Safe by sending an empty transaction.
+    function incrementNonce(SafeInstance memory instance) internal returns (uint256 newNonce) {
+        execTransaction(instance, address(0), 0, "", Enum.Operation.Call, 0, 0, 0, address(0), address(0), "");
+        return instance.safe.nonce();
+    }
+
+    /// @dev A wrapper for the full execTransaction method, if no signatures are provided it will
+    ///         generate them for all owners.
+    function execTransaction(
         SafeInstance memory instance,
-        uint256 pk,
         address to,
         uint256 value,
         bytes memory data,
@@ -347,16 +335,20 @@ library SafeTestLib {
         uint256 baseGas,
         uint256 gasPrice,
         address gasToken,
-        address refundReceiver
+        address refundReceiver,
+        bytes memory signatures
     )
         internal
-        view
-        returns (uint8 v, bytes32 r, bytes32 s)
+        returns (bool)
     {
-        bytes32 txDataHash;
+        if (instance.owners.length == 0) {
+            revert("SAFETEST: Instance not initialized. Call _setupSafe() to initialize a test safe");
+        }
+
+        bytes32 safeTxHash;
         {
             uint256 _nonce = instance.safe.nonce();
-            txDataHash = instance.safe.getTransactionHash({
+            safeTxHash = instance.safe.getTransactionHash({
                 to: to,
                 value: value,
                 data: data,
@@ -370,53 +362,61 @@ library SafeTestLib {
             });
         }
 
-        (v, r, s) = Vm(VM_ADDR).sign(pk, txDataHash);
-    }
-
-    /// @dev Increments the nonce of the Safe by sending an empty transaction.
-    function incrementNonce(SafeInstance memory instance) internal returns (uint256 newNonce) {
-        execTransaction(instance, address(0), 0, "", Enum.Operation.Call, 0, 0, 0, address(0), address(0), "");
-        return instance.safe.nonce();
-    }
-
-    /// @notice Get the previous owner in the linked list of owners
-    /// @param _owner The owner whose previous owner we want to find
-    /// @param _owners The list of owners
-    function getPrevOwner(address _owner, address[] memory _owners) internal pure returns (address prevOwner_) {
-        for (uint256 i = 0; i < _owners.length; i++) {
-            if (_owners[i] != _owner) continue;
-            if (i == 0) {
-                prevOwner_ = SENTINEL_OWNERS;
-                break;
+        if (signatures.length == 0) {
+            for (uint256 i; i < instance.ownerPKs.length; ++i) {
+                uint256 pk = instance.ownerPKs[i];
+                (uint8 v, bytes32 r, bytes32 s) = Vm(VM_ADDR).sign(pk, safeTxHash);
+                if (isSmartContractPK(pk)) {
+                    v = 0;
+                    address addr = decodeSmartContractWalletAsAddress(pk);
+                    assembly {
+                        r := addr
+                    }
+                    console.logBytes32(r);
+                }
+                signatures = bytes.concat(signatures, abi.encodePacked(r, s, v));
             }
-            prevOwner_ = _owners[i - 1];
         }
+
+        return instance.safe.execTransaction({
+            to: to,
+            value: value,
+            data: data,
+            operation: operation,
+            safeTxGas: safeTxGas,
+            baseGas: baseGas,
+            gasPrice: gasPrice,
+            gasToken: gasToken,
+            refundReceiver: payable(refundReceiver),
+            signatures: signatures
+        });
     }
 
-    /// @dev Given an array of owners to remove, this function will return an array of the previous owners
-    ///         in the order that they must be provided to the LivenessMoules's removeOwners() function.
-    ///         Because owners are removed one at a time, and not necessarily in order, we need to simulate
-    ///         the owners list after each removal, in order to identify the correct previous owner.
-    /// @param _ownersToRemove The owners to remove
-    /// @return prevOwners_ The previous owners in the linked list
-    function getPrevOwners(
+    /// @dev Executes either a CALL or DELEGATECALL transaction.
+    function execTransaction(
         SafeInstance memory instance,
-        address[] memory _ownersToRemove
+        address to,
+        uint256 value,
+        bytes memory data,
+        Enum.Operation operation
     )
         internal
-        returns (address[] memory prevOwners_)
+        returns (bool)
     {
-        OwnerSimulator ownerSimulator = new OwnerSimulator(instance.owners, 1);
-        prevOwners_ = new address[](_ownersToRemove.length);
-        address[] memory currentOwners;
-        for (uint256 i = 0; i < _ownersToRemove.length; i++) {
-            currentOwners = ownerSimulator.getOwners();
-            prevOwners_[i] = SafeTestLib.getPrevOwner(instance.owners[i], currentOwners);
+        return execTransaction(instance, to, value, data, operation, 0, 0, 0, address(0), address(0), "");
+    }
 
-            // Don't try to remove the last owner
-            if (currentOwners.length == 1) break;
-            ownerSimulator.removeOwnerWrapped(prevOwners_[i], _ownersToRemove[i], 1);
-        }
+    /// @dev Executes a CALL transaction.
+    function execTransaction(
+        SafeInstance memory instance,
+        address to,
+        uint256 value,
+        bytes memory data
+    )
+        internal
+        returns (bool)
+    {
+        return execTransaction(instance, to, value, data, Enum.Operation.Call, 0, 0, 0, address(0), address(0), "");
     }
 }