WIP: Split proxies in Depositable and NonDepositable

and combine them wiht Pinned/Upgradeable. Gas comparison. Interestingly enough, contract size increase but tx gas cost is decreasing. Before: | Contract | Method | Min | Max | Avg | # calls | usd (avg) |-------------------------|------------------------|-----------|-----------|-----------|---------------|-------------- | Kernel | initialize | - | - | 470206 | 2 | 0.25 | Kernel | newAppInstance | 321213 | 388578 | 348802 | 7 | 0.19 | Kernel | newPinnedAppInstance | 320402 | 343397 | 338798 | 5 | 0.18 | Kernel | setApp | 45406 | 61253 | 57948 | 15 | 0.03 | KernelDepositableMock | enableDeposits | - | - | 42110 | 1 | 0.02 | KernelDepositableMock | initialize | - | - | 364058 | 3 | 0.20 | KernelOverloadMock | newAppInstance | 334494 | 366437 | 356214 | 8 | 0.19 | KernelOverloadMock | newPinnedAppInstance | 314022 | 390551 | 355616 | 8 | 0.19 | Deployments | Min | Max | Avg | % of limit | usd (avg) |--------------------------------------------------|-----------|-----------|-----------|---------------|------------- | ACL | - | - | 3197196 | 6.4 % | 1.73 | ERCProxyMock | - | - | 129445 | 0.3 % | 0.07 | Kernel | 3217572 | 3238629 | 3220854 | 6.4 % | 1.74 | KernelDepositableMock | 3224984 | 3246041 | 3230248 | 6.5 % | 1.75 | KernelOverloadMock | 346334 | 346398 | 346387 | 0.7 % | 0.19 | KernelProxy | - | - | 337198 | 0.7 % | 0.18 After: | Contract | Method | Min | Max | Avg | # calls | usd (avg) |----------------------|------------------------|-----------|-----------|-----------|---------------|-------------- | Kernel | initialize | - | - | 431538 | 2 | 0.23 | Kernel | newAppInstance | 292794 | 315853 | 306642 | 5 | 0.17 | Kernel | newPinnedAppInstance | 281976 | 304971 | 300372 | 5 | 0.16 | Kernel | setApp | 45483 | 61330 | 58025 | 15 | 0.03 | KernelOverloadMock | newAppInstance | 295991 | 327846 | 317667 | 8 | 0.17 | KernelOverloadMock | newPinnedAppInstance | 285131 | 319201 | 302166 | 4 | 0.16 | Deployments | Min | Max | Avg | % of limit | usd (avg) |-----------------------------------------------|-----------|-----------|-----------|---------------|------------- | ACL | - | - | 3197196 | 6.4 % | 1.74 | ERCProxyMock | - | - | 129445 | 0.3 % | 0.07 | Kernel | 4341612 | 4362669 | 4345073 | 8.7 % | 2.37 | KernelOverloadMock | - | - | 260168 | 0.5 % | 0.14 | KernelProxy | 281819 | 281883 | 281875 | 0.6 % | 0.15
bingen · Sep 3, 2019 · ac3116c · ac3116c
1 parent c350823
commit ac3116c
Show file tree

Hide file tree

Showing 30 changed files with 267 additions and 214 deletions.
diff --git a/contracts/apps/AppProxyBase.sol b/contracts/apps/AppProxyBase.sol
@@ -1,12 +1,12 @@
 pragma solidity 0.4.24;
 
 import "./AppStorage.sol";
-import "../common/DepositableDelegateProxy.sol";
 import "../kernel/KernelConstants.sol";
 import "../kernel/IKernel.sol";
+import "../common/DelegateProxy.sol";
 
 
-contract AppProxyBase is AppStorage, DepositableDelegateProxy, KernelNamespaceConstants {
+contract AppProxyBase is AppStorage, KernelNamespaceConstants, DelegateProxy {
     /**
     * @dev Initialize AppProxy
     * @param _kernel Reference to organization kernel for the app

diff --git a/contracts/apps/AppProxyDepositable.sol b/contracts/apps/AppProxyDepositable.sol
@@ -0,0 +1,19 @@
+pragma solidity 0.4.24;
+
+import "./AppProxyBase.sol";
+import "../common/DepositableDelegateProxy.sol";
+
+
+contract AppProxyDepositable is AppProxyBase, DepositableDelegateProxy {
+    /**
+    * @dev Initialize AppProxyDepositable
+    * @param _kernel Reference to organization kernel for the app
+    * @param _appId Identifier for app
+    * @param _initializePayload Payload for call to be made after setup to initialize
+    */
+    constructor(IKernel _kernel, bytes32 _appId, bytes _initializePayload)
+        AppProxyBase(_kernel, _appId, _initializePayload)
+        public
+    {
+    }
+}
diff --git a/contracts/apps/AppProxyNonDepositable.sol b/contracts/apps/AppProxyNonDepositable.sol
@@ -0,0 +1,19 @@
+pragma solidity 0.4.24;
+
+import "./AppProxyBase.sol";
+import "../common/NonDepositableDelegateProxy.sol";
+
+
+contract AppProxyNonDepositable is AppProxyBase, NonDepositableDelegateProxy {
+    /**
+    * @dev Initialize AppProxyNonDepositable
+    * @param _kernel Reference to organization kernel for the app
+    * @param _appId Identifier for app
+    * @param _initializePayload Payload for call to be made after setup to initialize
+    */
+    constructor(IKernel _kernel, bytes32 _appId, bytes _initializePayload)
+        AppProxyBase(_kernel, _appId, _initializePayload)
+        public
+    {
+    }
+}
diff --git a/contracts/apps/AppProxyPinned.sol → contracts/apps/AppProxyPinnedBase.sol b/contracts/apps/AppProxyPinned.sol → contracts/apps/AppProxyPinnedBase.sol
@@ -2,29 +2,16 @@ pragma solidity 0.4.24;
 
 import "../common/UnstructuredStorage.sol";
 import "../common/IsContract.sol";
-import "./AppProxyBase.sol";
+//import "./AppProxyBase.sol";
+import "../lib/misc/ERCProxy.sol";
 
 
-contract AppProxyPinned is IsContract, AppProxyBase {
+contract AppProxyPinnedBase is ERCProxy {
     using UnstructuredStorage for bytes32;
 
     // keccak256("aragonOS.appStorage.pinnedCode")
     bytes32 internal constant PINNED_CODE_POSITION = 0xdee64df20d65e53d7f51cb6ab6d921a0a6a638a91e942e1d8d02df28e31c038e;
 
-    /**
-    * @dev Initialize AppProxyPinned (makes it an un-upgradeable Aragon app)
-    * @param _kernel Reference to organization kernel for the app
-    * @param _appId Identifier for app
-    * @param _initializePayload Payload for call to be made after setup to initialize
-    */
-    constructor(IKernel _kernel, bytes32 _appId, bytes _initializePayload)
-        AppProxyBase(_kernel, _appId, _initializePayload)
-        public // solium-disable-line visibility-first
-    {
-        setPinnedCode(getAppBase(_appId));
-        require(isContract(pinnedCode()));
-    }
-
     /**
      * @dev ERC897, the address the proxy would delegate calls to
      */

diff --git a/contracts/apps/AppProxyPinnedDepositable.sol b/contracts/apps/AppProxyPinnedDepositable.sol
@@ -0,0 +1,21 @@
+pragma solidity 0.4.24;
+
+import "./AppProxyPinnedBase.sol";
+import "./AppProxyDepositable.sol";
+
+
+contract AppProxyPinnedDepositable is AppProxyPinnedBase, AppProxyDepositable {
+    /**
+     * @dev Initialize AppProxyPinned (makes it an un-upgradeable Aragon app)
+     * @param _kernel Reference to organization kernel for the app
+     * @param _appId Identifier for app
+     * @param _initializePayload Payload for call to be made after setup to initialize
+     */
+    constructor(IKernel _kernel, bytes32 _appId, bytes _initializePayload)
+        AppProxyDepositable(_kernel, _appId, _initializePayload)
+        public // solium-disable-line visibility-first
+    {
+        setPinnedCode(getAppBase(_appId));
+        require(isContract(pinnedCode()));
+    }
+}
diff --git a/contracts/apps/AppProxyPinnedNonDepositable.sol b/contracts/apps/AppProxyPinnedNonDepositable.sol
@@ -0,0 +1,21 @@
+pragma solidity 0.4.24;
+
+import "./AppProxyPinnedBase.sol";
+import "./AppProxyNonDepositable.sol";
+
+
+contract AppProxyPinnedNonDepositable is AppProxyPinnedBase, AppProxyNonDepositable {
+    /**
+     * @dev Initialize AppProxyPinned (makes it an un-upgradeable Aragon app)
+     * @param _kernel Reference to organization kernel for the app
+     * @param _appId Identifier for app
+     * @param _initializePayload Payload for call to be made after setup to initialize
+     */
+    constructor(IKernel _kernel, bytes32 _appId, bytes _initializePayload)
+        AppProxyNonDepositable(_kernel, _appId, _initializePayload)
+        public // solium-disable-line visibility-first
+    {
+        setPinnedCode(getAppBase(_appId));
+        require(isContract(pinnedCode()));
+    }
+}
diff --git a/contracts/apps/AppProxyUpgradeable.sol b/contracts/apps/AppProxyUpgradeable.sol
diff --git a/contracts/apps/AppProxyUpgradeableBase.sol b/contracts/apps/AppProxyUpgradeableBase.sol
@@ -0,0 +1,19 @@
+pragma solidity 0.4.24;
+
+import "./AppProxyBase.sol";
+
+contract AppProxyUpgradeableBase is AppProxyBase {
+    /**
+     * @dev ERC897, the address the proxy would delegate calls to
+     */
+    function implementation() public view returns (address) {
+        return getAppBase(appId());
+    }
+
+    /**
+     * @dev ERC897, whether it is a forwarding (1) or an upgradeable (2) proxy
+     */
+    function proxyType() public pure returns (uint256 proxyTypeId) {
+        return UPGRADEABLE;
+    }
+}
diff --git a/contracts/apps/AppProxyUpgradeableDepositable.sol b/contracts/apps/AppProxyUpgradeableDepositable.sol
@@ -0,0 +1,20 @@
+pragma solidity 0.4.24;
+
+import "./AppProxyUpgradeableBase.sol";
+import "./AppProxyDepositable.sol";
+
+
+contract AppProxyUpgradeableDepositable is AppProxyUpgradeableBase, AppProxyDepositable {
+    /**
+     * @dev Initialize AppProxyUpgradeable (makes it an upgradeable Aragon app)
+     * @param _kernel Reference to organization kernel for the app
+     * @param _appId Identifier for app
+     * @param _initializePayload Payload for call to be made after setup to initialize
+     */
+    constructor(IKernel _kernel, bytes32 _appId, bytes _initializePayload)
+        AppProxyDepositable(_kernel, _appId, _initializePayload)
+        public // solium-disable-line visibility-first
+    {
+        // solium-disable-previous-line no-empty-blocks
+    }
+}
diff --git a/contracts/apps/AppProxyUpgradeableNonDepositable.sol b/contracts/apps/AppProxyUpgradeableNonDepositable.sol
@@ -0,0 +1,20 @@
+pragma solidity 0.4.24;
+
+import "./AppProxyUpgradeableBase.sol";
+import "./AppProxyNonDepositable.sol";
+
+
+contract AppProxyUpgradeableNonDepositable is AppProxyUpgradeableBase, AppProxyNonDepositable {
+    /**
+     * @dev Initialize AppProxyUpgradeable (makes it an upgradeable Aragon app)
+     * @param _kernel Reference to organization kernel for the app
+     * @param _appId Identifier for app
+     * @param _initializePayload Payload for call to be made after setup to initialize
+     */
+    constructor(IKernel _kernel, bytes32 _appId, bytes _initializePayload)
+        AppProxyNonDepositable(_kernel, _appId, _initializePayload)
+        public // solium-disable-line visibility-first
+    {
+        // solium-disable-previous-line no-empty-blocks
+    }
+}
diff --git a/contracts/common/DepositableDelegateProxy.sol b/contracts/common/DepositableDelegateProxy.sol
@@ -1,17 +1,15 @@
 pragma solidity 0.4.24;
 
 import "./DelegateProxy.sol";
-import "./DepositableStorage.sol";
 
 
-contract DepositableDelegateProxy is DepositableStorage, DelegateProxy {
+contract DepositableDelegateProxy is DelegateProxy {
     event ProxyDeposit(address sender, uint256 value);
 
     function () external payable {
         // send / transfer
         if (gasleft() < FWD_GAS_LIMIT) {
             require(msg.value > 0 && msg.data.length == 0);
-            require(isDepositable());
             emit ProxyDeposit(msg.sender, msg.value);
         } else { // all calls except for send or transfer
             address target = implementation();

diff --git a/contracts/common/DepositableStorage.sol b/contracts/common/DepositableStorage.sol
diff --git a/contracts/common/NonDepositableDelegateProxy.sol b/contracts/common/NonDepositableDelegateProxy.sol
@@ -0,0 +1,16 @@
+pragma solidity 0.4.24;
+
+import "./DelegateProxy.sol";
+
+
+contract NonDepositableDelegateProxy is DelegateProxy {
+    function () external payable {
+        // send / transfer
+        if (gasleft() < FWD_GAS_LIMIT) {
+            revert();
+        } else { // all calls except for send or transfer
+            address target = implementation();
+            delegatedFwd(target, msg.data);
+        }
+    }
+}
diff --git a/contracts/factory/APMRegistryFactory.sol b/contracts/factory/APMRegistryFactory.sol
@@ -78,6 +78,7 @@ contract APMRegistryFactory is APMInternalAppNames {
                 keccak256(abi.encodePacked(node, keccak256(abi.encodePacked(ENS_SUB_APP_NAME)))),
                 ensSubdomainRegistrarBase,
                 noInit,
+                false,
                 false
             )
         );
@@ -86,6 +87,7 @@ contract APMRegistryFactory is APMInternalAppNames {
                 keccak256(abi.encodePacked(node, keccak256(abi.encodePacked(APM_APP_NAME)))),
                 registryBase,
                 noInit,
+                false,
                 false
             )
         );

diff --git a/contracts/factory/AppProxyFactory.sol b/contracts/factory/AppProxyFactory.sol
@@ -1,7 +1,11 @@
 pragma solidity 0.4.24;
 
-import "../apps/AppProxyUpgradeable.sol";
-import "../apps/AppProxyPinned.sol";
+import "../apps/AppProxyUpgradeableBase.sol";
+import "../apps/AppProxyUpgradeableDepositable.sol";
+import "../apps/AppProxyUpgradeableNonDepositable.sol";
+import "../apps/AppProxyPinnedBase.sol";
+import "../apps/AppProxyPinnedDepositable.sol";
+import "../apps/AppProxyPinnedNonDepositable.sol";
 
 
 contract AppProxyFactory {
@@ -13,8 +17,8 @@ contract AppProxyFactory {
     * @param _appId Identifier for app
     * @return AppProxyUpgradeable
     */
-    function newAppProxy(IKernel _kernel, bytes32 _appId) public returns (AppProxyUpgradeable) {
-        return newAppProxy(_kernel, _appId, new bytes(0));
+    function newAppProxy(IKernel _kernel, bytes32 _appId) public returns (AppProxyUpgradeableBase) {
+        return newAppProxy(_kernel, _appId, new bytes(0), false);
     }
 
     /**
@@ -23,8 +27,13 @@ contract AppProxyFactory {
     * @param _appId Identifier for app
     * @return AppProxyUpgradeable
     */
-    function newAppProxy(IKernel _kernel, bytes32 _appId, bytes _initializePayload) public returns (AppProxyUpgradeable) {
-        AppProxyUpgradeable proxy = new AppProxyUpgradeable(_kernel, _appId, _initializePayload);
+    function newAppProxy(IKernel _kernel, bytes32 _appId, bytes _initializePayload, bool _depositable) public returns (AppProxyUpgradeableBase) {
+        AppProxyUpgradeableBase proxy;
+        if (_depositable) {
+            proxy = new AppProxyUpgradeableDepositable(_kernel, _appId, _initializePayload);
+        } else {
+            proxy = new AppProxyUpgradeableNonDepositable(_kernel, _appId, _initializePayload);
+        }
         emit NewAppProxy(address(proxy), true, _appId);
         return proxy;
     }
@@ -35,8 +44,8 @@ contract AppProxyFactory {
     * @param _appId Identifier for app
     * @return AppProxyPinned
     */
-    function newAppProxyPinned(IKernel _kernel, bytes32 _appId) public returns (AppProxyPinned) {
-        return newAppProxyPinned(_kernel, _appId, new bytes(0));
+    function newAppProxyPinned(IKernel _kernel, bytes32 _appId) public returns (AppProxyPinnedBase) {
+        return newAppProxyPinned(_kernel, _appId, new bytes(0), false);
     }
 
     /**
@@ -46,8 +55,13 @@ contract AppProxyFactory {
     * @param _initializePayload Proxy initialization payload
     * @return AppProxyPinned
     */
-    function newAppProxyPinned(IKernel _kernel, bytes32 _appId, bytes _initializePayload) public returns (AppProxyPinned) {
-        AppProxyPinned proxy = new AppProxyPinned(_kernel, _appId, _initializePayload);
+    function newAppProxyPinned(IKernel _kernel, bytes32 _appId, bytes _initializePayload, bool _depositable) public returns (AppProxyPinnedBase) {
+        AppProxyPinnedBase proxy;
+        if (_depositable) {
+            proxy = new AppProxyPinnedDepositable(_kernel, _appId, _initializePayload);
+        } else {
+            proxy = new AppProxyPinnedNonDepositable(_kernel, _appId, _initializePayload);
+        }
         emit NewAppProxy(address(proxy), false, _appId);
         return proxy;
     }

diff --git a/contracts/factory/EVMScriptRegistryFactory.sol b/contracts/factory/EVMScriptRegistryFactory.sol
@@ -28,7 +28,7 @@ contract EVMScriptRegistryFactory is EVMScriptRegistryConstants {
     */
     function newEVMScriptRegistry(Kernel _dao) public returns (EVMScriptRegistry reg) {
         bytes memory initPayload = abi.encodeWithSelector(reg.initialize.selector);
-        reg = EVMScriptRegistry(_dao.newPinnedAppInstance(EVMSCRIPT_REGISTRY_APP_ID, baseReg, initPayload, true));
+        reg = EVMScriptRegistry(_dao.newPinnedAppInstance(EVMSCRIPT_REGISTRY_APP_ID, baseReg, initPayload, true, false));
 
         ACL acl = ACL(_dao.acl());
-Original file line number
+Diff line change
@@ Expand Up / @@ -78,6 +78,7 @@ contract APMRegistryFactory is APMInternalAppNames { @@
                     keccak256(abi.encodePacked(node, keccak256(abi.encodePacked(ENS_SUB_APP_NAME)))),
                     ensSubdomainRegistrarBase,
                     noInit,
+                    false,
                     false
                 )
             );
@@ Expand All / @@ -86,6 +87,7 @@ contract APMRegistryFactory is APMInternalAppNames { @@
                     keccak256(abi.encodePacked(node, keccak256(abi.encodePacked(APM_APP_NAME)))),
                     registryBase,
                     noInit,
+                    false,
                     false
                 )
             );
@@ Expand Down @@