From af06fdcfd470f21ece991e4c6d405b432e4c42c1 Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 14 Sep 2023 17:32:47 -0300 Subject: [PATCH] Fix various documentation errors (#4601) --- contracts/access/manager/AccessManager.sol | 4 ++++ contracts/governance/Governor.sol | 10 +++++----- contracts/governance/TimelockController.sol | 2 +- .../governance/extensions/GovernorSettings.sol | 1 - .../extensions/GovernorTimelockControl.sol | 5 ++++- .../extensions/GovernorVotesQuorumFraction.sol | 1 - contracts/proxy/utils/Initializable.sol | 2 +- contracts/utils/cryptography/MerkleProof.sol | 6 ++++++ contracts/utils/structs/DoubleEndedQueue.sol | 13 +++++++------ 9 files changed, 28 insertions(+), 16 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 783ebecc5..e83b8a4da 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -559,6 +559,10 @@ contract AccessManager is Context, Multicall, IAccessManager { * scheduled operation from other occurrences of the same `operationId` in invocations of {execute} and {cancel}. * * Emits a {OperationScheduled} event. + * + * NOTE: It is not possible to concurrently schedule more than one operation with the same `target` and `data`. If + * this is necessary, a random byte can be appended to `data` to act as a salt that will be ignored by the target + * contract if it is using standard Solidity ABI encoding. */ function schedule( address target, diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 910f2e0bf..c2a60ce73 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -48,9 +48,9 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 mapping(uint256 proposalId => ProposalCore) private _proposals; - // This queue keeps track of the governor operating on itself. Calls to functions protected by the - // {onlyGovernance} modifier needs to be whitelisted in this queue. Whitelisting is set in {_beforeExecute}, - // consumed by the {onlyGovernance} modifier and eventually reset in {_afterExecute}. This ensures that the + // This queue keeps track of the governor operating on itself. Calls to functions protected by the {onlyGovernance} + // modifier needs to be whitelisted in this queue. Whitelisting is set in {execute}, consumed by the + // {onlyGovernance} modifier and eventually reset after {_executeOperations} completes. This ensures that the // execution of {onlyGovernance} protected calls can only be achieved through successful proposals. DoubleEndedQueue.Bytes32Deque private _governanceCall; @@ -135,7 +135,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * @dev See {IGovernor-state}. */ function state(uint256 proposalId) public view virtual returns (ProposalState) { - // ProposalCore is just one slot. We can load it from storage to stack with a single sload + // We read the struct fields into the stack at once so Solidity emits a single SLOAD ProposalCore storage proposal = _proposals[proposalId]; bool proposalExecuted = proposal.executed; bool proposalCanceled = proposal.canceled; @@ -375,7 +375,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * will revert. * * NOTE: Calling this function directly will NOT check the current state of the proposal, or emit the - * `ProposalQueued` event. Queuing a proposal should be done using {queue} or {_queue}. + * `ProposalQueued` event. Queuing a proposal should be done using {queue}. */ function _queueOperations( uint256 /*proposalId*/, diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 1ae4afb24..4439b9b8b 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -165,7 +165,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { /** * @dev Returns whether an id corresponds to a registered operation. This - * includes both Pending, Ready and Done operations. + * includes both Waiting, Ready, and Done operations. */ function isOperation(bytes32 id) public view returns (bool) { return getOperationState(id) != OperationState.Unset; diff --git a/contracts/governance/extensions/GovernorSettings.sol b/contracts/governance/extensions/GovernorSettings.sol index f6aba7d37..3b577aad8 100644 --- a/contracts/governance/extensions/GovernorSettings.sol +++ b/contracts/governance/extensions/GovernorSettings.sol @@ -93,7 +93,6 @@ abstract contract GovernorSettings is Governor { * Emits a {VotingPeriodSet} event. */ function _setVotingPeriod(uint32 newVotingPeriod) internal virtual { - // voting period must be at least one block long if (newVotingPeriod == 0) { revert GovernorInvalidVotingPeriod(0); } diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index e17ea267c..6c08b32ed 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -15,12 +15,15 @@ import {SafeCast} from "../../utils/math/SafeCast.sol"; * * Using this model means the proposal will be operated by the {TimelockController} and not by the {Governor}. Thus, * the assets and permissions must be attached to the {TimelockController}. Any asset sent to the {Governor} will be - * inaccessible. + * inaccessible from a proposal, unless executed via {Governor-relay}. * * WARNING: Setting up the TimelockController to have additional proposers or cancellers besides the governor is very * risky, as it grants them the ability to: 1) execute operations as the timelock, and thus possibly performing * operations or accessing funds that are expected to only be accessible through a vote, and 2) block governance * proposals that have been approved by the voters, effectively executing a Denial of Service attack. + * + * NOTE: `AccessManager` does not support scheduling more than one operation with the same target and calldata at + * the same time. See {AccessManager-schedule} for a workaround. */ abstract contract GovernorTimelockControl is Governor { TimelockController private _timelock; diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 100758e3b..eefe665ab 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -45,7 +45,6 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { * @dev Returns the quorum numerator at a specific timepoint. See {quorumDenominator}. */ function quorumNumerator(uint256 timepoint) public view virtual returns (uint256) { - // If history is empty, fallback to old storage uint256 length = _quorumNumeratorHistory._checkpoints.length; // Optimistic search, check the latest checkpoint diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index eb4ebb8a4..75a018b0c 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -145,7 +145,7 @@ abstract contract Initializable { * Note that versions can jump in increments greater than 1; this implies that if multiple reinitializers coexist in * a contract, executing them in the right order is up to the developer or operator. * - * WARNING: setting the version to 255 will prevent any future reinitialization. + * WARNING: Setting the version to 2**64 - 1 will prevent any future reinitialization. * * Emits an {Initialized} event. */ diff --git a/contracts/utils/cryptography/MerkleProof.sol b/contracts/utils/cryptography/MerkleProof.sol index 155ec81bf..8461e49cc 100644 --- a/contracts/utils/cryptography/MerkleProof.sol +++ b/contracts/utils/cryptography/MerkleProof.sol @@ -211,10 +211,16 @@ library MerkleProof { } } + /** + * @dev Sorts the pair (a, b) and hashes the result. + */ function _hashPair(bytes32 a, bytes32 b) private pure returns (bytes32) { return a < b ? _efficientHash(a, b) : _efficientHash(b, a); } + /** + * @dev Implementation of keccak256(abi.encode(a, b)) that doesn't allocate or expand memory. + */ function _efficientHash(bytes32 a, bytes32 b) private pure returns (bytes32 value) { /// @solidity memory-safe-assembly assembly { diff --git a/contracts/utils/structs/DoubleEndedQueue.sol b/contracts/utils/structs/DoubleEndedQueue.sol index 2369f12f2..294e50a7f 100644 --- a/contracts/utils/structs/DoubleEndedQueue.sol +++ b/contracts/utils/structs/DoubleEndedQueue.sol @@ -31,16 +31,13 @@ library DoubleEndedQueue { error QueueOutOfBounds(); /** - * @dev Indices are signed integers because the queue can grow in any direction. They are 128 bits so begin and end - * are packed in a single storage slot for efficient access. Since the items are added one at a time we can safely - * assume that these 128-bit indices will not overflow, and use unchecked arithmetic. + * @dev Indices are 128 bits so begin and end are packed in a single storage slot for efficient access. * * Struct members have an underscore prefix indicating that they are "private" and should not be read or written to * directly. Use the functions provided below instead. Modifying the struct manually may violate assumptions and * lead to unexpected behavior. * - * Indices are in the range [begin, end) which means the first item is at data[begin] and the last item is at - * data[end - 1]. + * The first item is at data[begin] and the last item is at data[end - 1]. This range can wrap around. */ struct Bytes32Deque { uint128 _begin; @@ -50,6 +47,8 @@ library DoubleEndedQueue { /** * @dev Inserts an item at the end of the queue. + * + * Reverts with {QueueFull} if the queue is full. */ function pushBack(Bytes32Deque storage deque, bytes32 value) internal { unchecked { @@ -63,7 +62,7 @@ library DoubleEndedQueue { /** * @dev Removes the item at the end of the queue and returns it. * - * Reverts with `QueueEmpty` if the queue is empty. + * Reverts with {QueueEmpty} if the queue is empty. */ function popBack(Bytes32Deque storage deque) internal returns (bytes32 value) { unchecked { @@ -78,6 +77,8 @@ library DoubleEndedQueue { /** * @dev Inserts an item at the beginning of the queue. + * + * Reverts with {QueueFull} if the queue is full. */ function pushFront(Bytes32Deque storage deque, bytes32 value) internal { unchecked {