Update natspec in SaferSafes (#17976)

alcueca · aliersh · web-flow · commit 1f22011b2753 · 2025-10-22T22:15:40.000Z
* Added a flowchart to the liveness module * Added version compatibility notice * patch version bump * Title for the state diagram * Trimmed comments to 100 characters * One extra slash slashed * semver-lock * semver-lock * refactor(prompt): enhance test quality guidance and add commit strategy (#17950) - add fuzz constraint for bounding value amounts to prevent overflow - change version testing from length checks to SemverComp.parse() validation - add version testing example showing wrong vs right approaches - add commit strategy guidance in methodology phases - add commit strategy section in PR submission guidelines * Added a flowchart to the liveness module * Added version compatibility notice * Title for the state diagram * Trimmed comments to 100 characters * One extra slash slashed * semver-lock * Remove the nonce-independent comment * We can't do state changes in `checkTransaction` * Revert "We can't do state changes in `checkTransaction`" This reverts commit ae5f394. --------- Co-authored-by: Ariel Diaz <65925295+aliersh@users.noreply.github.com>
diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json
@@ -208,8 +208,8 @@
     "sourceCodeHash": "0x7fc4789b082bc8ecd29c4c75a06058f0ff0b72f1c1028a42db6f1c35269c8865"
   },
   "src/safe/SaferSafes.sol:SaferSafes": {
-    "initCodeHash": "0x54eb5d9d4dd6c7a6ac5c223c7e166cf30e93e11acb8caefab73eac596dba5af7",
-    "sourceCodeHash": "0xeb7745b3f5626573d1e935affbbb0e6b455e729f1d8b2da84ab0d5a46f848377"
+    "initCodeHash": "0xe8ff14ba3d023064046251967c3f4d0eb7bc3c2eee6519523f1175b37c0f9ac9",
+    "sourceCodeHash": "0x046424a35624e13badd3d3f91993c27d4d6058b4696cba8bdda27b09bc972785"
   },
   "src/universal/OptimismMintableERC20.sol:OptimismMintableERC20": {
     "initCodeHash": "0x3c85eed0d017dca8eda6396aa842ddc12492587b061e8c756a8d32c4610a9658",
diff --git a/packages/contracts-bedrock/src/safe/LivenessModule2.sol b/packages/contracts-bedrock/src/safe/LivenessModule2.sol
@@ -12,9 +12,41 @@ import { GuardManager } from "safe-contracts/base/GuardManager.sol";
 ///         when the Safe becomes unresponsive. The fallback owner can initiate a challenge,
 ///         and if the Safe doesn't respond within the challenge period, ownership transfers
 ///         to the fallback owner.
+///         This contract is compatible only with the Safe contract version 1.4.1.
 /// @dev This is a singleton contract. To use it:
 ///      1. The Safe must first enable this module using ModuleManager.enableModule()
 ///      2. The Safe must then configure the module by calling configure() with params
+///
+///      Follows a state machine diagram for the lifecycle of this contract:
+///      +----------------------+
+///      | Start (no challenge) |<---------------------------+
+///      +----------------------+                            |
+///       |                                                  | respond() by Safe
+///       |  challenge() by fallbackOwner                    | OR
+///       |                                                  | configureLivenessModule() by Safe
+///       v                                                  | OR
+///      +--------------------------------------+            | clearLivenessModule() by Safe
+///      | Challenge Started                    |            |
+///      | challengeStartTime = block.timestamp |------------+
+///      +--------------------------------------+            |
+///       |                                                  |
+///       |  block.timestamp >= challengeStartTime +         |
+///       |                     livenessResponsePeriod       |
+///       v                                                  |
+///      +-----+-----------------------------------------+   |
+///      | Ready to transfer ownership to fallback owner |---+
+///      +-----+-----------------------------------------+
+///       |
+///       |  changeOwnershipToFallback() by fallbackOwner
+///       |
+///       v
+///      +------------------------------+
+///      | Ownership Transferred        |
+///      | - fallback owner sole owner  |
+///      | - guard cleared              |
+///      | - challenge cleared          |
+///      +------------------------------+
+///
 abstract contract LivenessModule2 {
     /// @notice Configuration for a Safe's liveness module.
     /// @custom:field livenessResponsePeriod The duration in seconds that Safe owners have to
@@ -169,8 +201,8 @@ abstract contract LivenessModule2 {
     ///      1. Safe disables the module via ModuleManager.disableModule().
     ///      2. Safe calls this clearLivenessModule() function to remove stored configuration.
     ///      3. If Safe later re-enables the module, it must call configureLivenessModule() again.
-    ///      Never calling clearLivenessModule() after disabling keeps configuration data persistent
-    ///      for potential future re-enabling.
+    ///      Never calling clearLivenessModule() after disabling keeps configuration data
+    ///      persistent for potential future re-enabling.
     function clearLivenessModule() external {
         Safe callingSafe = Safe(payable(msg.sender));
 
@@ -301,8 +333,8 @@ abstract contract LivenessModule2 {
 
         // Disable the guard
         // Note that this will remove whichever guard is currently set on the Safe,
-        // even if it is not the SaferSafes guard. This is intentional, as it is possible that the guard
-        // itself was the cause of the liveness failure which resulted in the transfer of ownership to
+        // even if it is not the SaferSafes guard. This is intentional, as it is possible that the
+        // guard was the cause of the liveness failure which resulted in the transfer of ownership to
         // the fallback owner.
         _safe.execTransactionFromModule({
             to: address(_safe),
@@ -317,8 +349,9 @@ abstract contract LivenessModule2 {
     //                   Internal View Functions                  //
     ////////////////////////////////////////////////////////////////
 
-    /// @notice Internal helper function which can be overriden in a child contract to check if the guard's
-    ///         configuration is valid in the context of other extensions that are enabled on the Safe.
+    /// @notice Internal helper function which can be overriden in a child contract to check if the
+    ///         guard's configuration is valid in the context of other extensions that are enabled
+    ///         on the Safe.
     function _checkCombinedConfig(Safe _safe) internal view virtual;
 
     /// @notice Asserts that the module is configured for the given Safe.
diff --git a/packages/contracts-bedrock/src/safe/SaferSafes.sol b/packages/contracts-bedrock/src/safe/SaferSafes.sol
@@ -10,30 +10,35 @@ import { TimelockGuard } from "./TimelockGuard.sol";
 import { ISemver } from "interfaces/universal/ISemver.sol";
 
 /// @title SaferSafes
-/// @notice Combined Safe extensions providing both liveness module and timelock guard functionality
+/// @notice Combined Safe extensions providing both liveness module and timelock guard
+///         functionality
+///         This contract is compatible only with the Safe contract version 1.4.1.
 /// @dev This contract can be enabled simultaneously as both a module and a guard on a Safe:
 ///      - As a module: provides liveness challenge functionality to prevent multisig deadlock
 ///      - As a guard: provides timelock functionality for transaction delays and cancellation
-///      The two components in this contract are almost entirely independent of each other, and can be treated as
-///      separate extensions to the Safe. The only shared logic is the _checkCombinedConfig which runs at the end of the
-///      configuration functions for both components and ensures that the resulting configuration is valid.
-///      Either component can be enabled or disabled independently of the other.
-///      When installing either component, it should first be enabled, and then configured. If a component's
-///      functionality is not desired, then there is no need to enable or configure it.
+///      The two components in this contract are almost entirely independent of each other, and can
+///      be treated as separate extensions to the Safe. The only shared logic is the
+///      _checkCombinedConfig which runs at the end of the configuration functions for both
+///      components and ensures that the resulting configuration is valid. Either component can be
+///      enabled or disabled independently of the other. When installing either component, it
+///      should first be enabled, and then configured. If a component's functionality is not
+///      desired, then there is no need to enable or configure it.
 contract SaferSafes is LivenessModule2, TimelockGuard, ISemver {
     /// @notice Semantic version.
-    /// @custom:semver 1.5.0
-    string public constant version = "1.5.0";
+    /// @custom:semver 1.5.2
+    string public constant version = "1.5.2";
 
     /// @notice Error for when the liveness response period is insufficient.
     error SaferSafes_InsufficientLivenessResponsePeriod();
 
-    /// @notice Internal helper function which can be overriden in a child contract to check if the guard's
-    ///         configuration is valid in the context of other extensions that are enabled on the Safe.
-    ///         This function acts as a FREI-PI invariant check to ensure the resulting config is valid, it MUST be
-    ///         called at the end of any configuration functions in the parent contract.
+    /// @notice Internal helper function which can be overriden in a child contract to check if the
+    ///         guard's configuration is valid in the context of other extensions that are enabled
+    ///         on the Safe. This function acts as a FREI-PI invariant check to ensure the
+    ///         resulting config is valid, it MUST be called at the end of any configuration
+    ///         functions in the parent contract.
     function _checkCombinedConfig(Safe _safe) internal view override(LivenessModule2, TimelockGuard) {
-        // We only need to perform this check if both the guard and the module are enabled on the Safe
+        // We only need to perform this check if both the guard and the module are enabled on the
+        // Safe
         if (!(_isGuardEnabled(_safe) && _safe.isModuleEnabled(address(this)))) {
             return;
         }
@@ -47,15 +52,16 @@ contract SaferSafes is LivenessModule2, TimelockGuard, ISemver {
             return;
         }
 
-        // If the liveness response period is 0, then the liveness module is enabled but not configured.
+        // If the liveness response period is 0, then the liveness module is enabled but not
+        // configured.
         // Challenging is not possible, so we don't need to perform any further checks.
         if (livenessResponsePeriod == 0) {
             return;
         }
 
-        // The liveness response period must be at least twice the timelock delay, this is necessary to prevent a
-        // situation in which a Safe is not able to respond because there is insufficient time to respond to a challenge
-        // after the timelock delay has expired.
+        // The liveness response period must be at least twice the timelock delay, this is
+        // necessary to prevent a situation in which a Safe is not able to respond because there is
+        // insufficient time to respond to a challenge after the timelock delay has expired.
         if (livenessResponsePeriod < 2 * timelockDelay) {
             revert SaferSafes_InsufficientLivenessResponsePeriod();
         }
diff --git a/packages/contracts-bedrock/src/safe/TimelockGuard.sol b/packages/contracts-bedrock/src/safe/TimelockGuard.sol
