-
Notifications
You must be signed in to change notification settings - Fork 370
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
Add new and modified precompiles to UsingPrecompiles.sol #2318
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2318 +/- ##
=======================================
Coverage 74.67% 74.67%
=======================================
Files 274 274
Lines 7731 7731
Branches 984 984
=======================================
Hits 5773 5773
Misses 1846 1846
Partials 112 112
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, mostly just checked that the names match with my PR
* @return address data | ||
*/ | ||
function getAddressFromBytes(bytes memory bs, uint256 start) internal pure returns (address) { | ||
require(bs.length >= start + 32, "slicing out of range"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we reuse the code in these three functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to reuse. Now with only 2 usages of "assembly" left, this PR addresses #1666.
@@ -2,7 +2,16 @@ pragma solidity ^0.5.3; | |||
|
|||
// TODO(asa): Limit assembly usage by using X.staticcall instead. | |||
contract UsingPrecompiles { | |||
address constant TRANSFER = address(0xff - 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI #2324
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Seems the mload from #2324 may be OK.
require(bs.length >= start + 32, "slicing out of range"); | ||
bytes32 x; | ||
assembly { | ||
x := mload(add(bs, add(0x20, start))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do add(bs, 32 + start)
? IMO the decimal representation is more readable
require(bs.length >= start + 32, "slicing out of range"); | ||
bytes32 x; | ||
assembly { | ||
x := mload(add(bs, add(0x20, start))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to add 32 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first word of an array is the array size.
@@ -2,7 +2,16 @@ pragma solidity ^0.5.3; | |||
|
|||
// TODO(asa): Limit assembly usage by using X.staticcall instead. | |||
contract UsingPrecompiles { | |||
address constant TRANSFER = address(0xff - 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Seems the mload from #2324 may be OK.
* master: (26 commits) Added new lint rule (#2349) [Slashing] Slash locked gold (#2317) [Slashing] Allow voting gold to be slashable (#2302) 1666 precompiles assembly rewrite (#2324) Small fixes to deploy web (#2343) Governance, downtime and double signing slasher contracts (#2267) Added daily limit for reserve spending (#2303) Fixing multisig tests (#2342) [Wallet] Implement new send & import flow (#2332) Limechain/16xx (#1982) Add infinite pagination to Leaderboard (#2339) cli: Add rewards:show (#2160) Correct broken link formatting Celotool command for deploying hotfixes (#2142) Governance ContractKit + CLI changes (#2139) Complete codependent slashing precompile PRs (#2333) Add new and modified precompiles to UsingPrecompiles.sol (#2318) Adding error messages to contracts (#1182) Upgrade i18next and react-i18next to latest across monorepo (#2311) [Wallet] Fix exchange input on iOS and feed item display (#2319) ... # Conflicts: # yarn.lock
* master: (35 commits) Remove rep sentence from brand kit page (#2350) Added new lint rule (#2349) [Slashing] Slash locked gold (#2317) [Slashing] Allow voting gold to be slashable (#2302) 1666 precompiles assembly rewrite (#2324) Small fixes to deploy web (#2343) Governance, downtime and double signing slasher contracts (#2267) Added daily limit for reserve spending (#2303) Fixing multisig tests (#2342) [Wallet] Implement new send & import flow (#2332) Limechain/16xx (#1982) Add infinite pagination to Leaderboard (#2339) cli: Add rewards:show (#2160) Correct broken link formatting Celotool command for deploying hotfixes (#2142) Governance ContractKit + CLI changes (#2139) Complete codependent slashing precompile PRs (#2333) Add new and modified precompiles to UsingPrecompiles.sol (#2318) Adding error messages to contracts (#1182) Upgrade i18next and react-i18next to latest across monorepo (#2311) ...
Description
Add and modify functions in
UsingPrecompiles.sol
to account for new and modified precompiles in recentcelo-blockchain
PRs (i.e. celo-org/celo-blockchain#772 and celo-org/celo-blockchain#767)Tested
numberValidatorsInCurrentSet
andvalidatorSignerAddressFromCurrentSet
are exercised in current end-to-end tests. Other changes are untested.Backwards compatibility
Fully backwards compatible