diff --git a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol index e100dafa82..27b051c898 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol @@ -31,21 +31,6 @@ contract MixinCumulativeRewards is { using LibSafeMath for uint256; - /// @dev Initializes Cumulative Rewards for a given pool. - /// @param poolId Unique id of pool. - function _initializeCumulativeRewards(bytes32 poolId) - internal - { - // Sets the default cumulative reward - _forceSetCumulativeReward( - poolId, - IStructs.Fraction({ - numerator: 0, - denominator: MIN_TOKEN_VALUE - }) - ); - } - /// @dev returns true iff Cumulative Rewards are set function _isCumulativeRewardSet(IStructs.Fraction memory cumulativeReward) internal @@ -58,22 +43,71 @@ contract MixinCumulativeRewards is return cumulativeReward.denominator != 0; } - /// @dev Sets a cumulative reward for `poolId` at `epoch`. - /// This can be used to overwrite an existing value. - /// @param poolId Unique Id of pool. - /// @param value Value of cumulative reward. - function _forceSetCumulativeReward( + /// @dev Sets a pool's cumulative delegator rewards for the current epoch, + /// given the rewards earned and stake from the last epoch, which will + /// be summed with the previous cumulative rewards for this pool. + /// If the last cumulative reward epoch is the current epoch, this is a + /// no-op. + /// @param poolId The pool ID. + /// @param reward The total reward earned by pool delegators from the last epoch. + /// @param stake The total delegated stake in the pool in the last epoch. + function _addCumulativeReward( bytes32 poolId, - IStructs.Fraction memory value + uint256 reward, + uint256 stake ) internal { + // Fetch the last epoch at which we stored an entry for this pool; + // this is the most up-to-date cumulative rewards for this pool. + uint256 lastStoredEpoch = _cumulativeRewardsByPoolLastStored[poolId]; uint256 currentEpoch_ = currentEpoch; - _cumulativeRewardsByPool[poolId][currentEpoch_] = value; - // Update state to reflect the most recent cumulative reward + + // If we already have a record for this epoch, don't overwrite it. + if (lastStoredEpoch == currentEpoch_) { + return; + } + + IStructs.Fraction memory mostRecentCumulativeReward = + _cumulativeRewardsByPool[poolId][lastStoredEpoch]; + + // Compute new cumulative reward + IStructs.Fraction memory cumulativeReward; + if (_isCumulativeRewardSet(mostRecentCumulativeReward)) { + // If we have a prior cumulative reward entry, we sum them as fractions. + (cumulativeReward.numerator, cumulativeReward.denominator) = LibFractions.add( + mostRecentCumulativeReward.numerator, + mostRecentCumulativeReward.denominator, + reward, + stake + ); + // Normalize to prevent overflows in future operations. + (cumulativeReward.numerator, cumulativeReward.denominator) = LibFractions.normalize( + cumulativeReward.numerator, + cumulativeReward.denominator + ); + } else { + (cumulativeReward.numerator, cumulativeReward.denominator) = (reward, stake); + } + + // Store cumulative rewards for this epoch. + _cumulativeRewardsByPool[poolId][currentEpoch_] = cumulativeReward; _cumulativeRewardsByPoolLastStored[poolId] = currentEpoch_; } + /// @dev Sets a pool's cumulative delegator rewards for the current epoch, + /// using the last stored cumulative rewards. If we've already set + /// a CR for this epoch, this is a no-op. + /// @param poolId The pool ID. + function _updateCumulativeReward(bytes32 poolId) + internal + { + // Just add empty rewards for this epoch, which will be added to + // the previous CR, so we end up with the previous CR being set for + // this epoch. + _addCumulativeReward(poolId, 0, 1); + } + /// @dev Computes a member's reward over a given epoch interval. /// @param poolId Uniqud Id of pool. /// @param memberStakeOverInterval Stake delegated to pool by member over @@ -100,13 +134,8 @@ contract MixinCumulativeRewards is require(beginEpoch < endEpoch, "CR_INTERVAL_INVALID"); // Sanity check begin reward - (IStructs.Fraction memory beginReward, uint256 beginRewardStoredAt) = _getCumulativeRewardAtEpoch(poolId, beginEpoch); - (IStructs.Fraction memory endReward, uint256 endRewardStoredAt) = _getCumulativeRewardAtEpoch(poolId, endEpoch); - - // If the rewards were stored at the same epoch then the computation will result in zero. - if (beginRewardStoredAt == endRewardStoredAt) { - return 0; - } + IStructs.Fraction memory beginReward = _getCumulativeRewardAtEpoch(poolId, beginEpoch); + IStructs.Fraction memory endReward = _getCumulativeRewardAtEpoch(poolId, endEpoch); // Compute reward reward = LibFractions.scaleDifference( @@ -122,7 +151,7 @@ contract MixinCumulativeRewards is /// @param poolId Unique ID of pool. /// @return cumulativeReward The most recent cumulative reward `poolId`. function _getMostRecentCumulativeReward(bytes32 poolId) - internal + private view returns (IStructs.Fraction memory cumulativeReward) { @@ -138,24 +167,21 @@ contract MixinCumulativeRewards is /// @return cumulativeReward The cumulative reward for `poolId` at `epoch`. /// @return cumulativeRewardStoredAt Epoch that the `cumulativeReward` is stored at. function _getCumulativeRewardAtEpoch(bytes32 poolId, uint256 epoch) - internal + private view - returns ( - IStructs.Fraction memory cumulativeReward, - uint256 cumulativeRewardStoredAt - ) + returns (IStructs.Fraction memory cumulativeReward) { // Return CR at `epoch`, given it's set. cumulativeReward = _cumulativeRewardsByPool[poolId][epoch]; if (_isCumulativeRewardSet(cumulativeReward)) { - return (cumulativeReward, epoch); + return cumulativeReward; } // Return CR at `epoch-1`, given it's set. uint256 lastEpoch = epoch.safeSub(1); cumulativeReward = _cumulativeRewardsByPool[poolId][lastEpoch]; if (_isCumulativeRewardSet(cumulativeReward)) { - return (cumulativeReward, lastEpoch); + return cumulativeReward; } // Return the most recent CR, given it's less than `epoch`. @@ -163,11 +189,11 @@ contract MixinCumulativeRewards is if (mostRecentEpoch < epoch) { cumulativeReward = _cumulativeRewardsByPool[poolId][mostRecentEpoch]; if (_isCumulativeRewardSet(cumulativeReward)) { - return (cumulativeReward, mostRecentEpoch); + return cumulativeReward; } } - // Could not find a CR for `epoch` - revert("CR_INVALID_EPOCH"); + // Otherwise return an empty CR. + return IStructs.Fraction(0, 1); } } diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPool.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPool.sol index 8869c00c64..66294d7244 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPool.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPool.sol @@ -70,10 +70,6 @@ contract MixinStakingPool is }); _poolById[poolId] = pool; - // initialize cumulative rewards for this pool; - // this is used to track rewards earned by delegators. - _initializeCumulativeRewards(poolId); - // Staking pool has been created emit StakingPoolCreated(poolId, operator, operatorShare); diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index 0fc899fd4d..3b78364784 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -20,7 +20,6 @@ pragma solidity ^0.5.9; pragma experimental ABIEncoderV2; import "@0x/contracts-exchange-libs/contracts/src/LibMath.sol"; -import "@0x/contracts-utils/contracts/src/LibFractions.sol"; import "@0x/contracts-utils/contracts/src/LibSafeMath.sol"; import "./MixinCumulativeRewards.sol"; import "../sys/MixinAbstract.sol"; @@ -138,11 +137,9 @@ contract MixinStakingPoolRewards is getWethContract().transfer(member, balance); } - // Add a cumulative reward entry for this epoch. - _forceSetCumulativeReward( - poolId, - _getMostRecentCumulativeReward(poolId) - ); + // Ensure a cumulative reward entry exists for this epoch, + // copying the previous epoch's CR if one doesn't exist already. + _updateCumulativeReward(poolId); } /// @dev Handles a pool's reward at the current epoch. @@ -181,30 +178,8 @@ contract MixinStakingPoolRewards is if (membersReward > 0) { // Increase the balance of the pool _increasePoolRewards(poolId, membersReward); - - // Fetch the last epoch at which we stored an entry for this pool; - // this is the most up-to-date cumulative rewards for this pool. - IStructs.Fraction memory mostRecentCumulativeReward = _getMostRecentCumulativeReward(poolId); - - // Compute new cumulative reward - IStructs.Fraction memory cumulativeReward; - (cumulativeReward.numerator, cumulativeReward.denominator) = LibFractions.add( - mostRecentCumulativeReward.numerator, - mostRecentCumulativeReward.denominator, - membersReward, - membersStake - ); - // Normalize to prevent overflows. - (cumulativeReward.numerator, cumulativeReward.denominator) = LibFractions.normalize( - cumulativeReward.numerator, - cumulativeReward.denominator - ); - - // Store cumulative rewards for this epoch. - _forceSetCumulativeReward( - poolId, - cumulativeReward - ); + // Create a cumulative reward entry at the current epoch. + _addCumulativeReward(poolId, membersReward, membersStake); } return (operatorReward, membersReward); diff --git a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol index bad0b3d143..94c254e7d3 100644 --- a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol +++ b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol @@ -43,16 +43,22 @@ contract TestCumulativeRewardTracking is function init() public {} - function _forceSetCumulativeReward( + function _addCumulativeReward( bytes32 poolId, - IStructs.Fraction memory value + uint256 reward, + uint256 stake ) internal { - emit SetCumulativeReward(poolId, currentEpoch); - MixinCumulativeRewards._forceSetCumulativeReward( + uint256 lastStoredEpoch = _cumulativeRewardsByPoolLastStored[poolId]; + MixinCumulativeRewards._addCumulativeReward( poolId, - value + reward, + stake ); + uint256 newLastStoredEpoch = _cumulativeRewardsByPoolLastStored[poolId]; + if (newLastStoredEpoch != lastStoredEpoch) { + emit SetCumulativeReward(poolId, currentEpoch); + } } } diff --git a/contracts/staking/contracts/test/TestDelegatorRewards.sol b/contracts/staking/contracts/test/TestDelegatorRewards.sol index e91ce5954c..f88cd9c0a6 100644 --- a/contracts/staking/contracts/test/TestDelegatorRewards.sol +++ b/contracts/staking/contracts/test/TestDelegatorRewards.sol @@ -71,7 +71,6 @@ contract TestDelegatorRewards is // Lazily initialize this pool. _poolById[poolId].operator = operatorAddress; _setOperatorShare(poolId, operatorReward, membersReward); - _initGenesisCumulativeRewards(poolId); } /// @dev Expose/wrap `_syncPoolRewards`. @@ -87,7 +86,6 @@ contract TestDelegatorRewards is // Lazily initialize this pool. _poolById[poolId].operator = operatorAddress; _setOperatorShare(poolId, operatorReward, membersReward); - _initGenesisCumulativeRewards(poolId); _syncPoolRewards( poolId, @@ -111,7 +109,6 @@ contract TestDelegatorRewards is ) external { - _initGenesisCumulativeRewards(poolId); _withdrawAndSyncDelegatorRewards( poolId, delegator @@ -136,7 +133,6 @@ contract TestDelegatorRewards is ) external { - _initGenesisCumulativeRewards(poolId); _withdrawAndSyncDelegatorRewards( poolId, delegator @@ -159,7 +155,6 @@ contract TestDelegatorRewards is ) external { - _initGenesisCumulativeRewards(poolId); _withdrawAndSyncDelegatorRewards( poolId, delegator @@ -204,19 +199,6 @@ contract TestDelegatorRewards is membersStake = reward.membersStake; } - /// @dev Create a cumulative rewards entry for a pool if one doesn't - /// already exist to get around having to create pools in advance. - function _initGenesisCumulativeRewards(bytes32 poolId) - private - { - uint256 lastRewardEpoch = _cumulativeRewardsByPoolLastStored[poolId]; - IStructs.Fraction memory cumulativeReward = - _cumulativeRewardsByPool[poolId][lastRewardEpoch]; - if (!_isCumulativeRewardSet(cumulativeReward)) { - _initializeCumulativeRewards(poolId); - } - } - /// @dev Set the operator share of a pool based on reward ratios. function _setOperatorShare( bytes32 poolId, diff --git a/contracts/staking/test/cumulative_reward_tracking_test.ts b/contracts/staking/test/cumulative_reward_tracking_test.ts index 1abd31c169..7693903d08 100644 --- a/contracts/staking/test/cumulative_reward_tracking_test.ts +++ b/contracts/staking/test/cumulative_reward_tracking_test.ts @@ -33,14 +33,10 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { describe('Tracking Cumulative Rewards (CR)', () => { it('pool created at epoch 0', async () => { - await simulation.runTestAsync([], [TestAction.CreatePool], [{ event: 'SetCumulativeReward', epoch: 0 }]); + await simulation.runTestAsync([], [TestAction.CreatePool], []); }); it('pool created in epoch >0', async () => { - await simulation.runTestAsync( - [TestAction.Finalize], - [TestAction.CreatePool], - [{ event: 'SetCumulativeReward', epoch: 1 }], - ); + await simulation.runTestAsync([TestAction.Finalize], [TestAction.CreatePool], []); }); it('delegating in the same epoch pool is created', async () => { await simulation.runTestAsync( @@ -53,7 +49,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { // Creates CR for epoch 1 TestAction.Delegate, ], - [{ event: 'SetCumulativeReward', epoch: 0 }], + [], ); }); it('re-delegating in the same epoch', async () => { @@ -68,7 +64,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { // Updates CR for epoch 0 TestAction.Delegate, ], - [{ event: 'SetCumulativeReward', epoch: 0 }, { event: 'SetCumulativeReward', epoch: 0 }], + [], ); }); it('delegating in new epoch', async () => { @@ -268,7 +264,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { // Clears CR for epoch 2 TestAction.Delegate, ], - [{ event: 'SetCumulativeReward', epoch: 3 }], + [], ); }); it('delegate in epoch 0 and 1, earn reward in epoch 3, then undelegate half', async () => { @@ -303,7 +299,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { // Clears CR for epoch 2 TestAction.Undelegate, ], - [{ event: 'SetCumulativeReward', epoch: 3 }], + [], ); }); it('delegate in epoch 1, 2, earn rewards in epoch 3, skip to epoch 4, then delegate', async () => { diff --git a/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts b/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts index f92406c207..e321c0b0cb 100644 --- a/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts +++ b/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts @@ -36,7 +36,8 @@ export class CumulativeRewardTrackingSimulation { private static _extractTestLogs(txReceiptLogs: DecodedLogs): TestLog[] { const logs = []; for (const log of txReceiptLogs) { - if (log.event === TestCumulativeRewardTrackingEvents.SetCumulativeReward) { + const wantedEvents = [TestCumulativeRewardTrackingEvents.SetCumulativeReward] as string[]; + if (wantedEvents.indexOf(log.event) !== -1) { logs.push({ event: log.event, epoch: log.args.epoch.toNumber(), @@ -53,10 +54,10 @@ export class CumulativeRewardTrackingSimulation { const expectedLog = expectedSequence[i]; const actualLog = logs[i]; expect(expectedLog.event).to.exist(''); - expect(expectedLog.event, `testing event name of ${JSON.stringify(expectedLog)}`).to.be.equal( - actualLog.event, + expect(actualLog.event, `testing event name of ${JSON.stringify(expectedLog)}`).to.be.equal( + expectedLog.event, ); - expect(expectedLog.epoch, `testing epoch of ${JSON.stringify(expectedLog)}`).to.be.equal(actualLog.epoch); + expect(actualLog.epoch, `testing epoch of ${JSON.stringify(expectedLog)}`).to.be.equal(expectedLog.epoch); } }