From a56234e3c0265878d5f105c6be2b81f2108f57d6 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Tue, 4 Sep 2018 19:50:34 +0200 Subject: [PATCH] Add ability to enable / disable depositable functionality (#401) --- .solcover.js | 8 +- contracts/common/DepositableDelegateProxy.sol | 4 +- contracts/common/DepositableStorage.sol | 19 ++ contracts/common/UnstructuredStorage.sol | 8 + contracts/factory/DAOFactory.sol | 2 +- ...rage.sol => AppProxyPinnedStorageMock.sol} | 2 +- ...{AppStubStorage.sol => AppStorageMock.sol} | 13 +- contracts/test/mocks/AppStub.sol | 6 +- .../test/mocks/AppStubConditionalRecovery.sol | 16 +- contracts/test/mocks/AppStubDepositable.sol | 26 +++ .../test/mocks/DepositableStorageMock.sol | 14 ++ .../test/mocks/InitializableStorageMock.sol | 14 ++ contracts/test/mocks/KeccakConstants.sol | 1 + .../test/mocks/KernelDepositableMock.sol | 17 ++ contracts/test/mocks/VaultMock.sol | 10 +- test/app_funds.js | 119 ++++++++++++ test/keccak_constants.js | 21 +- test/kernel_funds.js | 93 +++++++++ test/proxy_recover_funds.js | 74 +++---- test/unstructured_storage.js | 182 +++++++++++------- 20 files changed, 505 insertions(+), 144 deletions(-) create mode 100644 contracts/common/DepositableStorage.sol rename contracts/test/mocks/{AppStubPinnedStorage.sol => AppProxyPinnedStorageMock.sol} (94%) rename contracts/test/mocks/{AppStubStorage.sol => AppStorageMock.sol} (53%) create mode 100644 contracts/test/mocks/AppStubDepositable.sol create mode 100644 contracts/test/mocks/DepositableStorageMock.sol create mode 100644 contracts/test/mocks/InitializableStorageMock.sol create mode 100644 contracts/test/mocks/KernelDepositableMock.sol create mode 100644 test/app_funds.js create mode 100644 test/kernel_funds.js diff --git a/.solcover.js b/.solcover.js index 7c7d61fc8..706360613 100644 --- a/.solcover.js +++ b/.solcover.js @@ -1,4 +1,10 @@ -const skipFiles = ['lib', 'test', 'acl/ACLSyntaxSugar.sol'] +const skipFiles = [ + 'lib', + 'test', + 'acl/ACLSyntaxSugar.sol', + 'common/DepositableStorage.sol', // Used in tests that send ETH + 'common/UnstructuredStorage.sol' // Used in tests that send ETH +] module.exports = { norpc: true, diff --git a/contracts/common/DepositableDelegateProxy.sol b/contracts/common/DepositableDelegateProxy.sol index 0d691c154..eda7ca5a7 100644 --- a/contracts/common/DepositableDelegateProxy.sol +++ b/contracts/common/DepositableDelegateProxy.sol @@ -1,15 +1,17 @@ pragma solidity 0.4.24; import "./DelegateProxy.sol"; +import "./DepositableStorage.sol"; -contract DepositableDelegateProxy is DelegateProxy { +contract DepositableDelegateProxy is DepositableStorage, 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 new file mode 100644 index 000000000..9b9970de3 --- /dev/null +++ b/contracts/common/DepositableStorage.sol @@ -0,0 +1,19 @@ +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/UnstructuredStorage.sol b/contracts/common/UnstructuredStorage.sol index d9d29bdc6..db0a56d98 100644 --- a/contracts/common/UnstructuredStorage.sol +++ b/contracts/common/UnstructuredStorage.sol @@ -6,6 +6,10 @@ pragma solidity ^0.4.24; library UnstructuredStorage { + function getStorageBool(bytes32 position) internal view returns (bool data) { + assembly { data := sload(position) } + } + function getStorageAddress(bytes32 position) internal view returns (address data) { assembly { data := sload(position) } } @@ -18,6 +22,10 @@ library UnstructuredStorage { assembly { data := sload(position) } } + function setStorageBool(bytes32 position, bool data) internal { + assembly { sstore(position, data) } + } + function setStorageAddress(bytes32 position, address data) internal { assembly { sstore(position, data) } } diff --git a/contracts/factory/DAOFactory.sol b/contracts/factory/DAOFactory.sol index 9d0cd5396..a5f357d5a 100644 --- a/contracts/factory/DAOFactory.sol +++ b/contracts/factory/DAOFactory.sol @@ -63,7 +63,7 @@ contract DAOFactory { } emit DeployDAO(address(dao)); - + return dao; } } diff --git a/contracts/test/mocks/AppStubPinnedStorage.sol b/contracts/test/mocks/AppProxyPinnedStorageMock.sol similarity index 94% rename from contracts/test/mocks/AppStubPinnedStorage.sol rename to contracts/test/mocks/AppProxyPinnedStorageMock.sol index 6c196b663..5de96a493 100644 --- a/contracts/test/mocks/AppStubPinnedStorage.sol +++ b/contracts/test/mocks/AppProxyPinnedStorageMock.sol @@ -20,7 +20,7 @@ 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 AppStubPinnedStorage is AppProxyPinned, FakeAppConstants { +contract AppProxyPinnedStorageMock is AppProxyPinned, FakeAppConstants { constructor(KernelPinnedStorageMock _mockKernel) AppProxyPinned(IKernel(_mockKernel), FAKE_APP_ID, new bytes(0)) public // solium-disable-line visibility-first diff --git a/contracts/test/mocks/AppStubStorage.sol b/contracts/test/mocks/AppStorageMock.sol similarity index 53% rename from contracts/test/mocks/AppStubStorage.sol rename to contracts/test/mocks/AppStorageMock.sol index e3939a9af..53d977e69 100644 --- a/contracts/test/mocks/AppStubStorage.sol +++ b/contracts/test/mocks/AppStorageMock.sol @@ -1,14 +1,9 @@ pragma solidity 0.4.24; -import "../../apps/UnsafeAragonApp.sol"; +import "../../apps/AppStorage.sol"; -// Need to use UnsafeAragonApp to allow initialization -contract AppStubStorage is UnsafeAragonApp { - function initialize() onlyInit public { - initialized(); - } - +contract AppStorageMock is AppStorage { function setKernelExt(IKernel _kernel) public { setKernel(_kernel); } @@ -24,8 +19,4 @@ contract AppStubStorage is UnsafeAragonApp { function getAppIdPosition() public pure returns (bytes32) { return APP_ID_POSITION; } - - function getInitializationBlockPosition() public pure returns (bytes32) { - return INITIALIZATION_BLOCK_POSITION; - } } diff --git a/contracts/test/mocks/AppStub.sol b/contracts/test/mocks/AppStub.sol index 9adf90c89..3225e4518 100644 --- a/contracts/test/mocks/AppStub.sol +++ b/contracts/test/mocks/AppStub.sol @@ -5,12 +5,12 @@ import "../../apps/UnsafeAragonApp.sol"; import "../../kernel/IKernel.sol"; -contract AppSt { +contract AppStubStorage { uint a; string public stringTest; } -contract AppStub is AragonApp, AppSt { +contract AppStub is AragonApp, AppStubStorage { bytes32 constant public ROLE = keccak256("ROLE"); function initialize() onlyInit public { @@ -35,7 +35,7 @@ contract AppStub is AragonApp, AppSt { } } -contract AppStub2 is AragonApp, AppSt { +contract AppStub2 is AragonApp, AppStubStorage { function getValue() public constant returns (uint) { return a * 2; } diff --git a/contracts/test/mocks/AppStubConditionalRecovery.sol b/contracts/test/mocks/AppStubConditionalRecovery.sol index d2cebc1e6..65298671d 100644 --- a/contracts/test/mocks/AppStubConditionalRecovery.sol +++ b/contracts/test/mocks/AppStubConditionalRecovery.sol @@ -1,11 +1,17 @@ pragma solidity 0.4.24; import "../../apps/AragonApp.sol"; +import "../../common/DepositableStorage.sol"; -contract AppStubConditionalRecovery is AragonApp { - function allowRecoverability(address token) public view returns (bool) { - // Doesn't allow to recover ether - return token != address(0); - } +contract AppStubConditionalRecovery is AragonApp, DepositableStorage { + function initialize() onlyInit public { + initialized(); + setDepositable(true); + } + + function allowRecoverability(address token) public view returns (bool) { + // Doesn't allow to recover ether + return token != address(0); + } } diff --git a/contracts/test/mocks/AppStubDepositable.sol b/contracts/test/mocks/AppStubDepositable.sol new file mode 100644 index 000000000..095b89ee5 --- /dev/null +++ b/contracts/test/mocks/AppStubDepositable.sol @@ -0,0 +1,26 @@ +pragma solidity 0.4.24; + +import "../../apps/AragonApp.sol"; +import "../../apps/UnsafeAragonApp.sol"; +import "../../common/DepositableStorage.sol"; + + +contract AppStubDepositable is AragonApp, DepositableStorage { + function () external payable { + require(isDepositable()); + } + + function initialize() onlyInit public { + initialized(); + } + + function enableDeposits() external { + setDepositable(true); + } +} + +contract UnsafeAppStubDepositable is AppStubDepositable, UnsafeAragonApp { + constructor(IKernel _kernel) public { + setKernel(_kernel); + } +} diff --git a/contracts/test/mocks/DepositableStorageMock.sol b/contracts/test/mocks/DepositableStorageMock.sol new file mode 100644 index 000000000..ede41cde5 --- /dev/null +++ b/contracts/test/mocks/DepositableStorageMock.sol @@ -0,0 +1,14 @@ +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/InitializableStorageMock.sol b/contracts/test/mocks/InitializableStorageMock.sol new file mode 100644 index 000000000..7dd93f1a2 --- /dev/null +++ b/contracts/test/mocks/InitializableStorageMock.sol @@ -0,0 +1,14 @@ +pragma solidity 0.4.24; + +import "../../common/Initializable.sol"; + + +contract InitializableStorageMock is Initializable { + function initialize() onlyInit public { + initialized(); + } + + function getInitializationBlockPosition() public pure returns (bytes32) { + return INITIALIZATION_BLOCK_POSITION; + } +} diff --git a/contracts/test/mocks/KeccakConstants.sol b/contracts/test/mocks/KeccakConstants.sol index e5f522f68..45e8103fc 100644 --- a/contracts/test/mocks/KeccakConstants.sol +++ b/contracts/test/mocks/KeccakConstants.sol @@ -50,6 +50,7 @@ contract KeccakConstants is APMNamehash { // Unstructured storage bytes32 public constant initializationBlockPosition = keccak256(abi.encodePacked("aragonOS.initializable.initializationBlock")); + bytes32 public constant depositablePosition = keccak256(abi.encodePacked("aragonOS.depositableStorage.depositable")); bytes32 public constant kernelPosition = keccak256(abi.encodePacked("aragonOS.appStorage.kernel")); bytes32 public constant appIdPosition = keccak256(abi.encodePacked("aragonOS.appStorage.appId")); bytes32 public constant pinnedCodePosition = keccak256(abi.encodePacked("aragonOS.appStorage.pinnedCode")); diff --git a/contracts/test/mocks/KernelDepositableMock.sol b/contracts/test/mocks/KernelDepositableMock.sol new file mode 100644 index 000000000..cf0e16f11 --- /dev/null +++ b/contracts/test/mocks/KernelDepositableMock.sol @@ -0,0 +1,17 @@ +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/VaultMock.sol b/contracts/test/mocks/VaultMock.sol index c2220b52f..452f8ea25 100644 --- a/contracts/test/mocks/VaultMock.sol +++ b/contracts/test/mocks/VaultMock.sol @@ -1,11 +1,17 @@ pragma solidity 0.4.24; -import "../../apps/AragonApp.sol"; +import "../../apps/UnsafeAragonApp.sol"; +import "../../common/DepositableStorage.sol"; -contract VaultMock is AragonApp { +contract VaultMock is UnsafeAragonApp, DepositableStorage { event LogFund(address sender, uint256 amount); + function initialize() external { + initialized(); + setDepositable(true); + } + function () external payable { emit LogFund(msg.sender, msg.value); } diff --git a/test/app_funds.js b/test/app_funds.js new file mode 100644 index 000000000..a65579f4b --- /dev/null +++ b/test/app_funds.js @@ -0,0 +1,119 @@ +const { hash } = require('eth-ens-namehash') +const { assertRevert } = require('./helpers/assertThrow') +const { getBalance } = require('./helpers/web3') +const { onlyIf } = require('./helpers/onlyIf') + +const ACL = artifacts.require('ACL') +const Kernel = artifacts.require('Kernel') +const KernelProxy = artifacts.require('KernelProxy') +const AppProxyUpgradeable = artifacts.require('AppProxyUpgradeable') +const AppProxyPinned = artifacts.require('AppProxyPinned') + +// Mocks +const AppStub = artifacts.require('AppStub') +const UnsafeAppStub = artifacts.require('UnsafeAppStub') +const AppStubDepositable = artifacts.require('AppStubDepositable') +const UnsafeAppStubDepositable = artifacts.require('UnsafeAppStubDepositable') + +const APP_ID = hash('stub.aragonpm.test') +const EMPTY_BYTES = '0x' +const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies + +contract('App funds', accounts => { + let aclBase, kernelBase + let APP_BASES_NAMESPACE + + const permissionsRoot = accounts[0] + + before(async () => { + kernelBase = await Kernel.new(true) // petrify immediately + aclBase = await ACL.new() + + // Setup constants + APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE() + }) + + const appBases = [ + { + base: AppStub, + unsafeBase: UnsafeAppStub, + }, + { + base: AppStubDepositable, + unsafeBase: UnsafeAppStubDepositable, + } + ] + for ({ base: appBaseType, unsafeBase: unsafeAppBaseType } of appBases) { + context(`> ${appBaseType.contractName}`, () => { + const onlyAppStubDepositable = onlyIf(() => appBaseType === AppStubDepositable) + + // Test the app itself and when it's behind the proxies to make sure their behaviours are the same + const appProxyTypes = ['AppProxyUpgradeable', 'AppProxyPinned'] + for (const appType of ['App', ...appProxyTypes]) { + context(`> ${appType}`, () => { + let appBase, app + + before(async () => { + if (appProxyTypes.includes(appType)) { + // We can reuse the same app base for the proxies + appBase = await appBaseType.new() + } + }) + + beforeEach(async () => { + const kernel = Kernel.at((await KernelProxy.new(kernelBase.address)).address) + await kernel.initialize(aclBase.address, permissionsRoot) + + if (appType === 'App') { + // Use the unsafe version to use directly without a proxy + app = await unsafeAppBaseType.new(kernel.address) + } else { + // Install app + const acl = ACL.at(await kernel.acl()) + const APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE() + await acl.createPermission(permissionsRoot, kernel.address, APP_MANAGER_ROLE, permissionsRoot) + await kernel.setApp(APP_BASES_NAMESPACE, APP_ID, appBase.address) + + let appProxy + if (appType === 'AppProxyUpgradeable') { + appProxy = await AppProxyUpgradeable.new(kernel.address, APP_ID, EMPTY_BYTES) + } else if (appType === 'AppProxyPinned') { + appProxy = await AppProxyPinned.new(kernel.address, APP_ID, EMPTY_BYTES) + } + + app = appBaseType.at(appProxy.address) + } + + await app.initialize(); + }) + + it('cannot receive ETH', async () => { + assert.isTrue(await app.hasInitialized(), 'should have been initialized') + + await assertRevert(async () => { + await app.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) + }) + }) + + onlyAppStubDepositable(() => { + it('does not have depositing enabled by default', async () => { + assert.isTrue(await app.hasInitialized(), 'should have been initialized') + assert.isFalse(await app.isDepositable(), 'should not be depositable') + }) + + it('can receive ETH after being set to depositable', async () => { + const amount = 1 + const initialBalance = await getBalance(app.address) + + await app.enableDeposits() + assert.isTrue(await app.isDepositable(), 'should be depositable') + + await app.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) + assert.equal((await getBalance(app.address)).valueOf(), initialBalance.plus(amount)) + }) + }) + }) + } + }) + } +}) diff --git a/test/keccak_constants.js b/test/keccak_constants.js index e0792a470..ff0e08533 100644 --- a/test/keccak_constants.js +++ b/test/keccak_constants.js @@ -82,19 +82,28 @@ contract('Constants', accounts => { }) it('checks AppStorage unstructured storage constants', async () => { - const app = await getContract('AppStubStorage').new() + const appStorage = await getContract('AppStorageMock').new() - assert.equal(await app.getKernelPosition(), await keccakConstants.kernelPosition(), "kernelPosition doesn't match") - assert.equal(await app.getAppIdPosition(), await keccakConstants.appIdPosition(), "appIdPosition doesn't match") - assert.equal(await app.getInitializationBlockPosition(), await keccakConstants.initializationBlockPosition(), "initializationBlockPosition doesn't match") + assert.equal(await appStorage.getKernelPosition(), await keccakConstants.kernelPosition(), "kernelPosition doesn't match") + assert.equal(await appStorage.getAppIdPosition(), await keccakConstants.appIdPosition(), "appIdPosition doesn't match") }) it('checks AppProxyPinned unstructured storage constants', async () => { // Set up AppStubPinnedStorage const fakeApp = await getContract('AppStub').new() const kernelMock = await getContract('KernelPinnedStorageMock').new(fakeApp.address) - const app = await getContract('AppStubPinnedStorage').new(kernelMock.address) + const pinnedProxy = await getContract('AppProxyPinnedStorageMock').new(kernelMock.address) - assert.equal(await app.getPinnedCodePosition(), await keccakConstants.pinnedCodePosition(), "pinnedCodePosition doesn't match") + assert.equal(await pinnedProxy.getPinnedCodePosition(), await keccakConstants.pinnedCodePosition(), "pinnedCodePosition doesn't match") + }) + + it('checks DepositableStorage unstructured storage constants', async () => { + const depositableMock = await getContract('DepositableStorageMock').new() + assert.equal(await depositableMock.getDepositablePosition(), await keccakConstants.depositablePosition(), "depositablePosition doesn't match") + }) + + it('checks Initializable unstructured storage constants', async () => { + const initializableMock = await getContract('InitializableStorageMock').new() + assert.equal(await initializableMock.getInitializationBlockPosition(), await keccakConstants.initializationBlockPosition(), "initializationBlockPosition doesn't match") }) }) diff --git a/test/kernel_funds.js b/test/kernel_funds.js new file mode 100644 index 000000000..8096b8c6f --- /dev/null +++ b/test/kernel_funds.js @@ -0,0 +1,93 @@ +const { assertRevert } = require('./helpers/assertThrow') +const { getBalance } = require('./helpers/web3') +const { onlyIf } = require('./helpers/onlyIf') + +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', accounts => { + let aclBase + const permissionsRoot = accounts[0] + + // Initial setup + before(async () => { + 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(async () => { + await 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(async () => { + await kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) + }) + }) + + 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') + + // 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('can receive ETH after being enabled', async () => { + const amount = 1 + const initialBalance = await getBalance(kernel.address) + + 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') + + await kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) + assert.equal((await getBalance(kernel.address)).valueOf(), initialBalance.plus(amount)) + }) + }) + }) + } + }) + } +}) diff --git a/test/proxy_recover_funds.js b/test/proxy_recover_funds.js index de59a41d2..a9f3f72a4 100644 --- a/test/proxy_recover_funds.js +++ b/test/proxy_recover_funds.js @@ -1,6 +1,5 @@ const { assertRevert } = require('./helpers/assertThrow') const { skipCoverage } = require('./helpers/coverage') -const { onlyIf } = require('./helpers/onlyIf') const { getBalance } = require('./helpers/web3') const { hash } = require('eth-ens-namehash') @@ -11,7 +10,7 @@ const ACL = artifacts.require('ACL') const AppProxyUpgradeable = artifacts.require('AppProxyUpgradeable') // Mocks -const AppStub = artifacts.require('AppStub') +const AppStubDepositable = artifacts.require('AppStubDepositable') const AppStubConditionalRecovery = artifacts.require('AppStubConditionalRecovery') const TokenMock = artifacts.require('TokenMock') const VaultMock = artifacts.require('VaultMock') @@ -19,6 +18,7 @@ const VaultMock = artifacts.require('VaultMock') const getEvent = (receipt, event, arg) => { return receipt.logs.filter(l => l.event == event)[0].args[arg] } const APP_ID = hash('stub.aragonpm.test') +const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies contract('Proxy funds', accounts => { let aclBase, appBase, appConditionalRecoveryBase @@ -31,7 +31,7 @@ contract('Proxy funds', accounts => { const amount = 1 const initialBalance = await getBalance(target.address) const initialVaultBalance = await getBalance(vault.address) - const r = await target.sendTransaction({ value: 1, gas: 31000 }) + const r = await target.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) await target.transferToVault(ETH) assert.equal((await getBalance(target.address)).valueOf(), 0) @@ -55,7 +55,7 @@ contract('Proxy funds', accounts => { const vaultId = hash('vaultfake.aragonpm.test') const initialBalance = await getBalance(target.address) await kernel.setRecoveryVaultAppId(vaultId) - const r = await target.sendTransaction({ value: 1, gas: 31000 }) + const r = await target.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) return assertRevert(async () => { await target.transferToVault(ETH) @@ -65,7 +65,7 @@ contract('Proxy funds', accounts => { // Initial setup before(async () => { aclBase = await ACL.new() - appBase = await AppStub.new() + appBase = await AppStubDepositable.new() appConditionalRecoveryBase = await AppStubConditionalRecovery.new() // Setup constants @@ -77,9 +77,6 @@ contract('Proxy funds', accounts => { // Test both the Kernel itself and the KernelProxy to make sure their behaviours are the same for (const kernelType of ['Kernel', 'KernelProxy']) { - const onlyBaseKernel = onlyIf(() => kernelType === 'Kernel') - const onlyKernelProxy = onlyIf(() => kernelType === 'KernelProxy') - context(`> ${kernelType}`, () => { let kernelBase, kernel @@ -129,47 +126,26 @@ contract('Proxy funds', accounts => { const vaultProxyAddress = getEvent(receipt, 'NewAppProxy', 'proxy') vault = VaultMock.at(vaultProxyAddress) } + await vault.initialize() + await kernel.setApp(APP_ADDR_NAMESPACE, vaultId, vault.address) await kernel.setRecoveryVaultAppId(vaultId) }) - onlyBaseKernel(() => { - context('> Base kernel', () => { - it('cannot receive ETH', skipCoverageIfVaultProxy(async () => - await assertRevert( - () => kernel.sendTransaction({ value: 1, gas: 31000 }) - ) - )) - - it('recovers tokens', async () => { - await recoverTokens(kernel, vault) - }) - }) - }) - - onlyKernelProxy(() => { - context('> KernelProxy', () => { - beforeEach(() => { - target = kernel - }) - - it('recovers ETH', skipCoverageIfVaultProxy(async () => - await recoverEth(target, vault) - )) - - it('recovers tokens', async () => { - await recoverTokens(target, vault) - }) + it('kernel cannot receive ETH', async () => + await assertRevert( + () => kernel.sendTransaction({ value: 1, gas: 31000 }) + ) + ) - it('fails if vault is not contract', async () => { - await failWithoutVault(target, kernel) - }) - }) + it('kernel recovers tokens', async () => { + await recoverTokens(kernel, vault) }) context('> App without kernel', () => { beforeEach(async () => { - target = await AppStub.new() + target = await AppStubDepositable.new() + await target.enableDeposits() }) it('does not recover ETH', skipCoverageIfVaultProxy(async () => @@ -190,18 +166,21 @@ contract('Proxy funds', accounts => { // Setup app const receipt = await kernel.newAppInstance(APP_ID, appBase.address) const appProxy = getEvent(receipt, 'NewAppProxy', 'proxy') - target = AppStub.at(appProxy) + const app = AppStubDepositable.at(appProxy) + await app.enableDeposits() + + target = app }) - it('cannot sent 0 ETH to proxy', async () => { + it('cannot send 0 ETH to proxy', async () => { await assertRevert(async () => { - await target.sendTransaction({ value: 0, gas: 31000 }) + await target.sendTransaction({ value: 0, gas: SEND_ETH_GAS }) }) }) - it('cannot sent ETH with data to proxy', async () => { + it('cannot send ETH with data to proxy', async () => { await assertRevert(async () => { - await target.sendTransaction({ value: 1, data: '0x1', gas: 31000 }) + await target.sendTransaction({ value: 1, data: '0x1', gas: SEND_ETH_GAS }) }) }) @@ -223,7 +202,10 @@ contract('Proxy funds', accounts => { // Setup app with conditional recovery code const receipt = await kernel.newAppInstance(APP_ID, appConditionalRecoveryBase.address) const appProxy = getEvent(receipt, 'NewAppProxy', 'proxy') - target = AppStub.at(appProxy) + const app = AppStubConditionalRecovery.at(appProxy) + await app.initialize() + + target = app }) it('does not allow recovering ETH', skipCoverageIfVaultProxy( diff --git a/test/unstructured_storage.js b/test/unstructured_storage.js index 823158469..01a4362ac 100644 --- a/test/unstructured_storage.js +++ b/test/unstructured_storage.js @@ -1,85 +1,133 @@ const AppStub = artifacts.require('AppStub') -const AppStubStorage = artifacts.require('AppStubStorage') -const AppStubPinnedStorage = artifacts.require('AppStubPinnedStorage') const Kernel = artifacts.require('Kernel') // Mocks +const AppStorageMock = artifacts.require('AppStorageMock') +const AppProxyPinnedStorageMock = artifacts.require('AppProxyPinnedStorageMock') +const DepositableStorageMock = artifacts.require('DepositableStorageMock') +const InitializableStorageMock = artifacts.require('InitializableStorageMock') const KernelPinnedStorageMock = artifacts.require('KernelPinnedStorageMock') contract('Unstructured storage', accounts => { - let app, kernel + context('> AppStorage', () => { + let appStorage - beforeEach(async () => { - app = await AppStubStorage.new() - kernel = await Kernel.new(true) + beforeEach(async () => { + appStorage = await AppStorageMock.new() + }) - // Set up AppStubPinnedStorage - const fakeApp = await AppStub.new() - const kernelMock = await KernelPinnedStorageMock.new(fakeApp.address) - appPinned = await AppStubPinnedStorage.new(kernelMock.address) - }) + it('tests Kernel storage', async () => { + const kernel = await Kernel.new(true) + await appStorage.setKernelExt(kernel.address) + //checks + assert.equal( + await web3.eth.getStorageAt(appStorage.address, (await appStorage.getKernelPosition())), + (await appStorage.kernel()).toString(), + 'Kernel should match' + ) + assert.equal( + await web3.eth.getStorageAt(appStorage.address, (await appStorage.getKernelPosition())), + kernel.address, + 'Kernel original value should match' + ) + }) - it('tests init block', async () => { - // set values - await app.initialize() - const blockNumber = web3.eth.blockNumber - //checks - assert.equal( - parseInt(await web3.eth.getStorageAt(app.address, (await app.getInitializationBlockPosition())), 16), - blockNumber, - 'Init block should match' - ) - assert.equal( - parseInt(await web3.eth.getStorageAt(app.address, (await app.getInitializationBlockPosition())), 16), - (await app.getInitializationBlock()).toString(), - 'Init block should match' - ) + it('tests appID storage', async () => { + const appId = '0x1234000000000000000000000000000000000000000000000000000000000000' + await appStorage.setAppIdExt(appId) + //checks + assert.equal( + await web3.eth.getStorageAt(appStorage.address, (await appStorage.getAppIdPosition())), + (await appStorage.appId()).toString(), + 'appId should match' + ) + assert.equal( + await web3.eth.getStorageAt(appStorage.address, (await appStorage.getAppIdPosition())), + appId, + 'appId original value should match' + ) + }) }) - it('tests Kernel storage', async () => { - await app.setKernelExt(kernel.address) - //checks - assert.equal( - await web3.eth.getStorageAt(app.address, (await app.getKernelPosition())), - (await app.kernel()).toString(), - 'Kernel should match' - ) - assert.equal( - await web3.eth.getStorageAt(app.address, (await app.getKernelPosition())), - kernel.address, - 'Kernel original value should match' - ) + context('> AppProxyPinned', () => { + let appPinned + beforeEach(async () => { + // Set up AppStubPinnedStorage + const fakeApp = await AppStub.new() + const kernelMock = await KernelPinnedStorageMock.new(fakeApp.address) + appPinned = await AppProxyPinnedStorageMock.new(kernelMock.address) + }) + + it('tests pinnedCode storage', async () => { + const pinnedCode = '0x1200000000000000000000000000000000005678' + await appPinned.setPinnedCodeExt(pinnedCode) + //checks + assert.equal( + await web3.eth.getStorageAt(appPinned.address, (await appPinned.getPinnedCodePosition())), + (await appPinned.pinnedCodeExt()).toString(), + 'Pinned Code should match' + ) + assert.equal( + await web3.eth.getStorageAt(appPinned.address, (await appPinned.getPinnedCodePosition())), + pinnedCode, + 'Pinned Code original value should match' + ) + }) }) - it('tests appID storage', async () => { - const appId = '0x1234000000000000000000000000000000000000000000000000000000000000' - await app.setAppIdExt(appId) - //checks - assert.equal( - await web3.eth.getStorageAt(app.address, (await app.getAppIdPosition())), - (await app.appId()).toString(), - 'appId should match' - ) - assert.equal( - await web3.eth.getStorageAt(app.address, (await app.getAppIdPosition())), - appId, - 'appId original value should match' - ) + context('> DepositableStorage', () => { + let depositableMock + + beforeEach(async () => { + depositableMock = await DepositableStorageMock.new() + }) + + it('tests depositable', async () => { + // set values + await depositableMock.setDepositableExt(true) + //checks + assert.equal( + await web3.eth.getStorageAt(depositableMock.address, (await depositableMock.getDepositablePosition())), + true, + 'Depositable should match' + ) + }) }) - it('tests pinnedCode storage', async () => { - const pinnedCode = '0x1200000000000000000000000000000000005678' - await appPinned.setPinnedCodeExt(pinnedCode) - //checks - assert.equal( - await web3.eth.getStorageAt(appPinned.address, (await appPinned.getPinnedCodePosition())), - (await appPinned.pinnedCodeExt()).toString(), - 'Pinned Code should match' - ) - assert.equal( - await web3.eth.getStorageAt(appPinned.address, (await appPinned.getPinnedCodePosition())), - pinnedCode, - 'Pinned Code original value should match' - ) + context('> Initializable', () => { + let initializableMock + + beforeEach(async () => { + initializableMock = await InitializableStorageMock.new() + }) + + it('tests init block', async () => { + // set values + await initializableMock.initialize() + const blockNumber = web3.eth.blockNumber + //checks + assert.equal( + parseInt( + await web3.eth.getStorageAt( + initializableMock.address, + (await initializableMock.getInitializationBlockPosition()) + ), + 16 + ), + blockNumber, + 'Init block should match' + ) + assert.equal( + parseInt( + await web3.eth.getStorageAt( + initializableMock.address, + (await initializableMock.getInitializationBlockPosition()) + ), + 16 + ), + (await initializableMock.getInitializationBlock()).toString(), + 'Init block should match' + ) + }) }) })