Skip to content

ACL: optimize checkOracle() #500

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
Open
sohkai wants to merge 3 commits into
base: next
Choose a base branch
from
Open

ACL: optimize checkOracle() #500

sohkai 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 facuspagnuolo and izqui March 26, 2019 00:49
izqui
izqui approved these changes Mar 27, 2019
View reviewed changes
contracts/acl/ACL.sol Outdated
0x20 // uint256 return
)

if gt(success, 0) {
Copy link
Contributor

@izqui izqui Mar 27, 2019

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

@sohkai sohkai Mar 27, 2019

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

coveralls commented Mar 29, 2019

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

@facuspagnuolo facuspagnuolo Mar 29, 2019

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

2 more reviewers

@izqui izqui izqui approved these changes

@facuspagnuolo facuspagnuolo facuspagnuolo approved these changes

Reviewers whose approvals may not affect merge requirements

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.

5 participants

@sohkai @coveralls @izqui @facuspagnuolo