diff --git a/contracts/apps/AppProxyBase.sol b/contracts/apps/AppProxyBase.sol index 8c8930f96..f377a63c6 100644 --- 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 new file mode 100644 index 000000000..3501be96f --- /dev/null +++ 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 new file mode 100644 index 000000000..d2098c51f --- /dev/null +++ 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 b/contracts/apps/AppProxyPinnedBase.sol similarity index 59% rename from contracts/apps/AppProxyPinned.sol rename to contracts/apps/AppProxyPinnedBase.sol index 95a4c500b..ecd15e2f8 100644 --- a/contracts/apps/AppProxyPinned.sol +++ b/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 new file mode 100644 index 000000000..b224e0b97 --- /dev/null +++ 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 new file mode 100644 index 000000000..73e53f124 --- /dev/null +++ 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 deleted file mode 100644 index 84ca19578..000000000 --- a/contracts/apps/AppProxyUpgradeable.sol +++ /dev/null @@ -1,33 +0,0 @@ -pragma solidity 0.4.24; - -import "./AppProxyBase.sol"; - - -contract AppProxyUpgradeable is AppProxyBase { - /** - * @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) - AppProxyBase(_kernel, _appId, _initializePayload) - public // solium-disable-line visibility-first - { - // solium-disable-previous-line no-empty-blocks - } - - /** - * @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/AppProxyUpgradeableBase.sol b/contracts/apps/AppProxyUpgradeableBase.sol new file mode 100644 index 000000000..ea21b9bc9 --- /dev/null +++ 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 new file mode 100644 index 000000000..2bbc66870 --- /dev/null +++ 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 new file mode 100644 index 000000000..672f4d384 --- /dev/null +++ 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 index eda7ca5a7..0d691c154 100644 --- 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 deleted file mode 100644 index 9b9970de3..000000000 --- a/contracts/common/DepositableStorage.sol +++ /dev/null @@ -1,19 +0,0 @@ -pragma solidity 0.4.24; - -import "./UnstructuredStorage.sol"; - - -contract DepositableStorage { - using UnstructuredStorage for bytes32; - - // keccak256("aragonOS.depositableStorage.depositable") - bytes32 internal constant DEPOSITABLE_POSITION = 0x665fd576fbbe6f247aff98f5c94a561e3f71ec2d3c988d56f12d342396c50cea; - - function isDepositable() public view returns (bool) { - return DEPOSITABLE_POSITION.getStorageBool(); - } - - function setDepositable(bool _depositable) internal { - DEPOSITABLE_POSITION.setStorageBool(_depositable); - } -} diff --git a/contracts/common/NonDepositableDelegateProxy.sol b/contracts/common/NonDepositableDelegateProxy.sol new file mode 100644 index 000000000..a0294eb21 --- /dev/null +++ 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 index 7f0aa3fd3..8b21bcf5b 100644 --- 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 index d851c839c..1aa44a4d6 100644 --- 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 index 3ff9f3f62..052726b8d 100644 --- 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()); diff --git a/contracts/kernel/Kernel.sol b/contracts/kernel/Kernel.sol index 1fc919055..516d7d307 100644 --- a/contracts/kernel/Kernel.sol +++ b/contracts/kernel/Kernel.sol @@ -66,7 +66,15 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant auth(APP_MANAGER_ROLE, arr(KERNEL_APP_BASES_NAMESPACE, _appId)) returns (ERCProxy appProxy) { - return newAppInstance(_appId, _appBase, new bytes(0), false); + return newAppInstance(_appId, _appBase, new bytes(0), false, false); + } + + function newAppInstance(bytes32 _appId, address _appBase, bytes _initializePayload, bool _setDefault) + public + auth(APP_MANAGER_ROLE, arr(KERNEL_APP_BASES_NAMESPACE, _appId)) + returns (ERCProxy appProxy) + { + return newAppInstance(_appId, _appBase, new bytes(0), _setDefault, false); } /** @@ -81,13 +89,13 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant * like Vault for escape hatch mechanism. * @return AppProxy instance */ - function newAppInstance(bytes32 _appId, address _appBase, bytes _initializePayload, bool _setDefault) + function newAppInstance(bytes32 _appId, address _appBase, bytes _initializePayload, bool _setDefault, bool _depositable) public auth(APP_MANAGER_ROLE, arr(KERNEL_APP_BASES_NAMESPACE, _appId)) returns (ERCProxy appProxy) { _setAppIfNew(KERNEL_APP_BASES_NAMESPACE, _appId, _appBase); - appProxy = newAppProxy(this, _appId, _initializePayload); + appProxy = newAppProxy(this, _appId, _initializePayload, _depositable); // By calling setApp directly and not the internal functions, we make sure the params are checked // and it will only succeed if sender has permissions to set something to the namespace. if (_setDefault) { @@ -107,7 +115,15 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant auth(APP_MANAGER_ROLE, arr(KERNEL_APP_BASES_NAMESPACE, _appId)) returns (ERCProxy appProxy) { - return newPinnedAppInstance(_appId, _appBase, new bytes(0), false); + return newPinnedAppInstance(_appId, _appBase, new bytes(0), false, false); + } + + function newPinnedAppInstance(bytes32 _appId, address _appBase, bytes _initializePayload, bool _setDefault) + public + auth(APP_MANAGER_ROLE, arr(KERNEL_APP_BASES_NAMESPACE, _appId)) + returns (ERCProxy appProxy) + { + return newPinnedAppInstance(_appId, _appBase, new bytes(0), _setDefault, false); } /** @@ -122,13 +138,13 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant * like Vault for escape hatch mechanism. * @return AppProxy instance */ - function newPinnedAppInstance(bytes32 _appId, address _appBase, bytes _initializePayload, bool _setDefault) + function newPinnedAppInstance(bytes32 _appId, address _appBase, bytes _initializePayload, bool _setDefault, bool _depositable) public auth(APP_MANAGER_ROLE, arr(KERNEL_APP_BASES_NAMESPACE, _appId)) returns (ERCProxy appProxy) { _setAppIfNew(KERNEL_APP_BASES_NAMESPACE, _appId, _appBase); - appProxy = newAppProxyPinned(this, _appId, _initializePayload); + appProxy = newAppProxyPinned(this, _appId, _initializePayload, _depositable); // By calling setApp directly and not the internal functions, we make sure the params are checked // and it will only succeed if sender has permissions to set something to the namespace. if (_setDefault) { diff --git a/contracts/kernel/KernelProxy.sol b/contracts/kernel/KernelProxy.sol index d298d8e28..c234d6e33 100644 --- a/contracts/kernel/KernelProxy.sol +++ b/contracts/kernel/KernelProxy.sol @@ -3,11 +3,11 @@ pragma solidity 0.4.24; import "./IKernel.sol"; import "./KernelConstants.sol"; import "./KernelStorage.sol"; -import "../common/DepositableDelegateProxy.sol"; +import "../common/NonDepositableDelegateProxy.sol"; import "../common/IsContract.sol"; -contract KernelProxy is IKernelEvents, KernelStorage, KernelAppIds, KernelNamespaceConstants, IsContract, DepositableDelegateProxy { +contract KernelProxy is IKernelEvents, KernelStorage, KernelAppIds, KernelNamespaceConstants, IsContract, NonDepositableDelegateProxy { /** * @dev KernelProxy is a proxy contract to a kernel implementation. The implementation * can update the reference, which effectively upgrades the contract diff --git a/contracts/test/mocks/apps/AppProxyPinnedStorageMock.sol b/contracts/test/mocks/apps/AppProxyPinnedStorageMock.sol index 1c4ea2a11..99a981289 100644 --- a/contracts/test/mocks/apps/AppProxyPinnedStorageMock.sol +++ b/contracts/test/mocks/apps/AppProxyPinnedStorageMock.sol @@ -1,6 +1,6 @@ pragma solidity 0.4.24; -import "../../../apps/AppProxyPinned.sol"; +import "../../../apps/AppProxyPinnedDepositable.sol"; import "../../../kernel/IKernel.sol"; import "../../../kernel/Kernel.sol"; @@ -19,9 +19,9 @@ contract KernelPinnedStorageMock is Kernel, FakeAppConstants { // Testing this contract is a bit of a pain... we can't overload anything to make the contract check // pass in the constructor, so we're forced to initialize this with a mocked Kernel that already // sets a contract for the fake app. -contract AppProxyPinnedStorageMock is AppProxyPinned, FakeAppConstants { +contract AppProxyPinnedStorageMock is AppProxyPinnedDepositable, FakeAppConstants { constructor(KernelPinnedStorageMock _mockKernel) - AppProxyPinned(IKernel(_mockKernel), FAKE_APP_ID, new bytes(0)) + AppProxyPinnedDepositable(IKernel(_mockKernel), FAKE_APP_ID, new bytes(0)) public // solium-disable-line visibility-first { } diff --git a/contracts/test/mocks/apps/AppStubConditionalRecovery.sol b/contracts/test/mocks/apps/AppStubConditionalRecovery.sol index 9ba879c2f..32a039784 100644 --- a/contracts/test/mocks/apps/AppStubConditionalRecovery.sol +++ b/contracts/test/mocks/apps/AppStubConditionalRecovery.sol @@ -1,13 +1,11 @@ pragma solidity 0.4.24; import "../../../apps/AragonApp.sol"; -import "../../../common/DepositableStorage.sol"; -contract AppStubConditionalRecovery is AragonApp, DepositableStorage { +contract AppStubConditionalRecovery is AragonApp { function initialize() onlyInit public { initialized(); - setDepositable(true); } function allowRecoverability(address token) public view returns (bool) { diff --git a/contracts/test/mocks/apps/AppStubDepositable.sol b/contracts/test/mocks/apps/AppStubDepositable.sol index 17bdbd648..3b5a0d2c1 100644 --- a/contracts/test/mocks/apps/AppStubDepositable.sol +++ b/contracts/test/mocks/apps/AppStubDepositable.sol @@ -2,12 +2,10 @@ pragma solidity 0.4.24; import "../../../apps/AragonApp.sol"; import "../../../apps/UnsafeAragonApp.sol"; -import "../../../common/DepositableStorage.sol"; -contract AppStubDepositable is AragonApp, DepositableStorage { +contract AppStubDepositable is AragonApp { function () external payable { - require(isDepositable()); } function initialize() onlyInit public { @@ -15,7 +13,6 @@ contract AppStubDepositable is AragonApp, DepositableStorage { } function enableDeposits() external { - setDepositable(true); } } diff --git a/contracts/test/mocks/apps/VaultMock.sol b/contracts/test/mocks/apps/VaultMock.sol index 2e9ead9c9..9de3c3c96 100644 --- a/contracts/test/mocks/apps/VaultMock.sol +++ b/contracts/test/mocks/apps/VaultMock.sol @@ -1,15 +1,13 @@ pragma solidity 0.4.24; import "../../../apps/UnsafeAragonApp.sol"; -import "../../../common/DepositableStorage.sol"; -contract VaultMock is UnsafeAragonApp, DepositableStorage { +contract VaultMock is UnsafeAragonApp { event LogFund(address sender, uint256 amount); function initialize() external { initialized(); - setDepositable(true); } function () external payable { diff --git a/contracts/test/mocks/common/DepositableStorageMock.sol b/contracts/test/mocks/common/DepositableStorageMock.sol deleted file mode 100644 index 56f190a3b..000000000 --- a/contracts/test/mocks/common/DepositableStorageMock.sol +++ /dev/null @@ -1,14 +0,0 @@ -pragma solidity 0.4.24; - -import "../../../common/DepositableStorage.sol"; - - -contract DepositableStorageMock is DepositableStorage { - function setDepositableExt(bool _depositable) public { - setDepositable(_depositable); - } - - function getDepositablePosition() public pure returns (bytes32) { - return DEPOSITABLE_POSITION; - } -} diff --git a/contracts/test/mocks/factory/APMRegistryFactoryMock.sol b/contracts/test/mocks/factory/APMRegistryFactoryMock.sol index 594cea916..873842a36 100644 --- a/contracts/test/mocks/factory/APMRegistryFactoryMock.sol +++ b/contracts/test/mocks/factory/APMRegistryFactoryMock.sol @@ -57,6 +57,7 @@ contract APMRegistryFactoryMock is APMInternalAppNames { keccak256(abi.encodePacked(node, keccak256(abi.encodePacked(ENS_SUB_APP_NAME)))), ensSubdomainRegistrarBase, noInit, + false, false ) ); @@ -65,6 +66,7 @@ contract APMRegistryFactoryMock is APMInternalAppNames { keccak256(abi.encodePacked(node, keccak256(abi.encodePacked(APM_APP_NAME)))), registryBase, noInit, + false, false ) ); diff --git a/contracts/test/mocks/kernel/KernelDepositableMock.sol b/contracts/test/mocks/kernel/KernelDepositableMock.sol deleted file mode 100644 index 4de98fbb1..000000000 --- a/contracts/test/mocks/kernel/KernelDepositableMock.sol +++ /dev/null @@ -1,17 +0,0 @@ -pragma solidity 0.4.24; - -import "../../../common/DepositableStorage.sol"; -import "../../../kernel/Kernel.sol"; - -contract KernelDepositableMock is Kernel, DepositableStorage { - constructor(bool _shouldPetrify) Kernel(_shouldPetrify) public { - } - - function () external payable { - require(isDepositable()); - } - - function enableDeposits() external isInitialized { - setDepositable(true); - } -} diff --git a/contracts/test/mocks/kernel/KernelOverloadMock.sol b/contracts/test/mocks/kernel/KernelOverloadMock.sol index 66d87ec2b..14b3e9819 100644 --- a/contracts/test/mocks/kernel/KernelOverloadMock.sol +++ b/contracts/test/mocks/kernel/KernelOverloadMock.sol @@ -34,17 +34,17 @@ contract KernelOverloadMock { emit NewAppProxy(appProxy); } + function newPinnedAppInstance(bytes32 _appId, address _appBase) + public + returns (ERCProxy appProxy) + { + appProxy = kernel.newPinnedAppInstance(_appId, _appBase); + emit NewAppProxy(appProxy); + } /* function newPinnedAppInstance(bytes32 _appId, address _appBase, bytes _initializePayload, bool _setDefault) public auth(APP_MANAGER_ROLE, arr(KERNEL_APP_BASES_NAMESPACE, _appId)) returns (ERCProxy appProxy) */ - function newPinnedAppInstance(bytes32 _appId, address _appBase, bytes _initializePayload, bool _setDefault) - public - returns (ERCProxy appProxy) - { - appProxy = kernel.newPinnedAppInstance(_appId, _appBase, _initializePayload, _setDefault); - emit NewAppProxy(appProxy); - } } diff --git a/test/contracts/kernel/kernel_acl.js b/test/contracts/kernel/kernel_acl.js index e9f2fa97d..96535a9d6 100644 --- a/test/contracts/kernel/kernel_acl.js +++ b/test/contracts/kernel/kernel_acl.js @@ -73,7 +73,7 @@ contract('Kernel ACL', accounts => { it('cannot initialize proxied ACL outside of Kernel', async () => { // Set up ACL proxy await acl.createPermission(permissionsRoot, kernelAddr, APP_MANAGER_ROLE, permissionsRoot) - const receipt = await kernel.newAppInstance(DEFAULT_ACL_APP_ID, aclBase.address, EMPTY_BYTES, false) + const receipt = await kernel.newAppInstance(DEFAULT_ACL_APP_ID, aclBase.address) const newAcl = ACL.at(getNewProxyAddress(receipt)) await assertRevert(newAcl.initialize(permissionsRoot)) diff --git a/test/contracts/kernel/kernel_apps.js b/test/contracts/kernel/kernel_apps.js index 49fea658b..8c76558a7 100644 --- a/test/contracts/kernel/kernel_apps.js +++ b/test/contracts/kernel/kernel_apps.js @@ -96,7 +96,7 @@ contract('Kernel apps', ([permissionsRoot]) => { context(`> new ${appProxyType} instances`, () => { onlyAppProxy(() => it('creates a new upgradeable app proxy instance', async () => { - const receipt = await kernel.newAppInstance(APP_ID, appBase1.address, EMPTY_BYTES, false) + const receipt = await kernel.newAppInstance(APP_ID, appBase1.address) const appProxy = AppProxyUpgradeable.at(getNewProxyAddress(receipt)) assert.equal(await appProxy.kernel(), kernel.address, "new appProxy instance's kernel should be set to the originating kernel") @@ -132,16 +132,17 @@ contract('Kernel apps', ([permissionsRoot]) => { it('sets the app base when not previously registered', async() => { assert.equal(ZERO_ADDR, await kernel.getApp(APP_BASES_NAMESPACE, APP_ID)) - await kernelOverload[newInstanceFn](APP_ID, appBase1.address, EMPTY_BYTES, false) + await kernelOverload[newInstanceFn](APP_ID, appBase1.address) assert.equal(appBase1.address, await kernel.getApp(APP_BASES_NAMESPACE, APP_ID)) }) it("doesn't set the app base when already set", async() => { await kernel.setApp(APP_BASES_NAMESPACE, APP_ID, appBase1.address) - const receipt = await kernelOverload[newInstanceFn](APP_ID, appBase1.address, EMPTY_BYTES, false) + const receipt = await kernelOverload[newInstanceFn](APP_ID, appBase1.address) assertAmountOfEvents(receipt, 'SetApp', 0) }) + /* TODO it("also sets the default app", async () => { const receipt = await kernelOverload[newInstanceFn](APP_ID, appBase1.address, EMPTY_BYTES, true) const appProxyAddr = getNewProxyAddress(receipt) @@ -166,9 +167,10 @@ contract('Kernel apps', ([permissionsRoot]) => { assert.equal(await kernel.getApp(APP_BASES_NAMESPACE, APP_ID), appBase1.address, 'App base should be set') assert.equal(await kernel.getApp(APP_ADDR_NAMESPACE, APP_ID), ZERO_ADDR, "Default app shouldn't be set") }) + */ it("fails if the app base is not given", async() => { - await assertRevert(kernelOverload[newInstanceFn](APP_ID, ZERO_ADDR, EMPTY_BYTES, false)) + await assertRevert(kernelOverload[newInstanceFn](APP_ID, ZERO_ADDR)) }) it('fails if the given app base is different than the existing one', async() => { @@ -177,7 +179,7 @@ contract('Kernel apps', ([permissionsRoot]) => { assert.notEqual(existingBase, differentBase, 'appBase1 and appBase2 should have different addresses') await kernel.setApp(APP_BASES_NAMESPACE, APP_ID, existingBase) - await assertRevert(kernelOverload[newInstanceFn](APP_ID, differentBase, EMPTY_BYTES, false)) + await assertRevert(kernelOverload[newInstanceFn](APP_ID, differentBase)) }) }) diff --git a/test/contracts/kernel/kernel_funds.js b/test/contracts/kernel/kernel_funds.js index de80d198e..5b38870fd 100644 --- a/test/contracts/kernel/kernel_funds.js +++ b/test/contracts/kernel/kernel_funds.js @@ -6,9 +6,6 @@ const ACL = artifacts.require('ACL') const Kernel = artifacts.require('Kernel') const KernelProxy = artifacts.require('KernelProxy') -// Mocks -const KernelDepositableMock = artifacts.require('KernelDepositableMock') - const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies contract('Kernel funds', ([permissionsRoot]) => { @@ -19,70 +16,40 @@ contract('Kernel funds', ([permissionsRoot]) => { aclBase = await ACL.new() }) - for (const kernelBaseType of [Kernel, KernelDepositableMock]) { - context(`> ${kernelBaseType.contractName}`, () => { - const onlyKernelDepositable = onlyIf(() => kernelBaseType === KernelDepositableMock) - - // Test both the base itself and the KernelProxy to make sure their behaviours are the same - for (const kernelType of ['Base', 'Proxy']) { - context(`> ${kernelType}`, () => { - let kernelBase, kernel - - before(async () => { - if (kernelType === 'Proxy') { - // We can reuse the same kernel base for the proxies - kernelBase = await kernelBaseType.new(true) // petrify immediately - } - }) - - beforeEach(async () => { - if (kernelType === 'Base') { - kernel = await kernelBaseType.new(false) // don't petrify so it can be used - } else if (kernelType === 'Proxy') { - kernel = kernelBaseType.at((await KernelProxy.new(kernelBase.address)).address) - } - }) - - it('cannot receive ETH', async () => { - // Before initialization - assert.isFalse(await kernel.hasInitialized(), 'should not have been initialized') - - await assertRevert(kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS })) - - // After initialization - await kernel.initialize(aclBase.address, permissionsRoot) - assert.isTrue(await kernel.hasInitialized(), 'should have been initialized') - - await assertRevert(kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS })) - }) + context(`> Kernel`, () => { + // Test both the base itself and the KernelProxy to make sure their behaviours are the same + for (const kernelType of ['Base', 'Proxy']) { + context(`> ${kernelType}`, () => { + let kernelBase, kernel + + before(async () => { + if (kernelType === 'Proxy') { + // We can reuse the same kernel base for the proxies + kernelBase = await Kernel.new(true) // petrify immediately + } + }) - onlyKernelDepositable(() => { - it('does not have depositing enabled by default', async () => { - // Before initialization - assert.isFalse(await kernel.hasInitialized(), 'should not have been initialized') - assert.isFalse(await kernel.isDepositable(), 'should not be depositable') + beforeEach(async () => { + if (kernelType === 'Base') { + kernel = await Kernel.new(false) // don't petrify so it can be used + } else if (kernelType === 'Proxy') { + kernel = Kernel.at((await KernelProxy.new(kernelBase.address)).address) + } + }) - // After initialization - await kernel.initialize(aclBase.address, permissionsRoot) - assert.isTrue(await kernel.hasInitialized(), 'should have been initialized') - assert.isFalse(await kernel.isDepositable(), 'should not be depositable') - }) + it('cannot receive ETH', async () => { + // Before initialization + assert.isFalse(await kernel.hasInitialized(), 'should not have been initialized') - it('can receive ETH after being enabled', async () => { - const amount = 1 - const initialBalance = await getBalance(kernel.address) + await assertRevert(kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS })) - await kernel.initialize(aclBase.address, permissionsRoot) - await kernel.enableDeposits() - assert.isTrue(await kernel.hasInitialized(), 'should have been initialized') - assert.isTrue(await kernel.isDepositable(), 'should be depositable') + // After initialization + await kernel.initialize(aclBase.address, permissionsRoot) + assert.isTrue(await kernel.hasInitialized(), 'should have been initialized') - await kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) - assert.equal((await getBalance(kernel.address)).valueOf(), initialBalance.plus(amount)) - }) - }) + await assertRevert(kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS })) }) - } - }) - } + }) + } + }) }) diff --git a/test/contracts/kernel/kernel_lifecycle.js b/test/contracts/kernel/kernel_lifecycle.js index 832a05295..8bfd2eb7a 100644 --- a/test/contracts/kernel/kernel_lifecycle.js +++ b/test/contracts/kernel/kernel_lifecycle.js @@ -22,7 +22,7 @@ contract('Kernel lifecycle', ([root, someone]) => { assert.isFalse(await kernel.hasPermission(root, kernel.address, APP_MANAGER_ROLE, EMPTY_BYTES)) assert.isFalse(await kernel.hasPermission(someone, kernel.address, APP_MANAGER_ROLE, EMPTY_BYTES)) - await assertRevert(kernel.newAppInstance(APP_ID, appBase.address, EMPTY_BYTES, false)) + await assertRevert(kernel.newAppInstance(APP_ID, appBase.address)) await assertRevert(kernel.newPinnedAppInstance(APP_ID, appBase.address)) await assertRevert(kernel.setApp(APP_BASES_NAMESPACE, APP_ID, appBase.address)) await assertRevert(kernel.setRecoveryVaultAppId(VAULT_ID)) @@ -40,7 +40,7 @@ contract('Kernel lifecycle', ([root, someone]) => { assert.isFalse(await kernel.hasPermission(someone, kernel.address, APP_MANAGER_ROLE, EMPTY_BYTES)) // And finally test functionality - await kernel.newAppInstance(APP_ID, appBase.address, EMPTY_BYTES, false) + await kernel.newAppInstance(APP_ID, appBase.address) } // Initial setup