Upgrade contracts to solc 0.5.0 syntax #467

elenadimitrova · 2018-12-18T10:27:19Z

Closes #459

Implements fixes for the solc 0.5.0 breaking changes. The only exception not yet implemented is the address payable modifier and this is due to solidity-coverage and additionally [solium] (duaraghav8/Ethlint#246) not yet supporting it.

Since the CLNY token and MultiSig contracts were compiled and deployed with earlier version of solc here we start importing the already compiled contracts from colonyToken repo, as opposed to recompiling them from source internally here.

…ing issues

… 0.5.0

… _before_ error messages made it in

…tion

…modifier for addresses

kronosapiens · 2018-12-24T21:52:26Z

contracts/Colony.sol

@@ -275,4 +276,6 @@ contract Colony is ColonyStorage, PatriciaTreeProofs {
    emit DomainAdded(domainCount);
    emit PotAdded(potCount);
  }
+
+  function() external payable {}


What is this for? A fallback for receiving Ether?

You are on an outdated commit @kronosapiens . This was removed a day before I finished the PR. Not sure how you got it?

I was trying out a commit-by-commit review this time, to better see how the PR evolved over time. But it does make commenting a bit trickier.

Yeah I had to go back on part of the implementation. Normally I would just drop or amend commits but the change was tricker to extract this time. I'll try to keep a cleaner commit history going forward.

kronosapiens · 2018-12-24T21:55:08Z

contracts/ColonyNetworkMining.sol

@@ -85,8 +85,9 @@ contract ColonyNetworkMining is ColonyNetworkStorage {
    address clnyToken = IMetaColony(metaColony).getToken();
    require(clnyToken != address(0x0), "colony-reputation-mining-clny-token-invalid-address");

-    inactiveReputationMiningCycle = new EtherRouter();
-    EtherRouter(inactiveReputationMiningCycle).setResolver(miningCycleResolver);
+    EtherRouter e = new EtherRouter();


Why this change?

Because implicit conversion from a contract to address type is not supported in solc 0.5.0. Additionally the cast from non-payable address to a payable contract type (such as EtherRouter) is also not supported. Additionally the new single line replacement is much cleaner in its purpose.

kronosapiens · 2018-12-24T21:57:24Z

contracts/ContractRecovery.sol

@@ -72,7 +72,7 @@ contract ContractRecovery is CommonStorage {
  function exitRecoveryMode() public recovery auth {
    uint totalAuthorized = recoveryRolesCount;
    // Don't double count the owner (if set);
-    if (owner != address(0x0) && !CommonAuthority(authority).hasUserRole(owner, RECOVERY_ROLE)) { 
+    if (owner != address(0x0) && !CommonAuthority(address(authority)).hasUserRole(owner, RECOVERY_ROLE)) { 


How come owner doesn't need to be cast to address?

If you check the actual declarations of these vars, they are:

DSAuthority public authority; address public owner;

kronosapiens · 2018-12-24T21:57:41Z

contracts/ColonyTask.sol

@@ -452,7 +452,7 @@ contract ColonyTask is ColonyStorage {
      role.rating = role.rateFail ? TaskRatings.Unsatisfactory : TaskRatings.Satisfactory;
    }

-    uint256 payout = task.payouts[roleId][token];
+    uint256 payout = task.payouts[roleId][address(token)];


Interesting, elsewhere I saw that the address cast was removed when accessing a mapping.

kronosapiens · 2018-12-24T22:04:41Z

contracts/Colony.sol

@@ -24,6 +24,8 @@ import "./EtherRouter.sol";

 contract Colony is ColonyStorage, PatriciaTreeProofs {

+  function() external payable {}


Can we add a comment explaining what this function is for?

This is no longer there.

kronosapiens · 2018-12-24T22:05:13Z

test/colony-network-mining.js

@@ -204,7 +204,7 @@ contract("ColonyNetworkMining", accounts => {
      await giveUserCLNYTokens(colonyNetwork, OTHER_ACCOUNT, 9000);
      await clny.approve(tokenLocking.address, 10000, { from: OTHER_ACCOUNT });

-      await checkErrorRevert(tokenLocking.deposit(clny.address, 10000, { from: OTHER_ACCOUNT }), "ds-token-insufficient-balance");
+      await checkErrorRevert(tokenLocking.deposit(clny.address, 10000, { from: OTHER_ACCOUNT }));


kronosapiens · 2018-12-24T22:05:55Z

contracts/Colony.sol

@@ -15,7 +15,7 @@
  along with The Colony Network. If not, see <http://www.gnu.org/licenses/>.
 */

-pragma solidity >0.5.0;
+pragma solidity >=0.4.23;


Why, if we are committed to Sol 5 moving forward?

In the PR description I explained that we cannot fully commit to 0.5.0 yet which is mainly because solidity-coverage doesn't support it.

kronosapiens · 2018-12-24T22:08:29Z

How come in the last few commits it looks like some of the changes around address payable and the version pragma were rolled back?

kronosapiens · 2018-12-24T22:10:01Z

contracts/ReputationMiningCycleStorage.sol

@@ -15,8 +15,7 @@
  along with The Colony Network. If not, see <http://www.gnu.org/licenses/>.
 */

-pragma solidity ^0.4.23;
-pragma experimental "v0.5.0";
+pragma solidity >=0.4.23 <0.5.0;


Should this also omit the <0.5.0 ?

There are a handful of contracts which guaranteed will break if compiled under solc 0.5.0 so I've put a limit on those.

elenadimitrova · 2018-12-27T14:35:13Z

How come in the last few commits it looks like some of the changes around address payable and the version pragma were rolled back?

This is because due to solidity-coverage not yet supporting solc 0.5.0 we had to stay on 0.4 compiler but still with an upgraded syntax to support the incoming solc 0.5.0. With the exception of address payable with is only supported from 0.5.0 onwards and so cannot be used yet here.

elenadimitrova added the in-progress label Dec 18, 2018

elenadimitrova self-assigned this Dec 18, 2018

elenadimitrova force-pushed the maintenance/upgrade-solc-0.5.0 branch from 6714ae9 to b5a4d05 Compare December 21, 2018 13:33

elenadimitrova added 18 commits December 21, 2018 15:34

Switch pragma to 0.5.0 and fix function parameter memory keyword miss…

789b8ae

…ing issues

Make 0x0 value explicitely an address where necessary

6da0616

Explicitely convert contract references to address type

bb30145

Final set of fixes to migrate to 0.5.0

4e07930

Skip solium run on contracts due to solium's incompatibility for solc…

00bcef6

… 0.5.0

Minor ammends

04e9891

Amend script to provision folder if nonexistent

1d408c5

Remove named return parameters when they are payable address type

7750dc9

Give proper name to circle step

566b394

Exclude fallback function from recovery mods check

1c1b50a

Fix a failing test with CLNY since that was compiled with dappsys lib…

8c4684d

… _before_ error messages made it in

Remove the now deleted gnosis folder from solcover skip

17e2efc

Switch pragma back to supporting 0.4.23 and after solc version

063ba42

Remove payable modifier from address props and add solc <0.5.0 limita…

b3f8cfe

…tion

Remove solium exceptions as not needed while we stay with no payable …

4401163

…modifier for addresses

Fix a scope variable overlap

8be273a

Pull the right solc docker image

4221da6

Bundle the truffle compile and token contracts provisioning for solcover

9572dcf

elenadimitrova force-pushed the maintenance/upgrade-solc-0.5.0 branch from b5a4d05 to 9572dcf Compare December 21, 2018 13:34

elenadimitrova added 2 commits December 21, 2018 20:56

Remove setup upgradable token helper

b1d79ec

Minor fixes

967daf8

elenadimitrova changed the title ~~Upgrade contracts to solc 0.5.1~~ Upgrade contracts to solc 0.5.0 syntax Dec 21, 2018

elenadimitrova requested a review from kronosapiens December 21, 2018 19:46

elenadimitrova added ready-for-review and removed in-progress labels Dec 21, 2018

kronosapiens added under-review and removed ready-for-review labels Dec 24, 2018

kronosapiens reviewed Dec 24, 2018

View reviewed changes

kronosapiens approved these changes Dec 27, 2018

View reviewed changes

kronosapiens merged commit 2d59360 into develop Dec 27, 2018

kronosapiens deleted the maintenance/upgrade-solc-0.5.0 branch December 27, 2018 18:52

elenadimitrova removed the under-review label Dec 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade contracts to solc 0.5.0 syntax #467

Upgrade contracts to solc 0.5.0 syntax #467

elenadimitrova commented Dec 18, 2018 •

edited

Loading

kronosapiens Dec 24, 2018

elenadimitrova Dec 27, 2018

kronosapiens Dec 27, 2018

elenadimitrova Dec 28, 2018

kronosapiens Dec 24, 2018

elenadimitrova Dec 27, 2018

kronosapiens Dec 27, 2018

kronosapiens Dec 24, 2018

elenadimitrova Dec 27, 2018

kronosapiens Dec 27, 2018

kronosapiens Dec 24, 2018

kronosapiens Dec 24, 2018

elenadimitrova Dec 27, 2018

kronosapiens Dec 27, 2018

kronosapiens Dec 24, 2018

kronosapiens Dec 24, 2018

elenadimitrova Dec 27, 2018

kronosapiens Dec 27, 2018

kronosapiens commented Dec 24, 2018

kronosapiens Dec 24, 2018

elenadimitrova Dec 27, 2018

kronosapiens Dec 27, 2018

elenadimitrova commented Dec 27, 2018

		@@ -24,6 +24,8 @@ import "./EtherRouter.sol";

		contract Colony is ColonyStorage, PatriciaTreeProofs {

		function() external payable {}

Upgrade contracts to solc 0.5.0 syntax #467

Upgrade contracts to solc 0.5.0 syntax #467

Conversation

elenadimitrova commented Dec 18, 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

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

kronosapiens commented Dec 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenadimitrova commented Dec 27, 2018

elenadimitrova commented Dec 18, 2018 •

edited

Loading