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

Revert reasons #441

Merged
merged 2 commits into from Oct 16, 2018

Conversation

Projects
None yet
4 participants
@izqui
Copy link
Member

izqui commented Oct 16, 2018

Closes #438

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

@izqui izqui changed the base branch from dev to proxy-revert-reasons Oct 16, 2018

@bingen

bingen approved these changes Oct 16, 2018

Copy link
Contributor

bingen left a comment

There's one require missing in APMRegistryFactory.sol. Is that intended?

@@ -9,6 +9,10 @@ pragma solidity ^0.4.24;
* @dev Math operations with safety checks that revert on error
*/
library SafeMath {
string private constant ADD_OVERFLOW_ERROR = "MATH_ADD_OVERFLOW";

This comment has been minimized.

@bingen

bingen Oct 16, 2018

Contributor

Should we send this to Zeppelin?

This comment has been minimized.

@izqui

izqui Oct 16, 2018

Member

Yeah, eventually :)

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Oct 16, 2018

There's one require missing in APMRegistryFactory.sol. Is that intended?

Yep, this is code that is only used in test environments, where the factory can directly create the subnode for the registry, which is not the case on live environments since the factory won't be the owner of .eth :D

@sohkai

sohkai approved these changes Oct 16, 2018

Copy link
Member

sohkai left a comment

🥇

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

@izqui izqui force-pushed the revert-reasons branch from 616234f to 1fc7e4d Oct 16, 2018

@bingen bingen changed the base branch from proxy-revert-reasons to dev Oct 16, 2018

@bingen bingen changed the base branch from dev to proxy-revert-reasons Oct 16, 2018

Add revert reasons: _ERROR -> ERROR_
Address PR #441 comments.

@bingen bingen changed the base branch from proxy-revert-reasons to dev Oct 16, 2018

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Oct 16, 2018

Final bytecode diff from adding revert reasons:

                              CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
ACL.json                      425200 more gas      +2126              +359
APMRegistry.json              279800 more gas      +1399              +359
AragonApp.json                70800 more gas       +354               +379
Autopetrified.json            Same                 0                  +354
CallsScript.json              131600 more gas      +658               +354
ENSSubdomainRegistrar.json    336200 more gas      +1681              +359
EVMScriptRegistry.json        306800 more gas      +1534              +359
EVMScriptRegistryFactory.json Same                 0                  +2905
Kernel.json                   369600 more gas      +1848              +359
Repo.json                     247400 more gas      +1237              +359
SafeMath64Mock.json           161000 more gas      +805               0
SafeMath8Mock.json            161000 more gas      +805               0
TestACLInterpreter.json       425200 more gas      +2126              +359
TokenMock.json                71200 more gas       +356               0

@izqui izqui merged commit a684791 into dev Oct 16, 2018

3 of 4 checks passed

License Compliance FOSSA is analyzing this commit
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

@izqui izqui deleted the revert-reasons branch Oct 16, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 17, 2018

Coverage Status

Coverage increased (+0.01%) to 98.765% when pulling b2b7e87 on revert-reasons into d91b12c on dev.

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