From ce992852a18337f55f5a2b7488e2cc8d2c979146 Mon Sep 17 00:00:00 2001 From: Rekard0 <5880388+Rekard0@users.noreply.github.com> Date: Thu, 14 Nov 2024 11:52:00 +0100 Subject: [PATCH] Feat/update natspec and doc (#115) * update natspecs * fix: update auth natspects * fix: natspec ident * small fix --------- Co-authored-by: Giorgi Lagidze --- contracts/src/dao/IDAO.sol | 2 +- contracts/src/executors/Executor.sol | 19 +++++++++++++++---- contracts/src/executors/IExecutor.sol | 5 +++-- contracts/src/permission/auth/auth.sol | 6 +++++- contracts/src/plugin/IPlugin.sol | 10 +++++++++- contracts/src/plugin/Plugin.sol | 13 +++++++++---- contracts/src/plugin/PluginCloneable.sol | 13 +++++++++---- .../src/plugin/PluginUUPSUpgradeable.sol | 13 +++++++++---- .../plugin/extensions/proposal/IProposal.sol | 12 +++++++----- .../plugin/extensions/proposal/Proposal.sol | 11 ++++++----- .../proposal/ProposalUpgradeable.sol | 15 +++++++++------ .../src/utils/metadata/MetadataExtension.sol | 2 ++ .../metadata/MetadataExtensionUpgradeable.sol | 5 +++-- 13 files changed, 87 insertions(+), 39 deletions(-) diff --git a/contracts/src/dao/IDAO.sol b/contracts/src/dao/IDAO.sol index 133970a8..27efdb10 100644 --- a/contracts/src/dao/IDAO.sol +++ b/contracts/src/dao/IDAO.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.8; /// @title IDAO -/// @author Aragon X - 2022-2023 +/// @author Aragon X - 2022-2024 /// @notice The interface required for DAOs within the Aragon App DAO framework. /// @custom:security-contact sirt@aragon.org interface IDAO { diff --git a/contracts/src/executors/Executor.sol b/contracts/src/executors/Executor.sol index 21c97a31..52e62884 100644 --- a/contracts/src/executors/Executor.sol +++ b/contracts/src/executors/Executor.sol @@ -7,11 +7,14 @@ import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; import {IExecutor, Action} from "./IExecutor.sol"; import {flipBit, hasBit} from "../utils/math/BitMap.sol"; +/// @title IDAO +/// @author Aragon X - 2024 /// @notice Simple Executor that loops through the actions and executes them. /// @dev This doesn't use any type of permission for execution and can be called by anyone. -/// Most useful use-case is to deploy as non-upgradeable and call from another contract via delegatecall. -/// If used with delegatecall, DO NOT add state variables in sequential slots, otherwise this will overwrite -/// the storage of the calling contract. +/// Most useful use-case is to deploy it as non-upgradeable and call from another contract via delegatecall. +/// If used with delegatecall, DO NOT add state variables in sequential slots, otherwise this will overwrite +/// the storage of the calling contract. +/// @custom:security-contact sirt@aragon.org contract Executor is IExecutor, ERC165 { /// @notice The internal constant storing the maximal action array length. uint256 internal constant MAX_ACTIONS = 256; @@ -39,10 +42,15 @@ contract Executor is IExecutor, ERC165 { /// @notice Thrown if a call is reentrant. error ReentrantCall(); + /// @notice Initializes the contract with a non-entered reentrancy status. + /// @dev Sets the reentrancy guard status to `_NOT_ENTERED` to prevent reentrant calls from the start. constructor() { _storeReentrancyStatus(_NOT_ENTERED); } + /// @notice Prevents reentrant calls to a function. + /// @dev This modifier checks the reentrancy status before function execution. If already entered, it reverts with + /// `ReentrantCall()`. Sets the status to `_ENTERED` during execution and resets it to `_NOT_ENTERED` afterward. modifier nonReentrant() { if (_getReentrancyStatus() == _ENTERED) { revert ReentrantCall(); @@ -135,7 +143,10 @@ contract Executor is IExecutor, ERC165 { } } - /// @notice Stores the reentrancy status on a specific slot. + /// @notice Stores the reentrancy status at a specific storage slot. + /// @param _status The reentrancy status to be stored, typically `_ENTERED` or `_NOT_ENTERED`. + /// @dev Uses inline assembly to store the `_status` value at `REENTRANCY_GUARD_STORAGE_LOCATION` to manage + /// reentrancy status efficiently. function _storeReentrancyStatus(uint256 _status) private { // solhint-disable-next-line no-inline-assembly assembly { diff --git a/contracts/src/executors/IExecutor.sol b/contracts/src/executors/IExecutor.sol index c6b26896..760f3515 100644 --- a/contracts/src/executors/IExecutor.sol +++ b/contracts/src/executors/IExecutor.sol @@ -13,18 +13,19 @@ struct Action { } /// @title IExecutor -/// @author Aragon X - 2022-2024 +/// @author Aragon X - 2024 /// @notice The interface required for Executors within the Aragon App DAO framework. /// @custom:security-contact sirt@aragon.org interface IExecutor { /// @notice Emitted when a proposal is executed. + /// @dev The value of `callId` is defined by the component/contract calling the execute function. + /// A `Plugin` implementation can use it, for example, as a nonce. /// @param actor The address of the caller. /// @param callId The ID of the call. /// @param actions The array of actions executed. /// @param allowFailureMap The allow failure map encoding which actions are allowed to fail. /// @param failureMap The failure map encoding which actions have failed. /// @param execResults The array with the results of the executed actions. - /// @dev The value of `callId` is defined by the component/contract calling the execute function. A `Plugin` implementation can use it, for example, as a nonce. event Executed( address indexed actor, bytes32 callId, diff --git a/contracts/src/permission/auth/auth.sol b/contracts/src/permission/auth/auth.sol index 744e3c45..625b5077 100644 --- a/contracts/src/permission/auth/auth.sol +++ b/contracts/src/permission/auth/auth.sol @@ -4,6 +4,11 @@ pragma solidity ^0.8.8; import {IDAO} from "../../dao/IDAO.sol"; +/// @title DAO Authorization Utilities +/// @author Aragon X - 2022-2024 +/// @notice Provides utility functions for verifying if a caller has specific permissions in an associated DAO. +/// @custom:security-contact sirt@aragon.org + /// @notice Thrown if a call is unauthorized in the associated DAO. /// @param dao The associated DAO. /// @param where The context in which the authorization reverted. @@ -16,7 +21,6 @@ error DaoUnauthorized(address dao, address where, address who, bytes32 permissio /// @param _who The address (EOA or contract) owning the permission. /// @param _permissionId The permission identifier. /// @param _data The optional data passed to the `PermissionCondition` registered. -/// @custom:security-contact sirt@aragon.org function _auth( IDAO _dao, address _where, diff --git a/contracts/src/plugin/IPlugin.sol b/contracts/src/plugin/IPlugin.sol index 202e8e4b..22d29280 100644 --- a/contracts/src/plugin/IPlugin.sol +++ b/contracts/src/plugin/IPlugin.sol @@ -3,21 +3,29 @@ pragma solidity ^0.8.8; /// @title IPlugin -/// @author Aragon X - 2022-2023 +/// @author Aragon X - 2022-2024 /// @notice An interface defining the traits of a plugin. /// @custom:security-contact sirt@aragon.org interface IPlugin { + /// @notice Types of plugin implementations available within OSx. enum PluginType { UUPS, Cloneable, Constructable } + /// @notice Specifies the type of operation to perform. enum Operation { Call, DelegateCall } + /// @notice Configuration for the target contract that the plugin will interact with, including the address and operation type. + /// @dev By default, the plugin typically targets the associated DAO and performs a `Call` operation. However, this + /// configuration allows the plugin to specify a custom executor and select either `Call` or `DelegateCall` based on + /// the desired execution context. + /// @param target The address of the target contract, typically the associated DAO but configurable to a custom executor. + /// @param operation The type of operation (`Call` or `DelegateCall`) to execute on the target, as defined by `Operation`. struct TargetConfig { address target; Operation operation; diff --git a/contracts/src/plugin/Plugin.sol b/contracts/src/plugin/Plugin.sol index 0aaead0a..49bb2ea0 100644 --- a/contracts/src/plugin/Plugin.sol +++ b/contracts/src/plugin/Plugin.sol @@ -13,19 +13,20 @@ import {IPlugin} from "./IPlugin.sol"; import {IExecutor, Action} from "../executors/IExecutor.sol"; /// @title Plugin -/// @author Aragon X - 2022-2023 +/// @author Aragon X - 2022-2024 /// @notice An abstract, non-upgradeable contract to inherit from when creating a plugin being deployed via the `new` keyword. /// @custom:security-contact sirt@aragon.org abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion { using ERC165Checker for address; + /// @notice Stores the current target configuration, defining the target contract and operation type for a plugin. TargetConfig private currentTargetConfig; /// @notice Thrown when target is of type 'IDAO', but operation is `delegateCall`. /// @param targetConfig The target config to update it to. error InvalidTargetConfig(TargetConfig targetConfig); - /// @dev Emitted each time the TargetConfig is set. + /// @notice Emitted each time the TargetConfig is set. event TargetSet(TargetConfig newTargetConfig); /// @notice Thrown when `delegatecall` fails. @@ -62,6 +63,7 @@ abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion { } /// @dev Sets the target to a new target (`newTarget`). + /// The caller must have the `SET_TARGET_PERMISSION_ID` permission. /// @param _targetConfig The target Config containing the address and operation type. function setTargetConfig( TargetConfig calldata _targetConfig @@ -125,10 +127,13 @@ abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion { } /// @notice Forwards the actions to the `target` for the execution. - /// @param _target Forwards the actions to the specific target. + /// @param _target The address of the target contract. /// @param _callId Identifier for this execution. /// @param _actions actions that will be eventually called. - /// @param _allowFailureMap Bitmap-encoded number. TODO: + /// @param _allowFailureMap A bitmap allowing the execution to succeed, even if individual actions might revert. + /// If the bit at index `i` is 1, the execution succeeds even if the `i`th action reverts. + /// A failure map value of 0 requires every action to not revert. + /// @param _op The type of operation (`Call` or `DelegateCall`) to be used for the execution. /// @return execResults address of the implementation contract. /// @return failureMap address of the implementation contract. function _execute( diff --git a/contracts/src/plugin/PluginCloneable.sol b/contracts/src/plugin/PluginCloneable.sol index f9fb90ea..58fbbe1d 100644 --- a/contracts/src/plugin/PluginCloneable.sol +++ b/contracts/src/plugin/PluginCloneable.sol @@ -13,7 +13,7 @@ import {IPlugin} from "./IPlugin.sol"; import {IExecutor, Action} from "../executors/IExecutor.sol"; /// @title PluginCloneable -/// @author Aragon X - 2022-2023 +/// @author Aragon X - 2022-2024 /// @notice An abstract, non-upgradeable contract to inherit from when creating a plugin being deployed via the minimal clones pattern (see [ERC-1167](https://eips.ethereum.org/EIPS/eip-1167)). /// @custom:security-contact sirt@aragon.org abstract contract PluginCloneable is @@ -24,6 +24,7 @@ abstract contract PluginCloneable is { using ERC165CheckerUpgradeable for address; + /// @notice Stores the current target configuration, defining the target contract and operation type for a plugin. TargetConfig private currentTargetConfig; /// @notice Thrown when target is of type 'IDAO', but operation is `delegateCall`. @@ -33,7 +34,7 @@ abstract contract PluginCloneable is /// @notice Thrown when `delegatecall` fails. error DelegateCallFailed(); - /// @dev Emitted each time the TargetConfig is set. + /// @notice Emitted each time the TargetConfig is set. event TargetSet(TargetConfig newTargetConfig); /// @notice The ID of the permission required to call the `setTargetConfig` function. @@ -54,6 +55,7 @@ abstract contract PluginCloneable is } /// @dev Sets the target to a new target (`newTarget`). + /// The caller must have the `SET_TARGET_CONFIG_PERMISSION_ID` permission. /// @param _targetConfig The target Config containing the address and operation type. function setTargetConfig( TargetConfig calldata _targetConfig @@ -140,10 +142,13 @@ abstract contract PluginCloneable is } /// @notice Forwards the actions to the `target` for the execution. - /// @param _target Forwards the actions to the specific target. + /// @param _target The address of the target contract. /// @param _callId Identifier for this execution. /// @param _actions actions that will be eventually called. - /// @param _allowFailureMap Bitmap-encoded number. TODO: + /// @param _allowFailureMap A bitmap allowing the execution to succeed, even if individual actions might revert. + /// If the bit at index `i` is 1, the execution succeeds even if the `i`th action reverts. + /// A failure map value of 0 requires every action to not revert. + /// @param _op The type of operation (`Call` or `DelegateCall`) to be used for the execution. /// @return execResults address of the implementation contract. /// @return failureMap address of the implementation contract. function _execute( diff --git a/contracts/src/plugin/PluginUUPSUpgradeable.sol b/contracts/src/plugin/PluginUUPSUpgradeable.sol index e55f029e..8313b91c 100644 --- a/contracts/src/plugin/PluginUUPSUpgradeable.sol +++ b/contracts/src/plugin/PluginUUPSUpgradeable.sol @@ -15,7 +15,7 @@ import {IDAO} from "../dao/IDAO.sol"; import {IExecutor, Action} from "../executors/IExecutor.sol"; /// @title PluginUUPSUpgradeable -/// @author Aragon X - 2022-2023 +/// @author Aragon X - 2022-2024 /// @notice An abstract, upgradeable contract to inherit from when creating a plugin being deployed via the UUPS pattern (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)). /// @custom:security-contact sirt@aragon.org abstract contract PluginUUPSUpgradeable is @@ -29,6 +29,7 @@ abstract contract PluginUUPSUpgradeable is // NOTE: When adding new state variables to the contract, the size of `_gap` has to be adapted below as well. + /// @notice Stores the current target configuration, defining the target contract and operation type for a plugin. TargetConfig private currentTargetConfig; /// @notice Thrown when target is of type 'IDAO', but operation is `delegateCall`. @@ -41,7 +42,7 @@ abstract contract PluginUUPSUpgradeable is /// @notice Thrown when initialize is called after it has already been executed. error AlreadyInitialized(); - /// @dev Emitted each time the TargetConfig is set. + /// @notice Emitted each time the TargetConfig is set. event TargetSet(TargetConfig newTargetConfig); /// @notice The ID of the permission required to call the `setTargetConfig` function. @@ -97,6 +98,7 @@ abstract contract PluginUUPSUpgradeable is } /// @dev Sets the target to a new target (`newTarget`). + /// The caller must have the `SET_TARGET_CONFIG_PERMISSION_ID` permission. /// @param _targetConfig The target Config containing the address and operation type. function setTargetConfig( TargetConfig calldata _targetConfig @@ -167,10 +169,13 @@ abstract contract PluginUUPSUpgradeable is } /// @notice Forwards the actions to the `target` for the execution. - /// @param _target Forwards the actions to the specific target. + /// @param _target The address of the target contract. /// @param _callId Identifier for this execution. /// @param _actions actions that will be eventually called. - /// @param _allowFailureMap Bitmap-encoded number. TODO: + /// @param _allowFailureMap A bitmap allowing the execution to succeed, even if individual actions might revert. + /// If the bit at index `i` is 1, the execution succeeds even if the `i`th action reverts. + /// A failure map value of 0 requires every action to not revert. + /// @param _op The type of operation (`Call` or `DelegateCall`) to be used for the execution. /// @return execResults address of the implementation contract. /// @return failureMap address of the implementation contract. function _execute( diff --git a/contracts/src/plugin/extensions/proposal/IProposal.sol b/contracts/src/plugin/extensions/proposal/IProposal.sol index 1a7126cf..f0d20339 100644 --- a/contracts/src/plugin/extensions/proposal/IProposal.sol +++ b/contracts/src/plugin/extensions/proposal/IProposal.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.8; import {Action} from "../../../executors/IExecutor.sol"; /// @title IProposal -/// @author Aragon X - 2022-2023 +/// @author Aragon X - 2022-2024 /// @notice An interface to be implemented by DAO plugins that create and execute proposals. /// @custom:security-contact sirt@aragon.org interface IProposal { @@ -16,7 +16,9 @@ interface IProposal { /// @param endDate The end date of the proposal in seconds. /// @param metadata The metadata of the proposal. /// @param actions The actions that will be executed if the proposal passes. - /// @param allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. + /// @param allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. + /// If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. + /// A failure map value of 0 requires every action to not revert. event ProposalCreated( uint256 indexed proposalId, address indexed creator, @@ -66,9 +68,9 @@ interface IProposal { /// @return abi ABI of params in `data` of `createProposal`. function customProposalParamsABI() external view returns (string memory abi); - /// @notice Returns the proposal count determining the next proposal ID. - /// @dev This function has been deprecated but due to backwards compatibility, it still exists - /// in the interface but reverts to avoid ambiguity. + /// @notice Returns the proposal count which determines the next proposal ID. + /// @dev This function is deprecated but remains in the interface for backward compatibility. + /// It now reverts to prevent ambiguity. /// @return The proposal count. function proposalCount() external view returns (uint256); } diff --git a/contracts/src/plugin/extensions/proposal/Proposal.sol b/contracts/src/plugin/extensions/proposal/Proposal.sol index 85a0adae..aff5da28 100644 --- a/contracts/src/plugin/extensions/proposal/Proposal.sol +++ b/contracts/src/plugin/extensions/proposal/Proposal.sol @@ -7,8 +7,9 @@ import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; import {IProposal} from "./IProposal.sol"; /// @title Proposal -/// @author Aragon X - 2022-2023 -/// @notice An abstract contract containing the traits and internal functionality to create and execute proposals that can be inherited by non-upgradeable DAO plugins. +/// @author Aragon X - 2022-2024 +/// @notice An abstract contract containing the traits and internal functionality to create and execute proposals +/// that can be inherited by non-upgradeable DAO plugins. /// @custom:security-contact sirt@aragon.org abstract contract Proposal is IProposal, ERC165 { error FunctionDeprecated(); @@ -27,12 +28,12 @@ abstract contract Proposal is IProposal, ERC165 { } /// @notice Checks if this or the parent contract supports an interface by its ID. + /// @dev In addition to the current interfaceId, also support previous version of the interfaceId + /// that did not include the following functions: + /// `createProposal`, `hasSucceeded`, `execute`, `canExecute`, `customProposalParamsABI`. /// @param _interfaceId The ID of the interface. /// @return Returns `true` if the interface is supported. function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) { - // In addition to the current interfaceId, also support previous version of the interfaceId - // that did not include the following functions: - // `createProposal`, `hasSucceeded`, `execute`, `canExecute`, `customProposalParamsABI`. return _interfaceId == type(IProposal).interfaceId ^ diff --git a/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol b/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol index 6d22612f..40f5046e 100644 --- a/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol +++ b/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol @@ -8,8 +8,9 @@ import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/intro import {IProposal} from "./IProposal.sol"; /// @title ProposalUpgradeable -/// @author Aragon X - 2022-2023 -/// @notice An abstract contract containing the traits and internal functionality to create and execute proposals that can be inherited by upgradeable DAO plugins. +/// @author Aragon X - 2022-2024 +/// @notice An abstract contract containing the traits and internal functionality to create and execute proposals +/// that can be inherited by upgradeable DAO plugins. /// @custom:security-contact sirt@aragon.org abstract contract ProposalUpgradeable is IProposal, ERC165Upgradeable { using CountersUpgradeable for CountersUpgradeable.Counter; @@ -33,12 +34,12 @@ abstract contract ProposalUpgradeable is IProposal, ERC165Upgradeable { } /// @notice Checks if this or the parent contract supports an interface by its ID. + /// @dev In addition to the current interfaceId, also support previous version of the interfaceId + /// that did not include the following functions: + /// `createProposal`, `hasSucceeded`, `execute`, `canExecute`, `customProposalParamsABI`. /// @param _interfaceId The ID of the interface. /// @return Returns `true` if the interface is supported. function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) { - // In addition to the current interfaceId, also support previous version of the interfaceId - // that did not include the following functions: - // `createProposal`, `hasSucceeded`, `execute`, `canExecute`, `customProposalParamsABI`. return _interfaceId == type(IProposal).interfaceId ^ @@ -51,6 +52,8 @@ abstract contract ProposalUpgradeable is IProposal, ERC165Upgradeable { super.supportsInterface(_interfaceId); } - /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZeppelin's guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). + /// @notice This empty reserved space is put in place to allow future versions to add new variables + /// without shifting down storage in the inheritance chain + /// (see [OpenZeppelin's guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). uint256[49] private __gap; } diff --git a/contracts/src/utils/metadata/MetadataExtension.sol b/contracts/src/utils/metadata/MetadataExtension.sol index 8682c12c..d1172052 100644 --- a/contracts/src/utils/metadata/MetadataExtension.sol +++ b/contracts/src/utils/metadata/MetadataExtension.sol @@ -8,6 +8,7 @@ import {DaoAuthorizable} from "../../permission/auth/DaoAuthorizable.sol"; /// @title MetadataExtension /// @author Aragon X - 2024 +/// @notice An abstract, non upgradeable contract for managing and retrieving metadata associated with a plugin. /// @custom:security-contact sirt@aragon.org abstract contract MetadataExtension is ERC165, DaoAuthorizable { /// @notice The ID of the permission required to call the `setMetadata` function. @@ -16,6 +17,7 @@ abstract contract MetadataExtension is ERC165, DaoAuthorizable { /// @notice Emitted when metadata is set. event MetadataSet(bytes metadata); + /// @dev Stores the current plugin-specific metadata. bytes private metadata; /// @notice Checks if this or the parent contract supports an interface by its ID. diff --git a/contracts/src/utils/metadata/MetadataExtensionUpgradeable.sol b/contracts/src/utils/metadata/MetadataExtensionUpgradeable.sol index 58d2bc82..e03c9ef4 100644 --- a/contracts/src/utils/metadata/MetadataExtensionUpgradeable.sol +++ b/contracts/src/utils/metadata/MetadataExtensionUpgradeable.sol @@ -7,9 +7,10 @@ import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/intro import {DaoAuthorizableUpgradeable} from "../../permission/auth/DaoAuthorizableUpgradeable.sol"; /// @title MetadataExtensionUpgradeable -/// @dev Due to the requirements that already existing upgradeable plugins need to start inheritting from this, -/// we're required to use hardcoded/specific slots for storage instead of sequential slots with gaps. /// @author Aragon X - 2024 +/// @notice An abstract, upgradeable contract for managing and retrieving metadata associated with a plugin. +/// @dev Due to the requirements that already existing upgradeable plugins need to start inheritting from this, +/// we're required to use hardcoded/specific slots for storage instead of sequential slots with gaps. /// @custom:security-contact sirt@aragon.org abstract contract MetadataExtensionUpgradeable is ERC165Upgradeable, DaoAuthorizableUpgradeable { /// @notice The ID of the permission required to call the `setMetadata` function.