Add ability to enable / disable depositable functionality #401

sohkai · 2018-08-27T13:27:08Z

Before, only the App and Kernel proxies were able to receive ETH through generic send() or transfer()s. There was no way to control whether this was desired functionality, or a way to turn it off after a certain condition (e.g. ending a token sale).

This PR adds depositable functionality to the App and Kernel base contracts, as well as a way for Apps to turn off the ability to receive ETH.

816e166 and 01821c6: cleans up a bit of test code
fec0a9a adds DepositableStorage
3d48720 uses DepositableStorage to control depositing functionality in the App and Kernel
027c522 and 1baa570 fix some "surprising" test changes due to the changes.
- Notably, the recent changes have forced APMRegistryFactory.newAPM() to go over 6.9m gas again
- Kernels now have a fallback, so unregistered functions won't automatically revert

sohkai · 2018-08-27T13:28:26Z

contracts/kernel/Kernel.sol

@@ -28,6 +29,10 @@ contract Kernel is IKernel, KernelStorage, Petrifiable, IsContract, VaultRecover
        }
    }

+    function () external payable {
+        require(isDepositable());


To work around the changes in https://github.com/aragon/aragonOS/pull/401/files#diff-61de883632b625f81d4e28af414a9c90R64, we could also require that the value must be greater than 0 here and in AragonApp.

coveralls · 2018-08-27T13:49:41Z

Coverage decreased (-0.003%) to 99.063% when pulling 5b76372 on depositable-check into 5b880fc on dev.

izqui · 2018-08-27T16:29:43Z

contracts/kernel/Kernel.sol

@@ -44,6 +49,7 @@ contract Kernel is IKernel, KernelStorage, Petrifiable, IsContract, VaultRecover

        acl.initialize(_permissionsCreator);

+        setDepositable(true);


I think depositing to the kernel should be disabled by default, given that there is no reason to send funds to the kernel for now. I think it is good that the KernelProxy keeps the functionality (in case the Kernel having ETH makes sense at some point), but it should be disabled IMO.

My main reason for enabling it was because Kernels are assigned ENS names through our initial templates, and so I could see users sending ETH directly to that name.

However, thinking about it, it's less confusing if the kernel reverts on receiving ETH directly than requiring a user to manually use the escape hatch functionality to transfer the funds to the vault. In the future, a different ENS name could be used for a finance or funding app, e.g. finance.<dao name>.aragonid.eth.

izqui · 2018-08-27T16:45:08Z

contracts/apps/AragonApp.sol

@@ -27,6 +28,10 @@ contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, EVMScriptRunn
        _;
    }

+    function () external payable {
+        require(isDepositable());


What's the reason behind adding a fallback by default to all apps? IMO apps should define their own fallback function if they want to receive ETH. It may seem weird that depending on the gas value, a deposit can happen or not, but I'd say deposits and recovers should be used as the last resort and when a call is already forwarded to app, its own logic should be responsible for deciding on what to do.

In case we decide to keep this, we should definitely check for data being empty and value being greater than 0.

Good point. Now that DepositableDelegateProxys also need their depositing functionality explicitly enabled, I agree that apps should define their own fallback if they want to receive ETH.

This reverts commit 027c522.

sohkai · 2018-09-03T15:45:56Z

Updated to disable depositable functionality by default.

This reverts commit 1baa570.

bingen

Looks good to me, letting aside some minor comments I left (plus the well-known fact that I'm not a big fan of allowing our apps to work without a proxy ;-P)

bingen · 2018-09-04T10:32:25Z

.solcover.js

+  'test',
+  'acl/ACLSyntaxSugar.sol',
+  'common/DepositableStorage.sol', // Used in tests that send ETH
+  'common/UnstructuredStorage.sol' // Used in tests that send ETH


Why are we skipping those files? The last 2 ones at least do have tests.

Checking to see if we can remove those from the ignored list... Originally the problem was that instrumentation in those files meant using unstructured storage in a fallback was more expensive than 2.3k gas (the instrumentation emits two events before and after each line of code in those files).

Tried removing either one... and unfortunately coverage fails :(. I think they're not too important to get coverage stats on anyway, since they're just one-liners.

bingen · 2018-09-04T11:06:40Z

test/proxy_recover_funds.js

            await kernel.setApp(APP_ADDR_NAMESPACE, vaultId, vault.address)
            await kernel.setRecoveryVaultAppId(vaultId)
          })

-          onlyBaseKernel(() => {


onlyBaseKernel and onlyKernelProxy are not used anymore, right? Then we can remove above (lines 81, 82)

bingen · 2018-09-04T11:11:10Z

contracts/test/mocks/KernelDepositableMock.sol

@@ -0,0 +1,17 @@
+pragma solidity 0.4.24;


I'm not sure that we really need this test, as we are testing something that our codebase doesn't offer, right? Besides I don't think I like the idea of a depositable kernel, but maybe it's because I'm missing some possible use cases.

I'd like to keep it, as it's functionality that could be enabled, even if it's not directly offered.

bingen · 2018-09-04T11:20:20Z

test/proxy_recover_funds.js

@@ -129,47 +130,26 @@ contract('Proxy funds', accounts => {
              const vaultProxyAddress = getEvent(receipt, 'NewAppProxy', 'proxy')
              vault = VaultMock.at(vaultProxyAddress)
            }
+            await vault.initialize()


(I'm putting this comment here because the proper line has not been changed, which probably means that this comment actually belongs to a previous PR, haha)
If I'm not wrong, with these 2 nested for's for Kernel and Vault, we are testing 4 possibilities. I don't see the point of testing proxied Kernel with a proxy-less Vault, or proxy-less Kernel with a proxied Vault. It's giving me hard time to think about these tests, haha ;-)

Yes, it's a matrix of all the combinations.

I'd like to keep it, again to ensure that behaviour across all the combinations is the same. Even if it's rare, it simplifies the mental model–regardless of using a base or proxy, they should act the same. This PR moves even closer to that ideal, now that we can remove the onlyIf targeted tests.

sohkai added 6 commits August 27, 2018 14:01

test: separate mocks for unstructured storage tests

816e166

Cosmetic cleanup

01821c6

Add DepositableStorage

fec0a9a

Use DepositableStorage to control if contracts are depositable

3d48720

Fix kernel upgrade tests

027c522

Increase test gas limit for APMRegistryFactory

1baa570

sohkai added the epic: audit label Aug 27, 2018

sohkai requested review from izqui and bingen August 27, 2018 13:27

sohkai commented Aug 27, 2018

View reviewed changes

izqui reviewed Aug 27, 2018

View reviewed changes

sohkai added this to the sprint: 1.1 milestone Aug 29, 2018

sohkai self-assigned this Aug 30, 2018

sohkai added 5 commits September 3, 2018 15:25

Merge branch 'dev' into depositable-check

2e9ff00

Fix path for ignored coverage file

ee098c9

test: make AppSt's name more explicit

4cec497

Revert "Fix kernel upgrade tests"

fc27c6b

This reverts commit 027c522.

Remove fallbacks from base AragonApps and Kernels

f91d4cd

Revert "Increase test gas limit for APMRegistryFactory"

5c0bfb6

This reverts commit 1baa570.

bingen approved these changes Sep 4, 2018

View reviewed changes

izqui approved these changes Sep 4, 2018

View reviewed changes

test: remove unnecessary onlyIf import

5b76372

sohkai merged commit a56234e into dev Sep 4, 2018

sohkai deleted the depositable-check branch September 4, 2018 17:50

sohkai mentioned this pull request Sep 4, 2018

Merge Old vault and upgrade to aragonOS 4 aragon/aragon-apps#425

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to enable / disable depositable functionality #401

Add ability to enable / disable depositable functionality #401

sohkai commented Aug 27, 2018

sohkai Aug 27, 2018

coveralls commented Aug 27, 2018 •

edited

Loading

izqui Aug 27, 2018

sohkai Sep 3, 2018

izqui Aug 27, 2018

sohkai Sep 3, 2018

sohkai commented Sep 3, 2018

bingen left a comment

bingen Sep 4, 2018

sohkai Sep 4, 2018

sohkai Sep 4, 2018 •

edited

Loading

bingen Sep 4, 2018

bingen Sep 4, 2018

sohkai Sep 4, 2018

bingen Sep 4, 2018

sohkai Sep 4, 2018

		@@ -44,6 +49,7 @@ contract Kernel is IKernel, KernelStorage, Petrifiable, IsContract, VaultRecover

		acl.initialize(_permissionsCreator);

		setDepositable(true);

Add ability to enable / disable depositable functionality #401

Add ability to enable / disable depositable functionality #401

Conversation

sohkai commented Aug 27, 2018

Choose a reason for hiding this comment

coveralls commented Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai commented Sep 3, 2018

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 27, 2018 •

edited

Loading

sohkai Sep 4, 2018 •

edited

Loading