Improve natspec documentation and comments (#4581)

Co-authored-by: Francisco Giordano <fg@frang.io>

contracts/access/manager/AccessManaged.sol

@@ -102,13 +102,13 @@ abstract contract AccessManaged is Context, IAccessManaged {
      * @dev Reverts if the caller is not allowed to call the function identified by a selector.
      */
     function _checkCanCall(address caller, bytes calldata data) internal virtual {
-        (bool allowed, uint32 delay) = AuthorityUtils.canCallWithDelay(
+        (bool immediate, uint32 delay) = AuthorityUtils.canCallWithDelay(
             authority(),
             caller,
             address(this),
             bytes4(data)
         );
-        if (!allowed) {
+        if (!immediate) {
             if (delay > 0) {
                 _consumingSchedule = true;
                 IAccessManager(authority()).consumeScheduledOp(caller, data);

contracts/access/manager/AccessManager.sol

@@ -27,9 +27,9 @@ import {Time} from "../../utils/types/Time.sol";
  *
  * There is a special role defined by default named "public" which all accounts automatically have.
  *
- * Contracts where functions are mapped to roles are said to be in a "custom" mode, but contracts can also be
- * configured in two special modes: 1) the "open" mode, where all functions are allowed to the "public" role, and 2)
- * the "closed" mode, where no function is allowed to any role.
+ * In addition to the access rules defined by each target's functions being assigned to roles, then entire target can
+ * be "closed". This "closed" mode is set/unset by the admin using {setTargetClosed} and can be used to lock a contract
+ * while permissions are being (re-)configured.
  *
  * Since all the permissions of the managed system can be modified by the admins of this instance, it is expected that
  * they will be highly secured (e.g., a multisig or a well-configured DAO).
@@ -51,6 +51,7 @@ import {Time} from "../../utils/types/Time.sol";
 contract AccessManager is Context, Multicall, IAccessManager {
     using Time for *;
 
+    // Structure that stores the details for a target contract.
     struct TargetConfig {
         mapping(bytes4 selector => uint64 roleId) allowedRoles;
         Time.Delay adminDelay;
@@ -59,10 +60,10 @@ contract AccessManager is Context, Multicall, IAccessManager {
 
     // Structure that stores the details for a role/account pair. This structures fit into a single slot.
     struct Access {
-        // Timepoint at which the user gets the permission. If this is either 0, or in the future, the role permission
-        // is not available.
+        // Timepoint at which the user gets the permission. If this is either 0, or in the future, the role
+        // permission is not available.
         uint48 since;
-        // delay for execution. Only applies to restricted() / execute() calls.
+        // Delay for execution. Only applies to restricted() / execute() calls.
         Time.Delay delay;
     }
 
@@ -78,6 +79,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
         Time.Delay grantDelay;
     }
 
+    // Structure that stores the details for a scheduled operation. This structure fits into a single slot.
     struct Schedule {
         uint48 timepoint;
         uint32 nonce;
@@ -454,7 +456,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * - the caller must be a global admin
      *
-     * Emits a {FunctionAllowedRoleUpdated} event per selector
+     * Emits a {TargetFunctionRoleUpdated} event per selector
      */
     function setTargetFunctionRole(
         address target,
@@ -469,7 +471,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
     /**
      * @dev Internal version of {setFunctionAllowedRole} without access control.
      *
-     * Emits a {FunctionAllowedRoleUpdated} event
+     * Emits a {TargetFunctionRoleUpdated} event
      */
     function _setTargetFunctionRole(address target, bytes4 selector, uint64 roleId) internal virtual {
         _targets[target].allowedRoles[selector] = roleId;
@@ -477,22 +479,22 @@ contract AccessManager is Context, Multicall, IAccessManager {
     }
 
     /**
-     * @dev Set the delay for management operations on a given class of contract.
+     * @dev Set the delay for management operations on a given target contract.
      *
      * Requirements:
      *
      * - the caller must be a global admin
      *
-     * Emits a {FunctionAllowedRoleUpdated} event per selector
+     * Emits a {TargetAdminDelayUpdated} event per selector
      */
     function setTargetAdminDelay(address target, uint32 newDelay) public virtual onlyAuthorized {
         _setTargetAdminDelay(target, newDelay);
     }
 
     /**
-     * @dev Internal version of {setClassAdminDelay} without access control.
+     * @dev Internal version of {setTargetAdminDelay} without access control.
      *
-     * Emits a {ClassAdminDelayUpdated} event
+     * Emits a {TargetAdminDelayUpdated} event
      */
     function _setTargetAdminDelay(address target, uint32 newDelay) internal virtual {
         uint48 effect;
@@ -688,7 +690,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Requirements:
      *
-     * - the caller must be the proposer, or a guardian of the targeted function
+     * - the caller must be the proposer, a guardian of the targeted function, or a global admin
      *
      * Emits a {OperationCanceled} event.
      */
@@ -810,6 +812,13 @@ contract AccessManager is Context, Multicall, IAccessManager {
     // =================================================== HELPERS ====================================================
     /**
      * @dev An extended version of {canCall} for internal use that considers restrictions for admin functions.
+     *
+     * Returns:
+     * - bool immediate: whether the operation can be executed immediately (with no delay)
+     * - uint32 delay: the execution delay
+     *
+     * If immediate is true, the delay can be disregarded and the operation can be immediately executed.
+     * If immediate is false, the operation can be executed if and only if delay is greater than 0.
      */
     function _canCallExtended(address caller, address target, bytes calldata data) private view returns (bool, uint32) {
         if (target == address(this)) {

contracts/access/manager/AuthorityUtils.sol

@@ -15,17 +15,17 @@ library AuthorityUtils {
         address caller,
         address target,
         bytes4 selector
-    ) internal view returns (bool allowed, uint32 delay) {
+    ) internal view returns (bool immediate, uint32 delay) {
         (bool success, bytes memory data) = authority.staticcall(
             abi.encodeCall(IAuthority.canCall, (caller, target, selector))
         );
         if (success) {
             if (data.length >= 0x40) {
-                (allowed, delay) = abi.decode(data, (bool, uint32));
+                (immediate, delay) = abi.decode(data, (bool, uint32));
             } else if (data.length >= 0x20) {
-                allowed = abi.decode(data, (bool));
+                immediate = abi.decode(data, (bool));
             }
         }
-        return (allowed, delay);
+        return (immediate, delay);
     }
 }

contracts/governance/extensions/GovernorTimelockCompound.sol

@@ -105,7 +105,7 @@ abstract contract GovernorTimelockCompound is Governor {
     }
 
     /**
-     * @dev Overridden version of the {Governor-_cancel} function to cancel the timelocked proposal if it as already
+     * @dev Overridden version of the {Governor-_cancel} function to cancel the timelocked proposal if it has already
      * been queued.
      */
     function _cancel(

contracts/governance/extensions/GovernorTimelockControl.sol

@@ -111,7 +111,7 @@ abstract contract GovernorTimelockControl is Governor {
     }
 
     /**
-     * @dev Overridden version of the {Governor-_cancel} function to cancel the timelocked proposal if it as already
+     * @dev Overridden version of the {Governor-_cancel} function to cancel the timelocked proposal if it has already
      * been queued.
      */
     // This function can reenter through the external call to the timelock, but we assume the timelock is trusted and

contracts/proxy/utils/Initializable.sol

@@ -95,8 +95,9 @@ abstract contract Initializable {
      * @dev A modifier that defines a protected initializer function that can be invoked at most once. In its scope,
      * `onlyInitializing` functions can be used to initialize parent contracts.
      *
-     * Similar to `reinitializer(1)`, except that functions marked with `initializer` can be nested in the context of a
-     * constructor.
+     * Similar to `reinitializer(1)`, except that in the context of a constructor an `initializer` may be invoked any
+     * number of times. This behavior in the constructor can be useful during testing and is not expected to be used in
+     * production.
      *
      * Emits an {Initialized} event.
      */

contracts/token/ERC721/ERC721.sol

@@ -184,8 +184,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
      * @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in
      * particular (ignoring whether it is owned by `owner`).
      *
-     * WARNING: This function assumes that `owner` is the actual owner of `tokenId` and does not
-     * verify this assumption.
+     * WARNING: This function assumes that `owner` is the actual owner of `tokenId` and does not verify this
+     * assumption.
      */
     function _isAuthorized(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) {
         return
@@ -195,10 +195,11 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
 
     /**
      * @dev Checks if `spender` can operate on `tokenId`, assuming the provided `owner` is the actual owner.
-     * Reverts if `spender` has not approval for all assets of the provided `owner` nor the actual owner approved the `spender` for the specific `tokenId`.
+     * Reverts if `spender` does not have approval from the provided `owner` for the given token or for all its assets
+     * the `spender` for the specific `tokenId`.
      *
-     * WARNING: This function relies on {_isAuthorized}, so it doesn't check whether `owner` is the
-     * actual owner of `tokenId`.
+     * WARNING: This function assumes that `owner` is the actual owner of `tokenId` and does not verify this
+     * assumption.
      */
     function _checkAuthorized(address owner, address spender, uint256 tokenId) internal view virtual {
         if (!_isAuthorized(owner, spender, tokenId)) {

contracts/token/ERC721/extensions/ERC721Consecutive.sol

@@ -19,12 +19,12 @@ import {Checkpoints} from "../../../utils/structs/Checkpoints.sol";
  * Using this extension removes the ability to mint single tokens during contract construction. This ability is
  * regained after construction. During construction, only batch minting is allowed.
  *
- * IMPORTANT: This extension bypasses the hooks {_beforeTokenTransfer} and {_afterTokenTransfer} for tokens minted in
- * batch. The hooks will be only called once per batch, so you should take `batchSize` parameter into consideration
- * when relying on hooks.
+ * IMPORTANT: This extension does not call the {_update} function for tokens minted in batch. Any logic added to this
+ * function through overrides will not be triggered when token are minted in batch. You may want to also override
+ * {_increaseBalance} or {_mintConsecutive} to account for these mints.
  *
- * IMPORTANT: When overriding {_afterTokenTransfer}, be careful about call ordering. {ownerOf} may return invalid
- * values during the {_afterTokenTransfer} execution if the super call is not called first. To be safe, execute the
+ * IMPORTANT: When overriding {_mintConsecutive}, be careful about call ordering. {ownerOf} may return invalid
+ * values during the {_mintConsecutive} execution if the super call is not called first. To be safe, execute the
  * super call before your custom logic.
  */
 abstract contract ERC721Consecutive is IERC2309, ERC721 {

contracts/utils/cryptography/ECDSA.sol

@@ -33,8 +33,11 @@ library ECDSA {
     error ECDSAInvalidSignatureS(bytes32 s);
 
     /**
-     * @dev Returns the address that signed a hashed message (`hash`) with
-     * `signature` or error string. This address can then be used for verification purposes.
+     * @dev Returns the address that signed a hashed message (`hash`) with `signature` or an error. This will not
+     * return address(0) without also returning an error description. Errors are documented using an enum (error type)
+     * and a bytes32 providing additional information about the error.
+     *
+     * If no error is returned, then the address can be used for verification purposes.
      *
      * The `ecrecover` EVM precompile allows for malleable (non-unique) signatures:
      * this function rejects them by requiring the `s` value to be in the lower

contracts/utils/cryptography/MessageHashUtils.sol

@@ -20,7 +20,7 @@ library MessageHashUtils {
      * `"\x19Ethereum Signed Message:\n32"` and hashing the result. It corresponds with the
      * hash signed when using the https://eth.wiki/json-rpc/API#eth_sign[`eth_sign`] JSON-RPC method.
      *
-     * NOTE: The `hash` parameter is intended to be the result of hashing a raw message with
+     * NOTE: The `messageHash` parameter is intended to be the result of hashing a raw message with
      * keccak256, although any bytes32 value can be safely used because the final digest will
      * be re-hashed.
      *

contracts/utils/structs/EnumerableMap.sol

@@ -48,14 +48,10 @@ import {EnumerableSet} from "./EnumerableSet.sol";
 library EnumerableMap {
     using EnumerableSet for EnumerableSet.Bytes32Set;
 
-    // To implement this library for multiple types with as little code
-    // repetition as possible, we write it in terms of a generic Map type with
-    // bytes32 keys and values.
-    // The Map implementation uses private functions, and user-facing
-    // implementations (such as Uint256ToAddressMap) are just wrappers around
-    // the underlying Map.
-    // This means that we can only create new EnumerableMaps for types that fit
-    // in bytes32.
+    // To implement this library for multiple types with as little code repetition as possible, we write it in
+    // terms of a generic Map type with bytes32 keys and values. The Map implementation uses private functions,
+    // and user-facing implementations such as `UintToAddressMap` are just wrappers around the underlying Map.
+    // This means that we can only create new EnumerableMaps for types that fit in bytes32.
 
     /**
      * @dev Query for a nonexistent map key.

scripts/generate/templates/EnumerableMap.js

@@ -57,14 +57,10 @@ import {EnumerableSet} from "./EnumerableSet.sol";
 /* eslint-enable max-len */
 
 const defaultMap = () => `\
-// To implement this library for multiple types with as little code
-// repetition as possible, we write it in terms of a generic Map type with
-// bytes32 keys and values.
-// The Map implementation uses private functions, and user-facing
-// implementations (such as Uint256ToAddressMap) are just wrappers around
-// the underlying Map.
-// This means that we can only create new EnumerableMaps for types that fit
-// in bytes32.
+// To implement this library for multiple types with as little code repetition as possible, we write it in
+// terms of a generic Map type with bytes32 keys and values. The Map implementation uses private functions,
+// and user-facing implementations such as \`UintToAddressMap\` are just wrappers around the underlying Map.
+// This means that we can only create new EnumerableMaps for types that fit in bytes32.
 
 /**
  * @dev Query for a nonexistent map key.