Improve encapsulation on lifecycle, ownership and payments (#1269)

* Improve encapsulation on Pausable

* add the underscore

* Improve encapsulation on ownership

* fix rebase

* fix ownership

* Improve encapsulation on payments

* Add missing tests

* add missing test

* Do not prefix getters

* Fix tests.

* revert pending owner reset

* add missing underscore

* Add missing underscore

contracts/bounties/BreakInvariantBounty.sol

@@ -58,7 +58,7 @@ contract BreakInvariantBounty is PullPayment, Ownable {
    * @dev Transfers the current balance to the owner and terminates the contract.
    */
   function destroy() public onlyOwner {
-    selfdestruct(owner);
+    selfdestruct(owner());
   }
 
   /**

contracts/drafts/TokenVesting.sol

@@ -93,7 +93,7 @@ contract TokenVesting is Ownable {
 
     revoked[_token] = true;
 
-    _token.safeTransfer(owner, refund);
+    _token.safeTransfer(owner(), refund);
 
     emit Revoked();
   }

contracts/lifecycle/Pausable.sol

@@ -12,14 +12,21 @@ contract Pausable is Ownable {
   event Paused();
   event Unpaused();
 
-  bool public paused = false;
+  bool private paused_ = false;
 
 
+  /**
+   * @return true if the contract is paused, false otherwise.
+   */
+  function paused() public view returns(bool) {
+    return paused_;
+  }
+
   /**
    * @dev Modifier to make a function callable only when the contract is not paused.
    */
   modifier whenNotPaused() {
-    require(!paused);
+    require(!paused_);
     _;
   }
 
@@ -27,7 +34,7 @@ contract Pausable is Ownable {
    * @dev Modifier to make a function callable only when the contract is paused.
    */
   modifier whenPaused() {
-    require(paused);
+    require(paused_);
     _;
   }
 
@@ -35,7 +42,7 @@ contract Pausable is Ownable {
    * @dev called by the owner to pause, triggers stopped state
    */
   function pause() public onlyOwner whenNotPaused {
-    paused = true;
+    paused_ = true;
     emit Paused();
   }
 
@@ -43,7 +50,7 @@ contract Pausable is Ownable {
    * @dev called by the owner to unpause, returns to normal state
    */
   function unpause() public onlyOwner whenPaused {
-    paused = false;
+    paused_ = false;
     emit Unpaused();
   }
 }

contracts/ownership/CanReclaimToken.sol

@@ -20,7 +20,7 @@ contract CanReclaimToken is Ownable {
    */
   function reclaimToken(IERC20 _token) external onlyOwner {
     uint256 balance = _token.balanceOf(this);
-    _token.safeTransfer(owner, balance);
+    _token.safeTransfer(owner(), balance);
   }
 
 }

contracts/ownership/Ownable.sol

@@ -7,7 +7,7 @@ pragma solidity ^0.4.24;
  * functions, this simplifies the implementation of "user permissions".
  */
 contract Ownable {
-  address public owner;
+  address private owner_;
 
 
   event OwnershipRenounced(address indexed previousOwner);
@@ -22,14 +22,21 @@ contract Ownable {
    * account.
    */
   constructor() public {
-    owner = msg.sender;
+    owner_ = msg.sender;
+  }
+
+  /**
+   * @return the address of the owner.
+   */
+  function owner() public view returns(address) {
+    return owner_;
   }
 
   /**
    * @dev Throws if called by any account other than the owner.
    */
   modifier onlyOwner() {
-    require(msg.sender == owner);
+    require(msg.sender == owner_);
     _;
   }
 
@@ -40,8 +47,8 @@ contract Ownable {
    * modifier anymore.
    */
   function renounceOwnership() public onlyOwner {
-    emit OwnershipRenounced(owner);
+    emit OwnershipRenounced(owner_);
-    owner = address(0);
+    owner_ = address(0);
   }
 
   /**
@@ -58,7 +65,7 @@ contract Ownable {
    */
   function _transferOwnership(address _newOwner) internal {
     require(_newOwner != address(0));
-    emit OwnershipTransferred(owner, _newOwner);
+    emit OwnershipTransferred(owner_, _newOwner);
-    owner = _newOwner;
+    owner_ = _newOwner;
   }
 }

contracts/ownership/Superuser.sol

@@ -27,7 +27,7 @@ contract Superuser is Ownable, RBAC {
   }
 
   modifier onlyOwnerOrSuperuser() {
-    require(msg.sender == owner || isSuperuser(msg.sender));
+    require(msg.sender == owner() || isSuperuser(msg.sender));
     _;
   }
 

contracts/payment/RefundEscrow.sol

@@ -16,8 +16,8 @@ contract RefundEscrow is Ownable, ConditionalEscrow {
   event Closed();
   event RefundsEnabled();
 
-  State public state;
+  State private state_;
-  address public beneficiary;
+  address private beneficiary_;
 
   /**
    * @dev Constructor.
@@ -25,8 +25,22 @@ contract RefundEscrow is Ownable, ConditionalEscrow {
    */
   constructor(address _beneficiary) public {
     require(_beneficiary != address(0));
-    beneficiary = _beneficiary;
+    beneficiary_ = _beneficiary;
-    state = State.Active;
+    state_ = State.Active;
+  }
+
+  /**
+   * @return the current state of the escrow.
+   */
+  function state() public view returns(State) {
+    return state_;
+  }
+
+  /**
+   * @return the beneficiary of the escrow.
+   */
+  function beneficiary() public view returns(address) {
+    return beneficiary_;
   }
 
   /**
@@ -34,7 +48,7 @@ contract RefundEscrow is Ownable, ConditionalEscrow {
    * @param _refundee The address funds will be sent to if a refund occurs.
    */
   function deposit(address _refundee) public payable {
-    require(state == State.Active);
+    require(state_ == State.Active);
     super.deposit(_refundee);
   }
 
@@ -43,8 +57,8 @@ contract RefundEscrow is Ownable, ConditionalEscrow {
    * further deposits.
    */
   function close() public onlyOwner {
-    require(state == State.Active);
+    require(state_ == State.Active);
-    state = State.Closed;
+    state_ = State.Closed;
     emit Closed();
   }
 
@@ -52,8 +66,8 @@ contract RefundEscrow is Ownable, ConditionalEscrow {
    * @dev Allows for refunds to take place, rejecting further deposits.
    */
   function enableRefunds() public onlyOwner {
-    require(state == State.Active);
+    require(state_ == State.Active);
-    state = State.Refunding;
+    state_ = State.Refunding;
     emit RefundsEnabled();
   }
 
@@ -61,14 +75,14 @@ contract RefundEscrow is Ownable, ConditionalEscrow {
    * @dev Withdraws the beneficiary's funds.
    */
   function beneficiaryWithdraw() public {
-    require(state == State.Closed);
+    require(state_ == State.Closed);
-    beneficiary.transfer(address(this).balance);
+    beneficiary_.transfer(address(this).balance);
   }
 
   /**
    * @dev Returns whether refundees can withdraw their deposits (be refunded).
    */
   function withdrawalAllowed(address _payee) public view returns (bool) {
-    return state == State.Refunding;
+    return state_ == State.Refunding;
   }
 }

contracts/payment/SplitPayment.sol

@@ -11,12 +11,12 @@ import "../math/SafeMath.sol";
 contract SplitPayment {
   using SafeMath for uint256;
 
-  uint256 public totalShares = 0;
+  uint256 private totalShares_ = 0;
-  uint256 public totalReleased = 0;
+  uint256 private totalReleased_ = 0;
 
-  mapping(address => uint256) public shares;
+  mapping(address => uint256) private shares_;
-  mapping(address => uint256) public released;
+  mapping(address => uint256) private released_;
-  address[] public payees;
+  address[] private payees_;
 
   /**
    * @dev Constructor
@@ -35,25 +35,60 @@ contract SplitPayment {
    */
   function () external payable {}
 
+  /**
+   * @return the total shares of the contract.
+   */
+  function totalShares() public view returns(uint256) {
+    return totalShares_;
+  }
+
+  /**
+   * @return the total amount already released.
+   */
+  function totalReleased() public view returns(uint256) {
+    return totalReleased_;
+  }
+
+  /**
+   * @return the shares of an account.
+   */
+  function shares(address _account) public view returns(uint256) {
+    return shares_[_account];
+  }
+
+  /**
+   * @return the amount already released to an account.
+   */
+  function released(address _account) public view returns(uint256) {
+    return released_[_account];
+  }
+
+  /**
+   * @return the address of a payee.
+   */
+  function payee(uint256 index) public view returns(address) {
+    return payees_[index];
+  }
+
   /**
    * @dev Release one of the payee's proportional payment.
    * @param _payee Whose payments will be released.
    */
   function release(address _payee) public {
-    require(shares[_payee] > 0);
+    require(shares_[_payee] > 0);
 
-    uint256 totalReceived = address(this).balance.add(totalReleased);
+    uint256 totalReceived = address(this).balance.add(totalReleased_);
     uint256 payment = totalReceived.mul(
-      shares[_payee]).div(
+      shares_[_payee]).div(
-        totalShares).sub(
+        totalShares_).sub(
-          released[_payee]
+          released_[_payee]
     );
 
     require(payment != 0);
     assert(address(this).balance >= payment);
 
-    released[_payee] = released[_payee].add(payment);
+    released_[_payee] = released_[_payee].add(payment);
-    totalReleased = totalReleased.add(payment);
+    totalReleased_ = totalReleased_.add(payment);
 
     _payee.transfer(payment);
   }
@@ -66,10 +101,10 @@ contract SplitPayment {
   function _addPayee(address _payee, uint256 _shares) internal {
     require(_payee != address(0));
     require(_shares > 0);
-    require(shares[_payee] == 0);
+    require(shares_[_payee] == 0);
 
-    payees.push(_payee);
+    payees_.push(_payee);
-    shares[_payee] = _shares;
+    shares_[_payee] = _shares;
-    totalShares = totalShares.add(_shares);
+    totalShares_ = totalShares_.add(_shares);
   }
 }

contracts/token/ERC20/ERC20Mintable.sol

@@ -22,7 +22,7 @@ contract ERC20Mintable is ERC20, Ownable {
   }
 
   modifier hasMintPermission() {
-    require(msg.sender == owner);
+    require(msg.sender == owner());
     _;
   }
 

test/payment/RefundEscrow.test.js

@@ -28,6 +28,11 @@ contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2]
     });
 
     context('active state', function () {
+      it('has beneficiary and state', async function () {
+        (await this.escrow.beneficiary()).should.be.equal(beneficiary);
+        (await this.escrow.state()).should.be.bignumber.equal(0);
+      });
+
       it('accepts deposits', async function () {
         await this.escrow.deposit(refundee1, { from: owner, value: amount });
 

test/payment/SplitPayment.test.js

@@ -46,6 +46,17 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1,
       this.contract = await SplitPayment.new(this.payees, this.shares);
     });
 
+    it('should have total shares', async function () {
+      (await this.contract.totalShares()).should.be.bignumber.equal(20 + 10 + 70);
+    });
+
+    it('should have payees', async function () {
+      this.payees.forEach(async (payee, index) => {
+        (await this.payee(index)).should.be.equal(payee);
+        (await this.contract.released(payee)).should.be.bignumber.equal(0);
+      });
+    });
+
     it('should accept payments', async function () {
       await ethSendTransaction({ from: owner, to: this.contract.address, value: amount });
 
@@ -53,11 +64,11 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1,
     });
 
     it('should store shares if address is payee', async function () {
-      (await this.contract.shares.call(payee1)).should.be.bignumber.not.equal(0);
+      (await this.contract.shares(payee1)).should.be.bignumber.not.equal(0);
     });
 
     it('should not store shares if address is not payee', async function () {
-      (await this.contract.shares.call(nonpayee1)).should.be.bignumber.equal(0);
+      (await this.contract.shares(nonpayee1)).should.be.bignumber.equal(0);
     });
 
     it('should throw if no funds to claim', async function () {
@@ -96,7 +107,7 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1,
       (await ethGetBalance(this.contract.address)).should.be.bignumber.equal(0);
 
       // check correct funds released accounting
-      (await this.contract.totalReleased.call()).should.be.bignumber.equal(initBalance);
+      (await this.contract.totalReleased()).should.be.bignumber.equal(initBalance);
     });
   });
 });