Update after review coments I

Author: kronosapiens

contracts/extensions/VotingBase.sol

@@ -28,6 +28,7 @@ import "./ColonyExtension.sol";
 abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
 
   // Events
+
   event MotionCreated(uint256 indexed motionId, address creator, uint256 indexed domainId);
   event MotionStaked(uint256 indexed motionId, address indexed staker, uint256 indexed vote, uint256 amount);
   event MotionVoteSubmitted(uint256 indexed motionId, address indexed voter);
@@ -38,6 +39,7 @@ abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
   event MotionEventSet(uint256 indexed motionId, uint256 eventIndex);
 
   // Constants
+
   uint256 constant UINT128_MAX = 2**128 - 1;
 
   uint256 constant NAY = 0;
@@ -62,7 +64,29 @@ abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
 
   enum ExtensionState { Deployed, Active, Deprecated }
 
-  // Initialization data
+  // Data structures
+
+  enum MotionState { Null, Staking, Submit, Reveal, Closed, Finalizable, Finalized, Failed }
+
+  struct Motion {
+    uint64[3] events; // For recording motion lifecycle timestamps (STAKE, SUBMIT, REVEAL)
+    bytes32 rootHash;
+    uint256 domainId;
+    uint256 skillId;
+    uint256 paidVoterComp;
+    uint256[2] pastVoterComp; // [nay, yay]
+    uint256[2] stakes; // [nay, yay]
+    uint256[2][] votes; // [nay, yay]
+    uint256[] totalVotes;
+    uint256[] maxVotes;
+    bool escalated;
+    bool finalized;
+    address altTarget;
+    bytes action;
+  }
+
+  // Storage variables
+
   ExtensionState state;
 
   IColonyNetwork colonyNetwork;
@@ -90,6 +114,14 @@ abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
   uint256 revealPeriod; // Length of time for revealing votes
   uint256 escalationPeriod; // Length of time for escalating after a vote
 
+  uint256 motionCount;
+  mapping (uint256 => Motion) motions;
+  mapping (uint256 => mapping (address => mapping (uint256 => uint256))) stakes;
+  mapping (uint256 => mapping (address => bytes32)) voteSecrets;
+
+  mapping (bytes32 => uint256) expenditurePastVotes; // expenditure slot signature => voting power
+  mapping (bytes32 => uint256) expenditureMotionCounts; // expenditure struct signature => count
+
   // Modifiers
 
   modifier onlyRoot() {
@@ -102,7 +134,13 @@ abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
     _;
   }
 
-  // Public
+  // Virtual functions
+
+  function getInfluence(uint256 _motionId, address _user) public view virtual returns (uint256[] memory);
+  function postReveal(uint256 _motionId, address _user) internal virtual;
+  function postClaim(uint256 _motionId, address _user) internal virtual;
+
+  // Public functions
 
   /// @notice Install the extension
   /// @param _colony Base colony for the installation
@@ -115,6 +153,19 @@ abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
     token = colony.getToken();
   }
 
+  /// @notice Called when upgrading the extension
+  function finishUpgrade() public override auth {} // solhint-disable-line no-empty-blocks
+
+  /// @notice Called when deprecating (or undeprecating) the extension
+  function deprecate(bool _deprecated) public override auth {
+    deprecated = _deprecated;
+  }
+
+  /// @notice Called when uninstalling the extension
+  function uninstall() public override auth {
+    selfdestruct(address(uint160(address(colony))));
+  }
+
   /// @notice Initialise the extension
   /// @param _totalStakeFraction The fraction of the domain's reputation we need to stake
   /// @param _userMinStakeFraction The minimum per-user stake as fraction of total stake
@@ -166,56 +217,6 @@ abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
     emit ExtensionInitialised();
   }
 
-  /// @notice Called when upgrading the extension
-  function finishUpgrade() public override auth {} // solhint-disable-line no-empty-blocks
-
-  /// @notice Called when deprecating (or undeprecating) the extension
-  function deprecate(bool _deprecated) public override auth {
-    deprecated = _deprecated;
-  }
-
-  /// @notice Called when uninstalling the extension
-  function uninstall() public override auth {
-    selfdestruct(address(uint160(address(colony))));
-  }
-
-  // Data structures
-  enum MotionState { Null, Staking, Submit, Reveal, Closed, Finalizable, Finalized, Failed }
-
-  struct Motion {
-    uint64[3] events; // For recording motion lifecycle timestamps (STAKE, SUBMIT, REVEAL)
-    bytes32 rootHash;
-    uint256 domainId;
-    uint256 skillId;
-    uint256 paidVoterComp;
-    uint256[2] pastVoterComp; // [nay, yay]
-    uint256[2] stakes; // [nay, yay]
-    uint256[2][] votes; // [nay, yay]
-    uint256[] totalVotes;
-    uint256[] maxVotes;
-    bool escalated;
-    bool finalized;
-    address altTarget;
-    bytes action;
-  }
-
-  // Storage
-  uint256 motionCount;
-  mapping (uint256 => Motion) motions;
-  mapping (uint256 => mapping (address => mapping (uint256 => uint256))) stakes;
-  mapping (uint256 => mapping (address => bytes32)) voteSecrets;
-
-  mapping (bytes32 => uint256) expenditurePastVotes; // expenditure slot signature => voting power
-  mapping (bytes32 => uint256) expenditureMotionCounts; // expenditure struct signature => count
-
-  // Virtual functions
-
-  function getInfluence(uint256 _motionId, address _user) public view virtual returns (uint256[] memory);
-  function postReveal(uint256 _motionId, address _user) internal virtual;
-  function postClaim(uint256 _motionId, address _user) internal virtual;
-
-  // Public functions (interface)
-
   /// @notice Reveal a vote secret for a motion
   /// @param _motionId The id of the motion
   /// @param _salt The salt used to hash the vote
@@ -526,8 +527,9 @@ abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
     uint256 fractionUserInfluence;
 
     for (uint256 i; i < influence.length; i++) {
-      // TODO: Divide-by-zero ?
-      fractionUserInfluence = add(fractionUserInfluence, wdiv(influence[i], motion.totalVotes[i]));
+      if (motion.totalVotes[i] > 0) {
+        fractionUserInfluence = add(fractionUserInfluence, wdiv(influence[i], motion.totalVotes[i]));
+      }
     }
 
     fractionUserInfluence = fractionUserInfluence / influence.length;
@@ -559,9 +561,13 @@ abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
       // If user hasn't voted, add their influence to totalVotes
       uint256 pendingVote = (voteSecrets[_motionId][_user] == bytes32(0)) ? influence[i] : 0;
 
-      // TODO: Divide-by-zero ?
-      minFractionUserInfluence = add(minFractionUserInfluence, wdiv(influence[i], motion.maxVotes[i]));
-      maxFractionUserInfluence = add(maxFractionUserInfluence, wdiv(influence[i], add(motion.totalVotes[i], pendingVote)));
+      if (motion.maxVotes[i] > 0) {
+        minFractionUserInfluence = add(minFractionUserInfluence, wdiv(influence[i], motion.maxVotes[i]));
+      }
+
+      if (add(motion.totalVotes[i], pendingVote) > 0) {
+        maxFractionUserInfluence = add(maxFractionUserInfluence, wdiv(influence[i], add(motion.totalVotes[i], pendingVote)));
+      }
     }
 
     minFractionUserInfluence = minFractionUserInfluence / influence.length;
@@ -599,13 +605,13 @@ abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
     uint256 stakerReward;
     uint256 repPenalty;
 
-    uint256 sumVotes;
+    bool wasVote;
 
-    for (uint256 i; i < motion.votes.length; i++) {
-      sumVotes = add(sumVotes, add(motion.votes[i][NAY], motion.votes[i][YAY]));
+    for (uint256 i; i < motion.votes.length && !wasVote; i++) {
+      wasVote = add(motion.votes[i][NAY], motion.votes[i][YAY]) > 0;
     }
 
-    if (sumVotes > 0) {
+    if (wasVote) {
       // Went to a vote, use vote to determine reward or penalty
       (stakerReward, repPenalty) = getStakerRewardByVote(_motionId, _vote, stakeFraction, realStake);
     } else {
@@ -831,8 +837,9 @@ abstract contract VotingBase is ColonyExtension, PatriciaTreeProofs {
     for (uint256 i; i < motion.votes.length; i++) {
       yayWon = yayWon && motion.votes[i][NAY] < motion.votes[i][YAY];
 
-      // TODO: divide-by-zero ??
-      winFraction = add(winFraction, wdiv(motion.votes[i][_vote], motion.totalVotes[i]));
+      if (motion.totalVotes[i] > 0) {
+        winFraction = add(winFraction, wdiv(motion.votes[i][_vote], motion.totalVotes[i]));
+      }
     }
 
     winFraction = winFraction / motion.votes.length;

contracts/extensions/VotingReputation.sol

@@ -36,8 +36,8 @@ contract VotingReputation is VotingBase {
     return 1;
   }
 
-  // [motionId][skillId][user] => reputationBalance
-  mapping (uint256 => mapping (uint256 => mapping (address => uint256[]))) influences;
+  // [rootHash][skillId][user] => reputationBalance
+  mapping (bytes32 => mapping (uint256 => mapping (address => uint256[]))) influences;
 
   // Public
 
@@ -135,11 +135,11 @@ contract VotingReputation is VotingBase {
     public
     motionExists(_motionId)
   {
-    uint256 skillId = motions[_motionId].skillId;
-    if (influences[_motionId][skillId][msg.sender].length == 0) {
+    Motion storage motion = motions[_motionId];
+    if (influences[motion.rootHash][motion.skillId][msg.sender].length == 0) {
       uint256 userRep = getReputationFromProof(_motionId, msg.sender, _key, _value, _branchMask, _siblings);
-      influences[_motionId][skillId][msg.sender] = new uint256[](NUM_INFLUENCES);
-      influences[_motionId][skillId][msg.sender][0] = userRep;
+      influences[motion.rootHash][motion.skillId][msg.sender] = new uint256[](NUM_INFLUENCES);
+      influences[motion.rootHash][motion.skillId][msg.sender][0] = userRep;
     }
   }
 
@@ -152,7 +152,8 @@ contract VotingReputation is VotingBase {
     override
     returns (uint256[] memory influence)
   {
-    influence = influences[_motionId][motions[_motionId].skillId][_user];
+    Motion storage motion = motions[_motionId];
+    influence = influences[motion.rootHash][motion.skillId][_user];
   }
 
   function postReveal(uint256 _motionId, address _user) internal override {}