update constructor syntax for solidity 0.4.23 in numerous contracts

[pemulis]

🚀 Description

This eliminates a lot of compiler warnings.

• 📘 I've reviewed the OpenZeppelin Contributor Guidelines
• ✅ I've added tests where applicable to test my new functionality.
• 📖 I've made sure that my contracts are well-documented.
• 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

(edit: the linter error is resolved!)
The linter throws an error (below) on all of the changed contracts at the constructor line, but I think the linter is wrong, not the constructor.

error Syntax error: unexpected token (

Closes #932

[pemulis]

I'm curious why tests are failing and wonder if this is an issue with Ganache.

8.82s$ npm run test
> openzeppelin-solidity@1.9.0 test /home/travis/build/OpenZeppelin/openzeppelin-solidity
> scripts/test.sh
Starting our own ganache instance
Error parsing /home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/DayLimit.sol: ParsedContract.sol:19:14: ParserError: Expected identifier, got 'LParen'
  constructor(uint256 _limit) public {
             ^
Compilation failed. See above.

[naveensrinivasan]

@pemulis The tests are failing because the solc being used is version Solidity v0.4.21 (solc-js) https://travis-ci.org/OpenZeppelin/openzeppelin-solidity/jobs/372851241#L2432

The new constructor keyword was release in 0.4.22 https://github.com/ethereum/solidity/releases/tag/v0.4.22.

I would recommend as part of the PR also update the tests dependecies which should fix this.
HTH

[pemulis]

Thanks @naveensrinivasan! I'll take care of that.

[pemulis]

It looks like the solidity-coverage ^0.5.0 includes solc ^0.4.23, but that's two releases with breaking changes beyond the current version in package.json. I'll revisit this when I have time, so that upgrading to the latest solidity-coverage takes the breaking changes into account.

solidity-coverage docs: https://github.com/sc-forks/solidity-coverage

[shrugs]

We'll want to update the entire OZ project to .22 about a month after it's been released (to allow it to be "in the wild" for a bit). are there any other changes we want to make in a "update to 0.4.22" pr, beyond constructor()?

[pemulis]

Looks like the tests are all working now, so the constructor() update is good to go when you want to update the OZ project in a couple of weeks.

[come-maiz]

Thanks a lot for updating this.
All the tests are passing, so +1 from me.

@shrugs do you think it's necessary to update to 0.4.22 before moving to 0.4.23?
Also, why don't we have a explicit devDependency on solc?

[adklempner]

@shrugs 0.4.22 adds error reason strings for revert and require, would love to see them in OZ

[frangio]

@ElOpio We don't because solc comes with Truffle.

[shrugs]

@pemulis Let's upgrade all of the contracts to ^0.4.23. Running a quick grep grep -r -P "pragma solidity \^0.4.(?:(?\!23).)" --exclude-dir node_modules --include=*.sol

shows the following contracts are still on older versions:

contracts/mocks/SafeMathMock.sol:pragma solidity ^0.4.21;
contracts/mocks/SecureTargetBounty.sol:pragma solidity ^0.4.21;
contracts/mocks/ReentrancyAttack.sol:pragma solidity ^0.4.21;
contracts/mocks/BouncerMock.sol:pragma solidity ^0.4.18;
contracts/mocks/MessageHelper.sol:pragma solidity ^0.4.21;
contracts/mocks/LimitBalanceMock.sol:pragma solidity ^0.4.21;
contracts/mocks/MerkleProofWrapper.sol:pragma solidity ^0.4.21;
contracts/mocks/InsecureTargetBounty.sol:pragma solidity ^0.4.21;
contracts/mocks/WhitelistMock.sol:pragma solidity ^0.4.21;
contracts/mocks/ECRecoveryMock.sol:pragma solidity ^0.4.21;
contracts/mocks/ERC721BasicTokenMock.sol:pragma solidity ^0.4.21;
contracts/mocks/MathMock.sol:pragma solidity ^0.4.21;
contracts/token/ERC827/ERC827Token.sol:pragma solidity ^0.4.21;
contracts/token/ERC827/ERC827.sol:pragma solidity ^0.4.21;
contracts/token/ERC20/StandardToken.sol:pragma solidity ^0.4.21;
contracts/token/ERC20/StandardBurnableToken.sol:pragma solidity ^0.4.18;
contracts/token/ERC20/ERC20.sol:pragma solidity ^0.4.21;
contracts/token/ERC20/SafeERC20.sol:pragma solidity ^0.4.21;
contracts/token/ERC20/PausableToken.sol:pragma solidity ^0.4.21;
contracts/token/ERC20/ERC20Basic.sol:pragma solidity ^0.4.21;
contracts/token/ERC20/BurnableToken.sol:pragma solidity ^0.4.21;
contracts/token/ERC20/BasicToken.sol:pragma solidity ^0.4.21;
contracts/token/ERC20/MintableToken.sol:pragma solidity ^0.4.21;
contracts/token/ERC721/ERC721Receiver.sol:pragma solidity ^0.4.21;
contracts/token/ERC721/ERC721.sol:pragma solidity ^0.4.21;
contracts/token/ERC721/ERC721BasicToken.sol:pragma solidity ^0.4.21;
contracts/token/ERC721/ERC721Holder.sol:pragma solidity ^0.4.21;
contracts/token/ERC721/DeprecatedERC721.sol:pragma solidity ^0.4.21;
contracts/token/ERC721/ERC721Basic.sol:pragma solidity ^0.4.21;
contracts/ReentrancyGuard.sol:pragma solidity ^0.4.21;
contracts/access/SignatureBouncer.sol:pragma solidity ^0.4.18;
contracts/payment/PullPayment.sol:pragma solidity ^0.4.21;
contracts/lifecycle/Pausable.sol:pragma solidity ^0.4.21;
contracts/crowdsale/emission/MintedCrowdsale.sol:pragma solidity ^0.4.21;
contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol:pragma solidity ^0.4.21;
contracts/crowdsale/distribution/FinalizableCrowdsale.sol:pragma solidity ^0.4.21;
contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol:pragma solidity ^0.4.21;
contracts/crowdsale/validation/WhitelistedCrowdsale.sol:pragma solidity ^0.4.21;
contracts/Bounty.sol:pragma solidity ^0.4.21;
contracts/math/SafeMath.sol:pragma solidity ^0.4.21;
contracts/math/Math.sol:pragma solidity ^0.4.21;
contracts/ownership/Claimable.sol:pragma solidity ^0.4.21;
contracts/ownership/rbac/Roles.sol:pragma solidity ^0.4.21;
contracts/ownership/rbac/RBAC.sol:pragma solidity ^0.4.21;
contracts/ownership/HasNoTokens.sol:pragma solidity ^0.4.21;
contracts/ownership/NoOwner.sol:pragma solidity ^0.4.21;
contracts/ownership/Contactable.sol:pragma solidity ^0.4.21;
contracts/ownership/DelayedClaimable.sol:pragma solidity ^0.4.21;
contracts/ownership/Whitelist.sol:pragma solidity ^0.4.21;
contracts/ownership/CanReclaimToken.sol:pragma solidity ^0.4.21;
contracts/ownership/HasNoContracts.sol:pragma solidity ^0.4.21;
contracts/ECRecovery.sol:pragma solidity ^0.4.21;
contracts/MerkleProof.sol:pragma solidity ^0.4.21;
contracts/AddressUtils.sol:pragma solidity ^0.4.21;

I also confirmed at all of the constructor() args were changed, so that part looks good to me.

[pemulis]

@shrugs done!