From fb736138e7d6f406d58c3b5e9be280d654887482 Mon Sep 17 00:00:00 2001 From: Daniel Kronovet Date: Fri, 4 Sep 2020 10:24:24 -0700 Subject: [PATCH] Respond to review comments XII --- contracts/colony/IColony.sol | 2 +- contracts/extensions/VotingReputation.sol | 24 ++++++++++++----------- test/extensions/voting-rep.js | 14 ++++++------- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/contracts/colony/IColony.sol b/contracts/colony/IColony.sol index 752159bb15..ce85de4209 100644 --- a/contracts/colony/IColony.sol +++ b/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); diff --git a/contracts/extensions/VotingReputation.sol b/contracts/extensions/VotingReputation.sol index 8e4114414c..cb5f77f168 100644 --- a/contracts/extensions/VotingReputation.sol +++ b/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,7 +640,7 @@ 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) { @@ -647,10 +648,10 @@ contract VotingReputation is DSMath, PatriciaTreeProofs { } /// @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 diff --git a/test/extensions/voting-rep.js b/test/extensions/voting-rep.js index b36b0deda7..9d2069d26d 100644 --- a/test/extensions/voting-rep.js +++ b/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); }); });