refactor: type safety improvements (#383)

* Use curly bracket notation * fix: typos * feat: use encodeCall to make use of type checking * feature: pre-fill the OSx JIRA link so that you only need to add the number * chore: maintained changelog
aragon · Oct 4, 2023 · e35f6f8 · e35f6f8
1 parent 9cedaef
commit e35f6f8
Show file tree

Hide file tree

Showing 16 changed files with 324 additions and 335 deletions.
diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
@@ -2,7 +2,7 @@
 
 Please include a summary of the change and be sure you follow the contributions rules we do provide [here](./CONTRIBUTIONS.md)
 
-Task: [ID]()
+Task ID: [OS-?](https://aragonassociation.atlassian.net/browse/OS-?)
 
 ## Type of change
 See the framework lifecylce in `packages/contracts/docs/framework-lifecycle` to decide what kind of change this pull request is.

diff --git a/packages/contracts/CHANGELOG.md b/packages/contracts/CHANGELOG.md
@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Improved type safety by using `abi.encodeCall` instead of `abi.encodeWithSelector` and the more explicit bracket syntax for permissions.
 - Bumped OpenZeppelin dependencies to `4.9.3`.
 - Refactored the fallback in the `isGranted` function in `PermissionManager` to make conditions mutually exclusive: Specific conditions answering `false` do not fall back to generic caller conditions (`_who: ANY_ADDR`) or generic target conditions (`_where: ANY_ADDR`).
 - Renamed the `signatureValidator` variable in `DAO` to `__removed0`.

diff --git a/...es/contracts/docs/developer-portal/01-how-it-works/01-core/01-dao/01-actions.md b/...es/contracts/docs/developer-portal/01-how-it-works/01-core/01-dao/01-actions.md
@@ -59,9 +59,9 @@ We have an Aragon DAO deployed on the Goerli testnet. Now, we want to wrap `0.1
 IDAO.Action[] memory actions = new IDAO.Action[](1);
 
 actions[0] = IDAO.Action({
-  to: address(0xB4FBF271143F4FBf7B91A5ded31805e42b2208d6), //         The address of the WETH contract on Goerli
-  value: 0.1 ether, //                                                The Goerli ETH value to be sent with the function call
-  data: abi.encodeWithSelector(bytes4(keccak256('deposit()', []))) // The calldata
+  to: address(0xB4FBF271143F4FBf7B91A5ded31805e42b2208d6), // The address of the WETH contract on Goerli
+  value: 0.1 ether, //                                        The Goerli ETH value to be sent with the function call
+  data: abi.encodeCall(IDAO.deposit, ()) //                    The calldata
 });
 
 dao().execute({_callId: '', _actions: actions, _allowFailureMap: 0});

diff --git a/.../contracts/docs/developer-portal/02-how-to-guides/01-dao/02-action-execution.md b/.../contracts/docs/developer-portal/02-how-to-guides/01-dao/02-action-execution.md
@@ -106,7 +106,7 @@ function exampleFunctionCall(
   actions[0] = IDAO.Action({
     to: address(_calculator),
     value: 0, // 0 native tokens must be sent with this call
-    data: abi.encodeWithSelector(_calculator.add.selector, _a, _b)
+    data: abi.encodeCall(_calculator.add, (_a, _b))
   });
 
   // Execute the action array
@@ -141,7 +141,7 @@ function examplePayableFunctionCall(IDAO dao, bytes32 _callId, IWETH9 _wethToken
   actions[0] = IDAO.Action({
     to: address(_wethToken),
     value: 0.1 ether,
-    data: abi.encodeWithSelector(IWETH9.deposit.selector) // abi.encodeWithSelector(bytes4(keccak256('deposit()')), [])
+    data: abi.encodeCall(IWETH9.deposit, ())
   });
 
   // Execute the action array

diff --git a/...portal/02-how-to-guides/02-plugin-development/04-upgradeable-plugin/03-setup.md b/...portal/02-how-to-guides/02-plugin-development/04-upgradeable-plugin/03-setup.md
@@ -76,7 +76,7 @@ contract SimpleStorageBuild1Setup is PluginSetup {
 
     plugin = createERC1967Proxy(
       simpleStorageImplementation,
-      abi.encodeWithSelector(SimpleStorageBuild1.initializeBuild1.selector, _dao, number)
+      abi.encodeCall(SimpleStorageBuild1.initializeBuild1, (IDAO(_dao), number))
     );
 
     PermissionLib.MultiTargetPermission[]

diff --git a/packages/contracts/src/framework/plugin/repo/PluginRepoFactory.sol b/packages/contracts/src/framework/plugin/repo/PluginRepoFactory.sol
@@ -133,7 +133,7 @@ contract PluginRepoFactory is ERC165, ProtocolVersion {
         pluginRepo = PluginRepo(
             createERC1967Proxy(
                 pluginRepoBase,
-                abi.encodeWithSelector(PluginRepo.initialize.selector, _initialOwner)
+                abi.encodeCall(PluginRepo.initialize, (_initialOwner))
             )
         );
 

diff --git a/packages/contracts/src/framework/utils/TokenFactory.sol b/packages/contracts/src/framework/utils/TokenFactory.sol
@@ -88,7 +88,7 @@ contract TokenFactory {
         if (token != address(0)) {
             // Validate if token is ERC20
             bytes memory data = token.functionStaticCall(
-                abi.encodeWithSelector(IERC20Upgradeable.balanceOf.selector, address(this))
+                abi.encodeCall(IERC20Upgradeable.balanceOf, (address(this)))
             );
 
             if (data.length != 0x20) {

diff --git a/packages/contracts/src/plugins/counter-example/v1/CounterV1PluginSetup.sol b/packages/contracts/src/plugins/counter-example/v1/CounterV1PluginSetup.sol
@@ -4,6 +4,7 @@ pragma solidity 0.8.17;
 
 import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
 
+import {IDAO} from "../../../core/dao/IDAO.sol";
 import {PermissionLib} from "../../../core/permission/PermissionLib.sol";
 import {IPluginSetup} from "../../../framework/plugin/setup/IPluginSetup.sol";
 import {PluginSetup} from "../../../framework/plugin/setup/PluginSetup.sol";
@@ -44,11 +45,9 @@ contract CounterV1PluginSetup is PluginSetup {
             multiplyHelper = createERC1967Proxy(address(multiplyHelperBase), bytes(""));
         }
 
-        bytes memory initData = abi.encodeWithSelector(
-            bytes4(keccak256("initialize(address,address,uint256)")),
-            _dao,
-            multiplyHelper,
-            _num
+        bytes memory initData = abi.encodeCall(
+            CounterV1.initialize,
+            (IDAO(_dao), MultiplyHelper(multiplyHelper), _num)
         );
 
         PermissionLib.MultiTargetPermission[]
@@ -61,30 +60,30 @@ contract CounterV1PluginSetup is PluginSetup {
         plugin = createERC1967Proxy(address(counterBase), initData);
 
         // set permissions
-        permissions[0] = PermissionLib.MultiTargetPermission(
-            PermissionLib.Operation.Grant,
-            _dao,
-            plugin,
-            PermissionLib.NO_CONDITION,
-            keccak256("EXECUTE_PERMISSION")
-        );
-
-        permissions[1] = PermissionLib.MultiTargetPermission(
-            PermissionLib.Operation.Grant,
-            plugin,
-            _dao,
-            PermissionLib.NO_CONDITION,
-            counterBase.MULTIPLY_PERMISSION_ID()
-        );
+        permissions[0] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Grant,
+            where: _dao,
+            who: plugin,
+            condition: PermissionLib.NO_CONDITION,
+            permissionId: keccak256("EXECUTE_PERMISSION")
+        });
+
+        permissions[1] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Grant,
+            where: plugin,
+            who: _dao,
+            condition: PermissionLib.NO_CONDITION,
+            permissionId: counterBase.MULTIPLY_PERMISSION_ID()
+        });
 
         if (_multiplyHelper == address(0)) {
-            permissions[2] = PermissionLib.MultiTargetPermission(
-                PermissionLib.Operation.Grant,
-                multiplyHelper,
-                plugin,
-                PermissionLib.NO_CONDITION,
-                multiplyHelperBase.MULTIPLY_PERMISSION_ID()
-            );
+            permissions[2] = PermissionLib.MultiTargetPermission({
+                operation: PermissionLib.Operation.Grant,
+                where: multiplyHelper,
+                who: plugin,
+                condition: PermissionLib.NO_CONDITION,
+                permissionId: multiplyHelperBase.MULTIPLY_PERMISSION_ID()
+            });
         }
 
         // add helpers
@@ -106,30 +105,30 @@ contract CounterV1PluginSetup is PluginSetup {
         );
 
         // set permissions
-        permissions[0] = PermissionLib.MultiTargetPermission(
-            PermissionLib.Operation.Revoke,
-            _dao,
-            _payload.plugin,
-            PermissionLib.NO_CONDITION,
-            keccak256("EXECUTE_PERMISSION")
-        );
-
-        permissions[1] = PermissionLib.MultiTargetPermission(
-            PermissionLib.Operation.Revoke,
-            _payload.plugin,
-            _dao,
-            PermissionLib.NO_CONDITION,
-            counterBase.MULTIPLY_PERMISSION_ID()
-        );
+        permissions[0] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Revoke,
+            where: _dao,
+            who: _payload.plugin,
+            condition: PermissionLib.NO_CONDITION,
+            permissionId: keccak256("EXECUTE_PERMISSION")
+        });
+
+        permissions[1] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Revoke,
+            where: _payload.plugin,
+            who: _dao,
+            condition: PermissionLib.NO_CONDITION,
+            permissionId: counterBase.MULTIPLY_PERMISSION_ID()
+        });
 
         if (_payload.currentHelpers.length != 0) {
-            permissions[2] = PermissionLib.MultiTargetPermission(
-                PermissionLib.Operation.Revoke,
-                _payload.currentHelpers[0],
-                _payload.plugin,
-                PermissionLib.NO_CONDITION,
-                multiplyHelperBase.MULTIPLY_PERMISSION_ID()
-            );
+            permissions[2] = PermissionLib.MultiTargetPermission({
+                operation: PermissionLib.Operation.Revoke,
+                where: _payload.currentHelpers[0],
+                who: _payload.plugin,
+                condition: PermissionLib.NO_CONDITION,
+                permissionId: multiplyHelperBase.MULTIPLY_PERMISSION_ID()
+            });
         }
     }
 

diff --git a/packages/contracts/src/plugins/counter-example/v2/CounterV2PluginSetup.sol b/packages/contracts/src/plugins/counter-example/v2/CounterV2PluginSetup.sol
@@ -4,6 +4,7 @@ pragma solidity 0.8.17;
 
 import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
 
+import {IDAO} from "../../../core/dao/IDAO.sol";
 import {PermissionLib} from "../../../core/permission/PermissionLib.sol";
 import {IPluginSetup} from "../../../framework/plugin/setup/IPluginSetup.sol";
 import {PluginSetup} from "../../../framework/plugin/setup/PluginSetup.sol";
@@ -37,19 +38,20 @@ contract CounterV2PluginSetup is PluginSetup {
         returns (address plugin, PreparedSetupData memory preparedSetupData)
     {
         // Decode the parameters from the UI
-        (address _multiplyHelper, uint256 _num) = abi.decode(_data, (address, uint256));
+        (address _multiplyHelper, uint256 _num, uint256 _newVariable) = abi.decode(
+            _data,
+            (address, uint256, uint256)
+        );
 
         address multiplyHelper = _multiplyHelper;
 
         if (_multiplyHelper == address(0)) {
             multiplyHelper = createERC1967Proxy(address(multiplyHelperBase), bytes(""));
         }
 
-        bytes memory initData = abi.encodeWithSelector(
-            bytes4(keccak256("initialize(address,address,uint256)")),
-            _dao,
-            multiplyHelper,
-            _num
+        bytes memory initData = abi.encodeCall(
+            CounterV2.initialize,
+            (IDAO(_dao), MultiplyHelper(multiplyHelper), _num, _newVariable)
         );
 
         PermissionLib.MultiTargetPermission[]
@@ -62,30 +64,30 @@ contract CounterV2PluginSetup is PluginSetup {
         plugin = createERC1967Proxy(address(counterBase), initData);
 
         // set permissions
-        permissions[0] = PermissionLib.MultiTargetPermission(
-            PermissionLib.Operation.Grant,
-            _dao,
-            plugin,
-            PermissionLib.NO_CONDITION,
-            keccak256("EXECUTE_PERMISSION")
-        );
-
-        permissions[1] = PermissionLib.MultiTargetPermission(
-            PermissionLib.Operation.Grant,
-            plugin,
-            _dao,
-            PermissionLib.NO_CONDITION,
-            counterBase.MULTIPLY_PERMISSION_ID()
-        );
+        permissions[0] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Grant,
+            where: _dao,
+            who: plugin,
+            condition: PermissionLib.NO_CONDITION,
+            permissionId: keccak256("EXECUTE_PERMISSION")
+        });
+
+        permissions[1] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Grant,
+            where: plugin,
+            who: _dao,
+            condition: PermissionLib.NO_CONDITION,
+            permissionId: counterBase.MULTIPLY_PERMISSION_ID()
+        });
 
         if (_multiplyHelper == address(0)) {
-            permissions[2] = PermissionLib.MultiTargetPermission(
-                PermissionLib.Operation.Grant,
-                multiplyHelper,
-                plugin,
-                PermissionLib.NO_CONDITION,
-                multiplyHelperBase.MULTIPLY_PERMISSION_ID()
-            );
+            permissions[2] = PermissionLib.MultiTargetPermission({
+                operation: PermissionLib.Operation.Grant,
+                where: multiplyHelper,
+                who: plugin,
+                condition: PermissionLib.NO_CONDITION,
+                permissionId: multiplyHelperBase.MULTIPLY_PERMISSION_ID()
+            });
         }
 
         // add helpers
@@ -112,21 +114,18 @@ contract CounterV2PluginSetup is PluginSetup {
 
         if (_currentBuild == 1) {
             (_newVariable) = abi.decode(_payload.data, (uint256));
-            initData = abi.encodeWithSelector(
-                bytes4(keccak256("setNewVariable(uint256)")),
-                _newVariable
-            );
+            initData = abi.encodeCall(CounterV2.setNewVariable, (_newVariable));
         }
 
         PermissionLib.MultiTargetPermission[]
             memory permissions = new PermissionLib.MultiTargetPermission[](1);
-        permissions[0] = PermissionLib.MultiTargetPermission(
-            PermissionLib.Operation.Revoke,
-            _dao,
-            _payload.plugin,
-            PermissionLib.NO_CONDITION,
-            multiplyHelperBase.MULTIPLY_PERMISSION_ID()
-        );
+        permissions[0] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Revoke,
+            where: _dao,
+            who: _payload.plugin,
+            condition: PermissionLib.NO_CONDITION,
+            permissionId: multiplyHelperBase.MULTIPLY_PERMISSION_ID()
+        });
 
         // if another helper is deployed, put it inside activeHelpers + put old ones as well.
         address[] memory activeHelpers = new address[](1);
@@ -146,30 +145,30 @@ contract CounterV2PluginSetup is PluginSetup {
         );
 
         // set permissions
-        permissions[0] = PermissionLib.MultiTargetPermission(
-            PermissionLib.Operation.Revoke,
-            _dao,
-            _payload.plugin,
-            PermissionLib.NO_CONDITION,
-            keccak256("EXECUTE_PERMISSION")
-        );
-
-        permissions[1] = PermissionLib.MultiTargetPermission(
-            PermissionLib.Operation.Revoke,
-            _payload.plugin,
-            _dao,
-            PermissionLib.NO_CONDITION,
-            counterBase.MULTIPLY_PERMISSION_ID()
-        );
+        permissions[0] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Revoke,
+            where: _dao,
+            who: _payload.plugin,
+            condition: PermissionLib.NO_CONDITION,
+            permissionId: keccak256("EXECUTE_PERMISSION")
+        });
+
+        permissions[1] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Revoke,
+            where: _payload.plugin,
+            who: _dao,
+            condition: PermissionLib.NO_CONDITION,
+            permissionId: counterBase.MULTIPLY_PERMISSION_ID()
+        });
 
         if (_payload.currentHelpers.length != 0) {
-            permissions[2] = PermissionLib.MultiTargetPermission(
-                PermissionLib.Operation.Revoke,
-                _payload.currentHelpers[0],
-                _payload.plugin,
-                PermissionLib.NO_CONDITION,
-                multiplyHelperBase.MULTIPLY_PERMISSION_ID()
-            );
+            permissions[2] = PermissionLib.MultiTargetPermission({
+                operation: PermissionLib.Operation.Revoke,
+                where: _payload.currentHelpers[0],
+                who: _payload.plugin,
+                condition: PermissionLib.NO_CONDITION,
+                permissionId: multiplyHelperBase.MULTIPLY_PERMISSION_ID()
+            });
         }
     }