@0x/contracts-staking: Refactor MixinCumulativeRewards and MixinStakingPoolRewards to better encapsulate cumulative rewards.

`@0x/contracts-staking`: Make sure we don't overwrite an existing CR. `@0x/contracts-staking`: Remove the need to initialize cumulative rewards when creating a pool. `@0x/contracts-staking`: Just return an empty CR in `_getCumulativeRewardAtEpoch()` if one can't be found.
2019-10-13 21:37:41 +10:00
parent 43fa753a13
commit 54a03eacd6
7 changed files with 94 additions and 112 deletions
--- 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 create add an empty CR 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);
    }
 }
--- 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);

--- 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);
--- 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);
+        }
    }
 }
--- 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,