From 0717be213682fa83cc372ee923637be6dd240823 Mon Sep 17 00:00:00 2001 From: Michael Heuer Date: Tue, 24 Oct 2023 09:51:25 +0200 Subject: [PATCH] refactor: revert proxy factory addition --- contracts/src/utils/proxy/ProxyFactory.sol | 43 ----- contracts/src/utils/proxy/ProxyLib.sol | 43 ----- .../plugin/majority-voting/majority-voting.ts | 10 +- .../test/token/erc20/governance-erc20.ts | 10 +- contracts/test/utils/proxy/proxy-factory.ts | 182 ------------------ contracts/utils/proxy.ts | 79 +++----- 6 files changed, 33 insertions(+), 334 deletions(-) delete mode 100644 contracts/src/utils/proxy/ProxyFactory.sol delete mode 100644 contracts/src/utils/proxy/ProxyLib.sol delete mode 100644 contracts/test/utils/proxy/proxy-factory.ts diff --git a/contracts/src/utils/proxy/ProxyFactory.sol b/contracts/src/utils/proxy/ProxyFactory.sol deleted file mode 100644 index 4cd044f3..00000000 --- a/contracts/src/utils/proxy/ProxyFactory.sol +++ /dev/null @@ -1,43 +0,0 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later - -pragma solidity ^0.8.8; - -import {ProxyLib} from "./ProxyLib.sol"; - -/// @title ProxyFactory -/// @author Aragon Association - 2023 -/// @notice A factory to deploy proxies via the UUPS pattern (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)) and minimal proxy pattern (see [ERC-1167](https://eips.ethereum.org/EIPS/eip-1167)). -/// @custom:security-contact sirt@aragon.org -contract ProxyFactory { - using ProxyLib for address; - /// @notice The immutable logic contract address. - address public immutable LOGIC; - - /// @notice Emitted when an proxy contract is created. - /// @param proxy The proxy address. - event ProxyCreated(address proxy); - - /// @notice Initializes the contract with a logic contract address. - /// @param _logic The logic contract address. - constructor(address _logic) { - LOGIC = _logic; - } - - /// @notice Creates an [ERC-1967](https://eips.ethereum.org/EIPS/eip-1967) proxy contract pointing to the pre-set logic contract. - /// @param _data The initialization data for this contract. - /// @return proxy The address of the proxy contract created. - /// @dev If `_data` is non-empty, it is used in a delegate call to the `_logic` contract. This will typically be an encoded function call initializing the proxy (see [OpenZeppelin ERC1967Proxy-constructor](https://docs.openzeppelin.com/contracts/4.x/api/proxy#ERC1967Proxy-constructor-address-bytes-)). - function deployUUPSProxy(bytes memory _data) external returns (address proxy) { - proxy = LOGIC.deployUUPSProxy(_data); - emit ProxyCreated({proxy: proxy}); - } - - /// @notice Creates an [ERC-1167](https://eips.ethereum.org/EIPS/eip-1167) minimal proxy contract pointing to the pre-set logic contract. - /// @param _data The initialization data for this contract. - /// @return proxy The address of the proxy contract created. - /// @dev If `_data` is non-empty, it is used in a call to the clone contract. This will typically be an encoded function call initializing the storage of the contract. - function deployMinimalProxy(bytes memory _data) external returns (address proxy) { - proxy = LOGIC.deployMinimalProxy(_data); - emit ProxyCreated({proxy: proxy}); - } -} diff --git a/contracts/src/utils/proxy/ProxyLib.sol b/contracts/src/utils/proxy/ProxyLib.sol deleted file mode 100644 index 476770e1..00000000 --- a/contracts/src/utils/proxy/ProxyLib.sol +++ /dev/null @@ -1,43 +0,0 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later - -pragma solidity ^0.8.8; - -import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; -import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; -import {Address} from "@openzeppelin/contracts/utils/Address.sol"; - -/// @title ProxyLib -/// @author Aragon Association - 2023 -/// @notice A library containing methods for the deployment of proxies via the UUPS pattern (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)) and minimal proxy pattern (see [ERC-1167](https://eips.ethereum.org/EIPS/eip-1167)). -/// @custom:security-contact sirt@aragon.org -library ProxyLib { - using Clones for address; - using Address for address; - - /// @notice Creates an [ERC-1967](https://eips.ethereum.org/EIPS/eip-1967) UUPS proxy contract pointing to a logic contract. - /// @param _logic The logic contract the proxy is pointing to. - /// @param _data The initialization data for this contract. - /// @return uupsProxy The address of the UUPS proxy contract created. - /// @dev If `_data` is non-empty, it is used in a delegate call to the `_logic` contract. This will typically be an encoded function call initializing the storage of the proxy (see [OpenZeppelin ERC1967Proxy-constructor](https://docs.openzeppelin.com/contracts/4.x/api/proxy#ERC1967Proxy-constructor-address-bytes-)). - function deployUUPSProxy( - address _logic, - bytes memory _data - ) internal returns (address uupsProxy) { - uupsProxy = address(new ERC1967Proxy({_logic: _logic, _data: _data})); - } - - /// @notice Creates an [ERC-1167](https://eips.ethereum.org/EIPS/eip-1167) minimal proxy contract pointing to a logic contract. - /// @param _logic The logic contract the proxy is pointing to. - /// @param _data The initialization data for this contract. - /// @return minimalProxy The address of the minimal proxy contract created. - /// @dev If `_data` is non-empty, it is used in a call to the clone contract. This will typically be an encoded function call initializing the storage of the contract. - function deployMinimalProxy( - address _logic, - bytes memory _data - ) internal returns (address minimalProxy) { - minimalProxy = _logic.clone(); - if (_data.length > 0) { - minimalProxy.functionCall({data: _data}); - } - } -} diff --git a/contracts/test/plugin/majority-voting/majority-voting.ts b/contracts/test/plugin/majority-voting/majority-voting.ts index 9a6ff6b8..a8184e33 100644 --- a/contracts/test/plugin/majority-voting/majority-voting.ts +++ b/contracts/test/plugin/majority-voting/majority-voting.ts @@ -10,7 +10,7 @@ import { IProtocolVersion__factory, } from '../../../typechain'; import {getInterfaceId} from '../../../utils/interfaces'; -import {deployUUPSProxy} from '../../../utils/proxy'; +import {deployWithProxy} from '../../../utils/proxy'; import {pctToRatio} from '../../utils/math/ratio'; import {VotingSettings, VotingMode, ONE_HOUR, ONE_YEAR} from './voting-helpers'; import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; @@ -38,10 +38,8 @@ describe('MajorityVotingMock', function () { signers = await ethers.getSigners(); ownerAddress = await signers[0].getAddress(); - dao = await deployUUPSProxy(new DAO__factory(signers[0]), { - initializerName: 'initialize', - args: [[], ownerAddress, ethers.constants.AddressZero, 'examplURI'], - }); + dao = await deployWithProxy(new DAO__factory(signers[0])); + dao.initialize([], ownerAddress, ethers.constants.AddressZero, 'examplURI'); }); beforeEach(async () => { @@ -55,7 +53,7 @@ describe('MajorityVotingMock', function () { const MajorityVotingBase = new MajorityVotingMock__factory(signers[0]); - votingBase = await deployUUPSProxy(MajorityVotingBase); + votingBase = await deployWithProxy(MajorityVotingBase); await dao.grant( votingBase.address, ownerAddress, diff --git a/contracts/test/token/erc20/governance-erc20.ts b/contracts/test/token/erc20/governance-erc20.ts index 47b155eb..ae637197 100644 --- a/contracts/test/token/erc20/governance-erc20.ts +++ b/contracts/test/token/erc20/governance-erc20.ts @@ -9,8 +9,8 @@ import { IERC20Upgradeable__factory, IVotesUpgradeable__factory, } from '../../../typechain'; -import {deployUUPSProxy} from '../../../utils/events'; import {getInterfaceId} from '../../../utils/interfaces'; +import {deployWithProxy} from '../../../utils/proxy'; import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; import {expect} from 'chai'; import {ethers} from 'hardhat'; @@ -26,7 +26,7 @@ const governanceERC20Symbol = 'GOV'; const addressZero = ethers.constants.AddressZero; -describe('GovernanceERC20', function () { +describe.only('GovernanceERC20', function () { let signers: SignerWithAddress[]; let dao: DAO; let token: GovernanceERC20; @@ -43,10 +43,8 @@ describe('GovernanceERC20', function () { signers = await ethers.getSigners(); GovernanceERC20 = new GovernanceERC20__factory(signers[0]); - dao = await deployUUPSProxy(new DAO__factory(signers[0]), { - initializerName: 'initialize', - args: [[], signers[0].address, addressZero, 'exampleURI'], - }); + dao = await deployWithProxy(new DAO__factory(signers[0])); + await dao.initialize([], signers[0].address, addressZero, 'exampleURI'); from = signers[0]; to = signers[1]; diff --git a/contracts/test/utils/proxy/proxy-factory.ts b/contracts/test/utils/proxy/proxy-factory.ts deleted file mode 100644 index 47f47fb2..00000000 --- a/contracts/test/utils/proxy/proxy-factory.ts +++ /dev/null @@ -1,182 +0,0 @@ -import { - ProxyFactory, - ProxyFactory__factory, - TestGovernanceERC20, - TestGovernanceERC20__factory, -} from '../../../typechain'; -import {ProxyCreatedEvent} from '../../../typechain/src/utils/ProxyFactory'; -import {findEvent} from '../../../utils/events'; -import { - ERC1967_IMPLEMENTATION_SLOT, - OZ_INITIALIZED_SLOT_POSITION, - readStorage, -} from '../../../utils/storage'; -import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; -import {expect} from 'chai'; -import {ethers} from 'hardhat'; - -const MOCK_ADDRESS = `0x${'0123456789'.repeat(4)}`; -const DEFAULT_INITIALIZATION: [ - string, - string, - string, - {receivers: string[]; amounts: number[]} -] = [ - MOCK_ADDRESS, - 'GOV', - 'GOV', - { - receivers: [], - amounts: [], - }, -]; - -describe('ProxyFactory', function () { - let deployer: SignerWithAddress; - let proxyFactory: ProxyFactory; - let logic: TestGovernanceERC20; - - before(async () => { - deployer = (await ethers.getSigners())[0]; - - logic = await new TestGovernanceERC20__factory(deployer).deploy( - ...DEFAULT_INITIALIZATION - ); - proxyFactory = await new ProxyFactory__factory(deployer).deploy( - logic.address - ); - }); - - describe('initialization', async () => { - it('points to the right logic contract', async () => { - expect(await proxyFactory.LOGIC()).to.equal(logic.address); - }); - }); - - describe('deployUUPSProxy', async () => { - it('deploys the proxy unitialized if no data is provided', async () => { - const initData: any = []; - - const tx = await proxyFactory.deployUUPSProxy(initData); - const event = await findEvent(tx, 'ProxyCreated'); - if (!event) { - throw new Error('Failed to get the event'); - } - - const tokenProxy = TestGovernanceERC20__factory.connect( - event.args.proxy, - deployer - ); - - // Check that the proxy points to the right logic contract. - expect( - await readStorage(tokenProxy.address, ERC1967_IMPLEMENTATION_SLOT, [ - 'address', - ]) - ).to.equal(logic.address); - - // Check that the proxy is not initialized. - expect( - await readStorage(tokenProxy.address, OZ_INITIALIZED_SLOT_POSITION, [ - 'uint8', - ]) - ).to.equal(0); - - // Check that the DAO address is not set. - expect(await tokenProxy.dao()).to.equal(ethers.constants.AddressZero); - }); - - it('deploys the proxy initialized if data is provided', async () => { - const initData = - TestGovernanceERC20__factory.createInterface().encodeFunctionData( - 'initialize', - DEFAULT_INITIALIZATION - ); - - const tx = await proxyFactory.deployUUPSProxy(initData); - const event = await findEvent(tx, 'ProxyCreated'); - if (!event) { - throw new Error('Failed to get the event'); - } - - const tokenProxy = TestGovernanceERC20__factory.connect( - event.args.proxy, - deployer - ); - - // Check that the proxy points to the right logic contract. - expect( - await readStorage(tokenProxy.address, ERC1967_IMPLEMENTATION_SLOT, [ - 'address', - ]) - ).to.equal(logic.address); - - // Check that the proxy is initialized. - expect( - await readStorage(tokenProxy.address, OZ_INITIALIZED_SLOT_POSITION, [ - 'uint8', - ]) - ).to.equal(1); - - // Check that the DAO address is set. - expect(await tokenProxy.dao()).to.equal(MOCK_ADDRESS); - }); - }); - - describe('deployMinimalProxy', async () => { - it('deploys the proxy unitialized if no data is provided', async () => { - const initData: any = []; - - const tx = await proxyFactory.deployMinimalProxy(initData); - - const event = await findEvent(tx, 'ProxyCreated'); - if (!event) { - throw new Error('Failed to get the event'); - } - - const tokenProxy = TestGovernanceERC20__factory.connect( - event.args.proxy, - deployer - ); - - // Check that the proxy is not initialized. - expect( - await readStorage(tokenProxy.address, OZ_INITIALIZED_SLOT_POSITION, [ - 'uint8', - ]) - ).to.equal(0); - - // Check that the DAO address is not set. - expect(await tokenProxy.dao()).to.equal(ethers.constants.AddressZero); - }); - - it('deploys the proxy initialized if data is provided', async () => { - const initData = - TestGovernanceERC20__factory.createInterface().encodeFunctionData( - 'initialize', - DEFAULT_INITIALIZATION - ); - - const tx = await proxyFactory.deployMinimalProxy(initData); - const event = await findEvent(tx, 'ProxyCreated'); - if (!event) { - throw new Error('Failed to get the event'); - } - - const tokenProxy = TestGovernanceERC20__factory.connect( - event.args.proxy, - deployer - ); - - // Check that the proxy is initialized. - expect( - await readStorage(tokenProxy.address, OZ_INITIALIZED_SLOT_POSITION, [ - 'uint8', - ]) - ).to.equal(1); - - // Check that the DAO address is set. - expect(await tokenProxy.dao()).to.equal(MOCK_ADDRESS); - }); - }); -}); diff --git a/contracts/utils/proxy.ts b/contracts/utils/proxy.ts index 44bb788e..f7df90ba 100644 --- a/contracts/utils/proxy.ts +++ b/contracts/utils/proxy.ts @@ -1,58 +1,29 @@ -import {ProxyFactory__factory} from '../typechain'; -import {ProxyCreatedEvent} from '../typechain/src/utils/ProxyFactory'; -import {findEvent} from './events'; import {ContractFactory} from 'ethers'; - -export async function deployUUPSProxy( +import {upgrades} from 'hardhat'; + +type DeployOptions = { + constructurArgs?: unknown[]; + proxyType?: 'uups'; +}; + +// Used to deploy the implementation with the ERC1967 Proxy behind it. +// It is designed this way, because it might be desirable to avoid the OpenZeppelin upgrades package. +// In the future, this function might get replaced. +// NOTE: To avoid lots of changes in the whole test codebase, `deployWithProxy` +// won't automatically call `initialize` and it's the caller's responsibility to do so. +export async function deployWithProxy( contractFactory: ContractFactory, - initialization: {initializerName: string; args: any[]} | undefined = undefined + options: DeployOptions = {} ): Promise { - const logic = await contractFactory.deploy(); - const proxyFactory = await new ProxyFactory__factory( - contractFactory.signer - ).deploy(logic.address); - - const initData = - initialization !== undefined - ? contractFactory.interface.encodeFunctionData( - initialization.initializerName, - initialization.args - ) - : []; - - const tx = await proxyFactory.deployUUPSProxy(initData); - - const event = await findEvent(tx, 'ProxyCreated'); - if (!event) { - throw new Error('Failed to get the event'); - } - - return contractFactory.attach(event.args.proxy) as unknown as T; -} - -export async function deployMinimalClone( - contractFactory: ContractFactory, - initialization: {initializerName: string; args: any[]} | undefined = undefined -): Promise { - const logic = await contractFactory.deploy(); - const proxyFactory = await new ProxyFactory__factory( - contractFactory.signer - ).deploy(logic.address); - - const initData = - initialization !== undefined - ? contractFactory.interface.encodeFunctionData( - initialization.initializerName, - initialization.args - ) - : []; - - const tx = await proxyFactory.deployMinimalProxy(initData); - - const event = await findEvent(tx, 'ProxyCreated'); - if (!event) { - throw new Error('Failed to get the event'); - } - - return contractFactory.attach(event.args.proxy) as unknown as T; + // NOTE: taking this out of this file and putting this in each test file's + // before hook seems a good idea for efficiency, though, all test files become + // highly dependent on this package which is undesirable for now. + upgrades.silenceWarnings(); + + return upgrades.deployProxy(contractFactory, [], { + kind: options.proxyType || 'uups', + initializer: false, + unsafeAllow: ['constructor'], + constructorArgs: options.constructurArgs || [], + }) as unknown as Promise; }