Add a leading underscore to internal and private functions. (#1257)

* Add a leading underscore to internal and private functions.

Fixes #1176

* Remove super

* update the ERC721 changes

* add missing underscore after merge

* Fix mock

CODE_STYLE.md

@@ -43,8 +43,22 @@ Any exception or additions specific to our project are documented below.
 
     ```
     function test() {
-       uint256 functionVar;
-       ...
+      uint256 functionVar;
+      ...
+    }
+    ```
+
+* Internal and private functions should have an underscore prefix.
+
+    ```
+    function _testInternal() internal {
+      ...
+    }
+    ```
+
+    ```
+    function _testPrivate() private {
+      ...
     }
     ```
 

contracts/access/SignatureBouncer.sol

@@ -17,7 +17,7 @@ import "../cryptography/ECDSA.sol";
  * valid addresses on-chain, simply sign a grant of the form
  * keccak256(abi.encodePacked(`:contractAddress` + `:granteeAddress`)) using a valid bouncer address.
  * Then restrict access to your crowdsale/whitelist/airdrop using the
- * `onlyValidSignature` modifier (or implement your own using isValidSignature).
+ * `onlyValidSignature` modifier (or implement your own using _isValidSignature).
  * In addition to `onlyValidSignature`, `onlyValidSignatureAndMethod` and
  * `onlyValidSignatureAndData` can be used to restrict access to only a given method
  * or a given method with given parameters respectively.
@@ -42,7 +42,7 @@ contract SignatureBouncer is Ownable, RBAC {
    */
   modifier onlyValidSignature(bytes _signature)
   {
-    require(isValidSignature(msg.sender, _signature));
+    require(_isValidSignature(msg.sender, _signature));
     _;
   }
 
@@ -51,7 +51,7 @@ contract SignatureBouncer is Ownable, RBAC {
    */
   modifier onlyValidSignatureAndMethod(bytes _signature)
   {
-    require(isValidSignatureAndMethod(msg.sender, _signature));
+    require(_isValidSignatureAndMethod(msg.sender, _signature));
     _;
   }
 
@@ -60,7 +60,7 @@ contract SignatureBouncer is Ownable, RBAC {
    */
   modifier onlyValidSignatureAndData(bytes _signature)
   {
-    require(isValidSignatureAndData(msg.sender, _signature));
+    require(_isValidSignatureAndData(msg.sender, _signature));
     _;
   }
 
@@ -72,7 +72,7 @@ contract SignatureBouncer is Ownable, RBAC {
     onlyOwner
   {
     require(_bouncer != address(0));
-    addRole(_bouncer, ROLE_BOUNCER);
+    _addRole(_bouncer, ROLE_BOUNCER);
   }
 
   /**
@@ -82,19 +82,19 @@ contract SignatureBouncer is Ownable, RBAC {
     public
     onlyOwner
   {
-    removeRole(_bouncer, ROLE_BOUNCER);
+    _removeRole(_bouncer, ROLE_BOUNCER);
   }
 
   /**
    * @dev is the signature of `this + sender` from a bouncer?
    * @return bool
    */
-  function isValidSignature(address _address, bytes _signature)
+  function _isValidSignature(address _address, bytes _signature)
     internal
     view
     returns (bool)
   {
-    return isValidDataHash(
+    return _isValidDataHash(
       keccak256(abi.encodePacked(address(this), _address)),
       _signature
     );
@@ -104,7 +104,7 @@ contract SignatureBouncer is Ownable, RBAC {
    * @dev is the signature of `this + sender + methodId` from a bouncer?
    * @return bool
    */
-  function isValidSignatureAndMethod(address _address, bytes _signature)
+  function _isValidSignatureAndMethod(address _address, bytes _signature)
     internal
     view
     returns (bool)
@@ -113,7 +113,7 @@ contract SignatureBouncer is Ownable, RBAC {
     for (uint i = 0; i < data.length; i++) {
       data[i] = msg.data[i];
     }
-    return isValidDataHash(
+    return _isValidDataHash(
       keccak256(abi.encodePacked(address(this), _address, data)),
       _signature
     );
@@ -124,7 +124,7 @@ contract SignatureBouncer is Ownable, RBAC {
     * @notice the _signature parameter of the method being validated must be the "last" parameter
     * @return bool
     */
-  function isValidSignatureAndData(address _address, bytes _signature)
+  function _isValidSignatureAndData(address _address, bytes _signature)
     internal
     view
     returns (bool)
@@ -134,7 +134,7 @@ contract SignatureBouncer is Ownable, RBAC {
     for (uint i = 0; i < data.length; i++) {
       data[i] = msg.data[i];
     }
-    return isValidDataHash(
+    return _isValidDataHash(
       keccak256(abi.encodePacked(address(this), _address, data)),
       _signature
     );
@@ -145,7 +145,7 @@ contract SignatureBouncer is Ownable, RBAC {
    * and then recover the signature and check it against the bouncer role
    * @return bool
    */
-  function isValidDataHash(bytes32 _hash, bytes _signature)
+  function _isValidDataHash(bytes32 _hash, bytes _signature)
     internal
     view
     returns (bool)

contracts/access/Whitelist.sol

@@ -31,7 +31,7 @@ contract Whitelist is Ownable, RBAC {
     public
     onlyOwner
   {
-    addRole(_operator, ROLE_WHITELISTED);
+    _addRole(_operator, ROLE_WHITELISTED);
   }
 
   /**
@@ -70,7 +70,7 @@ contract Whitelist is Ownable, RBAC {
     public
     onlyOwner
   {
-    removeRole(_operator, ROLE_WHITELISTED);
+    _removeRole(_operator, ROLE_WHITELISTED);
   }
 
   /**

contracts/access/rbac/RBAC.sol

@@ -52,7 +52,7 @@ contract RBAC {
    * @param _operator address
    * @param _role the name of the role
    */
-  function addRole(address _operator, string _role)
+  function _addRole(address _operator, string _role)
     internal
   {
     roles[_role].add(_operator);
@@ -64,7 +64,7 @@ contract RBAC {
    * @param _operator address
    * @param _role the name of the role
    */
-  function removeRole(address _operator, string _role)
+  function _removeRole(address _operator, string _role)
     internal
   {
     roles[_role].remove(_operator);

contracts/bounties/BreakInvariantBounty.sol

@@ -27,7 +27,7 @@ contract BreakInvariantBounty is PullPayment, Ownable {
    * @return A target contract
    */
   function createTarget() public returns(Target) {
-    Target target = Target(deployContract());
+    Target target = Target(_deployContract());
     researchers[target] = msg.sender;
     emit TargetCreated(target);
     return target;
@@ -42,7 +42,7 @@ contract BreakInvariantBounty is PullPayment, Ownable {
     require(researcher != address(0));
     // Check Target contract invariants
     require(!_target.checkInvariant());
-    asyncTransfer(researcher, address(this).balance);
+    _asyncTransfer(researcher, address(this).balance);
     claimed = true;
   }
 
@@ -57,7 +57,7 @@ contract BreakInvariantBounty is PullPayment, Ownable {
    * @dev Internal function to deploy the target contract.
    * @return A target contract address
    */
-  function deployContract() internal returns(address);
+  function _deployContract() internal returns(address);
 
 }
 

contracts/crowdsale/distribution/FinalizableCrowdsale.sol

@@ -25,7 +25,7 @@ contract FinalizableCrowdsale is Ownable, TimedCrowdsale {
     require(!isFinalized);
     require(hasClosed());
 
-    finalization();
+    _finalization();
     emit CrowdsaleFinalized();
 
     isFinalized = true;
@@ -33,10 +33,10 @@ contract FinalizableCrowdsale is Ownable, TimedCrowdsale {
 
   /**
    * @dev Can be overridden to add finalization logic. The overriding function
-   * should call super.finalization() to ensure the chain of finalization is
+   * should call super._finalization() to ensure the chain of finalization is
    * executed entirely.
    */
-  function finalization() internal {
+  function _finalization() internal {
   }
 
 }

contracts/crowdsale/distribution/RefundableCrowdsale.sol

@@ -51,7 +51,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
   /**
    * @dev escrow finalization task, called when owner calls finalize()
    */
-  function finalization() internal {
+  function _finalization() internal {
     if (goalReached()) {
       escrow_.close();
       escrow_.beneficiaryWithdraw();
@@ -59,7 +59,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
       escrow_.enableRefunds();
     }
 
-    super.finalization();
+    super._finalization();
   }
 
   /**

contracts/examples/RBACWithAdmin.sol

@@ -37,7 +37,7 @@ contract RBACWithAdmin is RBAC {
   constructor()
     public
   {
-    addRole(msg.sender, ROLE_ADMIN);
+    _addRole(msg.sender, ROLE_ADMIN);
   }
 
   /**
@@ -49,7 +49,7 @@ contract RBACWithAdmin is RBAC {
     public
     onlyAdmin
   {
-    addRole(_account, _roleName);
+    _addRole(_account, _roleName);
   }
 
   /**
@@ -61,6 +61,6 @@ contract RBACWithAdmin is RBAC {
     public
     onlyAdmin
   {
-    removeRole(_account, _roleName);
+    _removeRole(_account, _roleName);
   }
 }

contracts/mocks/BouncerMock.sol

@@ -9,7 +9,7 @@ contract SignatureBouncerMock is SignatureBouncer {
     view
     returns (bool)
   {
-    return isValidSignature(_address, _signature);
+    return _isValidSignature(_address, _signature);
   }
 
   function onlyWithValidSignature(bytes _signature)
@@ -25,7 +25,7 @@ contract SignatureBouncerMock is SignatureBouncer {
     view
     returns (bool)
   {
-    return isValidSignatureAndMethod(_address, _signature);
+    return _isValidSignatureAndMethod(_address, _signature);
   }
 
   function onlyWithValidSignatureAndMethod(bytes _signature)
@@ -46,7 +46,7 @@ contract SignatureBouncerMock is SignatureBouncer {
     view
     returns (bool)
   {
-    return isValidSignatureAndData(_address, _signature);
+    return _isValidSignatureAndData(_address, _signature);
   }
 
   function onlyWithValidSignatureAndData(uint, bytes _signature)

contracts/mocks/ERC721Mock.sol

@@ -14,22 +14,22 @@ contract ERC721Mock is ERC721 {
   { }
 
   function mint(address _to, uint256 _tokenId) public {
-    super._mint(_to, _tokenId);
+    _mint(_to, _tokenId);
   }
 
   function burn(uint256 _tokenId) public {
-    super._burn(ownerOf(_tokenId), _tokenId);
+    _burn(ownerOf(_tokenId), _tokenId);
   }
 
   function exists(uint256 _tokenId) public view returns (bool) {
-    return super._exists(_tokenId);
+    return _exists(_tokenId);
   }
 
   function setTokenURI(uint256 _tokenId, string _uri) public {
-    super._setTokenURI(_tokenId, _uri);
+    _setTokenURI(_tokenId, _uri);
   }
 
-  function _removeTokenFrom(address _from, uint256 _tokenId) public {
-    super.removeTokenFrom(_from, _tokenId);
+  function removeTokenFrom(address _from, uint256 _tokenId) public {
+    _removeTokenFrom(_from, _tokenId);
   }
 }

contracts/mocks/InsecureInvariantTargetBounty.sol

@@ -14,7 +14,7 @@ contract InsecureInvariantTargetMock is Target {
 
 
 contract InsecureInvariantTargetBounty is BreakInvariantBounty {
-  function deployContract() internal returns (address) {
+  function _deployContract() internal returns (address) {
     return new InsecureInvariantTargetMock();
   }
 }

contracts/mocks/PullPaymentMock.sol

@@ -11,7 +11,7 @@ contract PullPaymentMock is PullPayment {
 
   // test helper function to call asyncTransfer
   function callTransfer(address _dest, uint256 _amount) public {
-    asyncTransfer(_dest, _amount);
+    _asyncTransfer(_dest, _amount);
   }
 
 }

contracts/mocks/RBACMock.sol

@@ -19,10 +19,10 @@ contract RBACMock is RBACWithAdmin {
   constructor(address[] _advisors)
     public
   {
-    addRole(msg.sender, ROLE_ADVISOR);
+    _addRole(msg.sender, ROLE_ADVISOR);
 
     for (uint256 i = 0; i < _advisors.length; i++) {
-      addRole(_advisors[i], ROLE_ADVISOR);
+      _addRole(_advisors[i], ROLE_ADVISOR);
     }
   }
 
@@ -64,6 +64,6 @@ contract RBACMock is RBACWithAdmin {
     checkRole(_account, ROLE_ADVISOR);
 
     // remove the advisor's role
-    removeRole(_account, ROLE_ADVISOR);
+    _removeRole(_account, ROLE_ADVISOR);
   }
 }

contracts/mocks/SecureInvariantTargetBounty.sol

@@ -14,7 +14,7 @@ contract SecureInvariantTargetMock is Target {
 
 
 contract SecureInvariantTargetBounty is BreakInvariantBounty {
-  function deployContract() internal returns (address) {
+  function _deployContract() internal returns (address) {
     return new SecureInvariantTargetMock();
   }
 }

contracts/ownership/Heritable.sol

@@ -86,7 +86,7 @@ contract Heritable is Ownable {
    * have to wait for `heartbeatTimeout` seconds.
    */
   function proclaimDeath() public onlyHeir {
-    require(ownerLives());
+    require(_ownerLives());
     emit OwnerProclaimedDead(owner, heir_, timeOfDeath_);
     // solium-disable-next-line security/no-block-members
     timeOfDeath_ = block.timestamp;
@@ -104,7 +104,7 @@ contract Heritable is Ownable {
    * @dev Allows heir to transfer ownership only if heartbeat has timed out.
    */
   function claimHeirOwnership() public onlyHeir {
-    require(!ownerLives());
+    require(!_ownerLives());
     // solium-disable-next-line security/no-block-members
     require(block.timestamp >= timeOfDeath_ + heartbeatTimeout_);
     emit OwnershipTransferred(owner, heir_);
@@ -113,7 +113,7 @@ contract Heritable is Ownable {
     timeOfDeath_ = 0;
   }
 
-  function ownerLives() internal view returns (bool) {
+  function _ownerLives() internal view returns (bool) {
     return timeOfDeath_ == 0;
   }
 }

contracts/ownership/Superuser.sol

@@ -15,7 +15,7 @@ contract Superuser is Ownable, RBAC {
   string public constant ROLE_SUPERUSER = "superuser";
 
   constructor () public {
-    addRole(msg.sender, ROLE_SUPERUSER);
+    _addRole(msg.sender, ROLE_SUPERUSER);
   }
 
   /**
@@ -48,8 +48,8 @@ contract Superuser is Ownable, RBAC {
    */
   function transferSuperuser(address _newSuperuser) public onlySuperuser {
     require(_newSuperuser != address(0));
-    removeRole(msg.sender, ROLE_SUPERUSER);
-    addRole(_newSuperuser, ROLE_SUPERUSER);
+    _removeRole(msg.sender, ROLE_SUPERUSER);
+    _addRole(_newSuperuser, ROLE_SUPERUSER);
   }
 
   /**

contracts/payment/PullPayment.sol

@@ -6,7 +6,7 @@ import "./Escrow.sol";
 /**
  * @title PullPayment
  * @dev Base contract supporting async send for pull payments. Inherit from this
- * contract and use asyncTransfer instead of send or transfer.
+ * contract and use _asyncTransfer instead of send or transfer.
  */
 contract PullPayment {
   Escrow private escrow;
@@ -36,7 +36,7 @@ contract PullPayment {
   * @param _dest The destination address of the funds.
   * @param _amount The amount to transfer.
   */
-  function asyncTransfer(address _dest, uint256 _amount) internal {
+  function _asyncTransfer(address _dest, uint256 _amount) internal {
     escrow.deposit.value(_amount)(_dest);
   }
 }

contracts/payment/SplitPayment.sol

@@ -26,7 +26,7 @@ contract SplitPayment {
     require(_payees.length > 0);
 
     for (uint256 i = 0; i < _payees.length; i++) {
-      addPayee(_payees[i], _shares[i]);
+      _addPayee(_payees[i], _shares[i]);
     }
   }
 
@@ -64,7 +64,7 @@ contract SplitPayment {
    * @param _payee The address of the payee to add.
    * @param _shares The number of shares owned by the payee.
    */
-  function addPayee(address _payee, uint256 _shares) internal {
+  function _addPayee(address _payee, uint256 _shares) internal {
     require(_payee != address(0));
     require(_shares > 0);
     require(shares[_payee] == 0);

contracts/token/ERC20/RBACMintableToken.sol

@@ -28,7 +28,7 @@ contract RBACMintableToken is ERC20Mintable, RBAC {
    * @param _minter address
    */
   function addMinter(address _minter) public onlyOwner {
-    addRole(_minter, ROLE_MINTER);
+    _addRole(_minter, ROLE_MINTER);
   }
 
   /**
@@ -36,6 +36,6 @@ contract RBACMintableToken is ERC20Mintable, RBAC {
    * @param _minter address
    */
   function removeMinter(address _minter) public onlyOwner {
-    removeRole(_minter, ROLE_MINTER);
+    _removeRole(_minter, ROLE_MINTER);
   }
 }

contracts/token/ERC721/ERC721.sol

@@ -125,8 +125,8 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 {
    * @param _to address representing the new owner of the given token ID
    * @param _tokenId uint256 ID of the token to be added to the tokens list of the given address
    */
-  function addTokenTo(address _to, uint256 _tokenId) internal {
-    super.addTokenTo(_to, _tokenId);
+  function _addTokenTo(address _to, uint256 _tokenId) internal {
+    super._addTokenTo(_to, _tokenId);
     uint256 length = ownedTokens[_to].length;
     ownedTokens[_to].push(_tokenId);
     ownedTokensIndex[_tokenId] = length;
@@ -137,8 +137,8 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 {
    * @param _from address representing the previous owner of the given token ID
    * @param _tokenId uint256 ID of the token to be removed from the tokens list of the given address
    */
-  function removeTokenFrom(address _from, uint256 _tokenId) internal {
-    super.removeTokenFrom(_from, _tokenId);
+  function _removeTokenFrom(address _from, uint256 _tokenId) internal {
+    super._removeTokenFrom(_from, _tokenId);
 
     // To prevent a gap in the array, we store the last token in the index of the token to delete, and
     // then delete the last slot.

contracts/token/ERC721/ERC721Basic.sol

@@ -130,12 +130,12 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic {
   )
     public
   {
-    require(isApprovedOrOwner(msg.sender, _tokenId));
+    require(_isApprovedOrOwner(msg.sender, _tokenId));
     require(_to != address(0));
 
-    clearApproval(_from, _tokenId);
-    removeTokenFrom(_from, _tokenId);
-    addTokenTo(_to, _tokenId);
+    _clearApproval(_from, _tokenId);
+    _removeTokenFrom(_from, _tokenId);
+    _addTokenTo(_to, _tokenId);
 
     emit Transfer(_from, _to, _tokenId);
   }
@@ -185,7 +185,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic {
   {
     transferFrom(_from, _to, _tokenId);
     // solium-disable-next-line arg-overflow
-    require(checkAndCallSafeTransfer(_from, _to, _tokenId, _data));
+    require(_checkAndCallSafeTransfer(_from, _to, _tokenId, _data));
   }
 
   /**
@@ -205,7 +205,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic {
    * @return bool whether the msg.sender is approved for the given token ID,
    *  is an operator of the owner, or is the owner of the token
    */
-  function isApprovedOrOwner(
+  function _isApprovedOrOwner(
     address _spender,
     uint256 _tokenId
   )
@@ -232,7 +232,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic {
    */
   function _mint(address _to, uint256 _tokenId) internal {
     require(_to != address(0));
-    addTokenTo(_to, _tokenId);
+    _addTokenTo(_to, _tokenId);
     emit Transfer(address(0), _to, _tokenId);
   }
 
@@ -242,8 +242,8 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic {
    * @param _tokenId uint256 ID of the token being burned by the msg.sender
    */
   function _burn(address _owner, uint256 _tokenId) internal {
-    clearApproval(_owner, _tokenId);
-    removeTokenFrom(_owner, _tokenId);
+    _clearApproval(_owner, _tokenId);
+    _removeTokenFrom(_owner, _tokenId);
     emit Transfer(_owner, address(0), _tokenId);
   }
 
@@ -253,7 +253,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic {
    * @param _owner owner of the token
    * @param _tokenId uint256 ID of the token to be transferred
    */
-  function clearApproval(address _owner, uint256 _tokenId) internal {
+  function _clearApproval(address _owner, uint256 _tokenId) internal {
     require(ownerOf(_tokenId) == _owner);
     if (tokenApprovals[_tokenId] != address(0)) {
       tokenApprovals[_tokenId] = address(0);
@@ -265,7 +265,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic {
    * @param _to address representing the new owner of the given token ID
    * @param _tokenId uint256 ID of the token to be added to the tokens list of the given address
    */
-  function addTokenTo(address _to, uint256 _tokenId) internal {
+  function _addTokenTo(address _to, uint256 _tokenId) internal {
     require(tokenOwner[_tokenId] == address(0));
     tokenOwner[_tokenId] = _to;
     ownedTokensCount[_to] = ownedTokensCount[_to].add(1);
@@ -276,7 +276,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic {
    * @param _from address representing the previous owner of the given token ID
    * @param _tokenId uint256 ID of the token to be removed from the tokens list of the given address
    */
-  function removeTokenFrom(address _from, uint256 _tokenId) internal {
+  function _removeTokenFrom(address _from, uint256 _tokenId) internal {
     require(ownerOf(_tokenId) == _from);
     ownedTokensCount[_from] = ownedTokensCount[_from].sub(1);
     tokenOwner[_tokenId] = address(0);
@@ -291,7 +291,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic {
    * @param _data bytes optional data to send along with the call
    * @return whether the call correctly returned the expected magic value
    */
-  function checkAndCallSafeTransfer(
+  function _checkAndCallSafeTransfer(
     address _from,
     address _to,
     uint256 _tokenId,

test/token/ERC721/ERC721.test.js

@@ -76,13 +76,13 @@ contract('ERC721', function (accounts) {
     describe('removeTokenFrom', function () {
       it('reverts if the correct owner is not passed', async function () {
         await assertRevert(
-          this.token._removeTokenFrom(anyone, firstTokenId, { from: creator })
+          this.token.removeTokenFrom(anyone, firstTokenId, { from: creator })
         );
       });
 
       context('once removed', function () {
         beforeEach(async function () {
-          await this.token._removeTokenFrom(creator, firstTokenId, { from: creator });
+          await this.token.removeTokenFrom(creator, firstTokenId, { from: creator });
         });
 
         it('has been removed', async function () {