Upgrade to Solidity 0.5.16 #499

jjgonecrypto · 2020-04-18T02:16:44Z

Here's the Solidity v5 upgrade branch. The lion's share of the effort was from @K-Ho 🙇; I cherry-picked his commits and massaged them into our develop branch.

Notes

MixinResolver, SelfDestructible and Pausable are now all abstract. This is because they inherit from Owned but don't invoke the Owned constructor (this is as intended, so that we don't have the "Base constructor used twice issue"
All compiler warnings are now gone 😮
I opted for the more permissive semver of solc ^0.516 (i.e. 0.5.x) - happy to discuss being more restrictive.

Note: I've opted to keep this upgrade separate from testing Solidity v4 and v5 side-by-side, in order to keep this PR down in size. After merging, I'll work on that.

codecov · 2020-04-18T18:21:51Z

Codecov Report

Merging #499 into develop will increase coverage by 0.55%.
The diff coverage is 92.57%.

@@             Coverage Diff             @@
##           develop     #499      +/-   ##
===========================================
+ Coverage    92.04%   92.59%   +0.55%     
===========================================
  Files           39       38       -1     
  Lines         1985     1984       -1     
  Branches       271      275       +4     
===========================================
+ Hits          1827     1837      +10     
+ Misses         158      147      -11

Impacted Files	Coverage Δ
contracts/EscrowChecker.sol	`0.00% <0.00%> (ø)`
contracts/FeePoolEternalStorage.sol	`20.00% <0.00%> (ø)`
contracts/IssuanceEternalStorage.sol	`100.00% <ø> (ø)`
contracts/LimitedSetup.sol	`100.00% <ø> (ø)`
contracts/Math.sol	`100.00% <ø> (ø)`
contracts/MultiCollateralSynth.sol	`88.23% <ø> (ø)`
contracts/Owned.sol	`100.00% <ø> (ø)`
contracts/RewardsDistributionRecipient.sol	`100.00% <ø> (ø)`
contracts/SynthetixState.sol	`91.66% <ø> (ø)`
contracts/EternalStorage.sol	`25.58% <33.33%> (ø)`
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c67c34d...13807c1. Read the comment docs.

maxsam4 · 2020-04-19T14:56:55Z

contracts/FeePoolEternalStorage.sol

-     * @param feePeriodIDs Array feePeriodIDs with the accounts last claim
-     */
-    function importFeeWithdrawalData(address[] accounts, uint[] feePeriodIDs) external onlyOwner onlyDuringSetup {
+    function importFeeWithdrawalData(address[] calldata accounts, uint[] calldata feePeriodIDs)


Any reason for removing the natspec comments?

We're removing natspec altogether in favor of the docs. Natspec doesn't currently support state variables and internal functions which we want to include in our docs...

maxsam4 · 2020-04-19T15:00:04Z

contracts/RewardsDistribution.sol

+                if (!success) {
+                    // Note: we're ignoring the return value as it will fail for contracts that do not implement RewardsDistributionRecipient.sol
+                }


Is there any need of having this empty if statement?

It prevents the compiler warning for not doing anything with the result

contracts/State.sol

maxsam4 · 2020-04-19T15:01:39Z

contracts/Synthetix.sol

-     * @param _resolver The address of the Synthetix Address Resolver
-     */
-    constructor(address _proxy, TokenState _tokenState, address _owner, uint _totalSupply, address _resolver)
+    constructor(


natspec comments

contracts/test-helpers/MockExchanger.sol

jjgonecrypto · 2020-04-19T16:03:27Z

PR feedback addressed @maxsam4 - RFAL

hav-noms · 2020-04-21T09:12:46Z

contracts/IssuanceEternalStorage.sol


 import "./EternalStorage.sol";


+// TODO: this contract is redundant and should be removed


Is this redundant? Can we remove it?

It is redundant, but removing it requires updating our test setup script, deploy script and our deployment.json files. As for naming, both it and DelegateApprovals should use the [Base][Instance] ordering. E.g. ProxyFeePool, TokenStatesUSD.
For EternalStorage instances, we should have EternalStateIssuer and EternalStateDelegateApprovals. This makes it much easier to reason about naming.

hav-noms · 2020-04-21T09:16:27Z

contracts/Owned.sol

-    /**
-     * @dev Owned Constructor
-     */
+    // /**


might as well remove these natspec comments right?

Yes, but let me do that in a follow up PR where I ensure everything is in the docs that need to be.

hav-noms · 2020-04-21T09:18:00Z

contracts/Proxy.sol

@@ -20,10 +20,14 @@ contract Proxy is Owned {
        useDELEGATECALL = value;


I also think its time to remove the useDELEGATECALL path. We never used it and it should never be used.

Good point. Again, I'd like to do it in a follow up PR

hav-noms · 2020-04-21T09:21:51Z

contracts/Proxyable.sol

@@ -17,17 +17,20 @@ contract Proxyable is Owned {
     * optionalProxy modifiers, otherwise their invocations can use stale values. */
    address public messageSender;


Vote to remove the public accessor from messageSender. It used to be omitted and during an audit it was put back in from remediations however etherscan now always shows the last address that called the contract on the contract.... it was optically better hidden

Seconded. Though this should be in its own PR.

hav-noms · 2020-04-21T09:24:20Z

contracts/PurgeableSynth.sol

@@ -70,6 +70,6 @@ contract PurgeableSynth is Synth {
    bytes32 private constant PURGED_SIG = keccak256("Purged(address,uint256)");

    function emitPurged(address account, uint value) internal {


@clementbalestrat @evgenyboxer from support issue today user asked where his iETH is that disappeared. We really need the exchange to show users they were purged out of a frozen iSynth and their inverse synth is now in sUSD

Agreed. We need Zoe to have a thought about it.

hav-noms · 2020-04-21T09:31:05Z

contracts/SelfDestructible.sol

    }

    /**
     * @notice Set the beneficiary address of this contract.
     * @dev Only the contract owner may call this. The provided beneficiary must be non-null.
     * @param _beneficiary The address to pay any eth contained in this contract to upon self-destruction.
     */
-    function setSelfDestructBeneficiary(address _beneficiary) external onlyOwner {
+    function setSelfDestructBeneficiary(address payable _beneficiary) external onlyOwner {


I know this is not part of this review but I would question if we event need a setSelfDestructBeneficiary anymore and our contracts would look optically better without setting a selfDestructBeneficiary address that gets all "funds" when blown up... It's only for ETH anyways and we have no contracts that need ETH sent to us that blow up. We definitely shouldn't use this for ETH collateral either. Doesn't look good to let the owner use selfdestruct on a contract and get sent all the ETH

I think that's a good point , worth a discussion (though separate PR).

hav-noms · 2020-04-21T09:34:41Z

contracts/SupplySchedule.sol

-    uint public constant MAX_MINTER_REWARD = 200 * SafeDecimalMath.unit();
+    // TODO Uncomment constant
+    /*constant*/
+    uint public MAX_MINTER_REWARD = 200 * SafeDecimalMath.unit();


wanna do 1e18 * 200 like the others?

Also thats the TODO

Yeah missed this, good spot.

hav-noms · 2020-04-21T09:35:58Z

contracts/Synthetix.sol

-     * @param _resolver The address of the Synthetix Address Resolver
-     */
-    constructor(address _proxy, TokenState _tokenState, address _owner, uint _totalSupply, address _resolver)
+    constructor(


hav-noms · 2020-04-21T09:38:31Z

contracts/SynthetixState.sol



 // https://docs.synthetix.io/contracts/SynthetixState
-contract SynthetixState is State, LimitedSetup {
+contract SynthetixState is Owned, State, LimitedSetup {


I much prefer how Owned has been lifted out

hav-noms · 2020-04-21T09:38:54Z

contracts/SynthetixState.sol

@@ -38,20 +39,18 @@ contract SynthetixState is State, LimitedSetup {
    // may not be issued against a given value of SNX.
    uint public issuanceRatio = SafeDecimalMath.unit() / 5;
    // No more synths may be issued than the value of SNX backing them.
-    uint constant MAX_ISSUANCE_RATIO = SafeDecimalMath.unit();
+    // TODO Uncomment constant


What is this?

Mistake. Good catch

jacko125 · 2020-04-21T10:45:27Z

contracts/ExternStateToken.sol

@@ -123,19 +128,30 @@ contract ExternStateToken is SelfDestructible, Proxyable {
    }

    /* ========== EVENTS ========== */
+    function addressToBytes32(address input) internal pure returns (bytes32) {


Another way of approaching conversions for address type could be a library for converting address to bytes32 instead of the internal function that gets inherited by the underlying contracts (Synth.sol and Synthetix.sol).

ExternStateToken.sol

contract ExternStateToken is Owned, SelfDestructible, Proxyable { using SafeMath for uint; using SafeDecimalMath for uint; using addressConversion for address; ... proxy._emit(abi.encode(value), 3, TRANSFER_SIG, from.toBytes32(), to.toBytes32(), 0); } library addressConversions { function toBytes32(address input) internal pure returns (bytes32) { return bytes32(uint256(uint160(input))); } }

Yeah I had considered that as FeePool needs this function as well - though is it worth managing a new library just for this?

With v5 solidity it makes sense to have a library for address conversion as theres also the possible need to do conversions between address and address payable which can be included in the library , ie currently in supplySchedule.

benefits would be reducing risks of inline conversions syntax error or copy-pasting addressToBytes32() into contracts that need it.

jacko125 · 2020-04-21T10:50:42Z

contracts/FeePool.sol

@@ -830,7 +844,7 @@ contract FeePool is Proxyable, SelfDestructible, LimitedSetup, MixinResolver {
            abi.encode(debtRatio, debtEntryIndex, feePeriodStartingDebtIndex),
            2,
            ISSUANCEDEBTRATIOENTRY_SIG,
-            bytes32(account),
+            bytes32(uint256(uint160(account))),


This could be re-using addressConversion library to convert address to Bytes32 for emitting the topics off the proxy.

contracts/Owned.sol

contracts/PurgeableSynth.sol

contracts/RewardsDistribution.sol

contracts/SupplySchedule.sol

contracts/SynthetixState.sol

jacko125

as per comments

jjgonecrypto · 2020-04-21T19:44:29Z

RFAL

hav-noms

LGTM

upgrade to Solidity 0.5.16

8a38c78

jjgonecrypto mentioned this pull request Apr 18, 2020

Upgrade to Solidity 0.5.16 #483

Closed

jjgonecrypto added 6 commits April 18, 2020 13:15

Fixing MixinResolver and warnings

1b7490e

Fixing MixinResolver and Pausable tests

56305e5

Allow empty blocks in solidity

7cdcbb6

Fixing the abstract contract tests

a498ed3

Fixing Proxy.setMessageSender issues with tests

de49955

Fixing final warning

929db0e

jjgonecrypto marked this pull request as ready for review April 18, 2020 18:58

jjgonecrypto requested review from jacko125, hav-noms and 0xclem April 18, 2020 18:59

Upgrading to buidler@1.3.0

f0f6f29

maxsam4 reviewed Apr 19, 2020

View reviewed changes

PR feedback

9c01a2a

maxsam4 approved these changes Apr 19, 2020

View reviewed changes

jjgonecrypto added 2 commits April 20, 2020 15:19

Switching to compile over deprecated compileStandardWrapper for solc

6f6f995

Merge branch 'develop' into upgrading-to-solidity-v5

4f10a48

hav-noms suggested changes Apr 21, 2020

View reviewed changes