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

Add revert reasons to proxies #440

Closed
wants to merge 1 commit into
base: dev
from

Conversation

Projects
None yet
3 participants
@izqui
Copy link
Member

izqui commented Oct 16, 2018

Partially solves #438

The naming of errors optimizes for proxy size, given that having strings in contract bytecode is quite expensive.

@izqui izqui requested review from bingen and sohkai Oct 16, 2018

@izqui izqui referenced this pull request Oct 16, 2018

Merged

Revert reasons #441

@bingen

bingen approved these changes Oct 16, 2018

@sohkai

sohkai approved these changes Oct 16, 2018

Copy link
Member

sohkai left a comment

Looks good!

Only thought is maybe to use ERROR as the variable prefix rather than suffix.

@sohkai sohkai added this to the Sprint: 2.2 milestone Oct 16, 2018

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Oct 16, 2018

Solidity doesn't optimize revert reasons well, adding just one character of reason results in ~115 bytes added to the contract (23k gas). Because it pads string constants with 0s so they fit in a 32 byte word, having short strings as we do in this PR doesn't save any gas (contract bytecode gets charged at 200 gas per byte regardless of whether it is 0 or not).

After discussing it offline, I think we shouldn't add revert reasons to proxies and just have them everywhere else, so proxies remain cheap to deploy. Adding revert reasons has resulted in a 33% gas increase for deploying proxies (300k to 400k), which given how easy it would be to debug them (if everything running in the implementation code has reasons, if there is no error data returned then it must be an issue in the proxy which is easier to debug)

@izqui izqui closed this Oct 16, 2018

@izqui izqui referenced this pull request Oct 16, 2018

Closed

Revert with reason #438

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment