Skip to content

ACL: optimize checkOracle()#500

Open
sohkai wants to merge 3 commits into
nextfrom
acl-optimize-check-oracle
Open

ACL: optimize checkOracle()#500
sohkai wants to merge 3 commits into
nextfrom
acl-optimize-check-oracle

Conversation

@sohkai
Copy link
Copy Markdown
Contributor

@sohkai sohkai commented Mar 26, 2019

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

Comment thread contracts/acl/ACL.sol Outdated
Comment thread contracts/acl/ACL.sol Outdated
0x20 // uint256 return
)

if gt(success, 0) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

Coverage Status

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

Comment thread contracts/acl/ACL.sol
0x20 // uint256 return
)

// solidity-coverage fails on assembly `if`
Copy link
Copy Markdown
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants