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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add ConversionHelpers lib for converting memory types #501

Merged
merged 6 commits into from Apr 5, 2019

Conversation

Projects
None yet
4 participants
@sohkai
Copy link
Member

commented Mar 26, 2019

Consolidates all the bytes<>uint256[] conversions into a library.

It's not too costly to add, and hopefully makes us all feel a bit better about this bit 馃槃.

Also fixes the solidity test runner, which must've broke at some point along the way (due to the assert logs not being decoded properly) 馃槄.

Bytecode diff:

                              CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
ACL.json                      5400 more gas        +27                0
APMRegistry.json              4400 more gas        +22                0
AragonApp.json                4000 more gas        +20                0
ENSSubdomainRegistrar.json    4000 more gas        +20                0
EVMScriptRegistry.json        4000 more gas        +20                0
EVMScriptRegistryFactory.json Same                 0                  +20
Kernel.json                   8000 less gas        -40                0
Repo.json                     4000 more gas        +20                0
TestACLInterpreter.json       5400 more gas        +27                0

@sohkai sohkai requested review from izqui and facuspagnuolo Mar 26, 2019

Show resolved Hide resolved contracts/acl/ACL.sol Outdated
mstore(output, intsLength)
}
}
}

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Mar 29, 2019

Contributor

WDYT about having separate helper libraries for uint arrays and bytes? I'm just thinking in case we need to provide more helper functions, having them split by type may make sense.

@sohkai sohkai marked this pull request as ready for review Apr 3, 2019

@sohkai sohkai requested a review from facuspagnuolo Apr 3, 2019

@sohkai sohkai force-pushed the conversion-helpers branch from 6069abe to 821dfa1 Apr 3, 2019

sohkai added some commits Apr 3, 2019

@sohkai sohkai force-pushed the conversion-helpers branch from 821dfa1 to 5c70c46 Apr 3, 2019

@coveralls

This comment has been minimized.

Copy link

commented Apr 3, 2019

Coverage Status

Coverage decreased (-0.004%) to 99.536% when pulling 5c70c46 on conversion-helpers into dca0b4b on dev.

@izqui

izqui approved these changes Apr 4, 2019

Copy link
Member

left a comment

Loving the new function names 馃槀

@facuspagnuolo
Copy link
Contributor

left a comment

Nice job @sohkai 馃憦!

arrBytesMemLoc := arrBytes
}
Assert.equal(arrMemLoc, arrBytesMemLoc, "should have same memory location after conversion");
}

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 4, 2019

Contributor

We could also check values here

Suggested change
}
// Check values
uint256 first;
uint256 second;
uint256 third;
assembly {
first := mload(add(arrBytesMemLoc, 0x20))
second := mload(add(arrBytesMemLoc, 0x40))
third := mload(add(arrBytesMemLoc, 0x60))
}
Assert.equal(first, FIRST, "should have correct first value");
Assert.equal(second, SECOND, "should have correct second value");
Assert.equal(third, THIRD, "should have correct third value");
}

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Author Member

Hehe yes... It's a bit of a superfluous test since we're just accessing raw memory that we know we set, but still good to make sure the cast doesn't do something funny to the memory 馃憤.

Did this and a bit of clean up in a6642aa.

sohkai added some commits Apr 5, 2019

@sohkai sohkai merged commit beac82a into dev Apr 5, 2019

1 of 4 checks passed

License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the conversion-helpers branch Apr 5, 2019

sohkai added a commit that referenced this pull request Apr 5, 2019

feat: add ConversionHelpers lib for converting memory types (#501)
Consolidates all the bytes<>uint256[] conversions into a library.

It's not _too_ costly to add, and hopefully makes us all feel a bit better about this bit 馃槃.

Also fixes the solidity test runner, which must've broke at some point along the way (due to the assert logs not being decoded properly) 馃槄.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.