DepositableDelegateProxy: optimize for EIP-1884 #551
Conversation
| @@ -1,12 +1,15 @@ | |||
| const { hash } = require('eth-ens-namehash') | |||
There was a problem hiding this comment.
It'd probably make sense to move all of these new tests to a new file (perhaps test/contracts/depositable_delegate_proxy.js), targeting just the DepositableProxy.
There was a problem hiding this comment.
Good idea. Reverted the changes in recovery_to_vault.js and created some specific tests for DepositableDelegateProxy.
Some of the things that we are testing here are already being tested in recovery_to_vault.js, app_funds.js and kernel_funds.js, so we could remove some tests from there.
There was a problem hiding this comment.
I wouldn't mind keeping those tests there; I view most of those tests as integration tests more than unit tests (pretty much only the bits in common are pure unit tests).
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>
izqui
force-pushed
the
depositable-optimize
branch
from
5ba0e20 to
67a7ef4
Compare
| const logs = decodeEventsOfType(receipt, DepositableDelegateProxyMock.abi, 'ProxyDeposit') | ||
| assertAmountOfEvents({ logs }, 'ProxyDeposit') | ||
| assertEvent({ logs }, 'ProxyDeposit', { sender: toChecksumAddress(ethSender.address), value }) | ||
| }) |
There was a problem hiding this comment.
Nit: perhaps it looks nice to wrap these into Constantinople and Istambul contexts
There was a problem hiding this comment.
It is not possible to perform a call with value from a contract with less than 2300 gas, as calls that transfer value get a 2300 gas stipend (at the protocol level).
I was doing some debugging and saw that receiver.transfer() sets the gas for the call to 0 if the value sent is greater than 0, and if it is 0 the compiler sets the gas for the call to 2300 gas (which is hardcoded in the bytecode). This results in the contract that receives a .transfer always having at least 2300 gas regardless of whether it is coming from the protocol stipend or from the call being sent with 2300 gas.
Even if you do a 'low-level call' (receiver.call.value(v).gas(g)()) if v is greater than 0, the actual gas that the receiver will be called with is 2300 + g. This makes it impossible to test the Istanbul scenario in which we'd need receiver to be called with 1700 gas.
| proxy = await DepositableDelegateProxyMock.new() | ||
| }) | ||
|
|
||
| const itForwardsToImplementationIfGasIsOverThreshold = () => { |
There was a problem hiding this comment.
Actually, this one is not guaranteeing anything related to the gas amount, I'd call it itForwardsToImplementation simply.
There was a problem hiding this comment.
It's implicitly guaranteeing it as by not setting the gas explicitly, it uses the gas limit value in truffle.js, but I think it is a good idea to make it explicit for these tests
There was a problem hiding this comment.
LGTM, should we set explicit limits as said in https://github.com/aragon/aragonOS/pull/551/files#r318560527?
There was a problem hiding this comment.
Looks good to me.
I've been poking around with the byte code, too bad I was not able to get Solidity && conditions translated into AND opcodes 🤷♂️
Anyway, I was wondering how useful is being able to set the depositable flag on run time. Could you guys remind me what the purpose was? Wouldn't it suffice to have it at deploy time, and then have DepositableDelegateProxy and NonDepositableDelegateProxy, or something like that? Kind of with Pinned and Upgradeable, although I guess having all the combinations could be messy.
My concern is that we are still too close to the limit, and without that extra SLOAD it would be way safer, as it seems there have been opinions for having it even more expensive.
A fundraising app that dynamically wants to start and stop receiving direct deposits. But yes, perhaps as we make more breaking changes in the future, we think about ways to remove this for most apps and only provide this dynamic behaviour to apps that really need it. |
|
Thanks for the reviews! Addressed all the comments and it is ready to be merged as soon as the CI passes. |
Is this used by Aragon's fundraising app? I couldn't find it. |
@bingen This would increase the size of the Kernel considerably if we want to support multiple combinations. We can think about it for the next major version, but I would keep changes to the minimum for this release. |
* DepositableDelegateProxy: optimize for EIP-1884 * chore: pin solidity-coverage to v0.6.2 (#552) Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>
|
Although precisely because how hard are migrations for Proxies the sooner the better, I completely agree that this is not a good time for big changes, so let's discuss later. |
|
For future reference, we have some numbers here: |
Optimizes the fallback function in the proxy so it can receive ETH after EIP-1884 is implemented in Istanbul.
With these changes, the gas cost for executing the fallback function has gone down from 1759 gas to 1619, which after the SLOAD opcode is repriced to 800 gas will still execute under 2300 gas (2219 gas).
Closes #549 (even though this issue doesn't tackle the potential migration for all existing DAOs whose Vaults would break)