diff --git a/contracts/src/mocks/plugin/PluginCloneableMock.sol b/contracts/src/mocks/plugin/PluginCloneableMock.sol index 71dfd8bf..de4c5bbc 100644 --- a/contracts/src/mocks/plugin/PluginCloneableMock.sol +++ b/contracts/src/mocks/plugin/PluginCloneableMock.sol @@ -16,6 +16,23 @@ contract PluginCloneableMockBuild1 is PluginCloneable { __PluginCloneable_init(_dao); state1 = 1; } + + function execute( + uint256 _callId, + IDAO.Action[] memory _actions, + uint256 _allowFailureMap + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute(bytes32(_callId), _actions, _allowFailureMap); + } + + function execute( + address _target, + uint256 _callId, + IDAO.Action[] memory _actions, + uint256 _allowFailureMap + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute(_target, bytes32(_callId), _actions, _allowFailureMap); + } } /// @notice A mock cloneable plugin to be deployed via the minimal proxy pattern. diff --git a/contracts/src/mocks/plugin/PluginMock.sol b/contracts/src/mocks/plugin/PluginMock.sol index 4fdaa558..3f1ab5e2 100644 --- a/contracts/src/mocks/plugin/PluginMock.sol +++ b/contracts/src/mocks/plugin/PluginMock.sol @@ -14,4 +14,21 @@ contract PluginMockBuild1 is Plugin { constructor(IDAO _dao) Plugin(_dao) { state1 = 1; } + + function execute( + uint256 _callId, + IDAO.Action[] memory _actions, + uint256 _allowFailureMap + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute(bytes32(_callId), _actions, _allowFailureMap); + } + + function execute( + address _target, + uint256 _callId, + IDAO.Action[] memory _actions, + uint256 _allowFailureMap + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute(_target, bytes32(_callId), _actions, _allowFailureMap); + } } diff --git a/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol b/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol index ea9d5306..e6429577 100644 --- a/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol +++ b/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol @@ -16,6 +16,23 @@ contract PluginUUPSUpgradeableMockBuild1 is PluginUUPSUpgradeable { __PluginUUPSUpgradeable_init(_dao); state1 = 1; } + + function execute( + uint256 _callId, + IDAO.Action[] memory _actions, + uint256 _allowFailureMap + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute(bytes32(_callId), _actions, _allowFailureMap); + } + + function execute( + address _target, + uint256 _callId, + IDAO.Action[] memory _actions, + uint256 _allowFailureMap + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute(_target, bytes32(_callId), _actions, _allowFailureMap); + } } /// @notice A mock upgradeable plugin to be deployed via the UUPS proxy pattern. diff --git a/contracts/src/mocks/plugin/extensions/proposal/ProposalMock.sol b/contracts/src/mocks/plugin/extensions/proposal/ProposalMock.sol new file mode 100644 index 00000000..b4624926 --- /dev/null +++ b/contracts/src/mocks/plugin/extensions/proposal/ProposalMock.sol @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.8; + +import {Proposal} from "../../../../plugin/extensions/proposal/Proposal.sol"; +import {IDAO} from "../../../../dao/IDAO.sol"; + +/// @notice A mock contract. +/// @dev DO NOT USE IN PRODUCTION! +contract ProposalMock is Proposal { + // We don't need to test these below functions as they will be tested in the actual plugins. + // This mock contract is only used to test `supportsInterface` function. + + function createProposal( + bytes memory data, + IDAO.Action[] memory actions, + uint64 startDate, + uint64 endDate + ) external returns (uint256 proposalId) {} + + function canExecute(uint256 proposalId) external returns (bool) {} +} diff --git a/contracts/src/mocks/plugin/extensions/proposal/ProposalUpgradeableMock.sol b/contracts/src/mocks/plugin/extensions/proposal/ProposalUpgradeableMock.sol new file mode 100644 index 00000000..7595cba6 --- /dev/null +++ b/contracts/src/mocks/plugin/extensions/proposal/ProposalUpgradeableMock.sol @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.8; + +import {ProposalUpgradeable} from "../../../../plugin/extensions/proposal/ProposalUpgradeable.sol"; +import {IDAO} from "../../../../dao/IDAO.sol"; + +/// @notice A mock contract. +/// @dev DO NOT USE IN PRODUCTION! +contract ProposalUpgradeableMock is ProposalUpgradeable { + // We don't need to test these below functions as they will be tested in the actual plugins. + // This mock contract is only used to test `supportsInterface` function. + + function createProposal( + bytes memory data, + IDAO.Action[] memory actions, + uint64 startDate, + uint64 endDate + ) external returns (uint256 proposalId) {} + + function canExecute(uint256 proposalId) external returns (bool) {} +} diff --git a/contracts/src/plugin/Plugin.sol b/contracts/src/plugin/Plugin.sol index 0b5d5997..939cc72d 100644 --- a/contracts/src/plugin/Plugin.sol +++ b/contracts/src/plugin/Plugin.sol @@ -15,11 +15,14 @@ import {IPlugin} from "./IPlugin.sol"; /// @notice An abstract, non-upgradeable contract to inherit from when creating a plugin being deployed via the `new` keyword. /// @custom:security-contact sirt@aragon.org abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion { - address public target; + address private target; /// @dev Emitted each time the Target is set. event TargetSet(address indexed previousTarget, address indexed newTarget); + /// @notice The ID of the permission required to call the `setTarget` function. + bytes32 public constant SET_TARGET_PERMISSION_ID = keccak256("SET_TARGET_PERMISSION"); + /// @notice Constructs the plugin by storing the associated DAO. /// @param _dao The DAO contract. constructor(IDAO _dao) DaoAuthorizable(_dao) {} @@ -30,11 +33,14 @@ abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion { } /// @dev Sets the target to a new target (`newTarget`). - /// @notice Can only be called by the current owner. TODO - function setTarget(address _target) public { - address previousTarget = target; - target = _target; - emit TargetSet(previousTarget, _target); + /// @param _target The target contract. + function setTarget(address _target) public auth(SET_TARGET_PERMISSION_ID) { + _setTarget(_target); + } + + /// @notice Returns the currently set target contract. + function getTarget() public view returns (address) { + return target; } /// @notice Checks if this or the parent contract supports an interface by its ID. @@ -44,9 +50,18 @@ abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion { return _interfaceId == type(IPlugin).interfaceId || _interfaceId == type(IProtocolVersion).interfaceId || + _interfaceId == this.setTarget.selector ^ this.getTarget.selector || super.supportsInterface(_interfaceId); } + /// @notice Sets the target to a new target (`newTarget`). + /// @param _target The target contract. + function _setTarget(address _target) internal virtual { + address previousTarget = target; + target = _target; + emit TargetSet(previousTarget, _target); + } + /// @notice Forwards the actions to the currently set `target` for the execution. /// @param _callId Identifier for this execution. /// @param _actions actions that will be eventually called. diff --git a/contracts/src/plugin/PluginCloneable.sol b/contracts/src/plugin/PluginCloneable.sol index 3f5d86ab..c9c01437 100644 --- a/contracts/src/plugin/PluginCloneable.sol +++ b/contracts/src/plugin/PluginCloneable.sol @@ -20,11 +20,14 @@ abstract contract PluginCloneable is DaoAuthorizableUpgradeable, ProtocolVersion { - address public target; + address private target; /// @dev Emitted each time the Target is set. event TargetSet(address indexed previousTarget, address indexed newTarget); + /// @notice The ID of the permission required to call the `setTarget` function. + bytes32 public constant SET_TARGET_PERMISSION_ID = keccak256("SET_TARGET_PERMISSION"); + /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized. /// @custom:oz-upgrades-unsafe-allow constructor constructor() { @@ -39,11 +42,9 @@ abstract contract PluginCloneable is } /// @dev Sets the target to a new target (`newTarget`). - /// @notice Can only be called by the current owner. TODO - function setTarget(address _target) public { - address previousTarget = target; - target = _target; - emit TargetSet(previousTarget, _target); + /// @param _target The target contract. + function setTarget(address _target) public auth(SET_TARGET_PERMISSION_ID) { + _setTarget(_target); } /// @inheritdoc IPlugin @@ -51,6 +52,11 @@ abstract contract PluginCloneable is return PluginType.Cloneable; } + /// @notice Returns the currently set target contract. + function getTarget() public view returns (address) { + return target; + } + /// @notice Checks if this or the parent contract supports an interface by its ID. /// @param _interfaceId The ID of the interface. /// @return Returns `true` if the interface is supported. @@ -58,9 +64,18 @@ abstract contract PluginCloneable is return _interfaceId == type(IPlugin).interfaceId || _interfaceId == type(IProtocolVersion).interfaceId || + _interfaceId == this.setTarget.selector ^ this.getTarget.selector || super.supportsInterface(_interfaceId); } + /// @notice Sets the target to a new target (`newTarget`). + /// @param _target The target contract. + function _setTarget(address _target) internal virtual { + address previousTarget = target; + target = _target; + emit TargetSet(previousTarget, _target); + } + /// @notice Forwards the actions to the currently set `target` for the execution. /// @param _callId Identifier for this execution. /// @param _actions actions that will be eventually called. diff --git a/contracts/src/plugin/PluginUUPSUpgradeable.sol b/contracts/src/plugin/PluginUUPSUpgradeable.sol index a3159438..ab13dc0b 100644 --- a/contracts/src/plugin/PluginUUPSUpgradeable.sol +++ b/contracts/src/plugin/PluginUUPSUpgradeable.sol @@ -24,11 +24,17 @@ abstract contract PluginUUPSUpgradeable is ProtocolVersion { // NOTE: When adding new state variables to the contract, the size of `_gap` has to be adapted below as well. - address public target; + address private target; /// @dev Emitted each time the Target is set. event TargetSet(address indexed previousTarget, address indexed newTarget); + /// @notice The ID of the permission required to call the `setTarget` function. + bytes32 public constant SET_TARGET_PERMISSION_ID = keccak256("SET_TARGET_PERMISSION"); + + /// @notice The ID of the permission required to call the `_authorizeUpgrade` function. + bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION"); + /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized. /// @custom:oz-upgrades-unsafe-allow constructor constructor() { @@ -40,8 +46,10 @@ abstract contract PluginUUPSUpgradeable is return PluginType.UUPS; } - /// @notice The ID of the permission required to call the `_authorizeUpgrade` function. - bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION"); + /// @notice Returns the currently set target contract. + function getTarget() public view returns (address) { + return target; + } /// @notice Initializes the plugin by storing the associated DAO. /// @param _dao The DAO contract. @@ -51,11 +59,9 @@ abstract contract PluginUUPSUpgradeable is } /// @dev Sets the target to a new target (`newTarget`). - /// @notice Can only be called by the current owner. TODO - function setTarget(address _target) public { - address previousTarget = target; - target = _target; - emit TargetSet(previousTarget, _target); + /// @param _target The target contract. + function setTarget(address _target) public auth(SET_TARGET_PERMISSION_ID) { + _setTarget(_target); } /// @notice Checks if an interface is supported by this or its parent contract. @@ -66,6 +72,7 @@ abstract contract PluginUUPSUpgradeable is _interfaceId == type(IPlugin).interfaceId || _interfaceId == type(IProtocolVersion).interfaceId || _interfaceId == type(IERC1822ProxiableUpgradeable).interfaceId || + _interfaceId == this.setTarget.selector ^ this.getTarget.selector || super.supportsInterface(_interfaceId); } @@ -75,6 +82,14 @@ abstract contract PluginUUPSUpgradeable is return _getImplementation(); } + /// @notice Sets the target to a new target (`newTarget`). + /// @param _target The target contract. + function _setTarget(address _target) internal virtual { + address previousTarget = target; + target = _target; + emit TargetSet(previousTarget, _target); + } + /// @notice Forwards the actions to the currently set `target` for the execution. /// @param _callId Identifier for this execution. /// @param _actions actions that will be eventually called. diff --git a/contracts/src/plugin/extensions/proposal/IProposal.sol b/contracts/src/plugin/extensions/proposal/IProposal.sol index a74d0f67..93f7305d 100644 --- a/contracts/src/plugin/extensions/proposal/IProposal.sol +++ b/contracts/src/plugin/extensions/proposal/IProposal.sol @@ -50,7 +50,8 @@ interface IProposal { function canExecute(uint256 proposalId) external returns (bool); /// @notice Returns the proposal count determining the next proposal ID. - /// @dev This function is deprecated TODO: + /// @dev This function has been deprecated but due to backwards compatibility, it still stays in the interface + /// but returns maximum value of uint256 to let consumers know not to depend on it anymore. /// @return The proposal count. function proposalCount() external view returns (uint256); } diff --git a/contracts/src/plugin/extensions/proposal/Proposal.sol b/contracts/src/plugin/extensions/proposal/Proposal.sol index 1538c23e..eb16e1de 100644 --- a/contracts/src/plugin/extensions/proposal/Proposal.sol +++ b/contracts/src/plugin/extensions/proposal/Proposal.sol @@ -2,10 +2,8 @@ pragma solidity ^0.8.8; -import {Counters} from "@openzeppelin/contracts/utils/Counters.sol"; import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; -import {IDAO} from "../../../dao/IDAO.sol"; import {IProposal} from "./IProposal.sol"; /// @title Proposal @@ -13,16 +11,9 @@ import {IProposal} from "./IProposal.sol"; /// @notice An abstract contract containing the traits and internal functionality to create and execute proposals that can be inherited by non-upgradeable DAO plugins. /// @custom:security-contact sirt@aragon.org abstract contract Proposal is IProposal, ERC165 { - using Counters for Counters.Counter; - - /// @notice The incremental ID for proposals and executions. - Counters.Counter private proposalCounter; - - /// Shall we remove this ? Does anyone use this ? if we keep having this, - // this will not return the correct value anyways anymore. /// @inheritdoc IProposal - function proposalCount() public view override returns (uint256) { - return 0; + function proposalCount() public pure override returns (uint256) { + return type(uint256).max; } /// @notice Checks if this or the parent contract supports an interface by its ID. diff --git a/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol b/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol index fce033f7..283be610 100644 --- a/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol +++ b/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol @@ -19,8 +19,8 @@ abstract contract ProposalUpgradeable is IProposal, ERC165Upgradeable { CountersUpgradeable.Counter private proposalCounter; /// @inheritdoc IProposal - function proposalCount() public view override returns (uint256) { - return proposalCounter.current(); + function proposalCount() public pure override returns (uint256) { + return type(uint256).max; } /// @notice Checks if this or the parent contract supports an interface by its ID. diff --git a/contracts/test/plugin/extensions/proposal.ts b/contracts/test/plugin/extensions/proposal.ts index d43d2bb1..5e9958e7 100644 --- a/contracts/test/plugin/extensions/proposal.ts +++ b/contracts/test/plugin/extensions/proposal.ts @@ -2,39 +2,20 @@ import { DAOMock, DAOMock__factory, IDAO, - IDAO__factory, IProposal__factory, ProposalMock, ProposalUpgradeableMock, ProposalMock__factory, ProposalUpgradeableMock__factory, } from '../../../typechain'; -import {ExecutedEvent} from '../../../typechain/src/dao/IDAO'; import {erc165ComplianceTests} from '../../helpers'; -import {findEventTopicLog, getInterfaceId} from '@aragon/osx-commons-sdk'; +import {getInterfaceId} from '@aragon/osx-commons-sdk'; import {IProposal__factory as IProposal_V1_0_0__factory} from '@aragon/osx-ethers-v1.0.0'; -import {loadFixture, time} from '@nomicfoundation/hardhat-network-helpers'; -import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; +import {loadFixture} from '@nomicfoundation/hardhat-network-helpers'; import {expect} from 'chai'; import {BigNumberish, BytesLike} from 'ethers'; import {ethers} from 'hardhat'; -export type ProposalData = { - metadata: BytesLike; - startDate: BigNumberish; - endDate: BigNumberish; - actions: IDAO.ActionStruct[]; - allowFailureMap: BigNumberish; -}; - -describe('IProposal', function () { - it('has the same interface ID as its initial version introduced in v1.0.0', async () => { - const current = getInterfaceId(IProposal__factory.createInterface()); - const initial = getInterfaceId(IProposal_V1_0_0__factory.createInterface()); - expect(current).to.equal(initial); - }); -}); - describe('Proposal', async () => { proposalBaseTests(proposalFixture); }); @@ -45,231 +26,65 @@ describe('ProposalUpgradeable', async () => { // Contains tests for functionality common for `ProposalMock` and `ProposalUpgradeableMock` to avoid duplication. function proposalBaseTests(fixture: () => Promise) { - it('counts proposals', async () => { - const {alice, bob, proposalMock, exampleData} = await loadFixture(fixture); - - expect(await proposalMock.proposalCount()).to.equal(0); - - const creator = bob; - - await proposalMock - .connect(alice) - .createProposal( - creator.address, - exampleData.metadata, - exampleData.startDate, - exampleData.endDate, - exampleData.actions, - exampleData.allowFailureMap - ); - - expect(await proposalMock.proposalCount()).to.equal(1); - }); - - it('creates proposalIds', async () => { + it('returns max value of integer for backwards-compatibility', async () => { const {proposalMock} = await loadFixture(fixture); - const expectedProposalId = 0; - const proposalId = await proposalMock.callStatic.createProposalId(); - - expect(proposalId).to.equal(expectedProposalId); - }); - - it('creates proposals', async () => { - const {alice, bob, proposalMock, exampleData} = await loadFixture(fixture); - - const expectedProposalId = 0; - const creator = bob; - - const proposalId = await proposalMock - .connect(alice) - .callStatic.createProposal( - creator.address, - exampleData.metadata, - exampleData.startDate, - exampleData.endDate, - exampleData.actions, - exampleData.allowFailureMap - ); - - expect(proposalId).to.equal(expectedProposalId); - }); - - it('emits the `ProposalCreated` event', async () => { - const {alice, bob, proposalMock, exampleData} = await loadFixture(fixture); - - const expectedProposalId = 0; - const creator = bob; - - await expect( - proposalMock - .connect(alice) - .createProposal( - creator.address, - exampleData.metadata, - exampleData.startDate, - exampleData.endDate, - exampleData.actions, - exampleData.allowFailureMap - ) - ) - .to.emit(proposalMock, 'ProposalCreated') - .withArgs( - expectedProposalId, - creator.address, - exampleData.startDate, - exampleData.endDate, - exampleData.metadata, - exampleData.actions, - exampleData.allowFailureMap - ); - }); - it('executes proposals', async () => { - const {alice, daoMock, proposalMock, exampleData} = await loadFixture( - fixture + expect(await proposalMock.proposalCount()).to.equal( + ethers.constants.MaxUint256 ); - - const expectedExecResults: string[] = []; - const expectedFailureMap: BigNumberish = 0; - - const proposalId = 0; - const [execResults, failureMap] = await proposalMock - .connect(alice) - .callStatic.executeProposal( - daoMock.address, - proposalId, - exampleData.actions, - exampleData.allowFailureMap - ); - - expect(execResults).to.deep.equal(expectedExecResults); - expect(failureMap).to.equal(expectedFailureMap); - }); - - it('emits the `ProposalExecuted` event', async () => { - const {alice, daoMock, proposalMock, exampleData} = await loadFixture( - fixture - ); - - const proposalId = 0; - - await expect( - proposalMock - .connect(alice) - .executeProposal( - daoMock.address, - proposalId, - exampleData.actions, - exampleData.allowFailureMap - ) - ) - .to.emit(proposalMock, 'ProposalExecuted') - .withArgs(proposalId); - }); - - it('emits the `Executed` event on the executing DAO', async () => { - const {alice, daoMock, proposalMock, exampleData} = await loadFixture( - fixture - ); - - const proposalId = 0; - const proposalIdAsBytes32 = ethers.utils.hexZeroPad( - ethers.utils.hexlify(proposalId), - 32 - ); - - const expectedActor = proposalMock.address; - const expectedExecResults: string[] = []; - const expectedFailureMap: BigNumberish = 0; - - const tx = await proposalMock - .connect(alice) - .executeProposal( - daoMock.address, - proposalId, - exampleData.actions, - exampleData.allowFailureMap - ); - const event = findEventTopicLog( - await tx.wait(), - IDAO__factory.createInterface(), - 'Executed' - ); - - expect(event.args.actor).to.equal(expectedActor); - expect(event.args.callId).to.equal(proposalIdAsBytes32); - expect(event.args.actions).to.deep.equal(exampleData.actions); - expect(event.args.allowFailureMap).to.equal(exampleData.allowFailureMap); - expect(event.args.failureMap).to.equal(expectedFailureMap); - expect(event.args.execResults).to.deep.equal(expectedExecResults); }); describe('ERC-165', async () => { it('supports the `ERC-165` standard', async () => { - const {alice, proposalMock} = await loadFixture(fixture); - await erc165ComplianceTests(proposalMock, alice); + const {proposalMock} = await loadFixture(fixture); + const signers = await ethers.getSigners(); + await erc165ComplianceTests(proposalMock, signers[0]); }); - it('supports the `IProposal` interface', async () => { + it('supports the `IProposal` interface for the new as well as old versions', async () => { const {proposalMock} = await loadFixture(fixture); - const iface = IProposal__factory.createInterface(); - expect(await proposalMock.supportsInterface(getInterfaceId(iface))).to.be - .true; + const newIface = IProposal__factory.createInterface(); + expect(await proposalMock.supportsInterface(getInterfaceId(newIface))).to + .be.true; + + const oldIface = IProposal_V1_0_0__factory.createInterface(); + expect(await proposalMock.supportsInterface(getInterfaceId(oldIface))).to + .be.true; }); }); } type BaseFixtureResult = { - alice: SignerWithAddress; - bob: SignerWithAddress; daoMock: DAOMock; - exampleData: ProposalData; }; async function baseFixture(): Promise { - const [alice, bob] = await ethers.getSigners(); - const daoMock = await new DAOMock__factory(alice).deploy(); - - const uri = ethers.utils.hexlify( - ethers.utils.toUtf8Bytes( - 'ipfs://QmTMcfhxgYA3ziwpnhEg1K3aFn7ixMH9dBxWXs5YTJdZwR' - ) - ); - const current = await time.latest(); + const signers = await ethers.getSigners(); + const daoMock = await new DAOMock__factory(signers[0]).deploy(); - const exampleData = { - metadata: uri, - startDate: current, - endDate: current + 12, - actions: [], - allowFailureMap: 0, - }; - - return {alice, bob, daoMock, exampleData}; + return {daoMock}; } type FixtureResult = { proposalMock: ProposalMock | ProposalUpgradeableMock; - alice: SignerWithAddress; - bob: SignerWithAddress; daoMock: DAOMock; - exampleData: ProposalData; }; async function proposalFixture(): Promise { - const {alice, bob, daoMock, exampleData} = await baseFixture(); - - const proposalMock = await new ProposalMock__factory(alice).deploy(); + const {daoMock} = await baseFixture(); + const signers = await ethers.getSigners(); + const proposalMock = await new ProposalMock__factory(signers[0]).deploy(); - return {alice, bob, proposalMock, daoMock, exampleData}; + return {proposalMock, daoMock}; } async function proposalUpgradeableFixture(): Promise { - const {alice, bob, daoMock, exampleData} = await baseFixture(); + const {daoMock} = await baseFixture(); + const signers = await ethers.getSigners(); const proposalMock = await new ProposalUpgradeableMock__factory( - alice + signers[0] ).deploy(); - return {alice, bob, proposalMock, daoMock, exampleData}; + return {proposalMock, daoMock}; } diff --git a/contracts/test/plugin/plugin-clonable.ts b/contracts/test/plugin/plugin-clonable.ts index 4166216e..072d30c9 100644 --- a/contracts/test/plugin/plugin-clonable.ts +++ b/contracts/test/plugin/plugin-clonable.ts @@ -112,6 +112,17 @@ describe('PluginCloneable', function () { expect(await implementation.supportsInterface(getInterfaceId(iface))).to .be.true; }); + + it('supports the `setTarget^getTarget` interface', async () => { + const {implementation} = await loadFixture(fixture); + const iface = PluginCloneableMockBuild1__factory.createInterface(); + + let interfaceId = ethers.BigNumber.from(iface.getSighash('setTarget')) + .xor(ethers.BigNumber.from(iface.getSighash('getTarget'))) + .toHexString(); + + expect(await implementation.supportsInterface(interfaceId)).to.be.true; + }); }); describe('Protocol version', async () => { @@ -122,12 +133,96 @@ describe('PluginCloneable', function () { ); }); }); + + describe('setTarget/getTarget', async () => { + it('reverts if caller does not have the permission', async () => { + const {deployer, proxy, daoMock} = await loadFixture(fixture); + + let newTarget = proxy.address; + + await expect(proxy.setTarget(newTarget)) + .to.be.revertedWithCustomError(proxy, 'DaoUnauthorized') + .withArgs( + daoMock.address, + proxy.address, + deployer.address, + ethers.utils.id('SET_TARGET_PERMISSION') + ); + }); + + it('updates the target and emits an appropriate event', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + // Set the `hasPermission` mock function to return `true`. + await daoMock.setHasPermissionReturnValueMock(true); // answer true for all permission requests + + let newTarget = proxy.address; + + await expect(proxy.setTarget(newTarget)) + .to.emit(proxy, 'TargetSet') + .withArgs(ethers.constants.AddressZero, newTarget); + + expect(await proxy.getTarget()).to.equal(newTarget); + }); + }); + + describe('Executions', async () => { + describe('execute with current target', async () => { + it('reverts with ambiguity if target is not set', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)'](1, [], 0) + ).to.be.reverted; + }); + + it('successfully forwards action to the currently set target', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + await daoMock.setHasPermissionReturnValueMock(true); + await proxy.setTarget(daoMock.address); + + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)'](1, [], 0) + ).to.emit(daoMock, 'Executed'); + }); + }); + + describe('execute with the custom target', async () => { + it('reverts with ambiguity if target address(0) is passed', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + await expect( + proxy['execute(address,uint256,(address,uint256,bytes)[],uint256)']( + ethers.constants.AddressZero, + 1, + [], + 0 + ) + ).to.be.reverted; + }); + + it('successfully forwards action to the currently set target', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + await expect( + proxy['execute(address,uint256,(address,uint256,bytes)[],uint256)']( + daoMock.address, + 1, + [], + 0 + ) + ).to.emit(daoMock, 'Executed'); + }); + }); + }); }); type FixtureResult = { deployer: SignerWithAddress; implementation: PluginCloneableMockBuild1; proxyFactory: ProxyFactory; + proxy: PluginCloneableMockBuild1; daoMock: DAOMock; initCalldata: string; Build1Factory: PluginCloneableMockBuild1__factory; @@ -150,10 +245,15 @@ async function fixture(): Promise { [daoMock.address] ); + const tx = await proxyFactory.deployUUPSProxy(initCalldata); + const event = findEvent(await tx.wait(), 'ProxyCreated'); + const proxy = Build1Factory.attach(event.args.proxy); + return { deployer, implementation, proxyFactory, + proxy, daoMock, initCalldata, Build1Factory, diff --git a/contracts/test/plugin/plugin-uups-upgradeable.ts b/contracts/test/plugin/plugin-uups-upgradeable.ts index 03501e8a..237c578a 100644 --- a/contracts/test/plugin/plugin-uups-upgradeable.ts +++ b/contracts/test/plugin/plugin-uups-upgradeable.ts @@ -125,6 +125,17 @@ describe('PluginUUPSUpgradeable', function () { expect(await implementation.supportsInterface(getInterfaceId(iface))).to .be.true; }); + + it('supports the `setTarget^getTarget` interface', async () => { + const {implementation} = await loadFixture(fixture); + const iface = PluginUUPSUpgradeableMockBuild1__factory.createInterface(); + + let interfaceId = ethers.BigNumber.from(iface.getSighash('setTarget')) + .xor(ethers.BigNumber.from(iface.getSighash('getTarget'))) + .toHexString(); + + expect(await implementation.supportsInterface(interfaceId)).to.be.true; + }); }); describe('Protocol version', async () => { @@ -136,6 +147,89 @@ describe('PluginUUPSUpgradeable', function () { }); }); + describe('setTarget/getTarget', async () => { + it('reverts if caller does not have the permission', async () => { + const {deployer, proxy, daoMock} = await loadFixture(fixture); + + let newTarget = proxy.address; + + await expect(proxy.setTarget(newTarget)) + .to.be.revertedWithCustomError(proxy, 'DaoUnauthorized') + .withArgs( + daoMock.address, + proxy.address, + deployer.address, + ethers.utils.id('SET_TARGET_PERMISSION') + ); + }); + + it('updates the target and emits an appropriate event', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + // Set the `hasPermission` mock function to return `true`. + await daoMock.setHasPermissionReturnValueMock(true); // answer true for all permission requests + + let newTarget = proxy.address; + + await expect(proxy.setTarget(newTarget)) + .to.emit(proxy, 'TargetSet') + .withArgs(ethers.constants.AddressZero, newTarget); + + expect(await proxy.getTarget()).to.equal(newTarget); + }); + }); + + describe('Executions', async () => { + describe('execute with current target', async () => { + it('reverts with ambiguity if target is not set', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)'](1, [], 0) + ).to.be.reverted; + }); + + it('successfully forwards action to the currently set target', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + await daoMock.setHasPermissionReturnValueMock(true); + await proxy.setTarget(daoMock.address); + + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)'](1, [], 0) + ).to.emit(daoMock, 'Executed'); + }); + }); + + describe('execute with the custom target', async () => { + it('reverts with ambiguity if target address(0) is passed', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + await expect( + proxy['execute(address,uint256,(address,uint256,bytes)[],uint256)']( + ethers.constants.AddressZero, + 1, + [], + 0 + ) + ).to.be.reverted; + }); + + it('successfully forwards action to the currently set target', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + await expect( + proxy['execute(address,uint256,(address,uint256,bytes)[],uint256)']( + daoMock.address, + 1, + [], + 0 + ) + ).to.emit(daoMock, 'Executed'); + }); + }); + }); + describe('Upgradeability', async () => { it('returns the implementation', async () => { const {proxyFactory, Build1Factory, implementation} = await loadFixture( @@ -273,6 +367,7 @@ type FixtureResult = { deployer: SignerWithAddress; implementation: PluginUUPSUpgradeableMockBuild1; proxyFactory: ProxyFactory; + proxy: PluginUUPSUpgradeableMockBuild1; daoMock: DAOMock; initCalldata: string; Build1Factory: PluginUUPSUpgradeableMockBuild1__factory; @@ -297,10 +392,15 @@ async function fixture(): Promise { [daoMock.address] ); + const tx = await proxyFactory.deployUUPSProxy(initCalldata); + const event = findEvent(await tx.wait(), 'ProxyCreated'); + const proxy = Build1Factory.attach(event.args.proxy); + return { deployer, implementation, proxyFactory, + proxy, initCalldata, daoMock, Build1Factory, diff --git a/contracts/test/plugin/plugin.ts b/contracts/test/plugin/plugin.ts index d5d8342f..923e30c8 100644 --- a/contracts/test/plugin/plugin.ts +++ b/contracts/test/plugin/plugin.ts @@ -55,6 +55,17 @@ describe('Plugin', function () { const iface = IProtocolVersion__factory.createInterface(); expect(await plugin.supportsInterface(getInterfaceId(iface))).to.be.true; }); + + it('supports the `setTarget^getTarget` interface', async () => { + const {plugin} = await loadFixture(fixture); + const iface = PluginMockBuild1__factory.createInterface(); + + let interfaceId = ethers.BigNumber.from(iface.getSighash('setTarget')) + .xor(ethers.BigNumber.from(iface.getSighash('getTarget'))) + .toHexString(); + + expect(await plugin.supportsInterface(interfaceId)).to.be.true; + }); }); describe('Protocol version', async () => { @@ -65,6 +76,89 @@ describe('Plugin', function () { ); }); }); + + describe('setTarget', async () => { + it('reverts if caller does not have the permission', async () => { + const {deployer, plugin, daoMock} = await loadFixture(fixture); + + let newTarget = plugin.address; + + await expect(plugin.setTarget(newTarget)) + .to.be.revertedWithCustomError(plugin, 'DaoUnauthorized') + .withArgs( + daoMock.address, + plugin.address, + deployer.address, + ethers.utils.id('SET_TARGET_PERMISSION') + ); + }); + + it('updates the target and emits an appropriate event', async () => { + const {plugin, daoMock} = await loadFixture(fixture); + + let newTarget = plugin.address; + + // Set the `hasPermission` mock function to return `true`. + await daoMock.setHasPermissionReturnValueMock(true); // answer true for all permission requests + + await expect(plugin.setTarget(newTarget)) + .to.emit(plugin, 'TargetSet') + .withArgs(ethers.constants.AddressZero, newTarget); + + expect(await plugin.getTarget()).to.equal(newTarget); + }); + }); + + describe('Executions', async () => { + describe('execute with current target', async () => { + it('reverts with ambiguity if target is not set', async () => { + const {plugin, daoMock} = await loadFixture(fixture); + + await expect( + plugin['execute(uint256,(address,uint256,bytes)[],uint256)'](1, [], 0) + ).to.be.reverted; + }); + + it('successfully forwards action to the currently set target', async () => { + const {plugin, daoMock} = await loadFixture(fixture); + + await daoMock.setHasPermissionReturnValueMock(true); + await plugin.setTarget(daoMock.address); + + await expect( + plugin['execute(uint256,(address,uint256,bytes)[],uint256)'](1, [], 0) + ).to.emit(daoMock, 'Executed'); + }); + }); + + describe('execute with the custom target', async () => { + it('reverts with ambiguity if target address(0) is passed', async () => { + const {plugin, daoMock} = await loadFixture(fixture); + + await expect( + plugin['execute(address,uint256,(address,uint256,bytes)[],uint256)']( + ethers.constants.AddressZero, + 1, + [], + 0 + ) + ).to.be.reverted; + }); + + it('successfully forwards action to the currently set target', async () => { + const {plugin, daoMock} = await loadFixture(fixture); + + await expect( + plugin['execute(address,uint256,(address,uint256,bytes)[],uint256)']( + daoMock.address, + 1, + [], + 0 + ) + ).to.emit(daoMock, 'Executed'); + }); + }); + }); }); async function fixture(): Promise {