diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index 70b6ecd74..c207c5e51 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/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); diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index e397f5d44..1db16feb1 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/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)) { diff --git a/contracts/access/manager/AuthorityUtils.sol b/contracts/access/manager/AuthorityUtils.sol index 49ba74ea2..caf4ca299 100644 --- a/contracts/access/manager/AuthorityUtils.sol +++ b/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); } } diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index a3d47bea1..701e89287 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/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( diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 34143c898..0a4499737 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/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 diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 7545bcca6..856160b8d 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/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. */ diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index c27172010..bf711689f 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/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)) { diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 1cf88dbf9..d5f995576 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/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 { diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 4e8259472..4a04a40af 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/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 diff --git a/contracts/utils/cryptography/MessageHashUtils.sol b/contracts/utils/cryptography/MessageHashUtils.sol index 7625bab78..0e6d602fe 100644 --- a/contracts/utils/cryptography/MessageHashUtils.sol +++ b/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. * diff --git a/contracts/utils/structs/EnumerableMap.sol b/contracts/utils/structs/EnumerableMap.sol index 030ed1eef..65f9ea26c 100644 --- a/contracts/utils/structs/EnumerableMap.sol +++ b/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. diff --git a/scripts/generate/templates/EnumerableMap.js b/scripts/generate/templates/EnumerableMap.js index 8184bfe18..7dbe6ca7f 100644 --- a/scripts/generate/templates/EnumerableMap.js +++ b/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.