feat(ctb): Address feedback

packages/contracts-bedrock/src/Safe/LivenessGuard.sol

@@ -81,7 +81,8 @@ contract LivenessGuard is ISemver, BaseGuard {
         // Cache the set of owners prior to execution.
         // This will be used in the checkAfterExecution method.
         address[] memory owners = SAFE.getOwners();
-        for (uint256 i = 0; i < owners.length; i++) {
+        uint256 ownersLength = owners.length;
+        for (uint256 i = 0; i < ownersLength; i++) {
             ownersBefore.add(owners[i]);
         }
 
@@ -133,8 +134,9 @@ contract LivenessGuard is ISemver, BaseGuard {
         }
         // Now iterate over the remaining ownersBefore entries. Any remaining addresses are no longer an owner, so we
         // delete them from the lastLive mapping.
-        for (uint256 j = 0; j < ownersBefore.length(); j++) {
-            address ownerBefore = ownersBefore.at(j);
+        uint256 ownersBeforeLength = ownersBefore.length();
+        for (uint256 i = 0; i < ownersBeforeLength; i++) {
+            address ownerBefore = ownersBefore.at(i);
             delete lastLive[ownerBefore];
         }
     }

packages/contracts-bedrock/test/LivenessGuard.t.sol

@@ -29,6 +29,7 @@ contract LivenessGuard_TestInit is Test, SafeTestTools {
 }
 
 contract LivenessGuard_Constructor_Test is LivenessGuard_TestInit {
+    /// @dev Tests that the constructor correctly sets the current time as the lastLive time for each owner
     function test_constructor_works() external {
         address[] memory owners = safeInstance.owners;
         livenessGuard = new LivenessGuard(safeInstance.safe);

packages/contracts-bedrock/test/LivenessModule.t.sol

@@ -20,6 +20,7 @@ contract OwnerSimulator is OwnerManager {
         setupOwners(_owners, _threshold);
     }
 
+    /// @dev Exposes the OwnerManager's removeOwner function so that anyone may call without needing auth
     function removeOwnerWrapped(address prevOwner, address owner, uint256 _threshold) public {
         OwnerManager(address(this)).removeOwner(prevOwner, owner, _threshold);
     }
@@ -194,9 +195,7 @@ contract LivenessModule_RemoveOwners_TestFail is LivenessModule_TestInit {
         /// Will sign a transaction with the first M owners in the owners list
         vm.warp(block.timestamp + livenessInterval);
         safeInstance.execTransaction({ to: address(1111), value: 0, data: hex"abba" });
-        vm.expectRevert(
-            "LivenessModule: the safe still has sufficient owners, or the owner to remove has signed recently"
-        );
+        vm.expectRevert("LivenessModule: the owner to remove has signed recently");
         _removeAnOwner(safeInstance.owners[0]);
     }
 
@@ -206,9 +205,7 @@ contract LivenessModule_RemoveOwners_TestFail is LivenessModule_TestInit {
         vm.warp(block.timestamp + livenessInterval);
         vm.prank(safeInstance.owners[0]);
         livenessGuard.showLiveness();
-        vm.expectRevert(
-            "LivenessModule: the safe still has sufficient owners, or the owner to remove has signed recently"
-        );
+        vm.expectRevert("LivenessModule: the owner to remove has signed recently");
         _removeAnOwner(safeInstance.owners[0]);
     }
 
@@ -242,8 +239,8 @@ contract LivenessModule_RemoveOwners_TestFail is LivenessModule_TestInit {
         livenessModule.removeOwners(prevOwners, ownersToRemove);
     }
 
-    /// @dev Tests if remove owners reverts if it removes too many owners without swapping to the fallback owner
-    function test_removeOwners_belowMinButNotToFallbackOwner_reverts() external {
+    /// @dev Tests if remove owners reverts if it removes too many owners without removing all of them
+    function test_removeOwners_belowMinButNotEmptied_reverts() external {
         // Remove all but one owner
         uint256 numOwners = safeInstance.owners.length - 1;
 
@@ -254,9 +251,7 @@ contract LivenessModule_RemoveOwners_TestFail is LivenessModule_TestInit {
         address[] memory prevOwners = _getPrevOwners(ownersToRemove);
 
         vm.warp(block.timestamp + livenessInterval + 1);
-        vm.expectRevert(
-            "LivenessModule: Safe must have the minimum number of owners or be owned solely by the fallback owner"
-        );
+        vm.expectRevert("LivenessModule: must transfer ownership to fallback owner");
         livenessModule.removeOwners(prevOwners, ownersToRemove);
     }
 

packages/contracts-bedrock/test/SafeSigners.t.sol

@@ -21,6 +21,7 @@ contract SafeSigners_Test is Test, SafeTestTools {
     /// @dev Maps every key to one of the 4 signatures types.
     ///      This is used in the tests below as a pseudorandom mechanism for determining which
     ///      signature type to use for each key.
+    /// @param _key The key to map to a signature type.
     function sigType(uint256 _key) internal pure returns (SigTypes sigType_) {
         uint256 t = _key % 4;
         sigType_ = SigTypes(t);