Update to Solc 0.5 #67

gskapka · 2018-11-26T10:53:43Z

Solc 0.5.0 Version of the API

Here it is, the API upgraded to the latest Solidity compiler, and (hopefully) following all the best practices. Rather than simply getting it to compile, many other changes have been made and so hence the need for a thorough review. Amongst those changes:

Added stuff to make it compile (a million memory keywords &c.)
More explicit castings (which broke stuff - see later) between types
Adding a safe version of parseInt() (original remains incase anyone using its behaviour)
Moved all global variables to the top of the contract
Moved all modifiers together
Standardized comment styles (/* ... */ for multilines instead of multiple //'s)
Standardized grammar in comments
Standardized white space between lines
Standardized variable naming (the few examples of snake_case are now camelCased)
Standardized use of whitespace between function declaration parens. vs. if/while/for conditionals
Standardized the use of whitespace between closing parens. & {s
Standardized the use of underscores before parameter names
Fix the failing RandomDS proof (which I broke during the above)

Notes:

Re which latter bullet point: It turns out that with the new recasting rules we must first cast to types of the same size. So to go from bytes(1) to uint we first need to go via either uint8 or bytes32:

uint256(bytes32(bytes1(...)));
// or
uint256(uint8(bytes1(...)));

The two casts pad differently hence the new explicitness ruling. As such, I broke (and then fixed) the proof verification thusly:

To Do

@D-Nice & @riccardopersiani please can you go over this PR with a fine tooth comb and make sure I haven't goofed anything? Also tell me if any of the above listed changes are a step too far/pointless/terrible/should be burnt with fire &c., &c.

Also, I have a private repo elsewhere with some (unpolished but passing) tests for some of the parsing helpers we offer in this API, plus all the truffle examples in the ethereum-examples repo have been updated to als use this API and all tests therein are also passing.
So it should be all good w/r/t it working, now it's just a case of arguing over the changes I've made.

… use of {'s

D-Nice · 2018-11-27T18:08:09Z

shall we also decide on a standard as to whether to return anonymous or named variables?

gskapka · 2018-11-27T18:45:54Z

Easy question for me - named variables everytime (unless we're talking point-free - but that's a different & more fun kettle of fish!). That whole:

naming is hard

thing is dumb. Naming is explicit and useful and enlightening.

D-Nice

Overall changes look good and definitely increased quality of code.

Can you confirm these have been run through your ethereum-examples testing suite @gskapka ?

Some changes recommended, clarifications, and points needing discussion, and acceptable for merge thereafter. Some points need @bertani feedback.

oraclizeAPI_0.5.sol

D-Nice · 2018-11-26T17:14:48Z

oraclizeAPI_0.5.sol

    struct buffer {
        bytes buf;
        uint capacity;
    }

-    function init(buffer memory buf, uint _capacity) internal pure {
+    function init(buffer memory _buf, uint _capacity) internal pure {


I much prefer this as well. It may be worth actually seeing about pushing these updates towards: https://github.com/smartcontractkit/solidity-cborutils

So we can stay consistent with that repo. Additonally may be useful to ensure there haven't been some bugfixes to that cbor library since?

I assume the reason Nick did not go this route, is the functions are pure anyways, and it's a library, so they won't themselves have any side effects(at least from a state perspective, it will affect memory in certain functions).

oraclizeAPI_0.5.sol

D-Nice · 2018-11-27T18:53:15Z

oraclizeAPI_0.5.sol

-    mapping(bytes32=>bool) oraclize_randomDS_sessionKeysHashVerified;
-
-    function verifySig(bytes32 tosignh, bytes dersig, bytes pubkey) internal returns (bool){
+    function verifySig(bytes32 _tosignh, bytes memory _dersig, bytes memory _pubkey) internal returns (bool) {


safe to set to view or pure even?

Ibid. re compiler warnings. Plus gives bonus extra compiler warnings about needing to set either payable or non-payable on it. Sigh.

D-Nice · 2018-11-27T18:54:03Z

oraclizeAPI_0.5.sol

        }
    }

-    function oraclize_randomDS_proofVerify__sessionKeyValidity(bytes proof, uint sig2offset) internal returns (bool) {
+    function oraclize_randomDS_proofVerify__sessionKeyValidity(bytes memory _proof, uint _sig2offset) internal returns (bool) {


could be view or pure again?

Ibid. re compiler warnings.

D-Nice · 2018-11-27T18:55:36Z

oraclizeAPI_0.5.sol

+     The following function has been written by Alex Beregszaszi (@axic), use it under the terms of the MIT license
+     Duplicate Solidity's ecrecover, but catching the CALL return value
+    */
+    function safer_ecrecover(bytes32 _hash, uint8 _v, bytes32 _r, bytes32 _s) internal returns (bool, address) {


Ibid. re compiler warnings & whole slew of new ones re payable etc.

D-Nice · 2018-11-27T18:56:52Z

oraclizeAPI_0.5.sol

+    /*
+     The following function has been written by Alex Beregszaszi (@axic), use it under the terms of the MIT license
+    */
+    function ecrecovery(bytes32 _hash, bytes memory _sig) internal returns (bool, address) {


Ibid. re all the other ibids.

gskapka · 2018-11-28T12:11:03Z

Update:

Okay so I've done anything from the above review that I can & pushed it. I've also compiled anything I couldn't do into this one post to make it easier to see & address.

Done:

✔️ Naming return params (and adding underscores to go along with current practices)
✔️ Fixing the license typo.
✔️ Removing white space from string [] type declarations.

I can confirm that all tests are passing on all examples that have test-suites per my tool on gitlab using this current iteration of the API.

Stuff that needs discussing/addressing:

📟 Paging @bertani & @D-Nice, whose relevance to issues at hand are marked after the issues:

Do we update the copyright information? (@bertani)
Should getPrice become view? (@bertani, @D-Nice)
Do we remove the coupon function? (@bertani)
Do we drop the safeMemoryCleaner function? (@bertani)
Do we revert with a message on finding an unexpectedly high price in a query? (@bertani)
Do we use view, pure &c., even where it results in a lot more compiler warnings? (@bertani, @D-Nice)
Do you hate my compulsion & aesthetic driven re-ordering of listed variables into length order enough to make me revert it? (@D-Nice)

gskapka · 2018-11-28T17:20:53Z

Responses to above from @bertani

Re copyright: No, it's fine as is.
Re getPrice becoming view: Whatever keeps compatability with the current connector.
Re coupon: Can be removed.
Re safeMemoryCleaner: Since @D-Nice implemented it, if it's no longer patching whatever it patched it can be dropped.
Re revert, if we didn't throw in the first place, the return value was probably there for a reason so it's probably best to keep the current behaviour.
Re compiler warnings: if they don't change behaviour, it'd be best to minimise compiler warnings at the expense of being explicit with the visibility specifiers.

ok, based on those replies, let's remove coupon. Also, based on our earlier discussion, and me diving into safeMemoryCleaner, I think it's best we still leave it in, because as mentioned, it can be useful for any other assembly that works on memory, but doesn't necessarily zero it out, so the safeMemoryCleaner can be re-used there.

gskapka · 2018-11-30T16:26:48Z

Done & done.

axic · 2018-11-30T17:54:58Z

Oh wow, this is great! :)

gskapka added 13 commits November 16, 2018 17:23

fix compiler warnings for memory keyword & add gitignore

66bef7b

fix keccak hashings

34c4eae

fix the lack of spaces around operators & curlies

048c5e4

fix missing newlines between functions

8cbafd9

fix address parser

0d33a9a

fix parseInt

979f10e

fix uint2str

37db98e

fix randomDS query & sig verifying

ed56303

fix wrong number in overloaded parseInt

52839b5

add safeParseInt()

9a928e6

large cleanup of empty lines & standardising spacing, comments styl &…

532e297

… use of {'s

fix the RandomDS failing proofs

590c050

add underscores to params

91a67e6

gskapka self-assigned this Nov 26, 2018

gskapka requested review from D-Nice and riccardopersiani November 26, 2018 11:20

gskapka added the enhancement label Nov 26, 2018

D-Nice reviewed Nov 27, 2018

View reviewed changes

D-Nice mentioned this pull request Nov 27, 2018

More Truffle examples - all now @ solc 0.5.0 provable-things/ethereum-examples#33

Merged

name return vars, fix typo in license & rm erroneous whitespace

9bb6090

rm redundant coupon modifier

0be2694

D-Nice merged commit 31f5fe3 into master Nov 30, 2018

D-Nice mentioned this pull request Nov 30, 2018

JUST AS REFERENCE Fixing warnings with Solc 0.4.24 #61

Closed

axic mentioned this pull request Nov 30, 2018

Tracking Solidity 0.5.0 support ethereum/solidity#5422

Closed

D-Nice deleted the solc-0.5 branch July 2, 2019 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to Solc 0.5 #67

Update to Solc 0.5 #67

gskapka commented Nov 26, 2018 •

edited

Loading

D-Nice commented Nov 27, 2018

gskapka commented Nov 27, 2018

D-Nice left a comment

D-Nice Nov 26, 2018

D-Nice Nov 26, 2018

D-Nice Nov 27, 2018

gskapka Nov 28, 2018 •

edited

Loading

D-Nice Nov 27, 2018

gskapka Nov 28, 2018

D-Nice Nov 27, 2018

gskapka Nov 28, 2018

D-Nice Nov 27, 2018

gskapka Nov 28, 2018

gskapka commented Nov 28, 2018

gskapka commented Nov 28, 2018

D-Nice commented Nov 30, 2018

gskapka commented Nov 30, 2018

axic commented Nov 30, 2018

Update to Solc 0.5 #67

Update to Solc 0.5 #67

Conversation

gskapka commented Nov 26, 2018 • edited Loading

Solc 0.5.0 Version of the API

Notes:

To Do

D-Nice commented Nov 27, 2018

gskapka commented Nov 27, 2018

D-Nice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gskapka Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gskapka commented Nov 28, 2018

Update:

Done:

Stuff that needs discussing/addressing:

gskapka commented Nov 28, 2018

Responses to above from @bertani

Next:

D-Nice commented Nov 30, 2018

gskapka commented Nov 30, 2018

axic commented Nov 30, 2018

gskapka commented Nov 26, 2018 •

edited

Loading

gskapka Nov 28, 2018 •

edited

Loading