Improve encapsulation on payments

Author: Leo Arias

contracts/ownership/Heritable.sol

@@ -108,7 +108,7 @@ contract Heritable is Ownable {
     // solium-disable-next-line security/no-block-members
     require(block.timestamp >= timeOfDeath_ + heartbeatTimeout_);
     emit HeirOwnershipClaimed(getOwner(), heir_);
-    transferOwnership(heir_);
+    _transferOwnership(heir_);
     timeOfDeath_ = 0;
   }
 

contracts/payment/RefundEscrow.sol

@@ -16,25 +16,39 @@ contract RefundEscrow is Ownable, ConditionalEscrow {
   event Closed();
   event RefundsEnabled();
 
-  State public state;
-  address public beneficiary;
+  State private state_;
+  address private beneficiary_;
 
   /**
    * @dev Constructor.
    * @param _beneficiary The beneficiary of the deposits.
    */
   constructor(address _beneficiary) public {
     require(_beneficiary != address(0));
-    beneficiary = _beneficiary;
-    state = State.Active;
+    beneficiary_ = _beneficiary;
+    state_ = State.Active;
+  }
+
+  /**
+   * @return the current state of the escrow.
+   */
+  function getState() public view returns(State) {
+    return state_;
+  }
+
+  /**
+   * @return the beneficiary of the escrow.
+   */
+  function getBeneficiary() public view returns(address) {
+    return beneficiary_;
   }
 
   /**
    * @dev Stores funds that may later be refunded.
    * @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,32 +57,32 @@ contract RefundEscrow is Ownable, ConditionalEscrow {
    * further deposits.
    */
   function close() public onlyOwner {
-    require(state == State.Active);
-    state = State.Closed;
+    require(state_ == State.Active);
+    state_ = State.Closed;
     emit Closed();
   }
 
   /**
    * @dev Allows for refunds to take place, rejecting further deposits.
    */
   function enableRefunds() public onlyOwner {
-    require(state == State.Active);
-    state = State.Refunding;
+    require(state_ == State.Active);
+    state_ = State.Refunding;
     emit RefundsEnabled();
   }
 
   /**
    * @dev Withdraws the beneficiary's funds.
    */
   function beneficiaryWithdraw() public {
-    require(state == State.Closed);
-    beneficiary.transfer(address(this).balance);
+    require(state_ == State.Closed);
+    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 public totalReleased = 0;
+  uint256 private totalShares_ = 0;
+  uint256 private totalReleased_ = 0;
 
-  mapping(address => uint256) public shares;
-  mapping(address => uint256) public released;
-  address[] public payees;
+  mapping(address => uint256) private shares_;
+  mapping(address => uint256) private released_;
+  address[] private payees_;
 
   /**
    * @dev Constructor
@@ -35,26 +35,61 @@ contract SplitPayment {
    */
   function () external payable {}
 
+  /**
+   * @return the total shares of the contract.
+   */
+  function getTotalShares() public view returns(uint256) {
+    return totalShares_;
+  }
+
+  /**
+   * @return the total amount already released.
+   */
+  function getTotalReleased() public view returns(uint256) {
+    return totalReleased_;
+  }
+
+  /**
+   * @return the shares of an account.
+   */
+  function getShares(address _account) public view returns(uint256) {
+    return shares_[_account];
+  }
+
+  /**
+   * @return the amount already released to an account.
+   */
+  function getReleased(address _account) public view returns(uint256) {
+    return released_[_account];
+  }
+
+  /**
+   * @return the address of a payee.
+   */
+  function getPayee(uint256 index) public view returns(address) {
+    return payees_[index];
+  }
+
   /**
    * @dev Claim your share of the balance.
    */
   function claim() public {
     address payee = msg.sender;
 
-    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(
-        totalShares).sub(
-          released[payee]
+      shares_[payee]).div(
+        totalShares_).sub(
+          released_[payee]
     );
 
     require(payment != 0);
     assert(address(this).balance >= payment);
 
-    released[payee] = released[payee].add(payment);
-    totalReleased = totalReleased.add(payment);
+    released_[payee] = released_[payee].add(payment);
+    totalReleased_ = totalReleased_.add(payment);
 
     payee.transfer(payment);
   }
@@ -67,10 +102,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);
-    shares[_payee] = _shares;
-    totalShares = totalShares.add(_shares);
+    payees_.push(_payee);
+    shares_[_payee] = _shares;
+    totalShares_ = totalShares_.add(_shares);
   }
 }

test/payment/RefundEscrow.test.js

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

test/payment/SplitPayment.test.js

@@ -53,11 +53,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.getShares(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.getShares(nonpayee1)).should.be.bignumber.equal(0);
     });
 
     it('should throw if no funds to claim', async function () {
@@ -96,7 +96,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.getTotalReleased()).should.be.bignumber.equal(initBalance);
     });
   });
 });