Respond to review comments XII

contracts/colony/IColony.sol

@@ -123,7 +123,7 @@ contract IColony is ColonyDataTypes, IRecovery {
 
   /// @notice Gets the bytes32 representation of the roles for a user in a given domain
   /// @param _user The user whose roles we want to get
-  /// @param _domain The_domain domain where we want to get roles for
+  /// @param _domain The domain we want to get roles in
   /// @return roles bytes32 representation of the held roles
   function getUserRoles(address _user, uint256 _domain) public view returns (bytes32 roles);
 

contracts/extensions/VotingReputation.sol

@@ -186,7 +186,7 @@ contract VotingReputation is DSMath, PatriciaTreeProofs {
   mapping (uint256 => mapping (address => mapping (uint256 => uint256))) stakes;
   mapping (uint256 => mapping (address => bytes32)) voteSecrets;
 
-  mapping (bytes32 => uint256) expenditurePastMotions; // expenditure slot signature => voting power
+  mapping (bytes32 => uint256) expenditurePastVotes; // expenditure slot signature => voting power
   mapping (bytes32 => uint256) expenditureMotionCounts; // expenditure struct signature => count
 
   // Public functions (interface)
@@ -231,8 +231,9 @@ contract VotingReputation is DSMath, PatriciaTreeProofs {
   )
     public
   {
+    // Check the function requires a non-root permission (and thus a domain proof)
     require(
-      colony.getCapabilityRoles(getSig(_action)) & ~ROOT_ROLES != bytes32(0),
+      colony.getCapabilityRoles(getSig(_action)) | ROOT_ROLES != ROOT_ROLES,
       "voting-rep-invalid-function"
     );
 
@@ -504,12 +505,12 @@ contract VotingReputation is DSMath, PatriciaTreeProofs {
         executeCall(_motionId, claimDelayAction);
       }
 
-      bytes32 slotHash = hashExpenditureSlot(motion.action);
+      bytes32 actionHash = hashExpenditureAction(motion.action);
       uint256 votePower = (add(motion.votes[NAY], motion.votes[YAY]) > 0) ?
         motion.votes[YAY] : motion.stakes[YAY];
 
-      if (expenditurePastMotions[slotHash] < votePower) {
-        expenditurePastMotions[slotHash] = votePower;
+      if (expenditurePastVotes[actionHash] < votePower) {
+        expenditurePastVotes[actionHash] = votePower;
         canExecute = canExecute && true;
       } else {
         canExecute = canExecute && false;
@@ -639,18 +640,18 @@ contract VotingReputation is DSMath, PatriciaTreeProofs {
     return stakes[_motionId][_staker][_vote];
   }
 
-  /// @notice Get the number of ongoing motions for a single expenditure / slot
+  /// @notice Get the number of ongoing motions for a single expenditure / expenditure slot
   /// @param _structHash The hash of the expenditureId or expenditureId*expenditureSlot
   /// @return The number of ongoing motions
   function getExpenditureMotionCount(bytes32 _structHash) public view returns (uint256) {
     return expenditureMotionCounts[_structHash];
   }
 
   /// @notice Get the largest past vote on a single expenditure variable
-  /// @param _slotHash The hash of the particular expenditure slot
+  /// @param _actionHash The hash of the particular expenditure action
   /// @return The largest past vote on this variable
-  function getExpenditurePastMotion(bytes32 _slotHash) public view returns (uint256) {
-    return expenditurePastMotions[_slotHash];
+  function getExpenditurePastVote(bytes32 _actionHash) public view returns (uint256) {
+    return expenditurePastVotes[_actionHash];
   }
 
   /// @notice Get the current state of the motion
@@ -908,9 +909,10 @@ contract VotingReputation is DSMath, PatriciaTreeProofs {
     }
   }
 
-  function hashExpenditureSlot(bytes memory action) internal returns (bytes32 hash) {
+  function hashExpenditureAction(bytes memory action) internal returns (bytes32 hash) {
     assembly {
-      // Hash all but the domain proof and value
+      // Hash all but the domain proof and value, so actions for the same
+      //   storage slot return the same value.
       // Recall: mload(action) gives length of bytes array
       // So skip past the three bytes32 (length + domain proof),
       //   plus 4 bytes for the sig. Subtract the same from the end, less

test/extensions/voting-rep.js

@@ -508,7 +508,7 @@ contract("Voting Reputation", (accounts) => {
       await checkErrorRevert(colony.claimExpenditurePayout(expenditureId, 0, token.address), "colony-expenditure-cannot-claim");
     });
 
-    it("cannot update the expenditure globalClaimDelay if the target is another colony", async () => {
+    it("does not update the expenditure globalClaimDelay if the target is another colony", async () => {
       const { colony: otherColony } = await setupRandomColony(colonyNetwork);
       await otherColony.makeExpenditure(1, UINT256_MAX, 1);
       const expenditureId = await otherColony.getExpenditureCount();
@@ -940,12 +940,12 @@ contract("Voting Reputation", (accounts) => {
       );
     });
 
-    it("cannot reveal a vote if voting is open", async () => {
+    it("cannot reveal a vote during the submit period", async () => {
       await voting.submitVote(motionId, soliditySha3(SALT, NAY), user0Key, user0Value, user0Mask, user0Siblings, { from: USER0 });
       await checkErrorRevert(voting.revealVote(motionId, SALT, YAY, FAKE, FAKE, 0, [], { from: USER0 }), "voting-rep-motion-not-reveal");
     });
 
-    it("cannot reveal a vote after voting closes", async () => {
+    it("cannot reveal a vote after the reveal period ends", async () => {
       await voting.submitVote(motionId, soliditySha3(SALT, NAY), user0Key, user0Value, user0Mask, user0Siblings, { from: USER0 });
 
       await forwardTime(SUBMIT_PERIOD, this);
@@ -1210,8 +1210,8 @@ contract("Voting Reputation", (accounts) => {
 
       await voting.finalizeMotion(motionId);
       const slotHash = hashExpenditureSlot(action);
-      const pastMotion = await voting.getExpenditurePastMotion(slotHash);
-      expect(pastMotion).to.eq.BN(WAD); // USER0 had 1 WAD of reputation
+      const pastVote = await voting.getExpenditurePastVote(slotHash);
+      expect(pastVote).to.eq.BN(WAD); // USER0 had 1 WAD of reputation
     });
 
     it("can use vote power correctly for different values of the same variable", async () => {
@@ -1258,8 +1258,8 @@ contract("Voting Reputation", (accounts) => {
 
       await voting.finalizeMotion(motionId);
       const slotHash = hashExpenditureSlot(action);
-      const pastMotion = await voting.getExpenditurePastMotion(slotHash);
-      expect(pastMotion).to.eq.BN(REQUIRED_STAKE);
+      const pastVote = await voting.getExpenditurePastVote(slotHash);
+      expect(pastVote).to.eq.BN(REQUIRED_STAKE);
     });
   });