Merge branch 'dev' into acl-log-set-params

.solcover.js

@@ -1,7 +1,15 @@
 // NOTE: Upgrading to solidity-coverage 0.4.x breaks our tests
 
 const libFiles = require('glob').sync('contracts/lib/**/*.sol').map(n => n.replace('contracts/', ''))
-const interfaces = ['common/IForwarder.sol', 'common/IVaultRecoverable.sol', 'kernel/IKernel.sol', 'evmscript/IEVMScriptExecutor.sol', 'apps/IAppProxy.sol', 'acl/IACL.sol', 'acl/ACLSyntaxSugar.sol']
+const interfaces = [
+  'acl/IACL.sol',
+  'acl/IACLOracle.sol',
+  'acl/ACLSyntaxSugar.sol',
+  'common/IForwarder.sol',
+  'common/IVaultRecoverable.sol',
+  'evmscript/IEVMScriptExecutor.sol',
+  'kernel/IKernel.sol',
+]
 
 module.exports = {
     norpc: true,

.soliumrc.json

@@ -4,22 +4,34 @@
     "rules": {
         "security/no-low-level-calls": "off",
         "security/no-inline-assembly": "off",
-        "function-order": "off",
-        "imports-on-top": 1,
-        "variable-declarations": 1,
-        "array-declarations": 1,
-        "operator-whitespace": 1,
-        "lbrace": 1,
-        "mixedcase": 0,
-        "camelcase": 1,
-        "uppercase": 1,
-        "no-empty-blocks": 1,
-        "no-unused-vars": 1,
-        "quotes": 1,
-        "indentation": 1,
+        "emit": "off",
+        "error-reason": "off",
+        "imports-on-top": "error",
+        "variable-declarations": "error",
+        "array-declarations": "error",
+        "operator-whitespace": "error",
+        "conditionals-whitespace": "error",
+        "comma-whitespace": "error",
+        "semicolon-whitespace": "error",
+        "function-whitespace": "error",
+        "lbrace": "error",
+        "mixedcase": "error",
+        "camelcase": "error",
+        "uppercase": "error",
+        "no-empty-blocks": "error",
+        "no-unused-vars": "error",
+        "quotes": "error",
+        "blank-lines": "error",
+        "indentation": "error",
         "arg-overflow": ["error", 8],
-        "whitespace": 1,
-        "deprecated-suicide": 1,
-        "pragma-on-top": 1
+        "whitespace": "error",
+        "deprecated-suicide": "error",
+        "pragma-on-top": "error",
+        "function-order": "error",
+        "no-constant": "error",
+        "value-in-payable": "error",
+        "max-len": "error",
+        "visibility-first": "error",
+        "linebreak-style": "error"
     }
 }

contracts/acl/ACL.sol

@@ -3,23 +3,21 @@ pragma solidity 0.4.18;
 import "../apps/AragonApp.sol";
 import "./ACLSyntaxSugar.sol";
 import "./IACL.sol";
+import "./IACLOracle.sol";
 
 
-interface ACLOracle {
-    function canPerform(address who, address where, bytes32 what, uint256[] how) public view returns (bool);
-}
-
-
+/* solium-disable function-order */
+// Allow public initialize() to be first
 contract ACL is IACL, AragonApp, ACLHelpers {
     // Hardcoded constant to save gas
     //bytes32 constant public CREATE_PERMISSIONS_ROLE = keccak256("CREATE_PERMISSIONS_ROLE");
     bytes32 constant public CREATE_PERMISSIONS_ROLE = 0x0b719b33c83b8e5d300c521cb8b54ae9bd933996a14bef8c2f4e0285d2d2400a;
 
-    // whether a certain entity has a permission
-    mapping (bytes32 => bytes32) internal permissions; // 0 for no permission, or parameters id
-    mapping (bytes32 => Param[]) internal permissionParams;
+    // Whether someone has a permission
+    mapping (bytes32 => bytes32) internal permissions; // permissions hash => params hash
+    mapping (bytes32 => Param[]) internal permissionParams; // params hash => params
 
-    // who is the manager of a permission
+    // Who is the manager of a permission
     mapping (bytes32 => address) internal permissionManager;
 
     enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types
@@ -60,7 +58,7 @@ contract ACL is IACL, AragonApp, ACLHelpers {
     * @notice Initializes an ACL instance and sets `_permissionsCreator` as the entity that can create other permissions
     * @param _permissionsCreator Entity that will be given permission over createPermission
     */
-    function initialize(address _permissionsCreator) onlyInit public {
+    function initialize(address _permissionsCreator) public onlyInit {
         initialized();
         require(msg.sender == address(kernel));
 
@@ -104,8 +102,8 @@ contract ACL is IACL, AragonApp, ACLHelpers {
     * @param _params Permission parameters
     */
     function grantPermissionP(address _entity, address _app, bytes32 _role, uint256[] _params)
-        onlyPermissionManager(_app, _role)
         public
+        onlyPermissionManager(_app, _role)
     {
         bytes32 paramsHash = _params.length > 0 ? _saveParams(_params) : EMPTY_PARAM_HASH;
         _setPermission(_entity, _app, _role, paramsHash);
@@ -119,8 +117,8 @@ contract ACL is IACL, AragonApp, ACLHelpers {
     * @param _role Identifier for the group of actions in app being revoked
     */
     function revokePermission(address _entity, address _app, bytes32 _role)
-        onlyPermissionManager(_app, _role)
         external
+        onlyPermissionManager(_app, _role)
     {
         _setPermission(_entity, _app, _role, NO_PERMISSION);
     }
@@ -132,8 +130,8 @@ contract ACL is IACL, AragonApp, ACLHelpers {
     * @param _role Identifier for the group of actions being transferred
     */
     function setPermissionManager(address _newManager, address _app, bytes32 _role)
-        onlyPermissionManager(_app, _role)
         external
+        onlyPermissionManager(_app, _role)
     {
         _setPermissionManager(_newManager, _app, _role);
     }
@@ -144,8 +142,8 @@ contract ACL is IACL, AragonApp, ACLHelpers {
     * @param _role Identifier for the group of actions being unmanaged
     */
     function removePermissionManager(address _app, bytes32 _role)
-        onlyPermissionManager(_app, _role)
         external
+        onlyPermissionManager(_app, _role)
     {
         _setPermissionManager(address(0), _app, _role);
     }
@@ -169,7 +167,11 @@ contract ACL is IACL, AragonApp, ACLHelpers {
     * @param _index Index of parameter in the array
     * @return Parameter (id, op, value)
     */
-    function getPermissionParam(address _entity, address _app, bytes32 _role, uint _index) external view returns (uint8 id, uint8 op, uint240 value) {
+    function getPermissionParam(address _entity, address _app, bytes32 _role, uint _index)
+        external
+        view
+        returns (uint8 id, uint8 op, uint240 value)
+    {
         Param storage param = permissionParams[permissions[permissionHash(_entity, _app, _role)]][_index];
         id = param.id;
         op = param.op;
@@ -303,7 +305,7 @@ contract ACL is IACL, AragonApp, ACLHelpers {
 
         // get value
         if (param.id == ORACLE_PARAM_ID) {
-            value = checkOracle(address(param.value), _who, _where, _what, _how) ? 1 : 0;
+            value = checkOracle(IACLOracle(param.value), _who, _where, _what, _how) ? 1 : 0;
             comparedTo = 1;
         } else if (param.id == BLOCK_NUMBER_PARAM_ID) {
             value = blockN();
@@ -327,7 +329,11 @@ contract ACL is IACL, AragonApp, ACLHelpers {
         return compare(value, Op(param.op), comparedTo);
     }
 
-    function evalLogic(Param _param, bytes32 _paramsHash, address _who, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) {
+    function evalLogic(Param _param, bytes32 _paramsHash, address _who, address _where, bytes32 _what, uint256[] _how)
+        internal
+        view
+        returns (bool)
+    {
         if (Op(_param.op) == Op.IF_ELSE) {
             var (condition, success, failure) = decodeParamsList(uint256(_param.value));
             bool result = evalParam(_paramsHash, condition, _who, _where, _what, _how);
@@ -369,11 +375,11 @@ contract ACL is IACL, AragonApp, ACLHelpers {
         return false;
     }
 
-    function checkOracle(address _oracleAddr, address _who, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) {
-        bytes4 sig = ACLOracle(_oracleAddr).canPerform.selector;
+    function checkOracle(IACLOracle _oracleAddr, address _who, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) {
+        bytes4 sig = _oracleAddr.canPerform.selector;
 
         // a raw call is required so we can return false if the call reverts, rather than reverting
-        bool ok = _oracleAddr.call(sig, _who, _where, _what, 0x80, _how.length, _how);
+        bool ok = address(_oracleAddr).call(sig, _who, _where, _what, 0x80, _how.length, _how);
         // 0x80 is the position where the array that goes there starts
 
         if (!ok) {
@@ -405,11 +411,11 @@ contract ACL is IACL, AragonApp, ACLHelpers {
         ChangePermissionManager(_app, _role, _newManager);
     }
 
-    function roleHash(address _where, bytes32 _what) pure internal returns (bytes32) {
+    function roleHash(address _where, bytes32 _what) internal pure returns (bytes32) {
         return keccak256(uint256(1), _where, _what);
     }
 
-    function permissionHash(address _who, address _where, bytes32 _what) pure internal returns (bytes32) {
+    function permissionHash(address _who, address _where, bytes32 _what) internal pure returns (bytes32) {
         return keccak256(uint256(2), _who, _where, _what);
     }
 

contracts/acl/IACLOracle.sol

@@ -0,0 +1,10 @@
+/*
+ * SPDX-License-Identitifer:    MIT
+ */
+
+pragma solidity ^0.4.18;
+
+
+interface IACLOracle {
+    function canPerform(address who, address where, bytes32 what, uint256[] how) public view returns (bool);
+}

contracts/apm/APMNamehash.sol

@@ -1,3 +1,7 @@
+/*
+ * SPDX-License-Identitifer:    MIT
+ */
+
 pragma solidity ^0.4.18;
 
 import "../ens/ENSConstants.sol";

contracts/apm/APMRegistry.sol

@@ -30,7 +30,7 @@ contract APMRegistry is AragonApp, AppProxyFactory, APMRegistryConstants {
     * NEEDS CREATE_NAME_ROLE and POINT_ROOTNODE_ROLE permissions on registrar
     * @param _registrar ENSSubdomainRegistrar instance that holds registry root node ownership
     */
-    function initialize(ENSSubdomainRegistrar _registrar) onlyInit public {
+    function initialize(ENSSubdomainRegistrar _registrar) public onlyInit {
         initialized();
 
         registrar = _registrar;
@@ -49,7 +49,7 @@ contract APMRegistry is AragonApp, AppProxyFactory, APMRegistryConstants {
     * @param _name Repo name, must be ununsed
     * @param _dev Address that will be given permission to create versions
     */
-    function newRepo(string _name, address _dev) auth(CREATE_REPO_ROLE) public returns (Repo) {
+    function newRepo(string _name, address _dev) public auth(CREATE_REPO_ROLE) returns (Repo) {
         return _newRepo(_name, _dev);
     }
 
@@ -67,7 +67,7 @@ contract APMRegistry is AragonApp, AppProxyFactory, APMRegistryConstants {
         uint16[3] _initialSemanticVersion,
         address _contractAddress,
         bytes _contentURI
-    ) auth(CREATE_REPO_ROLE) public returns (Repo)
+    ) public auth(CREATE_REPO_ROLE) returns (Repo)
     {
         Repo repo = _newRepo(_name, this); // need to have permissions to create version
         repo.newVersion(_initialSemanticVersion, _contractAddress, _contentURI);

contracts/apm/Repo.sol

@@ -29,7 +29,7 @@ contract Repo is AragonApp {
         uint16[3] _newSemanticVersion,
         address _contractAddress,
         bytes _contentURI
-    ) auth(CREATE_VERSION_ROLE) public
+    ) public auth(CREATE_VERSION_ROLE)
     {
         address contractAddress = _contractAddress;
         if (versions.length > 0) {
@@ -57,11 +57,19 @@ contract Repo is AragonApp {
         return getByVersionId(versions.length - 1);
     }
 
-    function getLatestForContractAddress(address _contractAddress) public view returns (uint16[3] semanticVersion, address contractAddress, bytes contentURI) {
+    function getLatestForContractAddress(address _contractAddress)
+        public
+        view
+        returns (uint16[3] semanticVersion, address contractAddress, bytes contentURI)
+    {
         return getByVersionId(latestVersionIdForContract[_contractAddress]);
     }
 
-    function getBySemanticVersion(uint16[3] _semanticVersion) public view returns (uint16[3] semanticVersion, address contractAddress, bytes contentURI) {
+    function getBySemanticVersion(uint16[3] _semanticVersion)
+        public
+        view
+        returns (uint16[3] semanticVersion, address contractAddress, bytes contentURI)
+    {
         return getByVersionId(versionIdForSemantic[semanticVersionHash(_semanticVersion)]);
     }
 

contracts/apps/AppProxyPinned.sol

@@ -11,7 +11,8 @@ contract AppProxyPinned is AppProxyBase {
     * @param _initializePayload Payload for call to be made after setup to initialize
     */
     function AppProxyPinned(IKernel _kernel, bytes32 _appId, bytes _initializePayload)
-             AppProxyBase(_kernel, _appId, _initializePayload) public
+        AppProxyBase(_kernel, _appId, _initializePayload)
+        public // solium-disable-line visibility-first
     {
         pinnedCode = getAppBase(appId);
         require(pinnedCode != address(0));

contracts/apps/AppProxyUpgradeable.sol

@@ -11,7 +11,8 @@ contract AppProxyUpgradeable is AppProxyBase {
     * @param _initializePayload Payload for call to be made after setup to initialize
     */
     function AppProxyUpgradeable(IKernel _kernel, bytes32 _appId, bytes _initializePayload)
-             AppProxyBase(_kernel, _appId, _initializePayload) public
+        AppProxyBase(_kernel, _appId, _initializePayload)
+        public // solium-disable-line visibility-first
     {
 
     }

contracts/apps/AragonApp.sol

@@ -19,23 +19,34 @@ contract AragonApp is AppStorage, Initializable, ACLSyntaxSugar, VaultRecoverabl
         _;
     }
 
-    modifier authP(bytes32 _role, uint256[] params) {
-        require(canPerform(msg.sender, _role, params));
+    modifier authP(bytes32 _role, uint256[] _params) {
+        require(canPerform(msg.sender, _role, _params));
         _;
     }
 
-    function canPerform(address _sender, bytes32 _role, uint256[] params) public view returns (bool) {
+    /**
+    * @dev Check whether an action can be performed by a sender for a particular role on this app
+    * @param _sender Sender of the call
+    * @param _role Role on this app
+    * @param _params Permission params for the role
+    * @return Boolean indicating whether the sender has the permissions to perform the action
+    */
+    function canPerform(address _sender, bytes32 _role, uint256[] _params) public view returns (bool) {
         bytes memory how; // no need to init memory as it is never used
-        if (params.length > 0) {
-            uint256 byteLength = params.length * 32;
+        if (_params.length > 0) {
+            uint256 byteLength = _params.length * 32;
             assembly {
-                how := params // forced casting
+                how := _params // forced casting
                 mstore(how, byteLength)
             }
         }
         return address(kernel) == 0 || kernel.hasPermission(_sender, address(this), _role, how);
     }
 
+    /**
+    * @dev Get the recovery vault for the app
+    * @return Recovery vault address for the app
+    */
     function getRecoveryVault() public view returns (address) {
         // Funds recovery via a vault is only available when used with a kernel
         require(address(kernel) != 0);

contracts/common/DelegateProxy.sol

@@ -14,10 +14,10 @@ contract DelegateProxy is ERCProxy, IsContract {
     */
     function delegatedFwd(address _dst, bytes _calldata) internal {
         require(isContract(_dst));
-        uint256 fwd_gas_limit = FWD_GAS_LIMIT;
+        uint256 fwdGasLimit = FWD_GAS_LIMIT;
 
         assembly {
-            let result := delegatecall(sub(gas, fwd_gas_limit), _dst, add(_calldata, 0x20), mload(_calldata), 0, 0)
+            let result := delegatecall(sub(gas, fwdGasLimit), _dst, add(_calldata, 0x20), mload(_calldata), 0, 0)
             let size := returndatasize
             let ptr := mload(0x40)
             returndatacopy(ptr, 0, size)

contracts/common/DepositableDelegateProxy.sol

@@ -6,7 +6,7 @@ import "./DelegateProxy.sol";
 contract DepositableDelegateProxy is DelegateProxy {
     event ProxyDeposit(address sender, uint256 value);
 
-    function () payable public {
+    function () external payable {
         // send / transfer
         if (msg.gas < FWD_GAS_LIMIT) {
             require(msg.value > 0 && msg.data.length == 0);