refactor: PluginSetup and deployment helpers (Proxy, `CloneFactor…

…y`) (#46) * refactor: deployment helpers and plugin setup * refactor: enforce setting the implementation through the setup constructor * chore: maintained changelog * style: run prettier * chore: bump package version * style: run eslint * fix: wrong variable name * style: run solhint * refactor: renaming * docs: improved natspec * refactor: added explicit function returning the implementation * refactor: provide default implementation for the initial build * build: clean typechain * refactor: remove default prepareUpdate implementation for the upgradeable setup * test: setups and deployment helpers * chore: maintained changelog * style: fix linting issues * test: use address constant * test: initialization * docs: typo * refactor: renaming * style: remove trailing underscore * style: improve library order * feat: added event to sdk * build: use sdk 0.0.1-alpha.5 * build: print coverage report in the console * test: use fixtures * fix: linter * refactor: fix names * refactor: reverting prepareUpdate default implementation * style: move fixtures to the end of the files * revert: remove default implementation * style: move fixture to the EOF * Apply suggestions from code review Co-authored-by: Mathias Scherer <[email protected]> --------- Co-authored-by: Mathias Scherer <[email protected]>
aragon · Jan 31, 2024 · e788454 · e788454
1 parent 67b6ccd
commit e788454
Show file tree

Hide file tree

Showing 26 changed files with 942 additions and 341 deletions.
diff --git a/contracts/.solcover.js b/contracts/.solcover.js
@@ -1,5 +1,5 @@
 module.exports = {
-  istanbulReporter: ['html', 'lcov'],
+  istanbulReporter: ['html', 'lcov', 'text'],
   providerOptions: {
     privateKey: process.env.PRIVATE_KEY,
   },

diff --git a/contracts/CHANGELOG.md b/contracts/CHANGELOG.md
@@ -9,10 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- Added an `address internal immutable IMPLEMENTATION` variable to `PluginSetup` and `PluginSetupUpgradeable` and its initialization through the respective constructors.
+
+- Added an abstract `PluginUpgradeableSetup` base contract.
+
 - Copied files from [aragon/osx commit e7ba46](https://github.com/aragon/osx/tree/e7ba46026db96931d3e4a585e8f30c585906e1fc)
 
   - interfaces `IDAO`, `IPermissionCondition`, `IPlugin`, `IMembership`, `IProposal`, `IPluginSetup`, `IProtocolVersion`,
   - abstract contracts `DaoAuthorizable`, `DaoAuthorizableUpgradeable`, `Plugin`, `PluginCloneable`, `PluginUUPSUpgradeable`, `PermissionCondition`, `PermissionConditionUpgradeable`, `Addresslist`, `Proposal`, `ProposalUpgradeable`, `PluginSetup`
   - contracts `CloneFactory`
   - libraries `PermissionLib`, `VersionComparisonLib`
   - free functions `auth`, `Proxy`, `BitMap`, `Ratio`, `UncheckedMath`
+
+### Changed
+
+- Replaced `Proxy` and `CloneFactory` by `ProxyLib` and `ProxyFactory`.
diff --git a/contracts/package.json b/contracts/package.json
@@ -2,13 +2,13 @@
   "name": "@aragon/osx-commons-contracts",
   "license": "AGPL-3.0-or-later",
   "description": "The Aragon OSx contracts package containing common utilities",
-  "version": "1.4.0-alpha.2",
+  "version": "1.4.0-alpha.3",
   "author": {
     "name": "aragon",
     "url": "https://github.com/aragon"
   },
   "devDependencies": {
-    "@aragon/osx-commons-sdk": "0.0.1-alpha.3",
+    "@aragon/osx-commons-sdk": "0.0.1-alpha.5",
     "@aragon/osx-ethers-v1.0.0": "npm:@aragon/[email protected]",
     "@aragon/osx-ethers-v1.3.0": "npm:@aragon/[email protected]",
     "@ethersproject/abi": "^5.7.0",
@@ -74,7 +74,7 @@
     "lint:ts": "cd .. && yarn run lint:contracts:ts",
     "test": "hardhat test",
     "typechain": "cross-env TS_NODE_TRANSPILE_ONLY=true hardhat typechain",
-    "clean": "rimraf ./artifacts ./cache ./coverage ./types ./coverage.json && yarn typechain",
+    "clean": "rimraf ./artifacts ./cache ./coverage ./typechain ./types ./coverage.json && yarn typechain",
     "docgen": "hardhat docgen"
   }
 }
diff --git a/contracts/src/mocks/plugin/PluginCloneableMock.sol b/contracts/src/mocks/plugin/PluginCloneableMock.sol
@@ -31,3 +31,14 @@ contract PluginCloneableMockBuild2 is PluginCloneable {
         state2 = 2;
     }
 }
+
+/// @notice A mock cloneable plugin missing an initializer function.
+/// @dev DO NOT USE IN PRODUCTION!
+contract PluginCloneableMockBad is PluginCloneable {
+    uint256 public state1;
+
+    function notAnInitializer(IDAO _dao) external {
+        __PluginCloneable_init(_dao);
+        state1 = 1;
+    }
+}
diff --git a/contracts/src/mocks/plugin/PluginCloneableSetupMock.sol b/contracts/src/mocks/plugin/PluginCloneableSetupMock.sol
@@ -6,6 +6,7 @@ pragma solidity ^0.8.8;
 import {PermissionLib} from "../../permission/PermissionLib.sol";
 import {IPluginSetup} from "../../plugin/setup/IPluginSetup.sol";
 import {PluginSetup} from "../../plugin/setup/PluginSetup.sol";
+import {ProxyLib} from "../../utils/deployment/ProxyLib.sol";
 import {IDAO} from "../../dao/IDAO.sol";
 import {mockPermissions, mockHelpers} from "./PluginSetupMockData.sol";
 import {PluginCloneableMockBuild1, PluginCloneableMockBuild2} from "./PluginCloneableMock.sol";
@@ -14,70 +15,68 @@ import {PluginCloneableMockBuild1, PluginCloneableMockBuild2} from "./PluginClon
 /// v1.1 (Release 1, Build 1)
 /// @dev DO NOT USE IN PRODUCTION!
 contract PluginCloneableSetupMockBuild1 is PluginSetup {
-    address internal pluginBase;
+    using ProxyLib for address;
 
-    constructor() {
-        pluginBase = address(new PluginCloneableMockBuild1());
-    }
+    uint16 internal constant THIS_BUILD = 1;
+
+    constructor() PluginSetup(address(new PluginCloneableMockBuild1())) {}
 
     /// @inheritdoc IPluginSetup
     function prepareInstallation(
         address _dao,
         bytes memory
-    ) external override returns (address plugin, PreparedSetupData memory preparedSetupData) {
+    ) external returns (address plugin, PreparedSetupData memory preparedSetupData) {
         bytes memory initData = abi.encodeCall(PluginCloneableMockBuild1.initialize, (IDAO(_dao)));
-        plugin = createERC1967Proxy(pluginBase, initData); // TODO createClone(pluginBase, initData); is missing! See task OS-794 and OS-675.
-        preparedSetupData.helpers = mockHelpers(1);
-        preparedSetupData.permissions = mockPermissions(0, 1, PermissionLib.Operation.Grant);
+        plugin = implementation().deployMinimalProxy(initData);
+        preparedSetupData.helpers = mockHelpers(THIS_BUILD);
+        preparedSetupData.permissions = mockPermissions(
+            0,
+            THIS_BUILD,
+            PermissionLib.Operation.Grant
+        );
     }
 
     /// @inheritdoc IPluginSetup
     function prepareUninstallation(
         address _dao,
         SetupPayload calldata _payload
-    ) external pure override returns (PermissionLib.MultiTargetPermission[] memory permissions) {
+    ) external pure returns (PermissionLib.MultiTargetPermission[] memory permissions) {
         (_dao, _payload);
-        permissions = mockPermissions(0, 1, PermissionLib.Operation.Revoke);
-    }
-
-    /// @inheritdoc IPluginSetup
-    function implementation() external view override returns (address) {
-        return address(pluginBase);
+        permissions = mockPermissions(0, THIS_BUILD, PermissionLib.Operation.Revoke);
     }
 }
 
 /// @notice A mock plugin setup of a cloneable plugin to be deployed via the minimal proxy pattern.
 /// v1.2 (Release 1, Build 2)
 /// @dev DO NOT USE IN PRODUCTION!
 contract PluginCloneableSetupMockBuild2 is PluginSetup {
-    address internal pluginBase;
+    using ProxyLib for address;
 
-    constructor() {
-        pluginBase = address(new PluginCloneableMockBuild2());
-    }
+    uint16 internal constant THIS_BUILD = 2;
+
+    constructor() PluginSetup(address(new PluginCloneableMockBuild2())) {}
 
     /// @inheritdoc IPluginSetup
     function prepareInstallation(
         address _dao,
         bytes memory
-    ) external override returns (address plugin, PreparedSetupData memory preparedSetupData) {
+    ) external returns (address plugin, PreparedSetupData memory preparedSetupData) {
         bytes memory initData = abi.encodeCall(PluginCloneableMockBuild2.initialize, (IDAO(_dao)));
-        plugin = createERC1967Proxy(pluginBase, initData); // TODO createClone(pluginBase, initData); is missing! See task OS-794 and OS-675.
-        preparedSetupData.helpers = mockHelpers(2);
-        preparedSetupData.permissions = mockPermissions(0, 2, PermissionLib.Operation.Grant);
+        plugin = implementation().deployMinimalProxy(initData);
+        preparedSetupData.helpers = mockHelpers(THIS_BUILD);
+        preparedSetupData.permissions = mockPermissions(
+            0,
+            THIS_BUILD,
+            PermissionLib.Operation.Grant
+        );
     }
 
     /// @inheritdoc IPluginSetup
     function prepareUninstallation(
         address _dao,
         SetupPayload calldata _payload
-    ) external pure override returns (PermissionLib.MultiTargetPermission[] memory permissions) {
+    ) external pure returns (PermissionLib.MultiTargetPermission[] memory permissions) {
         (_dao, _payload);
-        permissions = mockPermissions(0, 2, PermissionLib.Operation.Revoke);
-    }
-
-    /// @inheritdoc IPluginSetup
-    function implementation() external view override returns (address) {
-        return address(pluginBase);
+        permissions = mockPermissions(0, THIS_BUILD, PermissionLib.Operation.Revoke);
     }
 }
diff --git a/contracts/src/mocks/plugin/PluginSetupMock.sol b/contracts/src/mocks/plugin/PluginSetupMock.sol
@@ -14,17 +14,13 @@ import {PluginMockBuild1} from "./PluginMock.sol";
 /// v1.1 (Release 1, Build 1)
 /// @dev DO NOT USE IN PRODUCTION!
 contract PluginSetupMockBuild1 is PluginSetup {
-    address internal pluginBase;
-
-    constructor() {
-        pluginBase = address(new PluginMockBuild1(IDAO(address(0))));
-    }
+    constructor() PluginSetup(address(new PluginMockBuild1(IDAO(address(0))))) {}
 
     /// @inheritdoc IPluginSetup
     function prepareInstallation(
         address _dao,
         bytes memory
-    ) external override returns (address plugin, PreparedSetupData memory preparedSetupData) {
+    ) external returns (address plugin, PreparedSetupData memory preparedSetupData) {
         plugin = address(new PluginMockBuild1(IDAO(_dao)));
         preparedSetupData.helpers = mockHelpers(1);
         preparedSetupData.permissions = mockPermissions(0, 1, PermissionLib.Operation.Grant);
@@ -34,13 +30,8 @@ contract PluginSetupMockBuild1 is PluginSetup {
     function prepareUninstallation(
         address _dao,
         SetupPayload calldata _payload
-    ) external pure override returns (PermissionLib.MultiTargetPermission[] memory permissions) {
+    ) external pure returns (PermissionLib.MultiTargetPermission[] memory permissions) {
         (_dao, _payload);
         permissions = mockPermissions(0, 1, PermissionLib.Operation.Revoke);
     }
-
-    /// @inheritdoc IPluginSetup
-    function implementation() external view override returns (address) {
-        return address(pluginBase);
-    }
 }
diff --git a/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol b/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol
@@ -6,7 +6,7 @@ pragma solidity ^0.8.8;
 import {PluginUUPSUpgradeable} from "../../plugin/PluginUUPSUpgradeable.sol";
 import {IDAO} from "../../dao/IDAO.sol";
 
-/// @notice A mock cloneable plugin to be deployed via the UUPS proxy pattern.
+/// @notice A mock upgradeable plugin to be deployed via the UUPS proxy pattern.
 /// v1.1 (Release 1, Build 1)
 /// @dev DO NOT USE IN PRODUCTION!
 contract PluginUUPSUpgradeableMockBuild1 is PluginUUPSUpgradeable {
@@ -18,7 +18,7 @@ contract PluginUUPSUpgradeableMockBuild1 is PluginUUPSUpgradeable {
     }
 }
 
-/// @notice A mock cloneable plugin to be deployed via the UUPS proxy pattern.
+/// @notice A mock upgradeable plugin to be deployed via the UUPS proxy pattern.
 /// v1.1 (Release 1, Build 2)
 /// @dev DO NOT USE IN PRODUCTION!
 contract PluginUUPSUpgradeableMockBuild2 is PluginUUPSUpgradeable {
@@ -38,7 +38,7 @@ contract PluginUUPSUpgradeableMockBuild2 is PluginUUPSUpgradeable {
     }
 }
 
-/// @notice A mock cloneable plugin to be deployed via the UUPS proxy pattern.
+/// @notice A mock upgradeable plugin to be deployed via the UUPS proxy pattern.
 /// v1.1 (Release 1, Build 3)
 /// @dev DO NOT USE IN PRODUCTION!
 contract PluginUUPSUpgradeableMockBuild3 is PluginUUPSUpgradeable {
@@ -62,3 +62,14 @@ contract PluginUUPSUpgradeableMockBuild3 is PluginUUPSUpgradeable {
         }
     }
 }
+
+/// @notice A mock upgradeable plugin missing an initializer function.
+/// @dev DO NOT USE IN PRODUCTION!
+contract PluginUUPSUpgradeableMockBad is PluginUUPSUpgradeable {
+    uint256 public state1;
+
+    function notAnInitializer(IDAO _dao) external {
+        __PluginUUPSUpgradeable_init(_dao);
+        state1 = 1;
+    }
+}