Address review comments

aragon · Sep 2, 2019 · 948acea · 948acea
1 parent 1975607
commit 948acea
Show file tree

Hide file tree

Showing 5 changed files with 15 additions and 8 deletions.
diff --git a/contracts/common/DepositableDelegateProxy.sol b/contracts/common/DepositableDelegateProxy.sol
@@ -20,6 +20,9 @@ contract DepositableDelegateProxy is DepositableStorage, DelegateProxy {
                 // Only accept the deposit and emit an event if all of the following are true:
                 // the proxy accepts deposits (isDepositable), msg.data.length == 0, and msg.value > 0
                 if and(and(sload(isDepositablePosition), iszero(calldatasize)), gt(callvalue, 0)) {
+                    // Equivalent Solidity code for emitting the event:
+                    // emit ProxyDeposit(msg.sender, msg.value);
+
                     let logData := mload(0x40) // free memory pointer
                     mstore(logData, caller) // add 'msg.sender' to the log data (first event param)
                     mstore(add(logData, 0x20), callvalue) // add 'msg.value' to the log data (second event param)

diff --git a/test/contracts/apps/app_funds.js b/test/contracts/apps/app_funds.js
@@ -17,7 +17,8 @@ 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
+const TX_BASE_GAS = 21000
+const SEND_ETH_GAS = TX_BASE_GAS + 9999 // <10k gas is the threshold for depositing
 
 contract('App funds', ([permissionsRoot]) => {
   let aclBase, kernelBase

diff --git a/test/contracts/apps/recovery_to_vault.js b/test/contracts/apps/recovery_to_vault.js
@@ -20,7 +20,8 @@ const KernelDepositableMock = artifacts.require('KernelDepositableMock')
 
 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
+const TX_BASE_GAS = 21000
+const SEND_ETH_GAS = TX_BASE_GAS + 9999 // <10k gas is the threshold for depositing
 
 contract('Recovery to vault', ([permissionsRoot]) => {
   let aclBase, appBase, appConditionalRecoveryBase

diff --git a/test/contracts/common/depositable_delegate_proxy.js b/test/contracts/common/depositable_delegate_proxy.js
@@ -11,7 +11,8 @@ const ProxyTargetWithoutFallback = artifacts.require('ProxyTargetWithoutFallback
 const ProxyTargetWithFallback = artifacts.require('ProxyTargetWithFallback')
 
 const TX_BASE_GAS = 21000
-const SEND_ETH_GAS = TX_BASE_GAS + 9999 // 10k gas is the threshold for depositing
+const SEND_ETH_GAS = TX_BASE_GAS + 9999 // <10k gas is the threshold for depositing
+const PROXY_FORWARD_GAS = TX_BASE_GAS + 2e6 // high gas amount to ensure that the proxy forwards the call
 const FALLBACK_SETUP_GAS = process.env.SOLIDITY_COVERAGE ? 5000 : 100 // rough estimation of how much gas it spends before executing the fallback code
 const SOLIDITY_TRANSFER_GAS = 2300
 const ISTANBUL_SLOAD_GAS_INCREASE = 600
@@ -35,7 +36,7 @@ contract('DepositableDelegateProxy', ([ sender ]) => {
     context('when implementation address is set', () => {
       const itSuccessfullyForwardsCall = () => {
         it('forwards call with data', async () => {
-          const receipt = await target.ping()
+          const receipt = await target.ping({ gas: PROXY_FORWARD_GAS })
           assertAmountOfEvents(receipt, 'Pong')
         })
       }
@@ -61,18 +62,18 @@ contract('DepositableDelegateProxy', ([ sender ]) => {
         itSuccessfullyForwardsCall()
 
         it('reverts when sending ETH', async () => {
-          await assertRevert(target.sendTransaction({ value: 1 }))
+          await assertRevert(target.sendTransaction({ value: 1, gas: PROXY_FORWARD_GAS }))
         })
       })
     })
 
     context('when implementation address is not set', () => {
       it('reverts when a function is called', async () => {
-        await assertRevert(target.ping())
+        await assertRevert(target.ping({ gas: PROXY_FORWARD_GAS }))
       })
 
       it('reverts when sending ETH', async () => {
-        await assertRevert(target.sendTransaction({ value: 1 }))
+        await assertRevert(target.sendTransaction({ value: 1, gas: PROXY_FORWARD_GAS }))
       })
     })
   }

diff --git a/test/contracts/kernel/kernel_funds.js b/test/contracts/kernel/kernel_funds.js
@@ -9,7 +9,8 @@ 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
+const TX_BASE_GAS = 21000
+const SEND_ETH_GAS = TX_BASE_GAS + 9999 // <10k gas is the threshold for depositing
 
 contract('Kernel funds', ([permissionsRoot]) => {
   let aclBase