Reviewer refactor

contracts/colony/Colony.sol

@@ -209,14 +209,6 @@ contract Colony is BasicMetaTransaction, Multicall, ColonyStorage, PatriciaTreeP
     IColonyNetwork(colonyNetworkAddress).setReputationMiningCycleReward(_amount);
   }
 
-  function setReputationMiningCycle(uint256 _amount) public
-  stoppable
-  auth
-  {
-    IColonyNetwork(colonyNetworkAddress).setReputationMiningCycleReward(_amount);
-  }
-
-
   function addNetworkColonyVersion(uint256 _version, address _resolver) public
   stoppable
   auth
@@ -435,8 +427,6 @@ contract Colony is BasicMetaTransaction, Multicall, ColonyStorage, PatriciaTreeP
   }
 
   function setReputationMiningCycleRewardReputationScaling(uint256 _factor) public stoppable auth {
-    require(_factor <= WAD, "colony-invalid-scale-factor");
     IColonyNetwork(colonyNetworkAddress).setReputationMiningCycleRewardReputationScaling(_factor);
-    emit MiningReputationScalingSet(_factor);
   }
 }

contracts/colony/ColonyDataTypes.sol

@@ -349,9 +349,7 @@ interface ColonyDataTypes {
 
   event ArbitraryTransaction(address target, bytes data, bool success);
 
-  event DomainReputationScalingSet(uint256 domainId, bool enabled, uint256 factor);
-
-  event MiningReputationScalingSet(uint256 factor);
+  event DomainReputationScalingSet(uint256 domainId, uint256 factor);
 
   struct RewardPayoutCycle {
     // Reputation root hash at the time of reward payout creation

contracts/colony/ColonyDomains.sol

@@ -127,13 +127,11 @@ contract ColonyDomains is ColonyStorage {
     return domainCount;
   }
 
-  function setDomainReputationScaling(uint256 _domainId, bool _enabled, uint256 _factor) public stoppable auth {
+  function setDomainReputationScaling(uint256 _domainId, uint256 _factor) public stoppable auth {
     require(domainExists(_domainId), "colony-domain-does-not-exist");
-    require(_factor <= WAD, "colony-invalid-scale-factor");
-    require(_enabled || _factor == 0, "colony-invalid-configuration");
+    IColonyNetwork(colonyNetworkAddress).setSkillReputationScaling(domains[_domainId].skillId, _factor);
 
-    IColonyNetwork(colonyNetworkAddress).setDomainReputationScaling(_domainId, _enabled, _factor);
-    emit DomainReputationScalingSet(_domainId, _enabled, _factor);
+    emit DomainReputationScalingSet(_domainId, _factor);
   }
 
   // Internal

contracts/colony/ColonyExpenditure.sol

@@ -175,7 +175,6 @@ contract ColonyExpenditure is ColonyStorage {
     expenditureOnlyOwner(_id)
   {
     require(_slots.length == _skillIds.length, "colony-expenditure-bad-slots");
-    IColonyNetwork colonyNetworkContract = IColonyNetwork(colonyNetworkAddress);
 
     for (uint256 i; i < _slots.length; i++) {
       require(isValidGlobalOrLocalSkill(_skillIds[i]), "colony-not-valid-global-or-local-skill");
@@ -316,10 +315,11 @@ contract ColonyExpenditure is ColonyStorage {
 
       // Validate payout modifier
       if (offset == 2) {
-        if (!ColonyAuthority(address(authority)).hasUserRole(msgSender(), 1, uint8(ColonyDataTypes.ColonyRole.Root))){
-          require(int256(uint256(_value)) <= 0, "colony-expenditure-bad-payout-modifier");
-        }
-        require(int256(uint256(_value)) >= MIN_PAYOUT_MODIFIER, "colony-expenditure-bad-payout-modifier");
+        require(
+          (int256(uint256(_value)) <= 0 || IColony(address(this)).hasUserRole(msgSender(), 1, ColonyRole.Root)) &&
+          int256(uint256(_value)) >= MIN_PAYOUT_MODIFIER,
+          "colony-expenditure-bad-payout-modifier"
+        );
       }
 
     } else {
@@ -361,11 +361,11 @@ contract ColonyExpenditure is ColonyStorage {
     internal
   {
     for (uint256 i; i < _tokens.length; i++) {
-      (bool success, bytes memory returndata) = address(this).delegatecall(
+      (bool success, bytes memory returndata) = address(this).delegatecall( // solhint-disable-line avoid-low-level-calls
         abi.encodeWithSignature("setExpenditurePayouts(uint256,uint256[],address,uint256[])", _id, _slots[i], _tokens[i], _values[i])
       );
       if (!success) {
-        if (returndata.length == 0) revert();
+        if (returndata.length == 0) revert("colony-expenditure-null-return");
         assembly {
           revert(add(32, returndata), mload(returndata))
         }

contracts/colony/IColony.sol

@@ -1112,10 +1112,8 @@ interface IColony is ColonyDataTypes, IRecovery, IBasicMetaTransaction, IMultica
 
   /// @notice Call to set the reputation scaling applied to reputation earned in a domain
   /// @param domainId The domain to set the value of scaling in
-  /// @param enabled bool Whether we're enabling or disabling reputation scaling for this domain
-  /// If disabling, bool must be false
   /// @param factor The scale factor to apply, as a WAD
-  function setDomainReputationScaling(uint256 domainId, bool enabled, uint256 factor) external;
+  function setDomainReputationScaling(uint256 domainId, uint256 factor) external;
 
   /// @notice Call to set the reputation scaling applied to payouts made in a particular token
   /// @param _token The token we wish to apply scaling to

contracts/colonyNetwork/ColonyNetwork.sol

@@ -278,32 +278,20 @@ contract ColonyNetwork is ColonyDataTypes, BasicMetaTransaction, ColonyNetworkSt
     return payoutWhitelist[_token];
   }
 
-  function setDomainReputationScaling(uint256 _domainId, bool _enabled, uint256 _factor) public calledByColony stoppable
-  {
+  function setSkillReputationScaling(uint256 _skillId, uint256 _factor) public calledByColony stoppable {
     require(_factor <= WAD, "colony-network-invalid-reputation-scale-factor");
-    uint256 skillId = IColony(msgSender()).getDomain(_domainId).skillId;
-    skills[skillId].reputationScalingFactorComplement = WAD - _factor;
+    skills[_skillId].reputationScalingFactorComplement = WAD - _factor;
   }
 
   function getSkillReputationScaling(uint256 _skillId) public view returns (uint256) {
-    uint256 factor;
-    Skill storage s = skills[_skillId];
-    factor = WAD - s.reputationScalingFactorComplement;
-
-    while (s.nParents > 0) {
-      s = skills[s.parents[0]];
-      // If reputation scaling is in effect for this skill, then take the value for this skill in to
-      // account. Otherwise, no effect and continue walking up the tree
-      if (s.reputationScalingFactorComplement > 0) {
-        if (s.reputationScalingFactorComplement == 1){
-          // If scaling is in effect and is 0 (because factor = 1 - complement), we can short circuit - regardless of the rest of the tree
-          // the scaling factor will be 0
-          return 0;
-        } else {
-          factor = wmul(factor, WAD - s.reputationScalingFactorComplement);
-        }
-      }
+    Skill storage skill = skills[_skillId];
+    uint256 factor = WAD - skill.reputationScalingFactorComplement;
+
+    while (skill.nParents > 0 && factor > 0) {
+      skill = skills[skill.parents[0]];
+      factor = wmul(factor, WAD - skill.reputationScalingFactorComplement);
     }
+
     return factor;
   }
 

contracts/colonyNetwork/ColonyNetworkDataTypes.sol

@@ -152,6 +152,9 @@ interface ColonyNetworkDataTypes {
   /// @param denominator The new denominator of the decay rate
   event ColonyReputationDecayRateToChange(address colony, address fromCycleCompleted, uint256 numerator, uint256 denominator);
 
+  /// @notice Event logged when the reputation scaling factor for miners is set
+  /// @param factor The factor (from 0 to WAD) governing how reputation awards for miners is scaled
+  event MiningReputationScalingSet(uint256 factor);
 
   struct Skill {
     // total number of parent skills

contracts/colonyNetwork/ColonyNetworkMining.sol

@@ -197,13 +197,15 @@ contract ColonyNetworkMining is ColonyNetworkStorage, MultiChain, ScaleReputatio
       ITokenLocking(tokenLocking).depositFor(clnyToken, wmul(totalMinerRewardPerCycle, minerWeights[i]), stakers[i]);
     }
 
+    uint256 scaleFactor = WAD - skills[reputationMiningSkillId].reputationScalingFactorComplement;
+    uint256 scaledReward = uint256(scaleReputation(int256(totalMinerRewardPerCycle), scaleFactor));
+
     // This gives them reputation in the next update cycle.
     IReputationMiningCycle(inactiveReputationMiningCycle).rewardStakersWithReputation(
       stakers,
       minerWeights,
       metaColony,
-      // totalMinerRewardPerCycle,
-      uint256(scaleReputation(int256(totalMinerRewardPerCycle), WAD - skills[reputationMiningSkillId].reputationScalingFactorComplement)),
+      scaledReward,
       reputationMiningSkillId
     );
   }
@@ -287,10 +289,11 @@ contract ColonyNetworkMining is ColonyNetworkStorage, MultiChain, ScaleReputatio
     return totalMinerRewardPerCycle;
   }
 
-  function setReputationMiningCycleRewardReputationScaling(uint256 _factor) public calledByMetaColony stoppable
-  {
+  function setReputationMiningCycleRewardReputationScaling(uint256 _factor) public calledByMetaColony stoppable {
     require(_factor <= WAD, "colony-network-invalid-reputation-scale-factor");
     skills[reputationMiningSkillId].reputationScalingFactorComplement = WAD - _factor;
+
+    emit MiningReputationScalingSet(_factor);
   }
 
   uint256 constant UINT192_MAX = 2**192 - 1; // Used for updating the stake timestamp

contracts/colonyNetwork/IColonyNetwork.sol

@@ -469,13 +469,11 @@ interface IColonyNetwork is ColonyNetworkDataTypes, IRecovery, IBasicMetaTransac
   /// @return scaleFactor Returns the scale factor applied to reputation earned in this skill, as a WAD.
   function getSkillReputationScaling(uint256 _skillId) external view returns (uint256 scaleFactor);
 
-  /// @notice Call to set the reputation scaling applied to reputation earned in a domain.
+  /// @notice Call to set the reputation scaling applied to reputation earned in a skill
   /// @dev Only callable by a colony
-  /// @param _domainId The domain to set the value of scaling in
-  /// @param _enabled bool Whether we're enabling or disabling reputation scaling for this domain
-  /// If disabling, bool must be false
+  /// @param _skillId The skill to set the value of scaling in
   /// @param _factor The scale factor to apply, as a WAD
-  function setDomainReputationScaling(uint256 _domainId, bool _enabled, uint256 _factor) external;
+  function setSkillReputationScaling(uint256 _skillId, uint256 _factor) external;
 
   /// @notice Called by a colony to set the rate at which reputation in that colony decays
   /// @param _numerator The numerator of the fraction reputation does down by every reputation cycle
@@ -492,4 +490,4 @@ interface IColonyNetwork is ColonyNetworkDataTypes, IRecovery, IBasicMetaTransac
   /// @dev Calls the corresponding function on the ColonyNetwork.
   /// @param _factor The scale factor to apply to reputation mining rewards
   function setReputationMiningCycleRewardReputationScaling(uint256 _factor) external;
-}
+}

contracts/common/ScaleReputation.sol

@@ -20,35 +20,26 @@ import "../../lib/dappsys/math.sol";
 
 contract ScaleReputation is DSMath {
   // Note that scaleFactor should be a WAD.
-  function scaleReputation(int256 reputationAmount, uint256 scaleFactor) internal pure returns (int256) {
-    if (reputationAmount == 0 || scaleFactor == 0) {
-      return 0;
-    }
-
-    int256 scaledReputation;
-    int256 absAmount;
-
-    if (reputationAmount == type(int256).min){
-      absAmount = type(int256).max; // Off by one, but best we can do - probably gets capped anyway
-    } else {
-      absAmount = reputationAmount >= 0 ? reputationAmount : -reputationAmount;
-    }
-
-    int256 sgnAmount = reputationAmount >= 0 ? int(1) : -1;
+  function scaleReputation(int256 reputationAmount, uint256 scaleFactor)
+    internal
+    pure
+    returns (int256 scaledReputation)
+  {
+    if (reputationAmount == 0 || scaleFactor == 0) { return 0; }
+
+    int256 sgnAmount = (reputationAmount >= 0) ? int256(1) : -1;
+    int256 absAmount = (reputationAmount == type(int256).min)
+      ? type(int256).max // Off by one, but best we can do - probably gets capped anyway
+      : (reputationAmount >= 0) ? reputationAmount : -reputationAmount;
 
     // Guard against overflows during calculation with wmul
-    if (type(uint256).max / scaleFactor < uint256(absAmount)){
-      if (sgnAmount == 1){
-        scaledReputation = type(int128).max;
-      } else {
-        scaledReputation = type(int128).min;
-      }
+    if (type(uint256).max / scaleFactor < uint256(absAmount)) {
+      scaledReputation = (sgnAmount == 1) ? type(int128).max : type(int128).min;
     } else {
       scaledReputation = int256(wmul(scaleFactor, uint256(absAmount))) * sgnAmount;
       // Cap inside the range of int128, as we do for all reputations
       scaledReputation = imax(type(int128).min, scaledReputation);
       scaledReputation = imin(type(int128).max, scaledReputation);
     }
-    return scaledReputation;
   }
 }

docs/interfaces/icolony.md

@@ -1463,7 +1463,7 @@ Update the default global claim delay for expenditures
 |_globalClaimDelay|uint256|The new default global claim delay
 
 
-### ▸ `setDomainReputationScaling(uint256 domainId, bool enabled, uint256 factor)`
+### ▸ `setDomainReputationScaling(uint256 domainId, uint256 factor)`
 
 Call to set the reputation scaling applied to reputation earned in a domain
 
@@ -1473,7 +1473,6 @@ Call to set the reputation scaling applied to reputation earned in a domain
 |Name|Type|Description|
 |---|---|---|
 |domainId|uint256|The domain to set the value of scaling in
-|enabled|bool|bool Whether we're enabling or disabling reputation scaling for this domain If disabling, bool must be false
 |factor|uint256|The scale factor to apply, as a WAD
 
 

docs/interfaces/icolonynetwork.md

@@ -916,21 +916,6 @@ Called by a colony to set the rate at which reputation in that colony decays
 |_denominator|uint256|The denominator of the fraction reputation does down by every reputation cycle
 
 
-### ▸ `setDomainReputationScaling(uint256 _domainId, bool _enabled, uint256 _factor)`
-
-Call to set the reputation scaling applied to reputation earned in a domain.
-
-*Note: Only callable by a colony*
-
-**Parameters**
-
-|Name|Type|Description|
-|---|---|---|
-|_domainId|uint256|The domain to set the value of scaling in
-|_enabled|bool|bool Whether we're enabling or disabling reputation scaling for this domain If disabling, bool must be false
-|_factor|uint256|The scale factor to apply, as a WAD
-
-
 ### ▸ `setFeeInverse(uint256 _feeInverse)`
 
 Set the colony network fee to pay. e.g. if the fee is 1% (or 0.01), pass 100 as `_feeInverse`.
@@ -1041,6 +1026,20 @@ Set a new Reputation root hash and starts a new mining cycle. Can only be called
 |_stakers|address[]|Array of users who submitted or backed the hash, being accepted here as the new reputation root hash
 
 
+### ▸ `setSkillReputationScaling(uint256 _skillId, uint256 _factor)`
+
+Call to set the reputation scaling applied to reputation earned in a skill
+
+*Note: Only callable by a colony*
+
+**Parameters**
+
+|Name|Type|Description|
+|---|---|---|
+|_skillId|uint256|The skill to set the value of scaling in
+|_factor|uint256|The scale factor to apply, as a WAD
+
+
 ### ▸ `setTokenLocking(address _tokenLockingAddress)`
 
 Sets the token locking address. This is only set once, and can't be changed afterwards.

test/contracts-network/colony-permissions.js

@@ -433,7 +433,7 @@ contract("ColonyPermissions", (accounts) => {
 
       // Set scaling for each domain to a non-zero value
       for (let i = domainCount; i <= 150 + domainCount; i += 1) {
-        await colony.setDomainReputationScaling(i, true, WAD.muln(9).divn(10));
+        await colony.setDomainReputationScaling(i, WAD.muln(9).divn(10));
       }
 
       await colony.emitDomainReputationReward(150, USER1, 10000000000000);
@@ -457,8 +457,8 @@ contract("ColonyPermissions", (accounts) => {
       const domain2id = domainCount + 1;
       const domain3id = domainCount + 2;
 
-      await colony.setDomainReputationScaling(domain1id, true, WAD.muln(9).divn(10));
-      await colony.setDomainReputationScaling(domain3id, true, WAD.divn(2));
+      await colony.setDomainReputationScaling(domain1id, WAD.muln(9).divn(10));
+      await colony.setDomainReputationScaling(domain3id, WAD.divn(2));
 
       const repCycleAddress = await colonyNetwork.getReputationMiningCycle(false);
       const reputationMiningCycle = await IReputationMiningCycle.at(repCycleAddress);
@@ -505,7 +505,7 @@ contract("ColonyPermissions", (accounts) => {
       expect(lastLog.amount).to.eq.BN(10000000000000);
 
       // Set root domain such that no reputation is earned
-      await colony.setDomainReputationScaling(1, true, 0);
+      await colony.setDomainReputationScaling(1, 0);
 
       await colony.emitDomainReputationReward(domainCount, USER1, 10000000000000);
 
@@ -515,7 +515,7 @@ contract("ColonyPermissions", (accounts) => {
     });
 
     it("should take token-specific and domain scaling in to account when emitting reputation", async () => {
-      await colony.setDomainReputationScaling(1, true, WAD.muln(9).divn(10));
+      await colony.setDomainReputationScaling(1, WAD.muln(9).divn(10));
 
       await colony.setTokenReputationRate(token.address, WAD.divn(2));