Upgrade solidity version to 0.5.4 #393

justinkchen · 2019-02-23T02:40:27Z

Running yarn compile and gradually fixing changes.

There are a lot of breaking changes with https://solidity.readthedocs.io/en/v0.5.0/050-breaking-changes.html

Following this guide:
https://hackernoon.com/compile-solidity-smart-contracts-faster-in-truffle-projects-eb8bebb61ec3

Example new errors:
https://ethereum.stackexchange.com/questions/8959/solidity-compile-expected-token-comma-got-identifier
https://ethereum.stackexchange.com/questions/63426/running-out-of-gas-during-a-deploy-due-to-a-large-number-of-require-statements
sc-forks/solidity-parser#18
https://hackernoon.com/compile-solidity-smart-contracts-faster-in-truffle-projects-eb8bebb61ec3
Data location must be "calldata" for parameter in external function, but none was given.
Data location must be "storage" or "memory" for parameter in function, but none was given
Data location must be "memory" for return parameter in function, but none was given.

coveralls · 2019-02-24T09:29:47Z

Pull Request Test Coverage Report for Build 4405

66 of 66 (100.0%) changed or added relevant lines in 19 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 4402:	0.0%
Covered Lines:	811
Relevant Lines:	811

💛 - Coveralls

felix2feng

Good job getting this done. Do have a few questions and generally feel a little bit nervous about changing the external dependencies

felix2feng · 2019-02-25T06:03:36Z

package.json

@@ -18,10 +18,10 @@
    "test": "test"
  },
  "scripts": {
-    "chain": "yarn clean-chain && ganache-cli --db blockchain --networkId 50 --accounts 20 -l 19000000 -e 10000000000 -m 'concert load couple harbor equip island argue ramp clarify fence smart topic'",
+    "chain": "yarn clean-chain && ganache-cli --db blockchain --networkId 50 --accounts 20 -l 19000000 -e 10000000000 -m 'concert load couple harbor equip island argue ramp clarify fence smart topic' --allowUnlimitedContractSize",


Before we merge... let's try to do a test deploy on Kovan and check the gas costs and whether Core could be deployed. This way we can validate whether this flag interferes with deployment

yes i wanted to get this done too, need to make sure we can use the optimizer i think as mentioned here: https://ethereum.stackexchange.com/questions/63426/running-out-of-gas-during-a-deploy-due-to-a-large-number-of-require-statements

felix2feng · 2019-02-25T06:03:45Z

package.json

    "clean": "rm -rf build; rm -rf transpiled; rm -rf types/generated",
    "clean-chain": "rm -rf blockchain && cp -r snapshots/0x-Kyber blockchain",
-    "compile": "truffle compile --all",
+    "compile": "./node_modules/.bin/truffle compile --all",


any reason for this?

i think this was to use the upgraded version of solc compiler as specified in the truffle.js

https://hackernoon.com/compile-solidity-smart-contracts-faster-in-truffle-projects-eb8bebb61ec3

i think before this, it was still trying to use the old version since truffle v5 by default still uses 0.4.25

felix2feng · 2019-02-25T06:04:01Z

package.json

-    "solc": "^0.4.25",
-    "solidity-coverage": "^0.5.11",
+    "solc": "^0.5.4",
+    "solidity-coverage": "https://github.com/leapdao/solidity-coverage#master",


Any reason for changing to this?

sc-forks/solidity-parser#18 coverage was failing because of _ in variable names and some other weird stuff that the current version of solidity parser doesn't handle still. we can maybe fork leapdaos too if we want to keep it under us for now

felix2feng · 2019-02-25T06:07:38Z

contracts/core/tokens/SetToken.sol

@@ -127,7 +127,8 @@ contract SetToken is

            // Figure out which of the components has the minimum decimal value
            /* solium-disable-next-line security/no-low-level-calls */
-            if (currentComponent.call(bytes4(keccak256("decimals()")))) {
+            (bool success, ) = currentComponent.call(abi.encodeWithSignature("decimals()"));


Interesting new return of .call

felix2feng · 2019-02-25T06:08:04Z

contracts/core/tokens/SetTokenFactory.sol

-            _naturalUnit,
-            _name.bytes32ToString(),
-            _symbol.bytes32ToString()
+        return address(


why do we need to wrap this in address?

I forget the exact error message, but basically it was saying that we were returning an instance of a contract instead of an address and a lot of the solc 0.5 changes are all about being more explicit so have to convert to address i think

felix2feng · 2019-02-25T06:09:13Z

contracts/external/DappHub/note.sol

@@ -34,8 +34,13 @@ contract DSNote {
            bar := calldataload(36)
        }

-        emit LogNote(msg.sig, msg.sender, foo, bar, msg.value, msg.data);


This is a breaking change? Can't get msg.value directly?

yea can't use msg.value directly, needs to be in an internal function

Using msg.value in non-payable functions (or introducing it via a modifier) is disallowed as a security feature.

felix2feng · 2019-02-25T06:11:09Z

contracts/mocks/tokens/InvalidReturnTokenMock.sol

-  /**
-  * @dev Total number of tokens in existence
-  */
-  function totalSupply() external view returns (uint256) {


any reason this was removed?

It is already defined as a public variable on line 15 here, so the getter should automatically be generated. it was giving me an error about shadowing variables here

…ables of multi-return functions to work with new solc compiler

…d byte, contract to address, and cleaning up DappHub code

…tedContractSize

justinkchen force-pushed the justin_chen-upgrade-solidity-version branch from f0330d9 to a2d4556 Compare February 23, 2019 20:25

felix2feng reviewed Feb 25, 2019

View reviewed changes

felix2feng approved these changes Feb 25, 2019

View reviewed changes

justinkchen and others added 17 commits February 25, 2019 15:49

Bump solidity version to 0.5.4

c4e54dd

Upgrade openzeppelin and canonical-weth and move declarations of vari…

e36a53f

…ables of multi-return functions to work with new solc compiler

Update compile script

1ba7521

Add .env to gitignore

63a6586

Add explicit modifiers for calldata and memory for parameters

7dae211

Handle other compile errors like explicit conversions between uint an…

4adb074

…d byte, contract to address, and cleaning up DappHub code

Fix explicit data locations for IssuanceLibrary

5773679

Add more call data location parameters

00710e0

Fix typo in comment

61477eb

Allow for unlimited contract size in development

e582eb7

Use leapdao solidity coverage for 0.5 solidity support

589cd6e

Fix specs using deep include

ebee3fb

Remove unused variables from parameters

d2cc6bd

Refactor coreissuance to take up less bytecode and remove allowUnlimi…

dfca168

…tedContractSize

modify create to creatSet

c7edad6

REorder checks

8168553

Add back check

2757cb3

felix2feng force-pushed the justin_chen-upgrade-solidity-version branch from 77c250e to 2757cb3 Compare February 25, 2019 23:49

felix2feng merged commit f9c9a22 into master Feb 26, 2019

felix2feng deleted the justin_chen-upgrade-solidity-version branch February 26, 2019 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade solidity version to 0.5.4 #393

Upgrade solidity version to 0.5.4 #393

justinkchen commented Feb 23, 2019 •

edited

Loading

coveralls commented Feb 24, 2019 •

edited

Loading

felix2feng left a comment

felix2feng Feb 25, 2019

justinkchen Feb 25, 2019

felix2feng Feb 25, 2019

justinkchen Feb 25, 2019

felix2feng Feb 25, 2019

justinkchen Feb 25, 2019

felix2feng Feb 25, 2019

felix2feng Feb 25, 2019

justinkchen Feb 25, 2019

felix2feng Feb 25, 2019

justinkchen Feb 25, 2019

justinkchen Feb 25, 2019

felix2feng Feb 25, 2019

justinkchen Feb 25, 2019

Upgrade solidity version to 0.5.4 #393

Upgrade solidity version to 0.5.4 #393

Conversation

justinkchen commented Feb 23, 2019 • edited Loading

coveralls commented Feb 24, 2019 • edited Loading

Pull Request Test Coverage Report for Build 4405

💛 - Coveralls

felix2feng 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinkchen commented Feb 23, 2019 •

edited

Loading

coveralls commented Feb 24, 2019 •

edited

Loading