diff --git a/.gitignore b/.gitignore index 8180f988..4be16d13 100644 --- a/.gitignore +++ b/.gitignore @@ -14,6 +14,8 @@ packages/subgraph/imported packages/subgraph/generated packages/subgraph/tests/.bin +docs + # files *.env *.log diff --git a/packages/contracts/deploy/20_new_version/23_publish.ts b/packages/contracts/deploy/20_new_version/23_publish.ts index 7c9510f9..a0807eac 100644 --- a/packages/contracts/deploy/20_new_version/23_publish.ts +++ b/packages/contracts/deploy/20_new_version/23_publish.ts @@ -64,6 +64,7 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { // Check build number const latestBuild = (await pluginRepo.buildCount(VERSION.release)).toNumber(); + if (VERSION.build < latestBuild) { throw Error( `Publishing with build number ${VERSION.build} is not possible. The latest build is ${latestBuild}. Aborting publication...` @@ -142,8 +143,12 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { createVersion: { _release: VERSION.release, _pluginSetup: setup.address, - _buildMetadata: toHex(buildMetadataURI), - _releaseMetadata: toHex(releaseMetadataURI), + _buildMetadata: ethers.utils.hexlify( + ethers.utils.toUtf8Bytes(buildMetadataURI) + ), + _releaseMetadata: ethers.utils.hexlify( + ethers.utils.toUtf8Bytes(releaseMetadataURI) + ), }, }, ], diff --git a/packages/contracts/hardhat.config.ts b/packages/contracts/hardhat.config.ts index 88a4fbff..524f651c 100644 --- a/packages/contracts/hardhat.config.ts +++ b/packages/contracts/hardhat.config.ts @@ -20,6 +20,7 @@ import { import type {NetworkUserConfig} from 'hardhat/types'; import {resolve} from 'path'; import 'solidity-coverage'; +import 'solidity-docgen'; const dotenvConfigPath: string = process.env.DOTENV_CONFIG_PATH || '../../.env'; dotenvConfig({path: resolve(__dirname, dotenvConfigPath), override: true}); @@ -154,7 +155,6 @@ const config: HardhatUserConfig = { tests: './test', deploy: './deploy', }, - solidity: { version: '0.8.17', settings: { @@ -175,6 +175,13 @@ const config: HardhatUserConfig = { outDir: 'typechain', target: 'ethers-v5', }, + docgen: { + outputDir: 'docs', + theme: 'markdown', + pages: 'files', + collapseNewlines: true, + exclude: ['test', 'mocks'], + }, }; export default config; diff --git a/packages/contracts/package.json b/packages/contracts/package.json index a70f99c5..ee32f9a2 100644 --- a/packages/contracts/package.json +++ b/packages/contracts/package.json @@ -8,6 +8,7 @@ "lint:sol": "cd ../../ && yarn run lint:contracts:sol", "lint:ts": "cd ../../ && yarn run lint:contracts:ts", "test": "hardhat test", + "docgen": "hardhat docgen", "typechain": "cross-env TS_NODE_TRANSPILE_ONLY=true hardhat typechain", "clean": "rimraf ./artifacts ./cache ./coverage ./typechain ./types ./coverage.json && yarn typechain" }, @@ -35,6 +36,7 @@ "@openzeppelin/hardhat-upgrades": "^1.28.0", "@typechain/ethers-v5": "^10.1.1", "@typechain/hardhat": "^6.1.4", + "solidity-docgen": "^0.6.0-beta.35", "@types/chai": "^4.3.4", "@types/mocha": "^10.0.0", "@types/node": "^18.11.9", diff --git a/packages/contracts/src/ListedCheckCondition.sol b/packages/contracts/src/ListedCheckCondition.sol new file mode 100644 index 00000000..b2616676 --- /dev/null +++ b/packages/contracts/src/ListedCheckCondition.sol @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.8; + +import {Multisig} from "./Multisig.sol"; + +import {PermissionCondition} from "@aragon/osx-commons-contracts/src/permission/condition/PermissionCondition.sol"; + +contract ListedCheckCondition is PermissionCondition { + Multisig private immutable MULTISIG; + + constructor(address _multisig) { + MULTISIG = Multisig(_multisig); + } + + function isGranted( + address _where, + address _who, + bytes32 _permissionId, + bytes calldata _data + ) public view override returns (bool) { + (_where, _data, _permissionId); + + (bool onlyListed, ) = MULTISIG.multisigSettings(); + + if (onlyListed && !MULTISIG.isListed(_who)) { + return false; + } + + return true; + } +} diff --git a/packages/contracts/src/Multisig.sol b/packages/contracts/src/Multisig.sol index 54468b5c..9709eac4 100644 --- a/packages/contracts/src/Multisig.sol +++ b/packages/contracts/src/Multisig.sol @@ -9,20 +9,25 @@ import {IMembership} from "@aragon/osx-commons-contracts/src/plugin/extensions/m import {Addresslist} from "@aragon/osx-commons-contracts/src/plugin/extensions/governance/Addresslist.sol"; import {ProposalUpgradeable} from "@aragon/osx-commons-contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol"; import {PluginUUPSUpgradeable} from "@aragon/osx-commons-contracts/src/plugin/PluginUUPSUpgradeable.sol"; +import {IProposal} from "@aragon/osx-commons-contracts/src/plugin/extensions/proposal/IProposal.sol"; import {IDAO} from "@aragon/osx-commons-contracts/src/dao/IDAO.sol"; +import {Action} from "@aragon/osx-commons-contracts/src/executors/IExecutor.sol"; +import {MetadataExtensionUpgradeable} from "@aragon/osx-commons-contracts/src/utils/metadata/MetadataExtensionUpgradeable.sol"; import {IMultisig} from "./IMultisig.sol"; /* solhint-enable max-line-length */ /// @title Multisig -/// @author Aragon X - 2022-2023 +/// @author Aragon X - 2022-2024 /// @notice The on-chain multisig governance plugin in which a proposal passes if X out of Y approvals are met. -/// @dev v1.3 (Release 1, Build 3) +/// @dev v1.3 (Release 1, Build 3). For each upgrade, if the reinitialization step is required, +/// increment the version numbers in the modifier for both the initialize and initializeFrom functions. /// @custom:security-contact sirt@aragon.org contract Multisig is IMultisig, IMembership, + MetadataExtensionUpgradeable, PluginUUPSUpgradeable, ProposalUpgradeable, Addresslist @@ -43,8 +48,9 @@ contract Multisig is uint16 approvals; ProposalParameters parameters; mapping(address => bool) approvers; - IDAO.Action[] actions; + Action[] actions; uint256 allowFailureMap; + TargetConfig targetConfig; // added in build 3. } /// @notice A container for the proposal parameters. @@ -69,15 +75,25 @@ contract Multisig is /// @notice The [ERC-165](https://eips.ethereum.org/EIPS/eip-165) interface ID of the contract. bytes4 internal constant MULTISIG_INTERFACE_ID = - this.initialize.selector ^ - this.updateMultisigSettings.selector ^ - this.createProposal.selector ^ + this.updateMultisigSettings.selector ^ + bytes4( + keccak256( + "createProposal(bytes,(address,uint256,bytes)[],uint256,bool,bool,uint64,uint64)" + ) + ) ^ this.getProposal.selector; /// @notice The ID of the permission required to call the `addAddresses` and `removeAddresses` functions. bytes32 public constant UPDATE_MULTISIG_SETTINGS_PERMISSION_ID = keccak256("UPDATE_MULTISIG_SETTINGS_PERMISSION"); + /// @notice The ID of the permission required to call the `createProposal` function. + bytes32 public constant CREATE_PROPOSAL_PERMISSION_ID = keccak256("CREATE_PROPOSAL_PERMISSION"); + + /// @notice The ID of the permission required to call the `execute` function. + bytes32 public constant EXECUTE_PROPOSAL_PERMISSION_ID = + keccak256("EXECUTE_PROPOSAL_PERMISSION"); + /// @notice A mapping between proposal IDs and proposal information. // solhint-disable-next-line named-parameters-mapping mapping(uint256 => Proposal) internal proposals; @@ -94,6 +110,10 @@ contract Multisig is /// @param sender The sender address. error ProposalCreationForbidden(address sender); + /// @notice Thrown when a proposal doesn't exist. + /// @param proposalId The ID of the proposal which doesn't exist. + error NonexistentProposal(uint256 proposalId); + /// @notice Thrown if an approver is not allowed to cast an approve. This can be because the proposal /// - is not open, /// - was executed, or @@ -117,6 +137,10 @@ contract Multisig is /// @param actual The actual value. error AddresslistLengthOutOfBounds(uint16 limit, uint256 actual); + /// @notice Thrown if the proposal with the same id already exists. + /// @param proposalId The id of the proposal. + error ProposalAlreadyExists(uint256 proposalId); + /// @notice Thrown if a date is out of bounds. /// @param limit The limit value. /// @param actual The actual value. @@ -140,8 +164,10 @@ contract Multisig is function initialize( IDAO _dao, address[] calldata _members, - MultisigSettings calldata _multisigSettings - ) external initializer { + MultisigSettings calldata _multisigSettings, + TargetConfig calldata _targetConfig, + bytes calldata _pluginMetadata + ) external onlyCallAtInitialization reinitializer(2) { __PluginUUPSUpgradeable_init(_dao); if (_members.length > type(uint16).max) { @@ -152,6 +178,28 @@ contract Multisig is emit MembersAdded({members: _members}); _updateMultisigSettings(_multisigSettings); + _setMetadata(_pluginMetadata); + + _setTargetConfig(_targetConfig); + } + + /// @notice Reinitializes the TokenVoting after an upgrade from a previous protocol version. For each + /// reinitialization step, use the `_fromBuild` version to decide which internal functions to call + /// for reinitialization. + /// @dev WARNING: The contract should only be upgradeable through PSP to ensure that _fromBuild is not + /// incorrectly passed, and that the appropriate permissions for the upgrade are properly configured. + /// @param _fromBuild The build version number of the previous implementation contract this upgrade is transitioning from. + /// @param _initData The initialization data to be passed to via `upgradeToAndCall` (see [ERC-1967](https://docs.openzeppelin.com/contracts/4.x/api/proxy#ERC1967Upgrade)). + function initializeFrom(uint16 _fromBuild, bytes calldata _initData) external reinitializer(2) { + if (_fromBuild < 3) { + (TargetConfig memory targetConfig, bytes memory pluginMetadata) = abi.decode( + _initData, + (TargetConfig, bytes) + ); + + _setTargetConfig(targetConfig); + _setMetadata(pluginMetadata); + } } /// @notice Checks if this or the parent contract supports an interface by its ID. @@ -159,7 +207,13 @@ contract Multisig is /// @return Returns `true` if the interface is supported. function supportsInterface( bytes4 _interfaceId - ) public view virtual override(PluginUUPSUpgradeable, ProposalUpgradeable) returns (bool) { + ) + public + view + virtual + override(MetadataExtensionUpgradeable, PluginUUPSUpgradeable, ProposalUpgradeable) + returns (bool) + { return _interfaceId == MULTISIG_INTERFACE_ID || _interfaceId == type(IMultisig).interfaceId || @@ -230,17 +284,13 @@ contract Multisig is // solhint-disable-next-line code-complexity function createProposal( bytes calldata _metadata, - IDAO.Action[] calldata _actions, + Action[] calldata _actions, uint256 _allowFailureMap, bool _approveProposal, bool _tryExecution, uint64 _startDate, uint64 _endDate - ) external returns (uint256 proposalId) { - if (multisigSettings.onlyListed && !isListed(_msgSender())) { - revert ProposalCreationForbidden(_msgSender()); - } - + ) public auth(CREATE_PROPOSAL_PERMISSION_ID) returns (uint256 proposalId) { uint64 snapshotBlock; unchecked { // The snapshot block must be mined already to protect the transaction against backrunning transactions @@ -264,23 +314,24 @@ contract Multisig is revert DateOutOfBounds({limit: _startDate, actual: _endDate}); } - proposalId = _createProposal({ - _creator: _msgSender(), - _metadata: _metadata, - _startDate: _startDate, - _endDate: _endDate, - _actions: _actions, - _allowFailureMap: _allowFailureMap - }); + proposalId = _createProposalId(keccak256(abi.encode(_actions, _metadata))); // Create the proposal Proposal storage proposal_ = proposals[proposalId]; + // Multisig doesn't allow `minApprovals` settings to be less than 0. + // If it is, that means proposal hasn't been created yet. + if (proposal_.parameters.minApprovals != 0) { + revert ProposalAlreadyExists(proposalId); + } + proposal_.parameters.snapshotBlock = snapshotBlock; proposal_.parameters.startDate = _startDate; proposal_.parameters.endDate = _endDate; proposal_.parameters.minApprovals = multisigSettings.minApprovals; + proposal_.targetConfig = getTargetConfig(); + // Reduce costs if (_allowFailureMap != 0) { proposal_.allowFailureMap = _allowFailureMap; @@ -296,6 +347,51 @@ contract Multisig is if (_approveProposal) { approve(proposalId, _tryExecution); } + + _emitProposalCreatedEvent( + _actions, + _metadata, + _allowFailureMap, + _startDate, + _endDate, + proposalId + ); + } + + /// @inheritdoc IProposal + function createProposal( + bytes calldata _metadata, + Action[] calldata _actions, + uint64 _startDate, + uint64 _endDate, + bytes memory _data + ) external override returns (uint256 proposalId) { + // Note that this calls public function for permission check. + // Custom parameters + uint256 _allowFailureMap; + bool _approveProposal; + bool _tryExecution; + + if (_data.length != 0) { + (_allowFailureMap, _approveProposal, _tryExecution) = abi.decode( + _data, + (uint256, bool, bool) + ); + } + proposalId = createProposal( + _metadata, + _actions, + _allowFailureMap, + _approveProposal, + _tryExecution, + _startDate, + _endDate + ); + } + + /// @inheritdoc IProposal + function customProposalParamsABI() external pure override returns (string memory) { + return "(uint256 allowFailureMap, bool approveProposal, bool tryExecution)"; } /// @inheritdoc IMultisig @@ -317,21 +413,47 @@ contract Multisig is emit Approved({proposalId: _proposalId, approver: approver}); - if (_tryExecution && _canExecute(_proposalId)) { + if (!_tryExecution) { + return; + } + + if ( + _canExecute(_proposalId) && + dao().hasPermission(address(this), approver, EXECUTE_PROPOSAL_PERMISSION_ID, msg.data) + ) { _execute(_proposalId); } } /// @inheritdoc IMultisig function canApprove(uint256 _proposalId, address _account) external view returns (bool) { + if (!_proposalExists(_proposalId)) { + revert NonexistentProposal(_proposalId); + } + return _canApprove(_proposalId, _account); } /// @inheritdoc IMultisig - function canExecute(uint256 _proposalId) external view returns (bool) { + function canExecute(uint256 _proposalId) external view virtual override returns (bool) { + if (!_proposalExists(_proposalId)) { + revert NonexistentProposal(_proposalId); + } + return _canExecute(_proposalId); } + /// @inheritdoc IProposal + function hasSucceeded(uint256 _proposalId) external view virtual override returns (bool) { + if (!_proposalExists(_proposalId)) { + revert NonexistentProposal(_proposalId); + } + + Proposal storage proposal_ = proposals[_proposalId]; + + return proposal_.approvals >= proposal_.parameters.minApprovals; + } + /// @notice Returns all information for a proposal vote by its ID. /// @param _proposalId The ID of the proposal. /// @return executed Whether the proposal is executed or not. @@ -350,7 +472,7 @@ contract Multisig is bool executed, uint16 approvals, ProposalParameters memory parameters, - IDAO.Action[] memory actions, + Action[] memory actions, uint256 allowFailureMap ) { @@ -369,7 +491,7 @@ contract Multisig is } /// @inheritdoc IMultisig - function execute(uint256 _proposalId) public { + function execute(uint256 _proposalId) public auth(EXECUTE_PROPOSAL_PERMISSION_ID) { if (!_canExecute(_proposalId)) { revert ProposalExecutionForbidden(_proposalId); } @@ -389,12 +511,22 @@ contract Multisig is proposal_.executed = true; - _executeProposal( - dao(), - _proposalId, - proposals[_proposalId].actions, - proposals[_proposalId].allowFailureMap + _execute( + proposal_.targetConfig.target, + bytes32(_proposalId), + proposal_.actions, + proposal_.allowFailureMap, + proposal_.targetConfig.operation ); + + emit ProposalExecuted(_proposalId); + } + + /// @notice Checks if proposal exists or not. + /// @param _proposalId The ID of the proposal. + /// @return Returns `true` if proposal exists, otherwise false. + function _proposalExists(uint256 _proposalId) private view returns (bool) { + return proposals[_proposalId].parameters.snapshotBlock != 0; } /// @notice Internal function to check if an account can approve. It assumes the queried proposal exists. @@ -472,6 +604,26 @@ contract Multisig is }); } + /// @dev Helper function to avoid stack too deep. + function _emitProposalCreatedEvent( + Action[] memory _actions, + bytes memory _metadata, + uint256 _allowFailureMap, + uint64 _startDate, + uint64 _endDate, + uint256 _proposalId + ) private { + emit ProposalCreated( + _proposalId, + _msgSender(), + _startDate, + _endDate, + _metadata, + _actions, + _allowFailureMap + ); + } + /// @dev This empty reserved space is put in place to allow future versions to add new /// variables without shifting down storage in the inheritance chain. /// https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps diff --git a/packages/contracts/src/MultisigSetup.sol b/packages/contracts/src/MultisigSetup.sol index 8e9fd4ee..db91c0a7 100644 --- a/packages/contracts/src/MultisigSetup.sol +++ b/packages/contracts/src/MultisigSetup.sol @@ -6,25 +6,45 @@ pragma solidity ^0.8.8; import {PermissionLib} from "@aragon/osx-commons-contracts/src/permission/PermissionLib.sol"; import {IPluginSetup} from "@aragon/osx-commons-contracts/src/plugin/setup/IPluginSetup.sol"; import {PluginUpgradeableSetup} from "@aragon/osx-commons-contracts/src/plugin/setup/PluginUpgradeableSetup.sol"; +import {IPlugin} from "@aragon/osx-commons-contracts/src/plugin/IPlugin.sol"; + import {ProxyLib} from "@aragon/osx-commons-contracts/src/utils/deployment/ProxyLib.sol"; import {IDAO} from "@aragon/osx-commons-contracts/src/dao/IDAO.sol"; +import {ListedCheckCondition} from "./ListedCheckCondition.sol"; + import {Multisig} from "./Multisig.sol"; /* solhint-enable max-line-length */ /// @title MultisigSetup -/// @author Aragon X - 2022-2023 +/// @author Aragon X - 2022-2024 /// @notice The setup contract of the `Multisig` plugin. /// @dev v1.3 (Release 1, Build 3) /// @custom:security-contact sirt@aragon.org contract MultisigSetup is PluginUpgradeableSetup { using ProxyLib for address; - // TODO This permission identifier will be moved into a library in task OS-954. /// @notice The ID of the permission required to call the `execute` function. bytes32 internal constant EXECUTE_PERMISSION_ID = keccak256("EXECUTE_PERMISSION"); + /// @notice The ID of the permission required to call the `upgradeToAndCall` function. + bytes32 internal constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION"); + + /// @notice The ID of the permission required to call the `setTargetConfig` function. + bytes32 public constant SET_TARGET_CONFIG_PERMISSION_ID = + keccak256("SET_TARGET_CONFIG_PERMISSION"); + + /// @notice The ID of the permission required to call the `setMetadata` function. + bytes32 public constant SET_METADATA_PERMISSION_ID = keccak256("SET_METADATA_PERMISSION"); + + /// @notice The ID of the permission required to call the `updateMultisigSettings` function. + bytes32 public constant UPDATE_MULTISIG_SETTINGS_PERMISSION_ID = + keccak256("UPDATE_MULTISIG_SETTINGS_PERMISSION"); + + /// @notice A special address encoding permissions that are valid for any address `who` or `where`. + address internal constant ANY_ADDR = address(type(uint160).max); + /// @notice The contract constructor, that deploys the `Multisig` plugin logic contract. constructor() PluginUpgradeableSetup(address(new Multisig())) {} @@ -34,19 +54,26 @@ contract MultisigSetup is PluginUpgradeableSetup { bytes calldata _data ) external returns (address plugin, PreparedSetupData memory preparedSetupData) { // Decode `_data` to extract the params needed for deploying and initializing `Multisig` plugin. - (address[] memory members, Multisig.MultisigSettings memory multisigSettings) = abi.decode( - _data, - (address[], Multisig.MultisigSettings) - ); + ( + address[] memory members, + Multisig.MultisigSettings memory multisigSettings, + IPlugin.TargetConfig memory targetConfig, + bytes memory pluginMetadata + ) = abi.decode(_data, (address[], Multisig.MultisigSettings, IPlugin.TargetConfig, bytes)); // Deploy and initialize the plugin UUPS proxy. plugin = IMPLEMENTATION.deployUUPSProxy( - abi.encodeCall(Multisig.initialize, (IDAO(_dao), members, multisigSettings)) + abi.encodeCall( + Multisig.initialize, + (IDAO(_dao), members, multisigSettings, targetConfig, pluginMetadata) + ) ); + address listedCheckCondition = address(new ListedCheckCondition(plugin)); + // Prepare permissions PermissionLib.MultiTargetPermission[] - memory permissions = new PermissionLib.MultiTargetPermission[](2); + memory permissions = new PermissionLib.MultiTargetPermission[](6); // Set permissions to be granted. // Grant the list of permissions of the plugin to the DAO. @@ -55,10 +82,9 @@ contract MultisigSetup is PluginUpgradeableSetup { where: plugin, who: _dao, condition: PermissionLib.NO_CONDITION, - permissionId: Multisig(IMPLEMENTATION).UPDATE_MULTISIG_SETTINGS_PERMISSION_ID() + permissionId: UPDATE_MULTISIG_SETTINGS_PERMISSION_ID }); - // Grant `EXECUTE_PERMISSION` of the DAO to the plugin. permissions[1] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Grant, where: _dao, @@ -67,7 +93,42 @@ contract MultisigSetup is PluginUpgradeableSetup { permissionId: EXECUTE_PERMISSION_ID }); + permissions[2] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.GrantWithCondition, + where: plugin, + who: ANY_ADDR, + condition: listedCheckCondition, + permissionId: Multisig(IMPLEMENTATION).CREATE_PROPOSAL_PERMISSION_ID() + }); + + permissions[3] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Grant, + where: plugin, + who: _dao, + condition: PermissionLib.NO_CONDITION, + permissionId: SET_TARGET_CONFIG_PERMISSION_ID + }); + + permissions[4] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Grant, + where: plugin, + who: _dao, + condition: PermissionLib.NO_CONDITION, + permissionId: SET_METADATA_PERMISSION_ID + }); + + permissions[5] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Grant, + where: plugin, + who: ANY_ADDR, + condition: PermissionLib.NO_CONDITION, + permissionId: Multisig(IMPLEMENTATION).EXECUTE_PROPOSAL_PERMISSION_ID() + }); + preparedSetupData.permissions = permissions; + + preparedSetupData.helpers = new address[](1); + preparedSetupData.helpers[0] = listedCheckCondition; } /// @inheritdoc IPluginSetup @@ -78,24 +139,63 @@ contract MultisigSetup is PluginUpgradeableSetup { SetupPayload calldata _payload ) external - view override returns (bytes memory initData, PreparedSetupData memory preparedSetupData) { (initData); + if (_fromBuild < 3) { + address listedCheckCondition = address(new ListedCheckCondition(_payload.plugin)); + PermissionLib.MultiTargetPermission[] - memory permissions = new PermissionLib.MultiTargetPermission[](1); + memory permissions = new PermissionLib.MultiTargetPermission[](5); permissions[0] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Revoke, where: _payload.plugin, who: _dao, condition: PermissionLib.NO_CONDITION, - permissionId: Multisig(IMPLEMENTATION).UPGRADE_PLUGIN_PERMISSION_ID() + permissionId: UPGRADE_PLUGIN_PERMISSION_ID + }); + + permissions[1] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.GrantWithCondition, + where: _payload.plugin, + who: ANY_ADDR, + condition: listedCheckCondition, + permissionId: Multisig(IMPLEMENTATION).CREATE_PROPOSAL_PERMISSION_ID() + }); + + permissions[2] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Grant, + where: _payload.plugin, + who: _dao, + condition: PermissionLib.NO_CONDITION, + permissionId: SET_TARGET_CONFIG_PERMISSION_ID + }); + + permissions[3] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Grant, + where: _payload.plugin, + who: _dao, + condition: PermissionLib.NO_CONDITION, + permissionId: SET_METADATA_PERMISSION_ID + }); + + permissions[4] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Grant, + where: _payload.plugin, + who: ANY_ADDR, + condition: PermissionLib.NO_CONDITION, + permissionId: Multisig(IMPLEMENTATION).EXECUTE_PROPOSAL_PERMISSION_ID() }); preparedSetupData.permissions = permissions; + + preparedSetupData.helpers = new address[](1); + preparedSetupData.helpers[0] = listedCheckCondition; + + initData = abi.encodeCall(Multisig.initializeFrom, (_fromBuild, _payload.data)); } } @@ -105,7 +205,7 @@ contract MultisigSetup is PluginUpgradeableSetup { SetupPayload calldata _payload ) external view returns (PermissionLib.MultiTargetPermission[] memory permissions) { // Prepare permissions - permissions = new PermissionLib.MultiTargetPermission[](2); + permissions = new PermissionLib.MultiTargetPermission[](6); // Set permissions to be Revoked. permissions[0] = PermissionLib.MultiTargetPermission({ @@ -113,7 +213,7 @@ contract MultisigSetup is PluginUpgradeableSetup { where: _payload.plugin, who: _dao, condition: PermissionLib.NO_CONDITION, - permissionId: Multisig(IMPLEMENTATION).UPDATE_MULTISIG_SETTINGS_PERMISSION_ID() + permissionId: UPDATE_MULTISIG_SETTINGS_PERMISSION_ID }); permissions[1] = PermissionLib.MultiTargetPermission({ @@ -123,5 +223,37 @@ contract MultisigSetup is PluginUpgradeableSetup { condition: PermissionLib.NO_CONDITION, permissionId: EXECUTE_PERMISSION_ID }); + + permissions[2] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Revoke, + where: _payload.plugin, + who: _dao, + condition: PermissionLib.NO_CONDITION, + permissionId: SET_TARGET_CONFIG_PERMISSION_ID + }); + + permissions[3] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Revoke, + where: _payload.plugin, + who: _dao, + condition: PermissionLib.NO_CONDITION, + permissionId: SET_METADATA_PERMISSION_ID + }); + + permissions[4] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Revoke, + where: _payload.plugin, + who: ANY_ADDR, + condition: PermissionLib.NO_CONDITION, + permissionId: Multisig(IMPLEMENTATION).CREATE_PROPOSAL_PERMISSION_ID() + }); + + permissions[5] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Revoke, + where: _payload.plugin, + who: ANY_ADDR, + condition: PermissionLib.NO_CONDITION, + permissionId: Multisig(IMPLEMENTATION).EXECUTE_PROPOSAL_PERMISSION_ID() + }); } } diff --git a/packages/contracts/src/build-metadata.json b/packages/contracts/src/build-metadata.json index 6f46d68b..755861a4 100644 --- a/packages/contracts/src/build-metadata.json +++ b/packages/contracts/src/build-metadata.json @@ -1,5 +1,7 @@ { "ui": {}, + "name": "multisig", + "description": "description", "change": "v1.3\n - Removed an unneccessary permission that allowed the Dao to upgrade the plugin, because this is supposed to happens as part of the update itself. The unnecessary permission, which was granted on installation of previous versions, will be automatically removed with the update to this version.\n", "pluginSetup": { "prepareInstallation": { @@ -30,6 +32,32 @@ "name": "multisigSettings", "type": "tuple", "description": "The initial multisig settings." + }, + { + "components": [ + { + "internalType": "address", + "name": "target", + "type": "address", + "description": "The target contract to which actions will be forwarded to for execution." + }, + { + "internalType": "uint8", + "name": "operation", + "type": "uint8", + "description": "The operation type(either `call` or `delegatecall`) that will be used for execution forwarding." + } + ], + "internalType": "struct Multisig.TargetConfig", + "name": "TargetConfig", + "type": "tuple", + "description": "The initial target config" + }, + { + "internalType": "bytes", + "name": "metadata", + "type": "bytes", + "description": "The metadata that contains the information about the multisig." } ] }, @@ -41,6 +69,37 @@ "2": { "description": "No input is required for the update.", "inputs": [] + }, + "3": { + "description": "No input is required for the update.", + "inputs": [ + { + "components": [ + { + "internalType": "address", + "name": "target", + "type": "address", + "description": "The target contract to which actions will be forwarded to for execution." + }, + { + "internalType": "uint8", + "name": "operation", + "type": "uint8", + "description": "The operation type(either `call` or `delegatecall`) that will be used for execution forwarding." + } + ], + "internalType": "struct Multisig.TargetConfig", + "name": "TargetConfig", + "type": "tuple", + "description": "The initial target config" + }, + { + "internalType": "bytes", + "name": "metadata", + "type": "bytes", + "description": "The metadata that contains the information about the multisig." + } + ] } }, "prepareUninstallation": { diff --git a/packages/contracts/src/mocks/CustomExecutorMock.sol b/packages/contracts/src/mocks/CustomExecutorMock.sol new file mode 100644 index 00000000..31986842 --- /dev/null +++ b/packages/contracts/src/mocks/CustomExecutorMock.sol @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.8; + +import {Action} from "@aragon/osx-commons-contracts/src/executors/IExecutor.sol"; + +/// @dev DO NOT USE IN PRODUCTION! +contract CustomExecutorMock { + error FailedCustom(); + + event ExecutedCustom(); + + function execute( + bytes32 callId, + Action[] memory, + uint256 + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap); + + if (callId == bytes32(0)) { + revert FailedCustom(); + } else { + emit ExecutedCustom(); + } + } +} diff --git a/packages/contracts/test/10_unit-testing/11_plugin.ts b/packages/contracts/test/10_unit-testing/11_plugin.ts index f0a4e595..d99857fe 100644 --- a/packages/contracts/test/10_unit-testing/11_plugin.ts +++ b/packages/contracts/test/10_unit-testing/11_plugin.ts @@ -1,12 +1,15 @@ import {createDaoProxy} from '../20_integration-testing/test-helpers'; import { Addresslist__factory, + CustomExecutorMock__factory, + ERC1967Proxy__factory, IERC165Upgradeable__factory, IMembership__factory, IMultisig__factory, IPlugin__factory, IProposal__factory, IProtocolVersion__factory, + ListedCheckCondition__factory, ProxyFactory__factory, } from '../../typechain'; import {ExecutedEvent} from '../../typechain/@aragon/osx-commons-contracts/src/dao/IDAO'; @@ -17,24 +20,39 @@ import { ProposalExecutedEvent, } from '../../typechain/src/Multisig'; import { + ANY_ADDR, + CREATE_PROPOSAL_PERMISSION_ID, + CREATE_PROPOSAL_SIGNATURE, + CREATE_PROPOSAL_SIGNATURE_IProposal, + EXECUTE_PROPOSAL_PERMISSION_ID, MULTISIG_EVENTS, MULTISIG_INTERFACE, + Operation, + SET_TARGET_CONFIG_PERMISSION_ID, + TargetConfig, UPDATE_MULTISIG_SETTINGS_PERMISSION_ID, + latestInitializerVersion, } from '../multisig-constants'; import {Multisig__factory, Multisig} from '../test-utils/typechain-versions'; import { getInterfaceId, - proposalIdToBytes32, findEvent, findEventTopicLog, TIME, DAO_PERMISSIONS, } from '@aragon/osx-commons-sdk'; -import {DAO, DAOStructs, DAO__factory} from '@aragon/osx-ethers'; +import { + DAO, + DAOStructs, + DAO__factory, + PluginUUPSUpgradeableV1Mock__factory, +} from '@aragon/osx-ethers'; +import {defaultAbiCoder} from '@ethersproject/abi'; import {loadFixture, time} from '@nomicfoundation/hardhat-network-helpers'; import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; import {expect} from 'chai'; import {BigNumber} from 'ethers'; +import {keccak256} from 'ethers/lib/utils'; import {ethers} from 'hardhat'; type FixtureResult = { @@ -49,12 +67,38 @@ type FixtureResult = { defaultInitData: { members: string[]; settings: Multisig.MultisigSettingsStruct; + targetConfig: TargetConfig; + metadata: string; }; dao: DAO; dummyActions: DAOStructs.ActionStruct[]; dummyMetadata: string; }; +let chainId: number; + +async function createProposalId( + pluginAddress: string, + actions: DAOStructs.ActionStruct[], + metadata: string +): Promise { + const blockNumber = (await ethers.provider.getBlock('latest')).number; + const salt = keccak256( + defaultAbiCoder.encode( + ['tuple(address to,uint256 value,bytes data)[]', 'bytes'], + [actions, metadata] + ) + ); + return BigNumber.from( + keccak256( + defaultAbiCoder.encode( + ['uint256', 'uint256', 'address', 'bytes32'], + [chainId, blockNumber + 1, pluginAddress, salt] + ) + ) + ); +} + async function fixture(): Promise { const [deployer, alice, bob, carol, dave, eve] = await ethers.getSigners(); @@ -75,10 +119,21 @@ async function fixture(): Promise { onlyListed: true, minApprovals: 2, }, + targetConfig: { + target: dao.address, + operation: Operation.call, + }, + metadata: '0x11', }; const pluginInitdata = pluginImplementation.interface.encodeFunctionData( 'initialize', - [dao.address, defaultInitData.members, defaultInitData.settings] + [ + dao.address, + defaultInitData.members, + defaultInitData.settings, + defaultInitData.targetConfig, + defaultInitData.metadata, + ] ); const deploymentTx1 = await proxyFactory.deployUUPSProxy(pluginInitdata); const proxyCreatedEvent1 = findEvent( @@ -110,6 +165,18 @@ async function fixture(): Promise { }, ]; + await dao.grant( + initializedPlugin.address, + ANY_ADDR, + EXECUTE_PROPOSAL_PERMISSION_ID + ); + + await dao.grant( + uninitializedPlugin.address, + ANY_ADDR, + EXECUTE_PROPOSAL_PERMISSION_ID + ); + return { deployer, alice, @@ -126,7 +193,35 @@ async function fixture(): Promise { }; } +async function loadFixtureAndGrantCreatePermission(): Promise { + const data = await loadFixture(fixture); + const {deployer, alice, dao, initializedPlugin, uninitializedPlugin} = data; + + const condition = await new ListedCheckCondition__factory(deployer).deploy( + initializedPlugin.address + ); + + await dao.grantWithCondition( + initializedPlugin.address, + ANY_ADDR, + CREATE_PROPOSAL_PERMISSION_ID, + condition.address + ); + + await dao.grantWithCondition( + uninitializedPlugin.address, + ANY_ADDR, + CREATE_PROPOSAL_PERMISSION_ID, + condition.address + ); + + return data; +} + describe('Multisig', function () { + before(async () => { + chainId = (await ethers.provider.getNetwork()).chainId; + }); describe('initialize', async () => { it('reverts if trying to re-initialize', async () => { const {dao, initializedPlugin, defaultInitData} = await loadFixture( @@ -138,9 +233,11 @@ describe('Multisig', function () { initializedPlugin.initialize( dao.address, defaultInitData.members, - defaultInitData.settings + defaultInitData.settings, + defaultInitData.targetConfig, + defaultInitData.metadata ) - ).to.be.revertedWith('Initializable: contract is already initialized'); + ).to.be.revertedWithCustomError(initializedPlugin, 'AlreadyInitialized'); }); it('adds the initial addresses to the address list', async () => { @@ -157,7 +254,9 @@ describe('Multisig', function () { await plugin.initialize( dao.address, defaultInitData.members, - defaultInitData.settings + defaultInitData.settings, + defaultInitData.targetConfig, + defaultInitData.metadata ); // Check that all members from the init data have been listed as members. @@ -179,6 +278,13 @@ describe('Multisig', function () { ).to.be.eq(defaultInitData.settings.minApprovals); }); + it('sets the `metadata`', async () => { + const {initializedPlugin, defaultInitData} = await loadFixture(fixture); + expect(await initializedPlugin.getMetadata()).to.be.eq( + defaultInitData.metadata + ); + }); + it('sets `onlyListed`', async () => { const {initializedPlugin, defaultInitData} = await loadFixture(fixture); expect((await initializedPlugin.multisigSettings()).onlyListed).to.be.eq( @@ -194,7 +300,9 @@ describe('Multisig', function () { uninitializedPlugin.initialize( dao.address, defaultInitData.members, - defaultInitData.settings + defaultInitData.settings, + defaultInitData.targetConfig, + defaultInitData.metadata ) ) .to.emit(uninitializedPlugin, MULTISIG_EVENTS.MultisigSettingsUpdated) @@ -220,6 +328,8 @@ describe('Multisig', function () { dao.address, overflowingMemberList, defaultInitData.settings, + defaultInitData.targetConfig, + defaultInitData.metadata, { gasLimit: BigNumber.from(10).pow(8).toNumber(), } @@ -233,6 +343,73 @@ describe('Multisig', function () { }); }); + describe('reinitialize', async () => { + it('reverts if trying to re-reinitializeFrom', async () => { + const {uninitializedPlugin, deployer} = await loadFixture(fixture); + + const encodedData = ethers.utils.defaultAbiCoder.encode( + ['address', 'uint8', 'bytes'], + [deployer.address, Operation.delegatecall, '0x'] + ); + + // reinitialize the plugin. + await uninitializedPlugin.initializeFrom( + latestInitializerVersion, + encodedData + ); + + // Try to reinitialize the plugin. + await expect( + uninitializedPlugin.initializeFrom( + latestInitializerVersion, + encodedData + ) + ).to.be.revertedWith('Initializable: contract is already initialized'); + }); + + it('reverts if trying to initializeFrom an initialized plugin', async () => { + const {initializedPlugin, deployer} = await loadFixture(fixture); + + const encodedDummyTarget = ethers.utils.defaultAbiCoder.encode( + ['address', 'uint8'], + [deployer.address, Operation.delegatecall] + ); + + // Try to reinitialize the plugin. + await expect( + initializedPlugin.initializeFrom( + latestInitializerVersion, + encodedDummyTarget + ) + ).to.be.revertedWith('Initializable: contract is already initialized'); + }); + + // todo add test for checking that plugins already initialized on a previous version can not be initialized again + it('reverts if trying to initialize lower version plugin'); + + it('sets the `_targetConfig` when initializing an uninitialized plugin', async () => { + const {uninitializedPlugin, deployer} = await loadFixture(fixture); + + const encodedData = ethers.utils.defaultAbiCoder.encode( + ['address', 'uint8', 'bytes'], + [deployer.address, Operation.delegatecall, '0x'] + ); + + // reinitialize the plugin. + await uninitializedPlugin.initializeFrom( + latestInitializerVersion, + encodedData + ); + + expect((await uninitializedPlugin.getTargetConfig()).target).to.be.eq( + deployer.address + ); + expect((await uninitializedPlugin.getTargetConfig()).operation).to.be.eq( + Operation.delegatecall + ); + }); + }); + describe('ERC-165', async () => { it('does not support the empty interface', async () => { const {initializedPlugin: plugin} = await loadFixture(fixture); @@ -642,93 +819,153 @@ describe('Multisig', function () { }); }); - describe('createProposal', async () => { - it('increments the proposal count', async () => { + // These tests ensure that overriden `createProposal` function from `IProposal` + // successfully creates a proposal with default values(when `data` is not passed) + // and with custom values when it's passed. + describe('Proposal creation: IProposal Interface Function', async () => { + let data: FixtureResult; + beforeEach(async () => { + data = await loadFixtureAndGrantCreatePermission(); + const {deployer, dao, initializedPlugin: plugin} = data; + + await dao.grant( + plugin.address, + deployer.address, + UPDATE_MULTISIG_SETTINGS_PERMISSION_ID + ); + + await plugin.updateMultisigSettings({ + onlyListed: false, + minApprovals: 1, + }); + + await dao.grant( + dao.address, + plugin.address, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID + ); + }); + + it('creates proposal with default values if `data` param is encoded with custom values', async () => { const { alice, - initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + initializedPlugin: plugin, + } = data; - // Check that the proposal count is 0. - expect(await plugin.proposalCount()).to.equal(0); + const encodedParam = ethers.utils.defaultAbiCoder.encode( + ['uint256', 'bool', 'bool'], + [1, true, true] + ); - // Create a proposal as Alice. - const endDate = (await time.latest()) + TIME.HOUR; + const proposalId = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE_IProposal]( dummyMetadata, dummyActions, 0, - false, - false, - 0, - endDate + (await time.latest()) + TIME.HOUR, + encodedParam ); - // Check that the proposal count is 1. - expect(await plugin.proposalCount()).to.equal(1); - // Create another proposal as Alice. + const proposal = await plugin.getProposal(proposalId); + expect(proposal.allowFailureMap).to.equal(1); + + expect(await plugin.hasApproved(proposalId, alice.address)).to.be.true; + expect(proposal.executed).to.be.true; + }); + + it('creates proposal with default values if `data` param is passed as empty', async () => { + const { + alice, + dummyMetadata, + dummyActions, + initializedPlugin: plugin, + } = data; + + const proposalId = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE_IProposal]( dummyMetadata, dummyActions, 0, + (await time.latest()) + TIME.HOUR, + '0x' + ); + + const proposal = await plugin.getProposal(proposalId); + expect(proposal.actions.length).to.equal(dummyActions.length); + expect(proposal.allowFailureMap).to.equal(0); + + expect(await plugin.hasApproved(proposalId, alice.address)).to.be.false; + expect(proposal.executed).to.be.false; + }); + }); + + describe('createProposal', async () => { + let data: FixtureResult; + beforeEach(async () => { + data = await loadFixtureAndGrantCreatePermission(); + }); + + it('reverts if permission is not given', async () => { + const {deployer, dao, initializedPlugin: plugin} = data; + await dao.revoke(plugin.address, ANY_ADDR, CREATE_PROPOSAL_PERMISSION_ID); + + await expect( + plugin[CREATE_PROPOSAL_SIGNATURE]( + '0x', + [], + 0, false, false, 0, - endDate + await time.latest() + ) + ) + .to.be.revertedWithCustomError(plugin, 'DaoUnauthorized') + .withArgs( + dao.address, + plugin.address, + deployer.address, + CREATE_PROPOSAL_PERMISSION_ID ); - - // Check that the proposal count is 2. - expect(await plugin.proposalCount()).to.equal(2); }); - it('creates unique proposal IDs for each proposal', async () => { + it('generates the proposal id by hashing the actions + metadata', async () => { const { alice, initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + defaultInitData, + } = data; - // Make a static call to `createProposal` to get the first proposal ID ahead of time. - const endDate = (await time.latest()) + TIME.HOUR; - const proposalId0 = await plugin - .connect(alice) - .callStatic.createProposal( - dummyMetadata, - dummyActions, - 0, - false, - false, - 0, - endDate - ); + const proposalId = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); - // Create the new proposal as Alice. - await expect( - plugin - .connect(alice) - .createProposal( - dummyMetadata, - dummyActions, - 0, - false, - false, - 0, - endDate - ) - ).not.to.be.reverted; + // Create a proposal as Alice. + const endDate = (await time.latest()) + TIME.HOUR; - // Make a static call to `createProposal` to get the next proposal ID ahead of time. - const proposalId1 = await plugin + await plugin .connect(alice) - .callStatic.createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -738,26 +975,29 @@ describe('Multisig', function () { endDate ); - // Check that the proposal IDs are as expected. - expect(proposalId0).to.equal(0); - expect(proposalId1).to.equal(1); + // Check that proposal exists + const proposal = await plugin.getProposal(proposalId); + expect(proposal.actions.length).to.equal(dummyActions.length); + expect(proposal.parameters.minApprovals).to.equal( + defaultInitData.settings.minApprovals + ); }); it('emits the `ProposalCreated` event', async () => { - const { - alice, - initializedPlugin: plugin, - dummyMetadata, - } = await loadFixture(fixture); + const {alice, initializedPlugin: plugin, dummyMetadata} = data; // Create a proposal as Alice and check the event arguments. const startDate = (await time.latest()) + TIME.MINUTE; const endDate = startDate + TIME.HOUR; - const expectedProposalId = 0; + const expectedProposalId = await createProposalId( + plugin.address, + [], + dummyMetadata + ); await expect( plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, [], 0, @@ -780,11 +1020,7 @@ describe('Multisig', function () { }); it('reverts if the multisig settings have been changed in the same block', async () => { - const { - alice, - initializedPlugin: plugin, - dao, - } = await loadFixture(fixture); + const {alice, initializedPlugin: plugin, dao} = data; // Grant Alice the permission to update the settings. await dao.grant( @@ -815,11 +1051,18 @@ describe('Multisig', function () { uninitializedPlugin: plugin, dummyMetadata, dao, - } = await loadFixture(fixture); - await plugin.initialize(dao.address, [alice.address], { - onlyListed: true, - minApprovals: 1, - }); + defaultInitData, + } = data; + await plugin.initialize( + dao.address, + [alice.address], + { + onlyListed: true, + minApprovals: 1, + }, + defaultInitData.targetConfig, + defaultInitData.metadata + ); // Grant permissions between the DAO and the plugin. await dao.grant( @@ -852,7 +1095,7 @@ describe('Multisig', function () { await ethers.provider.send('evm_setAutomine', [false]); // Create and execute proposal #1 calling `updateMultisigSettings`. - await plugin.connect(alice).createProposal( + await plugin.connect(alice)[CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, [updateMultisigSettingsAction], 0, @@ -864,7 +1107,7 @@ describe('Multisig', function () { // Try to call update the settings a second time. await expect( - plugin.connect(alice).createProposal( + plugin.connect(alice)[CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, [updateMultisigSettingsAction], 0, @@ -889,7 +1132,7 @@ describe('Multisig', function () { initializedPlugin: plugin, dao, dummyMetadata, - } = await loadFixture(fixture); + } = data; // Grant Alice the permission to update settings. await dao.grant( @@ -908,12 +1151,16 @@ describe('Multisig', function () { const startDate = (await time.latest()) + TIME.MINUTE; const endDate = startDate + TIME.HOUR; - const expectedProposalId = 0; + const expectedProposalId = await createProposalId( + plugin.address, + [], + dummyMetadata + ); await expect( plugin .connect(dave) // Dave is not listed. - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, [], 0, @@ -939,18 +1186,19 @@ describe('Multisig', function () { describe('`onlyListed` is set to `true`', async () => { it('reverts if the caller is not listed and only listed accounts can create proposals', async () => { const { + dao, dave, initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Try to create a proposal as Dave (who is not listed), which should revert. const endDate = (await time.latest()) + TIME.HOUR; await expect( plugin .connect(dave) // Dave is not listed. - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -960,8 +1208,13 @@ describe('Multisig', function () { endDate ) ) - .to.be.revertedWithCustomError(plugin, 'ProposalCreationForbidden') - .withArgs(dave.address); + .to.be.revertedWithCustomError(plugin, 'DaoUnauthorized') + .withArgs( + dao.address, + plugin.address, + dave.address, + CREATE_PROPOSAL_PERMISSION_ID + ); }); it('reverts if caller is not listed in the current block although she was listed in the last block', async () => { @@ -973,7 +1226,7 @@ describe('Multisig', function () { dao, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Grant Alice the permission to update settings. await dao.grant( @@ -1001,7 +1254,7 @@ describe('Multisig', function () { await expect( plugin .connect(carol) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1011,13 +1264,18 @@ describe('Multisig', function () { endDate ) ) - .to.be.revertedWithCustomError(plugin, 'ProposalCreationForbidden') - .withArgs(carol.address); + .to.be.revertedWithCustomError(plugin, 'DaoUnauthorized') + .withArgs( + dao.address, + plugin.address, + carol.address, + CREATE_PROPOSAL_PERMISSION_ID + ); // Transaction 4: Create the proposal as Dave const tx4 = await plugin .connect(dave) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1026,7 +1284,11 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); // Check the listed members before the block is mined. expect(await plugin.isListed(carol.address)).to.equal(true); @@ -1073,19 +1335,19 @@ describe('Multisig', function () { initializedPlugin: plugin, defaultInitData, dummyMetadata, - } = await loadFixture(fixture); + } = data; // Create a proposal (ID 0) as Alice but don't approve on creation. const startDate = (await time.latest()) + TIME.MINUTE; const endDate = startDate + TIME.HOUR; const allowFailureMap = 0; await time.setNextBlockTimestamp(startDate); - const id = 0; + const id = await createProposalId(plugin.address, [], dummyMetadata); const approveProposal = false; await expect( - plugin.connect(alice).createProposal( + plugin.connect(alice)[CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, [], allowFailureMap, @@ -1128,7 +1390,7 @@ describe('Multisig', function () { initializedPlugin: plugin, defaultInitData, dummyMetadata, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice and approve on creation. const startDate = (await time.latest()) + TIME.MINUTE; @@ -1138,9 +1400,9 @@ describe('Multisig', function () { await time.setNextBlockTimestamp(startDate); - const id = 0; + const id = await createProposalId(plugin.address, [], dummyMetadata); await expect( - plugin.connect(alice).createProposal( + plugin.connect(alice)[CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, [], allowFailureMap, @@ -1192,7 +1454,7 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Set the clock to the start date. const startDate = (await time.latest()) + TIME.MINUTE; @@ -1204,7 +1466,7 @@ describe('Multisig', function () { await expect( plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1224,7 +1486,7 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Try to create a proposal as Alice where the end date is before the start date const startDate = (await time.latest()) + TIME.MINUTE; @@ -1232,7 +1494,7 @@ describe('Multisig', function () { await expect( plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1248,7 +1510,21 @@ describe('Multisig', function () { }); context('Approving and executing proposals', async () => { + let data: FixtureResult; + beforeEach(async () => { + data = await loadFixtureAndGrantCreatePermission(); + }); + describe('canApprove', async () => { + it('reverts if proposal does not exist', async () => { + const {initializedPlugin: plugin} = data; + const id = 10; + + await expect(plugin.canApprove(id, plugin.address)) + .to.be.revertedWithCustomError(plugin, 'NonexistentProposal') + .withArgs(id); + }); + it('returns `false` if the proposal is already executed', async () => { const { alice, @@ -1258,14 +1534,18 @@ describe('Multisig', function () { dao, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); - // Create a proposal (with ID 0) await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1274,7 +1554,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; await dao.grant( dao.address, @@ -1304,13 +1583,19 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice. const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1319,7 +1604,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Check that Dave who is not listed cannot approve. expect(await plugin.isListed(dave.address)).to.be.false; @@ -1332,14 +1616,19 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; - // Create a proposal (with ID 0) + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1348,7 +1637,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; await plugin.connect(alice).approve(id, false); expect(await plugin.canApprove(id, alice.address)).to.be.false; @@ -1360,14 +1648,19 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; - // Create a proposal (with ID 0) + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1376,7 +1669,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; expect(await plugin.canApprove(id, alice.address)).to.be.true; }); @@ -1387,15 +1679,20 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const startDate = (await time.latest()) + TIME.MINUTE; const endDate = startDate + TIME.HOUR; - // Create a proposal (with ID 0) + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1404,7 +1701,6 @@ describe('Multisig', function () { startDate, endDate ); - const id = 0; expect(await plugin.canApprove(id, alice.address)).to.be.false; @@ -1419,14 +1715,19 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; - // Create a proposal (with ID 0) + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1435,7 +1736,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; expect(await plugin.canApprove(id, alice.address)).to.be.true; @@ -1452,14 +1752,19 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; - // Create a proposal (with ID 0) + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1468,7 +1773,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; expect(await plugin.hasApproved(id, alice.address)).to.be.false; }); @@ -1479,13 +1783,18 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1494,7 +1803,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; await plugin.connect(alice).approve(id, false); expect(await plugin.hasApproved(id, alice.address)).to.be.true; @@ -1508,14 +1816,18 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; - // Create a proposal (with ID 0) + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1524,7 +1836,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; await plugin.connect(alice).approve(id, true); @@ -1541,14 +1852,19 @@ describe('Multisig', function () { dummyMetadata, dummyActions, dao, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; - // Create a proposal (with ID 0) + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1557,7 +1873,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Grant the plugin `EXECUTE_PERMISSION_ID` permission on the DAO. await dao.grant( @@ -1580,14 +1895,19 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; - // Create a proposal (with ID 0). + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1596,7 +1916,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Check that there are 0 approvals yet. expect((await plugin.getProposal(id)).approvals).to.equal(0); @@ -1619,14 +1938,20 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice that didn't started yet. const startDate = (await time.latest()) + TIME.MINUTE; const endDate = startDate + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1635,12 +1960,11 @@ describe('Multisig', function () { startDate, endDate ); - const id = 0; // Try to approve the proposal as Alice although being before the start date. await expect(plugin.connect(alice).approve(id, false)) .to.be.revertedWithCustomError(plugin, 'ApprovalCastForbidden') - .withArgs(0, alice.address); + .withArgs(id, alice.address); // Advance to the start date. await time.increaseTo(startDate); @@ -1656,15 +1980,20 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice that starts now. const startDate = (await time.latest()) + TIME.MINUTE; const endDate = startDate + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1673,7 +2002,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Advance time after the end date. await time.increaseTo(endDate + 1); @@ -1684,20 +2012,176 @@ describe('Multisig', function () { }); }); - describe('canExecute', async () => { + describe('hasSucceeded', async () => { + it('reverts if proposal does not exist', async () => { + const {initializedPlugin: plugin} = data; + const id = 10; + + await expect(plugin.hasSucceeded(id)) + .to.be.revertedWithCustomError(plugin, 'NonexistentProposal') + .withArgs(id); + }); + it('returns `false` if the proposal has not reached the minimum approval yet', async () => { const { alice, initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice. const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( + dummyMetadata, + dummyActions, + 0, + false, + false, + 0, + endDate + ); + + // Check that `minApprovals` isn't met yet. + const proposal = await plugin.getProposal(id); + expect(proposal.approvals).to.be.lt(proposal.parameters.minApprovals); + + // Check that the proposal has not yet succeeded. + expect(await plugin.hasSucceeded(id)).to.be.false; + }); + + it('returns `true` if threshold is met even if the proposal is executed or not', async () => { + const { + alice, + bob, + initializedPlugin: plugin, + dao, + dummyMetadata, + dummyActions, + } = data; + + // Create a proposal as Alice. + const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin + .connect(alice) + [CREATE_PROPOSAL_SIGNATURE]( + dummyMetadata, + dummyActions, + 0, + false, + false, + 0, + endDate + ); + + // Grant the plugin `EXECUTE_PERMISSION_ID` permission on the DAO. + await dao.grant( + dao.address, + plugin.address, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID + ); + + // Approve as Alice. + await plugin.connect(alice).approve(id, false); + // Approve and execute as Bob. + await plugin.connect(bob).approve(id, false); + + // It must still return true even if proposal is not executed yet. + expect(await plugin.hasSucceeded(id)).to.be.true; + + await plugin.execute(id); + + // Check that the proposal got executed. + expect((await plugin.getProposal(id)).executed).to.be.true; + + // It must still return true even if proposal has been executed. + expect(await plugin.hasSucceeded(id)).to.be.true; + }); + + it('returns `true` if threshold is met and time window expired', async () => { + const { + alice, + bob, + initializedPlugin: plugin, + dao, + dummyMetadata, + dummyActions, + } = data; + + // Create a proposal as Alice. + const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin + .connect(alice) + [CREATE_PROPOSAL_SIGNATURE]( + dummyMetadata, + dummyActions, + 0, + false, + false, + 0, + endDate + ); + + // Approve as Alice. + await plugin.connect(alice).approve(id, false); + // Approve and execute as Bob. + await plugin.connect(bob).approve(id, false); + + await time.increaseTo(endDate + 1); + + // Check that it still returns true even after time windows are expired. + // Ensures that this function doesn't depend on time checks. + expect(await plugin.hasSucceeded(id)).to.be.true; + }); + }); + + describe('canExecute', async () => { + it('reverts if proposal does not exist', async () => { + const {initializedPlugin: plugin} = data; + const id = 10; + + await expect(plugin.canExecute(id)) + .to.be.revertedWithCustomError(plugin, 'NonexistentProposal') + .withArgs(id); + }); + + it('returns `false` if the proposal has not reached the minimum approval yet', async () => { + const { + alice, + initializedPlugin: plugin, + dummyMetadata, + dummyActions, + } = data; + + // Create a proposal as Alice. + const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + + await plugin + .connect(alice) + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1706,7 +2190,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Check that `minApprovals` isn't met yet. const proposal = await plugin.getProposal(id); @@ -1724,13 +2207,18 @@ describe('Multisig', function () { dao, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice. const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1739,7 +2227,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Grant the plugin `EXECUTE_PERMISSION_ID` permission on the DAO. await dao.grant( @@ -1767,13 +2254,18 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1782,7 +2274,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; await plugin.connect(alice).approve(id, false); await plugin.connect(bob).approve(id, false); @@ -1799,14 +2290,19 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const startDate = (await time.latest()) + TIME.MINUTE; const endDate = startDate + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1815,7 +2311,6 @@ describe('Multisig', function () { startDate, endDate ); - const id = 0; expect(await plugin.canExecute(id)).to.be.false; @@ -1835,13 +2330,18 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1850,7 +2350,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; await plugin.connect(alice).approve(id, false); await plugin.connect(bob).approve(id, false); @@ -1872,13 +2371,19 @@ describe('Multisig', function () { dao, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice. const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1887,7 +2392,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Grant the plugin `EXECUTE_PERMISSION_ID` permission on the DAO. await dao.grant( @@ -1911,13 +2415,19 @@ describe('Multisig', function () { dao, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice. const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1926,7 +2436,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Grant the plugin `EXECUTE_PERMISSION_ID` permission on the DAO. await dao.grant( @@ -1965,13 +2474,18 @@ describe('Multisig', function () { dao, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -1980,7 +2494,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; await dao.grant( dao.address, @@ -2014,13 +2527,19 @@ describe('Multisig', function () { dao, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice. const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -2029,7 +2548,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Grant the plugin `EXECUTE_PERMISSION_ID` permission on the DAO. await dao.grant( @@ -2078,7 +2596,7 @@ describe('Multisig', function () { ); expect(event.args.actor).to.equal(plugin.address); - expect(event.args.callId).to.equal(proposalIdToBytes32(id)); + expect(event.args.callId).to.equal(ethers.utils.hexZeroPad(id, 32)); expect(event.args.actions.length).to.equal(1); expect(event.args.actions[0].to).to.equal(dummyActions[0].to); expect(event.args.actions[0].value).to.equal(dummyActions[0].value); @@ -2112,13 +2630,19 @@ describe('Multisig', function () { dao, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice. const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -2127,7 +2651,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Grant the plugin `EXECUTE_PERMISSION_ID` on the DAO. await dao.grant( @@ -2156,13 +2679,19 @@ describe('Multisig', function () { dao, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice. const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -2171,7 +2700,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Grant the plugin `EXECUTE_PERMISSION_ID` permission on the DAO. await dao.grant( @@ -2199,14 +2727,20 @@ describe('Multisig', function () { dao, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice. const startDate = (await time.latest()) + TIME.MINUTE; const endDate = startDate + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -2215,7 +2749,6 @@ describe('Multisig', function () { startDate, endDate ); - const id = 0; // Grant the plugin `EXECUTE_PERMISSION_ID` permission on the DAO. await dao.grant( @@ -2247,14 +2780,20 @@ describe('Multisig', function () { initializedPlugin: plugin, dummyMetadata, dummyActions, - } = await loadFixture(fixture); + } = data; // Create a proposal as Alice. const startDate = (await time.latest()) + TIME.MINUTE; const endDate = startDate + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + await plugin .connect(alice) - .createProposal( + [CREATE_PROPOSAL_SIGNATURE]( dummyMetadata, dummyActions, 0, @@ -2263,7 +2802,6 @@ describe('Multisig', function () { 0, endDate ); - const id = 0; // Approve the proposal but do not execute yet. await plugin.connect(alice).approve(id, false); @@ -2277,6 +2815,204 @@ describe('Multisig', function () { plugin.connect(bob).execute(id) ).to.be.revertedWithCustomError(plugin, 'ProposalExecutionForbidden'); }); + + it('executes target with delegate call', async () => { + const {alice, bob, dummyMetadata, dummyActions, deployer, dao} = data; + + let {initializedPlugin: plugin} = data; + + const executorFactory = new CustomExecutorMock__factory(deployer); + const executor = await executorFactory.deploy(); + + const abiA = CustomExecutorMock__factory.abi; + const abiB = Multisig__factory.abi; + // @ts-ignore + const mergedABI = abiA.concat(abiB); + + await dao.grant( + plugin.address, + deployer.address, + SET_TARGET_CONFIG_PERMISSION_ID + ); + + await plugin.connect(deployer).setTargetConfig({ + target: executor.address, + operation: Operation.delegatecall, + }); + + plugin = (await ethers.getContractAt( + mergedABI, + plugin.address + )) as Multisig; + + const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + + await plugin.connect(alice)[CREATE_PROPOSAL_SIGNATURE]( + dummyMetadata, + dummyActions, + 1, + true, // approve right away. + false, + 0, + endDate + ); + + await expect(plugin.connect(bob).approve(id, true)) + .to.emit(plugin, 'ExecutedCustom') + .to.emit(plugin, 'ProposalExecuted'); + }); + + it('can not execute if execute permission is not granted', async () => { + const { + alice, + bob, + initializedPlugin: plugin, + defaultInitData, + dao, + dummyMetadata, + dummyActions, + } = data; + + // Create a proposal as Alice. + const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + + await plugin + .connect(alice) + [CREATE_PROPOSAL_SIGNATURE]( + dummyMetadata, + dummyActions, + 0, + false, + false, + 0, + endDate + ); + + // Grant the plugin `EXECUTE_PERMISSION_ID` permission on the DAO. + await dao.grant( + dao.address, + plugin.address, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID + ); + + // Approve with Alice and Bob. + await plugin.connect(alice).approve(id, false); + await plugin.connect(bob).approve(id, false); + + // Check that the `minApprovals` threshold is met. + const proposal = await plugin.getProposal(id); + expect(proposal.parameters.minApprovals).to.equal( + defaultInitData.settings.minApprovals + ); + expect(proposal.approvals).to.be.eq( + defaultInitData.settings.minApprovals + ); + + // Check that the proposal can be executed. + expect(await plugin.canExecute(id)).to.be.true; + + // Revoke execute permission from ANY_ADDR + await dao.revoke( + plugin.address, + ANY_ADDR, + EXECUTE_PROPOSAL_PERMISSION_ID + ); + + // Check that it executes. + await expect(plugin.connect(alice).execute(id)) + .to.be.revertedWithCustomError(plugin, 'DaoUnauthorized') + .withArgs( + dao.address, + plugin.address, + alice.address, + EXECUTE_PROPOSAL_PERMISSION_ID + ); + }); + + it('records approve correctly without execting when tryExecution is selected & execute permission is not granted', async () => { + const { + alice, + bob, + initializedPlugin: plugin, + defaultInitData, + dao, + dummyMetadata, + dummyActions, + } = data; + + // Create a proposal as Alice. + const endDate = (await time.latest()) + TIME.HOUR; + const id = await createProposalId( + plugin.address, + dummyActions, + dummyMetadata + ); + + await plugin + .connect(alice) + [CREATE_PROPOSAL_SIGNATURE]( + dummyMetadata, + dummyActions, + 0, + false, + false, + 0, + endDate + ); + + // Grant the plugin `EXECUTE_PERMISSION_ID` permission on the DAO. + await dao.grant( + dao.address, + plugin.address, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID + ); + + // Check that the no one submited an approve yet. + let proposal = await plugin.getProposal(id); + expect(proposal.approvals).to.be.equal(0); + + // Approve with Alice, but without try execution.. + await plugin.connect(alice).approve(id, false); + + // Check that the `minApprovals` threshold is not met yet. + expect(proposal.parameters.minApprovals).to.equal( + defaultInitData.settings.minApprovals + ); + expect(proposal.approvals).to.be.lt( + defaultInitData.settings.minApprovals + ); + proposal = await plugin.getProposal(id); + expect(proposal.approvals).to.be.equal(1); + + // Revoke execute permission from ANY_ADDR + await dao.revoke( + plugin.address, + ANY_ADDR, + EXECUTE_PROPOSAL_PERMISSION_ID + ); + + // Approve with Bob and try execution. + await plugin.connect(bob).approve(id, true); + + // Check that the `minApprovals` threshold is met. + proposal = await plugin.getProposal(id); + expect(proposal.approvals).to.be.equal( + defaultInitData.settings.minApprovals + ); + expect(await plugin.canExecute(id)).to.be.true; + expect(proposal.approvals).to.be.equal(2); + expect(proposal.executed).to.be.equal(false); + }); }); }); }); diff --git a/packages/contracts/test/10_unit-testing/12_plugin-setup.ts b/packages/contracts/test/10_unit-testing/12_plugin-setup.ts index e8b6f39c..75309b75 100644 --- a/packages/contracts/test/10_unit-testing/12_plugin-setup.ts +++ b/packages/contracts/test/10_unit-testing/12_plugin-setup.ts @@ -2,9 +2,17 @@ import {createDaoProxy} from '../20_integration-testing/test-helpers'; import {METADATA} from '../../plugin-settings'; import {MultisigSetup, MultisigSetup__factory} from '../../typechain'; import { + ANY_ADDR, + CREATE_PROPOSAL_PERMISSION_ID, MULTISIG_INTERFACE, + SET_TARGET_CONFIG_PERMISSION_ID, + TargetConfig, + SET_METADATA_PERMISSION_ID, UPDATE_MULTISIG_SETTINGS_PERMISSION_ID, + UPGRADE_PLUGIN_PERMISSION_ID, + EXECUTE_PROPOSAL_PERMISSION_ID, } from '../multisig-constants'; +import {Operation as op} from '../multisig-constants'; import {Multisig__factory, Multisig} from '../test-utils/typechain-versions'; import { getInterfaceId, @@ -28,9 +36,12 @@ type FixtureResult = { bob: SignerWithAddress; carol: SignerWithAddress; pluginSetup: MultisigSetup; + defaultTargetConfig: TargetConfig; + updateTargetConfig: TargetConfig; defaultMembers: string[]; defaultMultisigSettings: Multisig.MultisigSettingsStruct; prepareInstallationInputs: string; + prepareUpdateBuild3Inputs: string; prepareUninstallationInputs: string; dao: DAO; }; @@ -52,12 +63,20 @@ async function fixture(): Promise { minApprovals: 1, }; + const defaultTargetConfig = {target: dao.address, operation: op.call}; + const defaultMetadata = '0x'; + // Provide installation inputs const prepareInstallationInputs = ethers.utils.defaultAbiCoder.encode( getNamedTypesFromMetadata( METADATA.build.pluginSetup.prepareInstallation.inputs ), - [defaultMembers, Object.values(defaultMultisigSettings)] + [ + defaultMembers, + Object.values(defaultMultisigSettings), + defaultTargetConfig, + defaultMetadata, + ] ); // Provide uninstallation inputs @@ -68,6 +87,19 @@ async function fixture(): Promise { [] ); + const updateTargetConfig = { + target: pluginSetup.address, + operation: op.delegatecall, + }; + + // Provide update inputs + const prepareUpdateBuild3Inputs = ethers.utils.defaultAbiCoder.encode( + getNamedTypesFromMetadata( + METADATA.build.pluginSetup.prepareUpdate[3].inputs + ), + [updateTargetConfig, defaultMetadata] + ); + return { deployer, alice, @@ -75,8 +107,11 @@ async function fixture(): Promise { carol, pluginSetup, defaultMembers, + defaultTargetConfig, + updateTargetConfig, defaultMultisigSettings, prepareInstallationInputs, + prepareUpdateBuild3Inputs, prepareUninstallationInputs, dao, }; @@ -128,8 +163,13 @@ describe('MultisigSetup', function () { }); it('reverts if zero members are provided in the initialization data', async () => { - const {deployer, pluginSetup, dao, defaultMultisigSettings} = - await loadFixture(fixture); + const { + deployer, + pluginSetup, + dao, + defaultMultisigSettings, + defaultTargetConfig, + } = await loadFixture(fixture); // Create input data containing an empty list of initial members. const noMembers: string[] = []; @@ -137,7 +177,7 @@ describe('MultisigSetup', function () { getNamedTypesFromMetadata( METADATA.build.pluginSetup.prepareInstallation.inputs ), - [noMembers, defaultMultisigSettings] + [noMembers, defaultMultisigSettings, defaultTargetConfig, '0x'] ); // Anticipate the plugin proxy address that will be deployed. @@ -166,7 +206,8 @@ describe('MultisigSetup', function () { }); it('reverts if the `minApprovals` value in `_data` is zero', async () => { - const {deployer, pluginSetup, dao} = await loadFixture(fixture); + const {deployer, pluginSetup, dao, defaultTargetConfig} = + await loadFixture(fixture); // Create input data containing a `minApprovals` threshold of 0. const multisigSettings: Multisig.MultisigSettingsStruct = { @@ -178,7 +219,7 @@ describe('MultisigSetup', function () { getNamedTypesFromMetadata( METADATA.build.pluginSetup.prepareInstallation.inputs ), - [members, multisigSettings] + [members, multisigSettings, defaultTargetConfig, '0x'] ); // Anticipate the plugin proxy address that will be deployed. @@ -207,7 +248,8 @@ describe('MultisigSetup', function () { }); it('reverts if the `minApprovals` value in `_data` is greater than the number of members', async () => { - const {deployer, pluginSetup, dao} = await loadFixture(fixture); + const {deployer, pluginSetup, dao, defaultTargetConfig} = + await loadFixture(fixture); // Create input data containing an initial member list with a length lower that the specified `minApprovals` // threshold. @@ -220,7 +262,7 @@ describe('MultisigSetup', function () { getNamedTypesFromMetadata( METADATA.build.pluginSetup.prepareInstallation.inputs ), - [members, multisigSettings] + [members, multisigSettings, defaultTargetConfig, '0x'] ); // Anticipate the plugin proxy address that will be deployed. @@ -273,8 +315,11 @@ describe('MultisigSetup', function () { // Check the return data. expect(plugin).to.be.equal(anticipatedPluginAddress); - expect(helpers.length).to.be.equal(0); - expect(permissions.length).to.be.equal(2); + expect(helpers.length).to.be.equal(1); + expect(permissions.length).to.be.equal(6); + + const condition = helpers[0]; + expect(permissions).to.deep.equal([ [ Operation.Grant, @@ -290,6 +335,34 @@ describe('MultisigSetup', function () { AddressZero, DAO_PERMISSIONS.EXECUTE_PERMISSION_ID, ], + [ + Operation.GrantWithCondition, + plugin, + ANY_ADDR, + condition, + CREATE_PROPOSAL_PERMISSION_ID, + ], + [ + Operation.Grant, + plugin, + dao.address, + AddressZero, + SET_TARGET_CONFIG_PERMISSION_ID, + ], + [ + Operation.Grant, + plugin, + dao.address, + AddressZero, + SET_METADATA_PERMISSION_ID, + ], + [ + Operation.Grant, + plugin, + ANY_ADDR, + AddressZero, + EXECUTE_PROPOSAL_PERMISSION_ID, + ], ]); }); @@ -334,7 +407,9 @@ describe('MultisigSetup', function () { describe('prepareUpdate', async () => { it('returns the permissions expected for the update from build 1', async () => { - const {pluginSetup, dao} = await loadFixture(fixture); + const {pluginSetup, dao, prepareUpdateBuild3Inputs} = await loadFixture( + fixture + ); const plugin = ethers.Wallet.createRandom().address; // Make a static call to check that the plugin update data being returned is correct. @@ -346,14 +421,19 @@ describe('MultisigSetup', function () { ethers.Wallet.createRandom().address, ethers.Wallet.createRandom().address, ], - data: [], + data: prepareUpdateBuild3Inputs, plugin, }); // Check the return data. - expect(initData).to.be.eq('0x'); - expect(permissions.length).to.be.eql(1); - expect(helpers).to.be.eql([]); + expect(initData).to.be.eq( + Multisig__factory.createInterface().encodeFunctionData( + 'initializeFrom', + [1, prepareUpdateBuild3Inputs] + ) + ); + expect(permissions.length).to.be.equal(5); + expect(helpers.length).to.be.equal(1); // check correct permission is revoked expect(permissions).to.deep.equal([ [ @@ -363,11 +443,41 @@ describe('MultisigSetup', function () { AddressZero, PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, ], + [ + Operation.GrantWithCondition, + plugin, + ANY_ADDR, + helpers[0], + CREATE_PROPOSAL_PERMISSION_ID, + ], + [ + Operation.Grant, + plugin, + dao.address, + AddressZero, + SET_TARGET_CONFIG_PERMISSION_ID, + ], + [ + Operation.Grant, + plugin, + dao.address, + AddressZero, + SET_METADATA_PERMISSION_ID, + ], + [ + Operation.Grant, + plugin, + ANY_ADDR, + AddressZero, + EXECUTE_PROPOSAL_PERMISSION_ID, + ], ]); }); it('returns the permissions expected for the update from build 2', async () => { - const {pluginSetup, dao} = await loadFixture(fixture); + const {pluginSetup, dao, prepareUpdateBuild3Inputs} = await loadFixture( + fixture + ); const plugin = ethers.Wallet.createRandom().address; // Make a static call to check that the plugin update data being returned is correct. @@ -379,14 +489,19 @@ describe('MultisigSetup', function () { ethers.Wallet.createRandom().address, ethers.Wallet.createRandom().address, ], - data: [], + data: prepareUpdateBuild3Inputs, plugin, }); // Check the return data. - expect(initData).to.be.eq('0x'); - expect(permissions.length).to.be.eql(1); - expect(helpers).to.be.eql([]); + expect(initData).to.be.eq( + Multisig__factory.createInterface().encodeFunctionData( + 'initializeFrom', + [2, prepareUpdateBuild3Inputs] + ) + ); + expect(permissions.length).to.be.equal(5); + expect(helpers.length).to.be.equal(1); // check correct permission is revoked expect(permissions).to.deep.equal([ [ @@ -396,6 +511,34 @@ describe('MultisigSetup', function () { AddressZero, PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, ], + [ + Operation.GrantWithCondition, + plugin, + ANY_ADDR, + helpers[0], + CREATE_PROPOSAL_PERMISSION_ID, + ], + [ + Operation.Grant, + plugin, + dao.address, + AddressZero, + SET_TARGET_CONFIG_PERMISSION_ID, + ], + [ + Operation.Grant, + plugin, + dao.address, + AddressZero, + SET_METADATA_PERMISSION_ID, + ], + [ + Operation.Grant, + plugin, + ANY_ADDR, + AddressZero, + EXECUTE_PROPOSAL_PERMISSION_ID, + ], ]); }); }); @@ -421,7 +564,7 @@ describe('MultisigSetup', function () { ); // Check the return data. - expect(permissions.length).to.be.equal(2); + expect(permissions.length).to.be.equal(6); expect(permissions).to.deep.equal([ [ Operation.Revoke, @@ -437,6 +580,34 @@ describe('MultisigSetup', function () { AddressZero, DAO_PERMISSIONS.EXECUTE_PERMISSION_ID, ], + [ + Operation.Revoke, + plugin, + dao.address, + AddressZero, + SET_TARGET_CONFIG_PERMISSION_ID, + ], + [ + Operation.Revoke, + plugin, + dao.address, + AddressZero, + SET_METADATA_PERMISSION_ID, + ], + [ + Operation.Revoke, + plugin, + ANY_ADDR, + AddressZero, + CREATE_PROPOSAL_PERMISSION_ID, + ], + [ + Operation.Revoke, + plugin, + ANY_ADDR, + AddressZero, + EXECUTE_PROPOSAL_PERMISSION_ID, + ], ]); }); }); diff --git a/packages/contracts/test/20_integration-testing/21_deployment.ts b/packages/contracts/test/20_integration-testing/21_deployment.ts index 1e3ba978..1449af41 100644 --- a/packages/contracts/test/20_integration-testing/21_deployment.ts +++ b/packages/contracts/test/20_integration-testing/21_deployment.ts @@ -79,6 +79,8 @@ describe(`Deployment on network '${productionNetworkName}'`, function () { expect(results.buildMetadata).to.equal( ethers.utils.hexlify(ethers.utils.toUtf8Bytes(buildMetadataURI)) ); + + expect(results.tag.build).to.equal(VERSION.build); }); }); }); diff --git a/packages/contracts/test/20_integration-testing/22_setup-processing.ts b/packages/contracts/test/20_integration-testing/22_setup-processing.ts index cff58961..51e3d67a 100644 --- a/packages/contracts/test/20_integration-testing/22_setup-processing.ts +++ b/packages/contracts/test/20_integration-testing/22_setup-processing.ts @@ -1,6 +1,11 @@ import {METADATA, VERSION} from '../../plugin-settings'; import {MultisigSetup, Multisig__factory} from '../../typechain'; import {getProductionNetworkName, findPluginRepo} from '../../utils/helpers'; +import { + Operation, + TargetConfig, + latestInitializerVersion, +} from '../multisig-constants'; import {Multisig} from '../test-utils/typechain-versions'; import { createDaoProxy, @@ -41,6 +46,8 @@ type FixtureResult = { defaultInitData: { members: string[]; settings: Multisig.MultisigSettingsStruct; + targetConfig: TargetConfig; + metadata: string; }; psp: PluginSetupProcessor; pluginRepo: PluginRepo; @@ -90,6 +97,11 @@ async function fixture(): Promise { onlyListed: true, minApprovals: 1, }, + targetConfig: { + target: dao.address, + operation: Operation.call, + }, + metadata: '0x', }; const pluginSetupRefLatestBuild = { @@ -115,8 +127,15 @@ async function fixture(): Promise { describe(`PluginSetup processing on network '${productionNetworkName}'`, function () { it('installs & uninstalls the current build', async () => { - const {alice, bob, deployer, psp, dao, pluginSetupRefLatestBuild} = - await loadFixture(fixture); + const { + alice, + bob, + deployer, + psp, + dao, + pluginSetupRefLatestBuild, + defaultInitData, + } = await loadFixture(fixture); // Grant deployer all required permissions await dao @@ -153,7 +172,12 @@ describe(`PluginSetup processing on network '${productionNetworkName}'`, functio getNamedTypesFromMetadata( METADATA.build.pluginSetup.prepareInstallation.inputs ), - [initialMembers, multisigSettings] + [ + initialMembers, + multisigSettings, + defaultInitData.targetConfig, + defaultInitData.metadata, + ] ) ); @@ -162,7 +186,7 @@ describe(`PluginSetup processing on network '${productionNetworkName}'`, functio deployer ); - // Check that the setup worked + // // Check that the setup worked expect(await plugin.isMember(alice.address)).to.be.true; // Uninstall the current build. @@ -178,7 +202,7 @@ describe(`PluginSetup processing on network '${productionNetworkName}'`, functio ), [] ), - [] + results.preparedEvent.args.preparedSetupData.helpers ); }); @@ -200,7 +224,8 @@ describe(`PluginSetup processing on network '${productionNetworkName}'`, functio pluginSetupRefLatestBuild, 1, [defaultInitData.members, Object.values(defaultInitData.settings)], - [] + [defaultInitData.targetConfig, defaultInitData.metadata], + latestInitializerVersion ); }); @@ -222,7 +247,8 @@ describe(`PluginSetup processing on network '${productionNetworkName}'`, functio pluginSetupRefLatestBuild, 2, [defaultInitData.members, Object.values(defaultInitData.settings)], - [] + [defaultInitData.targetConfig, defaultInitData.metadata], + latestInitializerVersion ); }); }); diff --git a/packages/contracts/test/20_integration-testing/test-helpers.ts b/packages/contracts/test/20_integration-testing/test-helpers.ts index d1a47612..fc2c5f41 100644 --- a/packages/contracts/test/20_integration-testing/test-helpers.ts +++ b/packages/contracts/test/20_integration-testing/test-helpers.ts @@ -27,6 +27,8 @@ import {expect} from 'chai'; import {ContractTransaction} from 'ethers'; import {ethers} from 'hardhat'; +const OZ_INITIALIZED_SLOT_POSITION = 0; + export async function installPLugin( signer: SignerWithAddress, psp: PluginSetupProcessor, @@ -242,7 +244,8 @@ export async function updateFromBuildTest( pluginSetupRefLatestBuild: PluginSetupProcessorStructs.PluginSetupRefStruct, build: number, installationInputs: any[], - updateInputs: any[] + updateInputs: any[], + reinitializedVersion: number ) { // Grant deployer all required permissions await dao @@ -272,6 +275,7 @@ export async function updateFromBuildTest( }, pluginSetupRepo: pluginRepo.address, }; + const installationResults = await installPLugin( deployer, psp, @@ -279,7 +283,12 @@ export async function updateFromBuildTest( pluginSetupRefPreviousBuild, ethers.utils.defaultAbiCoder.encode( getNamedTypesFromMetadata( - METADATA.build.pluginSetup.prepareInstallation.inputs + // NOTE that this approach is not efficient and in reality, we should be + // fetching `build`'s ipfs cid from pluginRepo and getting the abi from there. + [ + METADATA.build.pluginSetup.prepareInstallation.inputs[0], + METADATA.build.pluginSetup.prepareInstallation.inputs[1], + ] ), installationInputs ) @@ -324,7 +333,7 @@ export async function updateFromBuildTest( pluginSetupRefLatestBuild, ethers.utils.defaultAbiCoder.encode( getNamedTypesFromMetadata( - METADATA.build.pluginSetup.prepareUpdate[1].inputs + METADATA.build.pluginSetup.prepareUpdate[3].inputs ), updateInputs ) @@ -342,9 +351,18 @@ export async function updateFromBuildTest( deployer ).implementation(); expect(await plugin.implementation()).to.equal(implementationLatestBuild); + + // check the plugin was reinitialized, OZs `_initialized` at storage slot [0] is correct + expect( + ethers.BigNumber.from( + await ethers.provider.getStorageAt( + plugin.address, + OZ_INITIALIZED_SLOT_POSITION + ) + ).toNumber() + ).to.equal(reinitializedVersion); } -// TODO Move into OSX commons as part of Task OS-928. export async function createDaoProxy( deployer: SignerWithAddress, dummyMetadata: string diff --git a/packages/contracts/test/30_regression-testing/31_upgradeability.ts b/packages/contracts/test/30_regression-testing/31_upgradeability.ts index 6f65cf69..debb2dd5 100644 --- a/packages/contracts/test/30_regression-testing/31_upgradeability.ts +++ b/packages/contracts/test/30_regression-testing/31_upgradeability.ts @@ -1,7 +1,8 @@ import {createDaoProxy} from '../20_integration-testing/test-helpers'; +import {Operation, TargetConfig} from '../multisig-constants'; import { - Multisig_V1_1__factory, - Multisig_V1_2__factory, + Multisig_V1_0_0__factory, + Multisig_V1_3_0__factory, Multisig__factory, Multisig, } from '../test-utils/typechain-versions'; @@ -10,6 +11,7 @@ import { deployAndUpgradeSelfCheck, getProtocolVersion, } from '../test-utils/uups-upgradeable'; +import {latestInitializerVersion} from './../multisig-constants'; import {PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS} from '@aragon/osx-commons-sdk'; import {DAO} from '@aragon/osx-ethers'; import {loadFixture} from '@nomicfoundation/hardhat-network-helpers'; @@ -17,6 +19,9 @@ import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; import {expect} from 'chai'; import {ethers} from 'hardhat'; +const AlreadyInitializedSignature = + Multisig__factory.createInterface().encodeErrorResult('AlreadyInitialized'); + describe('Upgrades', () => { it('upgrades to a new implementation', async () => { const {deployer, alice, dao, defaultInitData} = await loadFixture(fixture); @@ -25,7 +30,13 @@ describe('Upgrades', () => { await deployAndUpgradeSelfCheck( deployer, alice, - [dao.address, defaultInitData.members, defaultInitData.settings], + [ + dao.address, + defaultInitData.members, + defaultInitData.settings, + defaultInitData.targetConfig, + defaultInitData.metadata, + ], 'initialize', currentContractFactory, PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, @@ -33,22 +44,52 @@ describe('Upgrades', () => { ); }); - it('upgrades from v1.1', async () => { - const {deployer, alice, dao, defaultInitData} = await loadFixture(fixture); + it('upgrades from v1.0.0 with initializeFrom', async () => { + const {deployer, alice, dao, defaultInitData, encodeDataForUpgrade} = + await loadFixture(fixture); const currentContractFactory = new Multisig__factory(deployer); - const legacyContractFactory = new Multisig_V1_1__factory(deployer); + const legacyContractFactory = new Multisig_V1_0_0__factory(deployer); + + const data = [ + deployer, + alice, + [dao.address, defaultInitData.members, defaultInitData.settings], + 'initialize', + legacyContractFactory, + currentContractFactory, + PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, + dao, + 'initialize', + [ + dao.address, + defaultInitData.members, + defaultInitData.settings, + defaultInitData.targetConfig, + defaultInitData.metadata, + ], + ]; - const {fromImplementation, toImplementation} = + // Ensure that on the `upgrade`, `initialize` can not be called. + try { await deployAndUpgradeFromToCheck( - deployer, - alice, - [dao.address, defaultInitData.members, defaultInitData.settings], - 'initialize', - legacyContractFactory, - currentContractFactory, - PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, - dao + // @ts-ignore + ...data ); + throw new Error(''); + } catch (err: any) { + expect(err.data).to.equal(AlreadyInitializedSignature); + } + + data[8] = 'initializeFrom'; + // @ts-ignore + data[9] = [latestInitializerVersion, encodeDataForUpgrade]; + + const {proxy, fromImplementation, toImplementation} = + await deployAndUpgradeFromToCheck( + // @ts-ignore + ...data + ); + expect(toImplementation).to.not.equal(fromImplementation); // The build did change const fromProtocolVersion = await getProtocolVersion( @@ -61,25 +102,77 @@ describe('Upgrades', () => { expect(fromProtocolVersion).to.not.deep.equal(toProtocolVersion); expect(fromProtocolVersion).to.deep.equal([1, 0, 0]); expect(toProtocolVersion).to.deep.equal([1, 4, 0]); + + // expects the plugin was reinitialized + const newMultisig = Multisig__factory.connect(proxy.address, deployer); + + expect((await newMultisig.getTargetConfig()).target).to.deep.equal( + deployer.address + ); + expect((await newMultisig.getTargetConfig()).operation).to.deep.equal( + Operation.delegatecall + ); + + // `initializeFrom` was called on the upgrade, make sure + // `initialize` can not be called. + await expect( + proxy.initialize( + dao.address, + defaultInitData.members, + defaultInitData.settings, + defaultInitData.targetConfig, + defaultInitData.metadata + ) + ).to.be.revertedWithCustomError(proxy, 'AlreadyInitialized'); }); - it('from v1.2', async () => { - const {deployer, alice, dao, defaultInitData} = await loadFixture(fixture); + it('from v1.3.0 with initializeFrom', async () => { + const {deployer, alice, dao, defaultInitData, encodeDataForUpgrade} = + await loadFixture(fixture); const currentContractFactory = new Multisig__factory(deployer); - const legacyContractFactory = new Multisig_V1_2__factory(deployer); + const legacyContractFactory = new Multisig_V1_3_0__factory(deployer); - const {fromImplementation, toImplementation} = + const data = [ + deployer, + alice, + [dao.address, defaultInitData.members, defaultInitData.settings], + 'initialize', + legacyContractFactory, + currentContractFactory, + PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, + dao, + 'initialize', + [ + dao.address, + defaultInitData.members, + defaultInitData.settings, + defaultInitData.targetConfig, + defaultInitData.metadata, + ], + ]; + + // Ensure that on the `upgrade`, `initialize` can not be called. + try { await deployAndUpgradeFromToCheck( - deployer, - alice, - [dao.address, defaultInitData.members, defaultInitData.settings], - 'initialize', - legacyContractFactory, - currentContractFactory, - PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, - dao + // @ts-ignore + ...data ); - expect(toImplementation).to.not.equal(fromImplementation); + throw new Error(''); + } catch (err: any) { + expect(err.data).to.equal(AlreadyInitializedSignature); + } + + data[8] = 'initializeFrom'; + // @ts-ignore + data[9] = [latestInitializerVersion, encodeDataForUpgrade]; + + const {proxy, fromImplementation, toImplementation} = + await deployAndUpgradeFromToCheck( + // @ts-ignore + ...data + ); + + expect(toImplementation).to.not.equal(fromImplementation); // The build did change const fromProtocolVersion = await getProtocolVersion( legacyContractFactory.attach(fromImplementation) @@ -91,6 +184,27 @@ describe('Upgrades', () => { expect(fromProtocolVersion).to.not.deep.equal(toProtocolVersion); expect(fromProtocolVersion).to.deep.equal([1, 0, 0]); expect(toProtocolVersion).to.deep.equal([1, 4, 0]); + + // expects the plugin was reinitialized + const newMultisig = Multisig__factory.connect(proxy.address, deployer); + + expect((await newMultisig.getTargetConfig()).target).to.deep.equal( + deployer.address + ); + expect((await newMultisig.getTargetConfig()).operation).to.deep.equal( + Operation.delegatecall + ); + // `initializeFrom` was called on the upgrade, make sure + // `initialize` can not be called. + await expect( + proxy.initialize( + dao.address, + defaultInitData.members, + defaultInitData.settings, + defaultInitData.targetConfig, + defaultInitData.metadata + ) + ).to.be.revertedWithCustomError(proxy, 'AlreadyInitialized'); }); }); @@ -102,8 +216,11 @@ type FixtureResult = { defaultInitData: { members: string[]; settings: Multisig.MultisigSettingsStruct; + targetConfig: TargetConfig; + metadata: string; }; dao: DAO; + encodeDataForUpgrade: string; }; async function fixture(): Promise { @@ -120,8 +237,18 @@ async function fixture(): Promise { onlyListed: true, minApprovals: 2, }, + targetConfig: { + target: dao.address, + operation: Operation.call, + }, + metadata: '0x', }; + const encodeDataForUpgrade = ethers.utils.defaultAbiCoder.encode( + ['address', 'uint8', 'bytes'], + [deployer.address, Operation.delegatecall, '0x'] + ); + return { deployer, alice, @@ -129,5 +256,6 @@ async function fixture(): Promise { carol, dao, defaultInitData, + encodeDataForUpgrade, }; } diff --git a/packages/contracts/test/multisig-constants.ts b/packages/contracts/test/multisig-constants.ts index d9e55641..400a47b0 100644 --- a/packages/contracts/test/multisig-constants.ts +++ b/packages/contracts/test/multisig-constants.ts @@ -6,12 +6,51 @@ export const MULTISIG_EVENTS = { }; export const MULTISIG_INTERFACE = new ethers.utils.Interface([ - 'function initialize(address,address[],tuple(bool,uint16))', 'function updateMultisigSettings(tuple(bool,uint16))', - 'function createProposal(bytes,tuple(address,uint256,bytes)[],uint256,bool,bool,uint64,uint64) ', + 'function createProposal(bytes,tuple(address,uint256,bytes)[],uint256,bool,bool,uint64,uint64)', 'function getProposal(uint256)', ]); export const UPDATE_MULTISIG_SETTINGS_PERMISSION_ID = ethers.utils.id( 'UPDATE_MULTISIG_SETTINGS_PERMISSION' ); + +export const UPGRADE_PLUGIN_PERMISSION_ID = ethers.utils.id( + 'UPGRADE_PLUGIN_PERMISSION' +); + +export const CREATE_PROPOSAL_PERMISSION_ID = ethers.utils.id( + 'CREATE_PROPOSAL_PERMISSION' +); + +export const SET_TARGET_CONFIG_PERMISSION_ID = ethers.utils.id( + 'SET_TARGET_CONFIG_PERMISSION' +); + +export const SET_METADATA_PERMISSION_ID = ethers.utils.id( + 'SET_METADATA_PERMISSION' +); + +export const EXECUTE_PROPOSAL_PERMISSION_ID = ethers.utils.id( + 'EXECUTE_PROPOSAL_PERMISSION' +); + +export const CREATE_PROPOSAL_SIGNATURE = + 'createProposal(bytes,(address,uint256,bytes)[],uint256,bool,bool,uint64,uint64)'; + +export const CREATE_PROPOSAL_SIGNATURE_IProposal = + 'createProposal(bytes,(address,uint256,bytes)[],uint64,uint64,bytes)'; + +export const ANY_ADDR = '0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF'; + +export enum Operation { + call, + delegatecall, +} + +export type TargetConfig = { + target: string; + operation: number; +}; + +export const latestInitializerVersion = 2; diff --git a/packages/contracts/test/test-utils/typechain-versions.ts b/packages/contracts/test/test-utils/typechain-versions.ts index 34988e62..e9f430bd 100644 --- a/packages/contracts/test/test-utils/typechain-versions.ts +++ b/packages/contracts/test/test-utils/typechain-versions.ts @@ -2,8 +2,8 @@ /// The version specified in src is the factory and contract without the version number. /// Import as needed in the test files, and use the correct version of the contract. -export {Multisig__factory as Multisig_V1_1__factory} from '../../typechain/factories/@aragon/osx-v1.0.0/plugins/governance/multisig/Multisig__factory'; -export {Multisig__factory as Multisig_V1_2__factory} from '../../typechain/factories/@aragon/osx-v1.3.0/plugins/governance/multisig/Multisig__factory'; +export {Multisig__factory as Multisig_V1_0_0__factory} from '../../typechain/factories/@aragon/osx-v1.0.0/plugins/governance/multisig/Multisig__factory'; +export {Multisig__factory as Multisig_V1_3_0__factory} from '../../typechain/factories/@aragon/osx-v1.3.0/plugins/governance/multisig/Multisig__factory'; export {Multisig__factory} from '../../typechain/factories/src/Multisig__factory'; export {Multisig} from '../../typechain/src/Multisig'; export {IMultisig} from '../../typechain/src/IMultisig'; diff --git a/packages/contracts/test/test-utils/uups-upgradeable.ts b/packages/contracts/test/test-utils/uups-upgradeable.ts index 80183815..afc1dd5b 100644 --- a/packages/contracts/test/test-utils/uups-upgradeable.ts +++ b/packages/contracts/test/test-utils/uups-upgradeable.ts @@ -33,7 +33,7 @@ export async function deployAndUpgradeSelfCheck( { kind: 'uups', initializer: initializerName, - unsafeAllow: ['constructor'], + unsafeAllow: ['constructor', 'delegatecall'], constructorArgs: [], } ); @@ -49,7 +49,7 @@ export async function deployAndUpgradeSelfCheck( if (managingContract === undefined) { await expect( upgrades.upgradeProxy(proxy.address, factory.connect(upgrader), { - unsafeAllow: ['constructor'], + unsafeAllow: ['constructor', 'delegatecall'], constructorArgs: [], }) ) @@ -62,7 +62,7 @@ export async function deployAndUpgradeSelfCheck( else { await expect( upgrades.upgradeProxy(proxy.address, factory.connect(upgrader), { - unsafeAllow: ['constructor'], + unsafeAllow: ['constructor', 'delegatecall'], constructorArgs: [], }) ) @@ -102,20 +102,22 @@ export async function deployAndUpgradeFromToCheck( from: ContractFactory, to: ContractFactory, upgradePermissionId: string, - managingDao?: DAO | PluginRepo + managingDao?: DAO | PluginRepo, + reinitializerName?: string, + reinitArgs?: any ): Promise<{ proxy: Contract; fromImplementation: string; toImplementation: string; }> { // Deploy proxy and implementation - const proxy = await upgrades.deployProxy( + let proxy = await upgrades.deployProxy( from.connect(deployer), Object.values(initArgs), { kind: 'uups', initializer: initializerName, - unsafeAllow: ['constructor'], + unsafeAllow: ['constructor', 'delegatecall'], constructorArgs: [], } ); @@ -134,7 +136,7 @@ export async function deployAndUpgradeFromToCheck( if (managingDao === undefined) { await expect( upgrades.upgradeProxy(proxy.address, to.connect(upgrader), { - unsafeAllow: ['constructor'], + unsafeAllow: ['constructor', 'delegatecall'], constructorArgs: [], }) ) @@ -145,7 +147,7 @@ export async function deployAndUpgradeFromToCheck( } else { await expect( upgrades.upgradeProxy(proxy.address, to.connect(upgrader), { - unsafeAllow: ['constructor'], + unsafeAllow: ['constructor', 'delegatecall'], constructorArgs: [], }) ) @@ -155,10 +157,19 @@ export async function deployAndUpgradeFromToCheck( await managingDao.connect(deployer).grant(...grantArgs); } + let call; + if (reinitializerName && reinitArgs) { + call = { + fn: reinitializerName, + args: reinitArgs, + }; + } + // Upgrade the proxy to a new implementation from a different factory - await upgrades.upgradeProxy(proxy.address, to.connect(upgrader), { - unsafeAllow: ['constructor'], + proxy = await upgrades.upgradeProxy(proxy.address, to.connect(upgrader), { + unsafeAllow: ['constructor', 'delegatecall'], constructorArgs: [], + call: call, }); const toImplementation = await ethers.provider