diff --git a/contracts/colony/Colony.sol b/contracts/colony/Colony.sol index 6619d228d0..405bda5fbb 100755 --- a/contracts/colony/Colony.sol +++ b/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); } } diff --git a/contracts/colony/ColonyDataTypes.sol b/contracts/colony/ColonyDataTypes.sol index fc79211cd8..1ed6495aba 100755 --- a/contracts/colony/ColonyDataTypes.sol +++ b/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 diff --git a/contracts/colony/ColonyDomains.sol b/contracts/colony/ColonyDomains.sol index 6e6de81177..a034463e52 100644 --- a/contracts/colony/ColonyDomains.sol +++ b/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 diff --git a/contracts/colony/ColonyExpenditure.sol b/contracts/colony/ColonyExpenditure.sol index a464e86f47..4ad9c76fa6 100644 --- a/contracts/colony/ColonyExpenditure.sol +++ b/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)) } diff --git a/contracts/colony/IColony.sol b/contracts/colony/IColony.sol index 6b9f6fac10..640f888297 100644 --- a/contracts/colony/IColony.sol +++ b/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 diff --git a/contracts/colonyNetwork/ColonyNetwork.sol b/contracts/colonyNetwork/ColonyNetwork.sol index 95f47f4ee3..1e186ce2af 100644 --- a/contracts/colonyNetwork/ColonyNetwork.sol +++ b/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; } diff --git a/contracts/colonyNetwork/ColonyNetworkDataTypes.sol b/contracts/colonyNetwork/ColonyNetworkDataTypes.sol index a7d6a9a2bc..5caf62dea4 100755 --- a/contracts/colonyNetwork/ColonyNetworkDataTypes.sol +++ b/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 diff --git a/contracts/colonyNetwork/ColonyNetworkMining.sol b/contracts/colonyNetwork/ColonyNetworkMining.sol index 99dfb7739f..63fe67d6db 100644 --- a/contracts/colonyNetwork/ColonyNetworkMining.sol +++ b/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 diff --git a/contracts/colonyNetwork/IColonyNetwork.sol b/contracts/colonyNetwork/IColonyNetwork.sol index 5992ce47da..bc41a7d5b6 100644 --- a/contracts/colonyNetwork/IColonyNetwork.sol +++ b/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; -} \ No newline at end of file +} diff --git a/contracts/common/ScaleReputation.sol b/contracts/common/ScaleReputation.sol index 190650f61a..0e229ac92d 100644 --- a/contracts/common/ScaleReputation.sol +++ b/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; } } diff --git a/docs/interfaces/icolony.md b/docs/interfaces/icolony.md index 9677a63b1a..138ca3ae01 100644 --- a/docs/interfaces/icolony.md +++ b/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 diff --git a/docs/interfaces/icolonynetwork.md b/docs/interfaces/icolonynetwork.md index 8a17bfc06b..03e84f7496 100644 --- a/docs/interfaces/icolonynetwork.md +++ b/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. diff --git a/test/contracts-network/colony-permissions.js b/test/contracts-network/colony-permissions.js index 59052f0f41..26e7ac9ef5 100644 --- a/test/contracts-network/colony-permissions.js +++ b/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)); diff --git a/test/contracts-network/colony.js b/test/contracts-network/colony.js index 254fa08e2b..503ddf6986 100755 --- a/test/contracts-network/colony.js +++ b/test/contracts-network/colony.js @@ -539,38 +539,38 @@ contract("Colony", (accounts) => { describe("when setting the domain reputation scaling factor", async () => { it("cannot set scale factor for a domain that does not exist", async () => { - await checkErrorRevert(colony.setDomainReputationScaling(UINT256_MAX, true, WAD.divn(2)), "colony-domain-does-not-exist"); + await checkErrorRevert(colony.setDomainReputationScaling(UINT256_MAX, WAD.divn(2)), "colony-domain-does-not-exist"); }); it("cannot set scale factor to larger than 1", async () => { - await checkErrorRevert(colony.setDomainReputationScaling(1, true, WAD.muln(2)), "colony-invalid-scale-factor"); + await checkErrorRevert(colony.setDomainReputationScaling(1, WAD.muln(2)), "colony-invalid-scale-factor"); }); it("non-root users cannot set domain scale factor", async () => { - await checkErrorRevert(colony.setDomainReputationScaling(1, true, WAD.muln(2), { from: USER1 }), "ds-auth-unauthorized"); + await checkErrorRevert(colony.setDomainReputationScaling(1, WAD.muln(2), { from: USER1 }), "ds-auth-unauthorized"); }); it("the domain reputation scaling factor can be removed", async () => { - await colony.setDomainReputationScaling(1, true, WAD.divn(2)); + await colony.setDomainReputationScaling(1, WAD.divn(2)); const domain = await colony.getDomain(1); let skill = await colonyNetwork.getSkill(domain.skillId); expect(skill.reputationScalingFactorComplement).to.be.eq.BN(WAD.divn(2)); - await colony.setDomainReputationScaling(1, false, 0); + await colony.setDomainReputationScaling(1, 0); skill = await colonyNetwork.getSkill(domain.skillId); expect(skill.reputationScalingFactorComplement).to.be.eq.BN(WAD); }); it("setting domain reputation scaling to false with a nonzero scale factor fails", async () => { - await colony.setDomainReputationScaling(1, true, WAD.divn(2)); + await colony.setDomainReputationScaling(1, WAD.divn(2)); - await checkErrorRevert(colony.setDomainReputationScaling(1, false, 1), "colony-invalid-configuration"); + await checkErrorRevert(colony.setDomainReputationScaling(1, 1), "colony-invalid-configuration"); }); it("an event is emitted when reputation scaling is changed", async () => { - const tx = await colony.setDomainReputationScaling(1, true, WAD.divn(2)); + const tx = await colony.setDomainReputationScaling(1, WAD.divn(2)); await expectEvent(tx, "DomainReputationScalingSet(uint256,bool,uint256)", [1, true, WAD.divn(2)]); }); }); diff --git a/test/contracts-network/common-scale-reputation.js b/test/contracts-network/common-scale-reputation.js index d7dbf65ea8..afef3a3ead 100644 --- a/test/contracts-network/common-scale-reputation.js +++ b/test/contracts-network/common-scale-reputation.js @@ -10,9 +10,9 @@ chai.use(bnChai(web3.utils.BN)); const ScaleReputationTest = artifacts.require("ScaleReputationTest"); -let scaleReputationTest; - contract("ScaleReputation", () => { + let scaleReputationTest; + before(async () => { scaleReputationTest = await ScaleReputationTest.new(); });