Targeting solidity compiler v0.4.18 and updated to silence warnings #23

AdamJLemmon · 2017-11-20T20:41:14Z

Updated method visibility and mutability(public, pure, view), replaced sha3 with keccak256, all throws updated to requires and unused variables removed or "used".

Note the initial commit seems to be an issue with atom new line chars where several lines were removed and re-added but shown as diffs.

Single warning remains:

oraclizeAPI_0.4.sol:129:5: Warning: Function state mutability can be restricted to pure
    function __callback(bytes32 myid, string result, bytes proof) public {

Would need to update state to silence this and cannot add the pure modifier as inheriting contracts can not change the mutability modifier and thus would be bound to pure.

Have tested all examples in the browser solidity fork as well as the nested and computation examples from the test query page. Also deployed to kovan and tested the randomDS(https://github.com/oraclize/ethereum-examples/blob/master/solidity/random-datasource/randomExample.sol).

… to be ignored

…de explicitly abstract

…ds compatibility

…ted 0.4 file back to 0.4 compatible version

D-Nice

Showcase submitted comments

D-Nice · 2017-11-20T21:30:16Z

oraclizeAPI_0.4.sol

@@ -126,7 +126,9 @@ contract usingOraclize {
    function __callback(bytes32 myid, string result) public {
        __callback(myid, result, new bytes(0));
    }
-    function __callback(bytes32 myid, string result, bytes proof) public;
+    function __callback(bytes32 myid, string result, bytes proof) public {
+      myid; result; proof; // Silence compiler warnings


change to

return; myid; result; proof; // Silence compiler warnings

so it never needlessly actually adds them to the stack, but just satisfies the compiler?

Strange that the compiler doesn't complain about an empty statement. Do you think it won't be changed in the future?

D-Nice · 2017-11-21T17:21:58Z

oraclizeAPI_0.4.sol

@@ -28,7 +28,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 THE SOFTWARE.
 */

-pragma solidity ^0.4.0;//please import oraclizeAPI_pre0.4.sol when solidity < 0.4.0
+pragma solidity ^0.4.18;//please import oraclizeAPI_pre0.4.sol when solidity < 0.4.0


Update comment message appropriately:

//please import oraclizeAPI_0.4.sol when solidity < 0.4.18

EDIT: Ignore, this comment is based on the erroneous assumption that this contract would supplement the current one and not replace it. However, might still update it saying to change the compiler version to the needed 0.4.x one but that the api is targeted at 0.4.18

D-Nice · 2017-11-21T17:23:17Z

oraclizeAPI_0.4.sol

@@ -70,7 +70,7 @@ contract usingOraclize {
    OraclizeI oraclize;
    modifier oraclizeAPI {
        if((address(OAR)==0)||(getCodeSize(address(OAR))==0))
-            oraclize_setNetwork();
+            oraclize_setNetwork(networkID_auto);


API by default should not use the dummy oraclize_setNetwork(uint8) function, but the direct oraclize_setNetwork() function for efficiency.

I agree with @D-Nice. It's cheaper gas wise

D-Nice · 2017-11-21T17:46:43Z

oraclizeAPI_0.4.sol

@@ -763,7 +763,7 @@ contract usingOraclize {
    }

    function oraclize_newRandomDSQuery(uint _delay, uint _nbytes, uint _customGasLimit) internal returns (bytes32){
-        if ((_nbytes == 0)||(_nbytes > 32)) throw;
+        require((_nbytes != 0) && (_nbytes <= 32));


maybe _nbytes > 0 check is more appropriate for first condition?

Agreed, more for clarity than for a security need. Since _nbytes is uint, sending a negative value would simply overflow the variable and that would be catched by the second conditions.

D-Nice · 2017-11-21T17:47:39Z

oraclizeAPI_0.4.sol


        bool proofVerified = oraclize_randomDS_proofVerify__main(_proof, _queryId, bytes(_result), oraclize_getNetworkName());
-        if (proofVerified == false) throw;
+        require(proofVerified);


would an assert be more appropriate here?

If you read here, it is actually the opposite: the require-style exception uses the new revert opcode, while the assert uses the throw opcode. So, it is should be require.

yes, I understand, hence why I'm recommending assert. Otherwise, Oraclize can reward itself for providing wrong queries, with proofs that purposefully fail, taking leftover gas as profit.

Right, but it could be useful for offchain payments and gas refunds.

D-Nice · 2017-11-21T17:53:41Z

oraclizeAPI_0.4.sol

@@ -119,10 +119,10 @@ contract usingOraclize {
        return false;
    }

-    function __callback(bytes32 myid, string result) {
+    function __callback(bytes32 myid, string result) public {


should the default methods implement check to ensure msg.sender is Oraclize cbAddress??? Or is it expected to fallback/be overriden.

That extra-check would slightly increase the gas usage on deployment for every contract, as most of them only deploy the _callback with proof. And I also believe it would be redundant, as most of them will have that check any way in the implemented callback. So I believe it is better to leave it as it is.

D-Nice · 2017-11-21T17:56:34Z

oraclizeAPI_0.4.sol

    function oraclize_newRandomDSQuery(uint _delay, uint _nbytes, uint _customGasLimit) internal returns (bytes32){
-        if ((_nbytes == 0)||(_nbytes > 32)) throw;
+        require((_nbytes != 0) && (_nbytes <= 32));


For clarity, greater than check replacement on first condition?

_nbytes > 0

D-Nice · 2017-11-21T17:57:15Z

oraclizeAPI_0.4.sol

        bool proofVerified = oraclize_randomDS_proofVerify__main(_proof, _queryId, bytes(_result), oraclize_getNetworkName());
-        if (proofVerified == false) throw;
-
+        require(proofVerified);


assert here?

reasoning is, to not reward Oraclize with leftover gas on a failed proof.

Same as previous comment

D-Nice · 2017-11-21T19:42:23Z

oraclizeAPI_0.4.sol

        __callback(myid, result, new bytes(0));
    }
-    function __callback(bytes32 myid, string result, bytes proof) {
+    function __callback(bytes32 myid, string result, bytes proof) public {
+      myid; result; proof; // Silence compiler warnings


prepend return, before pushing args to stack. Although it should be overridden and not used in general.

D-Nice · 2017-11-21T19:44:53Z

oraclizeAPI_0.4.sol

@@ -84,6 +84,10 @@ contract usingOraclize {
    }

    function oraclize_setNetwork(uint8 networkID) internal returns(bool){


Don't use this dummy function now directly in the contract here, replace with a direct call to oraclize_setNetwork() if @marcogiglio @bertani can confirm no backwards compatibility breaks there?

…to 0.4 or pre 0.4 where necessary

…ector

…rd oraclize with the remaining

…ported within the connector

marcogiglio

Only things I see that needs to be changed is to move back to require, everything else LGMT

marcogiglio · 2017-11-29T15:51:35Z

oraclizeAPI_0.5.sol

+        require((_proof[0] == "L") && (_proof[1] == "P") && (_proof[2] == 1));
+
+        bool proofVerified = oraclize_randomDS_proofVerify__main(_proof, _queryId, bytes(_result), oraclize_getNetworkName());
+        assert(proofVerified);


Move back to require

…n raised

AdamJLemmon · 2017-11-29T16:54:41Z

oraclizeAPI_0.5.sol

@@ -853,7 +853,7 @@ contract usingOraclize {
        require((_proof[0] == "L") && (_proof[1] == "P") && (_proof[2] == 1));

        bool proofVerified = oraclize_randomDS_proofVerify__main(_proof, _queryId, bytes(_result), oraclize_getNetworkName());
-        assert(proofVerified);
+        require(proofVerified);


@marcogiglio @D-Nice Reverted back to require from assert.

AdamJLemmon added 13 commits November 20, 2017 08:36

odd issue with atom editor, removing and re-inserting multiple lines,…

93638f5

… to be ignored

all if throw's converted to requires

8d16ad6

public modifiers explicitly added to all default public methods

b2d21ac

sha3 converted to keccak256

4cef9b8

public modifier added to OraclizeAddrResolverI getAddress method

dab6ac9

var converted to uint256

6551cbc

added state mutability modifiers where applicable

b6c0fbd

explicit conversion of decimal to byte data type

5cb8617

remove unused networkID param from set network

430fdf2

setNetwork unused param removed on method call and callback method ma…

82cabde

…de explicitly abstract

added setNetwork method with input param in order to maintain backwar…

0394647

…ds compatibility

updated compiler version to 0.4.18

3d1b197

added placeholder __callback method to maintain backwards compatibility

6a3f405

AdamJLemmon requested review from bertani, marcogiglio and D-Nice November 20, 2017 20:41

AdamJLemmon changed the title ~~Tagretting solidity compiler v0.4.18 and updated to silence warnings~~ Tagreting solidity compiler v0.4.18 and updated to silence warnings Nov 20, 2017

AdamJLemmon changed the title ~~Tagreting solidity compiler v0.4.18 and updated to silence warnings~~ Targeting solidity compiler v0.4.18 and updated to silence warnings Nov 20, 2017

moved updated api targeting 0.4.18 to a new 0.5 api version and rever…

ef27f51

…ted 0.4 file back to 0.4 compatible version

D-Nice reviewed Nov 24, 2017

View reviewed changes

AdamJLemmon added 7 commits November 24, 2017 11:37

updated initial comment specifying the use of this api and to revert …

db63f33

…to 0.4 or pre 0.4 where necessary

updated oraclizeI visibility modifiers to match the connector

1ae72c5

return added before variable use to silence warnings

62bc14c

removed methods from oraclizeI that are no longer present in the conn…

d50ea57

…ector

check changed to bytes > 0 not != 0

f5c4bba

assert used in place of require in order to burn all gas and not rewa…

d6a3c7e

…rd oraclize with the remaining

set config and use coupon removed entirely from api as not longer sup…

023914a

…ported within the connector

marcogiglio approved these changes Nov 29, 2017

View reviewed changes

marcogiglio suggested changes Nov 29, 2017

View reviewed changes

assert changed back to require in order to save the gas when exceptio…

0ada9bd

…n raised

AdamJLemmon commented Nov 29, 2017

View reviewed changes

AdamJLemmon merged commit 338a56b into master Nov 30, 2017

marcogiglio mentioned this pull request Dec 5, 2017

Refactored error handling and minor code cleanup #15

Closed

D-Nice deleted the AL_solc4.18_silence_warnings branch July 2, 2019 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Targeting solidity compiler v0.4.18 and updated to silence warnings #23

Targeting solidity compiler v0.4.18 and updated to silence warnings #23

AdamJLemmon commented Nov 20, 2017 •

edited

Loading

D-Nice left a comment

D-Nice Nov 20, 2017

marcogiglio Nov 29, 2017 •

edited

Loading

D-Nice Nov 21, 2017

D-Nice Nov 21, 2017

marcogiglio Nov 29, 2017 •

edited

Loading

D-Nice Nov 21, 2017

marcogiglio Nov 29, 2017

D-Nice Nov 21, 2017

marcogiglio Nov 29, 2017

D-Nice Nov 29, 2017

marcogiglio Nov 29, 2017

D-Nice Nov 21, 2017

marcogiglio Nov 29, 2017 •

edited

Loading

D-Nice Nov 21, 2017

D-Nice Nov 21, 2017

D-Nice Nov 24, 2017

marcogiglio Nov 29, 2017 •

edited

Loading

D-Nice Nov 21, 2017

D-Nice Nov 21, 2017

marcogiglio left a comment •

edited

Loading

marcogiglio Nov 29, 2017

AdamJLemmon Nov 29, 2017

		@@ -84,6 +84,10 @@ contract usingOraclize {
		}

		function oraclize_setNetwork(uint8 networkID) internal returns(bool){

Targeting solidity compiler v0.4.18 and updated to silence warnings #23

Targeting solidity compiler v0.4.18 and updated to silence warnings #23

Conversation

AdamJLemmon commented Nov 20, 2017 • edited Loading

D-Nice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcogiglio Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcogiglio Nov 29, 2017 • 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

Choose a reason for hiding this comment

marcogiglio Nov 29, 2017 • 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

marcogiglio Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcogiglio left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamJLemmon commented Nov 20, 2017 •

edited

Loading

marcogiglio Nov 29, 2017 •

edited

Loading

marcogiglio Nov 29, 2017 •

edited

Loading

marcogiglio Nov 29, 2017 •

edited

Loading

marcogiglio Nov 29, 2017 •

edited

Loading

marcogiglio left a comment •

edited

Loading