Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DepositableDelegateProxy: optimize for EIP-1884 #551

Merged
merged 9 commits into from Sep 3, 2019

Conversation

@izqui
Copy link
Member

commented Aug 27, 2019

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)

@auto-assign auto-assign bot requested review from facuspagnuolo and sohkai Aug 27, 2019

@sohkai
Copy link
Member

left a comment

🙌 Assembly looks good, have a few suggestions.

contracts/common/DepositableDelegateProxy.sol Outdated Show resolved Hide resolved
contracts/common/DepositableDelegateProxy.sol Outdated Show resolved Hide resolved
@@ -1,12 +1,15 @@
const { hash } = require('eth-ens-namehash')

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 27, 2019

Member

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.

This comment has been minimized.

Copy link
@izqui

izqui Aug 28, 2019

Author Member

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.

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 28, 2019

Member

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).

test/contracts/apps/recovery_to_vault.js Outdated Show resolved Hide resolved
test/contracts/apps/recovery_to_vault.js Outdated Show resolved Hide resolved
izqui and others added 4 commits Aug 28, 2019
Update contracts/common/DepositableDelegateProxy.sol
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>

@izqui izqui force-pushed the depositable-optimize branch from 5ba0e20 to 67a7ef4 Aug 28, 2019

@facuspagnuolo
Copy link
Member

left a comment

Looks really good! Nothing to comment on the assembly. Left some minor comments for the tests that look really good too! 👏

test/contracts/common/depositable_delegate_proxy.js Outdated Show resolved Hide resolved
const logs = decodeEventsOfType(receipt, DepositableDelegateProxyMock.abi, 'ProxyDeposit')
assertAmountOfEvents({ logs }, 'ProxyDeposit')
assertEvent({ logs }, 'ProxyDeposit', { sender: toChecksumAddress(ethSender.address), value })
})

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 28, 2019

Member

Nit: perhaps it looks nice to wrap these into Constantinople and Istambul contexts

This comment has been minimized.

Copy link
@izqui

izqui Aug 28, 2019

Author Member

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 = () => {

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 28, 2019

Member

Actually, this one is not guaranteeing anything related to the gas amount, I'd call it itForwardsToImplementation simply.

This comment has been minimized.

Copy link
@izqui

izqui Aug 28, 2019

Author Member

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

test/contracts/common/depositable_delegate_proxy.js Outdated Show resolved Hide resolved
izqui added 2 commits Aug 28, 2019
@coveralls

This comment has been minimized.

Copy link

commented Aug 28, 2019

Coverage Status

Coverage decreased (-0.009%) to 99.116% when pulling d5bdcb0 on depositable-optimize into c350823 on next.

@izqui izqui referenced this pull request Sep 2, 2019
21 of 22 tasks complete
@sohkai
sohkai approved these changes Sep 2, 2019
Copy link
Member

left a comment

LGTM, should we set explicit limits as said in https://github.com/aragon/aragonOS/pull/551/files#r318560527?

@bingen
bingen approved these changes Sep 2, 2019
Copy link
Member

left a 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.

@sohkai

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

Anyway, I was wondering how useful is being able to set the depositable flag on run time

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.

@izqui

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

Thanks for the reviews! Addressed all the comments and it is ready to be merged as soon as the CI passes.

@bingen

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

A fundraising app that dynamically wants to start and stop receiving direct deposits.

Is this used by Aragon's fundraising app? I couldn't find it.
Anyway I'm still not sure it's worth the trouble. I guess for this kind of cases I would just make it non-depositable and then force receiving through a function where I kind implement this. I also guess in most cases it will be needed so for any other particular business logic.

@izqui

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

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.

@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.

@izqui izqui merged commit 0df3c81 into next Sep 3, 2019

5 of 7 checks passed

coverage/coveralls Coverage decreased (-0.009%) to 99.116%
Details
License Compliance FOSSA is analyzing this commit
Details
Gas Usage Gas usage increased for 53 items and decreased for 2 items
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@delete-merged-branch delete-merged-branch bot deleted the depositable-optimize branch Sep 3, 2019

facuspagnuolo added a commit that referenced this pull request Sep 3, 2019
DepositableDelegateProxy: optimize for EIP-1884 (#551)
* DepositableDelegateProxy: optimize for EIP-1884

* chore: pin solidity-coverage to v0.6.2 (#552)

Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>
@bingen

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

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.
But then I would remove Pinned proxies (which I never saw the point of). Actually, there's no single usage of it in our templates, right? Do we know of any real use of them?
About the Kernel size, do you mean the bytecode size?

@bingen

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

For future reference, we have some numbers here:
bingen@ac3116c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.