Remove redundant require statements #1409
Conversation
Now that SafeMath uses require, the require statements are redundant. They were also previously inconsistent because they were only included in some functions, but not others
|
I've been thinking about this one for the last couple days. The good thing about those This is of course not a great reason to keep them, specially considering we already have coverage for them, and they make the contracts more expensive for no reason, but wanted to get some thoughts on the matter. @cgewecke is there some way we could improve |
|
In general, I think relying on "line coverage" isn't the best way to make sure that every edge case is covered, and the tests will just have to be written in such a way that we are confident that there is proper test coverage rather than adding lines to solidity and making sure that require statements are hit both as true and false |
|
Fair enough, I'm fine with merging this. Thanks a lot @BrendanChou! I think there are some more scattered examples of this throughout the codebase, so we should also take care of those. |
|
Amazing! |
…lidity into feature/token-recover * 'master' of https://github.com/vittominacori/zeppelin-solidity: (98 commits) Renamed roles private variables to adhere to code style. (OpenZeppelin#1507) Remove extraneous quantity check, fixes OpenZeppelin#1454 (OpenZeppelin#1455) Remove redundant require statements (OpenZeppelin#1409) Add the step to delete the build dir to the RELEASE notes (OpenZeppelin#1467) add an address typecast to this per issue OpenZeppelin#1457 (OpenZeppelin#1471) add improvement in simpletoken example OpenZeppelin#1458 (OpenZeppelin#1473) SafeMath Test Coverage Improved (OpenZeppelin#1477) The beneficiary parameter of claimRefund is replaced with refundee (OpenZeppelin#1481) fix ERC20.sol#L174 and ERC20.sol#L187 should be casted to an address type. (OpenZeppelin#1470) Fix/add comment erc721 burnable OpenZeppelin#1464 (OpenZeppelin#1469) Release v2.0.0 Release candidate v2.0.0-rc.4 Improved some ERC721 internal shenanigans (OpenZeppelin#1450) Add warning about trading tokens before refundable crowdsale goal is met (OpenZeppelin#1452) Crowdsale.buyTokens is now nonReentrant. (OpenZeppelin#1438) InitialRate must be strictly larger than finalRate. (OpenZeppelin#1441) Fixed how allowance crowdsale checks remaining tokens. (OpenZeppelin#1449) Deleted unnecessary import. (OpenZeppelin#1437) Made SampleCrowdsale a bit clearer. (OpenZeppelin#1448) Now setting the finalized flag before doing finalization to prevent possbile reentrancy issues. (OpenZeppelin#1447) ...
Now that SafeMath uses require, the require statements are redundant. They were also previously inconsistent because they were only included in some functions, but not others