@0x/contracts-staking: Always do _withdrawAndSyncDelegatorRewards() before staking operations and always add a CR (if unset) in _withdrawSyncDelegatorRewards()`.

2019-09-25 16:34:06 -04:00
parent 5b77e2c8ac
commit f925c35344
5 changed files with 42 additions and 91 deletions
--- a/contracts/staking/contracts/src/stake/MixinStake.sol
+++ b/contracts/staking/contracts/src/stake/MixinStake.sol
@@ -187,9 +187,10 @@ contract MixinStake is
        // Sanity check the pool we're delegating to exists.
        _assertStakingPoolExists(poolId);

-        // Cache amount delegated to pool by staker.
-        IStructs.StoredBalance memory initDelegatedStakeToPoolByOwner =
-            _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]);
+        _withdrawAndSyncDelegatorRewards(
+            poolId,
+            staker
+        );

        // Increment how much stake the staker has delegated to the input pool.
        _increaseNextBalance(
@@ -199,12 +200,6 @@ contract MixinStake is

        // Increment how much stake has been delegated to pool.
        _increaseNextBalance(_delegatedStakeByPoolId[poolId], amount);
-
-        _withdrawAndSyncDelegatorRewards(
-            poolId,
-            staker,
-            initDelegatedStakeToPoolByOwner
-        );
    }

    /// @dev Un-Delegates a owners stake from a staking pool.
@@ -221,9 +216,10 @@ contract MixinStake is
        // sanity check the pool we're undelegating from exists
        _assertStakingPoolExists(poolId);

-        // Cache amount delegated to pool by staker.
-        IStructs.StoredBalance memory initDelegatedStakeToPoolByOwner =
-            _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]);
+        _withdrawAndSyncDelegatorRewards(
+            poolId,
+            staker
+        );

        // decrement how much stake the staker has delegated to the input pool
        _decreaseNextBalance(
@@ -233,12 +229,6 @@ contract MixinStake is

        // decrement how much stake has been delegated to pool
        _decreaseNextBalance(_delegatedStakeByPoolId[poolId], amount);
-
-        _withdrawAndSyncDelegatorRewards(
-            poolId,
-            staker,
-            initDelegatedStakeToPoolByOwner
-        );
    }

    /// @dev Returns a storage pointer to a user's stake in a given status.
--- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol
+++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol
@@ -46,8 +46,7 @@ contract MixinStakingPoolRewards is

        _withdrawAndSyncDelegatorRewards(
            poolId,
-            member,
-            _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[member][poolId])
+            member
        );

        // Update stored balance with synchronized version; this prevents
@@ -104,7 +103,6 @@ contract MixinStakingPoolRewards is
        return _computeDelegatorReward(
            poolId,
            member,
-            _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]),
            unfinalizedMembersReward,
            unfinalizedMembersStake
        );
@@ -114,12 +112,9 @@ contract MixinStakingPoolRewards is
    ///      withdrawing rewards and adding/removing dependencies on cumulative rewards.
    /// @param poolId Unique id of pool.
    /// @param member of the pool.
-    /// @param unsyncedStake The member's unsynced delegated balance at the
-    ///        beginning of this transaction.
    function _withdrawAndSyncDelegatorRewards(
        bytes32 poolId,
-        address member,
-        IStructs.StoredBalance memory unsyncedStake
+        address member
    )
        internal
    {
@@ -130,7 +125,6 @@ contract MixinStakingPoolRewards is
        uint256 balance = _computeDelegatorReward(
            poolId,
            member,
-            unsyncedStake,
            // No unfinalized values because we ensured the pool is already
            // finalized.
            0,
@@ -146,12 +140,13 @@ contract MixinStakingPoolRewards is
            getWethContract().transfer(member, balance);
        }

-        // Add dependencies on cumulative rewards for this epoch and the next
-        // epoch, if necessary.
-        _setCumulativeRewardDependenciesForDelegator(
-            poolId,
-            member
-        );
+        // Add a cumulative reward entry for this epoch.
+        if (!_isCumulativeRewardSet(_cumulativeRewardsByPool[poolId][currentEpoch])) {
+            _forceSetCumulativeReward(
+                poolId,
+                _getMostRecentCumulativeReward(poolId)
+            );
+        }
    }

    /// @dev Handles a pool's reward at the current epoch.
@@ -253,14 +248,12 @@ contract MixinStakingPoolRewards is
    /// @dev Computes the reward balance in ETH of a specific member of a pool.
    /// @param poolId Unique id of pool.
    /// @param member of the pool.
-    /// @param unsyncedStake Unsynced delegated stake to pool by staker
    /// @param unfinalizedMembersReward Unfinalized total members reward (if any).
    /// @param unfinalizedMembersStake Unfinalized total members stake (if any).
    /// @return reward Balance in WETH.
    function _computeDelegatorReward(
        bytes32 poolId,
        address member,
-        IStructs.StoredBalance memory unsyncedStake,
        uint256 unfinalizedMembersReward,
        uint256 unfinalizedMembersStake
    )
@@ -275,6 +268,9 @@ contract MixinStakingPoolRewards is
            return 0;
        }

+        IStructs.StoredBalance memory unsyncedStake =
+            _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]);
+
        // There can be no rewards if the last epoch when stake was synced is
        // equal to the current epoch, because all prior rewards, including
        // rewards finalized this epoch have been claimed.
@@ -357,39 +353,6 @@ contract MixinStakingPoolRewards is
        );
    }

-    /// @dev Adds or removes cumulative reward dependencies for a delegator.
-    ///      A delegator always depends on the cumulative reward for the current
-    ///      and next epoch, if they would still have stake in the next epoch.
-    /// @param poolId Unique id of pool.
-    /// @param member of the pool.
-    function _setCumulativeRewardDependenciesForDelegator(
-        bytes32 poolId,
-        address member
-    )
-        private
-    {
-        IStructs.StoredBalance memory finalDelegatedStake =
-            _loadSyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]);
-
-        // The delegator depends on the Cumulative Reward for this epoch
-        // only if they are currently staked or will be staked next epoch.
-        if (finalDelegatedStake.currentEpochBalance == 0 && finalDelegatedStake.nextEpochBalance == 0) {
-            return;
-        }
-
-        // Get the most recent cumulative reward, which will serve as a
-        // reference point when updating dependencies
-        IStructs.Fraction memory mostRecentCumulativeReward = _getMostRecentCumulativeReward(poolId);
-
-        // Delegator depends on the Cumulative Reward for this epoch - ensure it is set.
-        if (!_isCumulativeRewardSet(_cumulativeRewardsByPool[poolId][finalDelegatedStake.currentEpoch])) {
-            _forceSetCumulativeReward(
-                poolId,
-                mostRecentCumulativeReward
-            );
-        }
-    }
-
    /// @dev Increases rewards for a pool.
    /// @param poolId Unique id of pool.
    /// @param amount Amount to increment rewards by.
--- a/contracts/staking/contracts/test/TestDelegatorRewards.sol
+++ b/contracts/staking/contracts/test/TestDelegatorRewards.sol
@@ -110,7 +110,10 @@ contract TestDelegatorRewards is
        external
    {
        _initGenesisCumulativeRewards(poolId);
-        IStructs.StoredBalance memory initialStake = _delegatedStakeToPoolByOwner[delegator][poolId];
+        _withdrawAndSyncDelegatorRewards(
+            poolId,
+            delegator
+        );
        IStructs.StoredBalance storage _stake = _delegatedStakeToPoolByOwner[delegator][poolId];
        _stake.isInitialized = true;
        _stake.currentEpochBalance += uint96(stake);
@@ -118,8 +121,7 @@ contract TestDelegatorRewards is
        _stake.currentEpoch = uint32(currentEpoch);
        _withdrawAndSyncDelegatorRewards(
            poolId,
-            delegator,
-            initialStake
+            delegator
        );
    }

@@ -134,7 +136,10 @@ contract TestDelegatorRewards is
        external
    {
        _initGenesisCumulativeRewards(poolId);
-        IStructs.StoredBalance memory initialStake = _delegatedStakeToPoolByOwner[delegator][poolId];
+        _withdrawAndSyncDelegatorRewards(
+            poolId,
+            delegator
+        );
        IStructs.StoredBalance storage _stake = _delegatedStakeToPoolByOwner[delegator][poolId];
        if (_stake.currentEpoch < currentEpoch) {
            _stake.currentEpochBalance = _stake.nextEpochBalance;
@@ -142,11 +147,6 @@ contract TestDelegatorRewards is
        _stake.isInitialized = true;
        _stake.nextEpochBalance += uint96(stake);
        _stake.currentEpoch = uint32(currentEpoch);
-        _withdrawAndSyncDelegatorRewards(
-            poolId,
-            delegator,
-            initialStake
-        );
    }

    /// @dev Clear stake that will occur in the next epoch
@@ -160,7 +160,10 @@ contract TestDelegatorRewards is
        external
    {
        _initGenesisCumulativeRewards(poolId);
-        IStructs.StoredBalance memory initialStake = _delegatedStakeToPoolByOwner[delegator][poolId];
+        _withdrawAndSyncDelegatorRewards(
+            poolId,
+            delegator
+        );
        IStructs.StoredBalance storage _stake = _delegatedStakeToPoolByOwner[delegator][poolId];
        if (_stake.currentEpoch < currentEpoch) {
            _stake.currentEpochBalance = _stake.nextEpochBalance;
@@ -168,11 +171,6 @@ contract TestDelegatorRewards is
        _stake.isInitialized = true;
        _stake.nextEpochBalance -= uint96(stake);
        _stake.currentEpoch = uint32(currentEpoch);
-        _withdrawAndSyncDelegatorRewards(
-            poolId,
-            delegator,
-            initialStake
-        );
    }

    // solhint-disable no-simple-event-func-name
--- a/contracts/staking/contracts/test/TestStakingNoWETH.sol
+++ b/contracts/staking/contracts/test/TestStakingNoWETH.sol
@@ -22,7 +22,7 @@ pragma experimental ABIEncoderV2;
 import "../src/Staking.sol";


-// solhint-disable no-empty-blocks
+// solhint-disable no-empty-blocks,no-simple-event-func-name
 /// @dev A version of the staking contract with WETH-related functions
 ///      overridden to do nothing.
 contract TestStakingNoWETH is
--- a/contracts/staking/test/cumulative_reward_tracking_test.ts
+++ b/contracts/staking/test/cumulative_reward_tracking_test.ts
@@ -128,7 +128,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
                    // Clears CR for epoch 1
                    TestAction.Delegate,
                ],
-                [{ event: 'SetCumulativeReward', epoch: 2 } ],
+                [{ event: 'SetCumulativeReward', epoch: 2 }],
            );
        });
        it('delegate in epoch 1 then undelegate in epoch 2', async () => {
@@ -152,7 +152,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
                    // Clear CR for epoch 1
                    TestAction.Undelegate,
                ],
-                [{ event: 'SetCumulativeReward', epoch: 2 } ],
+                [{ event: 'SetCumulativeReward', epoch: 2 }],
            );
        });
        it('delegate in epoch 0 and epoch 1, then undelegate half in epoch 2', async () => {
@@ -181,7 +181,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
                    // Clears CR for epoch 1
                    TestAction.Undelegate,
                ],
-                [{ event: 'SetCumulativeReward', epoch: 2 } ],
+                [{ event: 'SetCumulativeReward', epoch: 2 }],
            );
        });
        it('delegate in epoch 1 and 2 then again in 3', async () => {
@@ -210,7 +210,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
                    // Clears CR for epoch 1
                    TestAction.Delegate,
                ],
-                [{ event: 'SetCumulativeReward', epoch: 2 } ],
+                [{ event: 'SetCumulativeReward', epoch: 2 }],
            );
        });
        it('delegate in epoch 0, earn reward in epoch 1', async () => {
@@ -233,7 +233,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
                    // Sets MRCR to epoch 2
                    TestAction.Finalize,
                ],
-                [{ event: 'SetCumulativeReward', epoch: 2 } ],
+                [{ event: 'SetCumulativeReward', epoch: 2 }],
            );
        });
        it('delegate in epoch 0, epoch 2, earn reward in epoch 3, then delegate', async () => {
@@ -341,7 +341,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
                    // Clears CR for epoch 1
                    TestAction.Delegate,
                ],
-                [{ event: 'SetCumulativeReward', epoch: 4 } ],
+                [{ event: 'SetCumulativeReward', epoch: 4 }],
            );
        });
        it('earn reward in epoch 1 with no stake, then delegate', async () => {
@@ -397,7 +397,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
                    // Creates CR for epoch 5
                    TestAction.Delegate,
                ],
-                [{ event: 'SetCumulativeReward', epoch: 4 } ],
+                [{ event: 'SetCumulativeReward', epoch: 4 }],
            );
        });
        it('delegate in epoch 1, then epoch 3', async () => {
@@ -425,7 +425,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
                    // Clears CR for epoch 2
                    TestAction.Delegate,
                ],
-                [{ event: 'SetCumulativeReward', epoch: 3 } ],
+                [{ event: 'SetCumulativeReward', epoch: 3 }],
            );
        });
    });