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.

Sign up for GitHub

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

Jump to bottom

ACL: optimize checkOracle() #500

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

ACL: optimize checkOracle() #500

wants to merge 3 commits into from

Conversation

sohkai
Copy link
Contributor

@sohkai sohkai commented Mar 26, 2019 •
edited
Loading

Updates the ACL's staticcall to use a more-optimized one similar to SafeERC20's invokeAndCheckSuccess().

Bytecode comparison:

                        CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
ACL.json                5600 less gas        -28                0
TestACLInterpreter.json 67400 more gas       +337               0

@sohkai sohkai requested review from izqui and facuspagnuolo March 26, 2019 00:49
izqui
izqui approved these changes Mar 27, 2019
View reviewed changes
contracts/acl/ACL.sol Outdated Show resolved Hide resolved
contracts/acl/ACL.sol Outdated
0x20 // uint256 return
)

if gt(success, 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like coverage is crashing when instrumenting this line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh shoot, I remember having this issue with SafeERC20 but ended up putting that in the skip list.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.009%) to 99.531% when pulling 2047972 on acl-optimize-check-oracle into dca0b4b on dev.

facuspagnuolo
facuspagnuolo approved these changes Mar 29, 2019
View reviewed changes
contracts/acl/ACL.sol
0x20 // uint256 return
)

// solidity-coverage fails on assembly `if`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️

@sohkai sohkai force-pushed the acl-optimize-check-oracle branch from 2047972 to ea30416 Compare April 9, 2019 17:33
@sohkai sohkai changed the base branch from dev to next July 11, 2019 09:28
@sohkai sohkai added this to the aragonOS 5.0 milestone Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facuspagnuolo facuspagnuolo facuspagnuolo approved these changes

@izqui izqui izqui approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
component: acl
Projects
None yet
Milestone
aragonOS 5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sohkai @coveralls @izqui @facuspagnuolo