Merge pull request #2262 from 0xProject/fix/staking/simplify-finalization

Staking readability improvements

contracts/integrations/package.json

@@ -80,7 +80,6 @@
         "@0x/contracts-exchange": "^2.2.0-beta.0",
         "@0x/contracts-exchange-forwarder": "^3.1.0-beta.0",
         "@0x/contracts-exchange-libs": "^3.1.0-beta.0",
-        "@0x/contracts-extensions": "^4.1.0-beta.0",
         "@0x/contracts-multisig": "^3.2.0-beta.0",
         "@0x/contracts-staking": "^1.1.0-beta.0",
         "@0x/contracts-test-utils": "^3.2.0-beta.0",

contracts/staking/contracts/src/ZrxVault.sol

@@ -35,7 +35,7 @@ contract ZrxVault is
     using LibSafeMath for uint256;
 
     // Address of staking proxy contract
-    address payable public stakingProxyAddress;
+    address public stakingProxyAddress;
 
     // True iff vault has been set to Catastrophic Failure Mode
     bool public isInCatastrophicFailure;
@@ -91,7 +91,7 @@ contract ZrxVault is
     /// @dev Sets the address of the StakingProxy contract.
     /// Note that only the contract owner can call this function.
     /// @param _stakingProxyAddress Address of Staking proxy contract.
-    function setStakingProxy(address payable _stakingProxyAddress)
+    function setStakingProxy(address _stakingProxyAddress)
         external
         onlyAuthorized
     {

contracts/staking/contracts/src/interfaces/IStructs.sol

@@ -94,12 +94,10 @@ interface IStructs {
     }
 
     /// @dev Holds the metadata for a staking pool.
-    /// @param initialized True iff the balance struct is initialized.
     /// @param operator of the pool.
     /// @param operatorShare Fraction of the total balance owned by the operator, in ppm.
     struct Pool {
-        bool initialized;
+        address operator;
-        address payable operator;
         uint32 operatorShare;
     }
 }

contracts/staking/contracts/src/interfaces/IZrxVault.sol

@@ -50,7 +50,7 @@ interface IZrxVault {
     /// @dev Sets the address of the StakingProxy contract.
     /// Note that only the contract staker can call this function.
     /// @param _stakingProxyAddress Address of Staking proxy contract.
-    function setStakingProxy(address payable _stakingProxyAddress)
+    function setStakingProxy(address _stakingProxyAddress)
         external;
 
     /// @dev Vault enters into Catastrophic Failure Mode.

contracts/staking/contracts/src/stake/MixinStake.sol

@@ -35,7 +35,7 @@ contract MixinStake is
     function stake(uint256 amount)
         external
     {
-        address payable staker = msg.sender;
+        address staker = msg.sender;
 
         // deposit equivalent amount of ZRX into vault
         getZrxVault().depositFrom(staker, amount);
@@ -109,7 +109,7 @@ contract MixinStake is
     )
         external
     {
-        address payable staker = msg.sender;
+        address staker = msg.sender;
 
         // handle delegation
         if (from.status == IStructs.StakeStatus.DELEGATED) {
@@ -154,7 +154,7 @@ contract MixinStake is
     /// @param amount Amount of stake to delegate.
     function _delegateStake(
         bytes32 poolId,
-        address payable staker,
+        address staker,
         uint256 amount
     )
         private
@@ -192,7 +192,7 @@ contract MixinStake is
     /// @param amount Amount of stake to un-delegate.
     function _undelegateStake(
         bytes32 poolId,
-        address payable staker,
+        address staker,
         uint256 amount
     )
         private

contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol

@@ -70,10 +70,7 @@ contract MixinCumulativeRewards is
     {
         uint256 currentEpoch_ = currentEpoch;
         _cumulativeRewardsByPool[poolId][currentEpoch_] = value;
-        
         // Update state to reflect the most recent cumulative reward
-        uint256 currentMostRecentEpoch = _cumulativeRewardsByPoolLastStored[poolId];
-        assert(currentEpoch_ >= currentMostRecentEpoch);
         _cumulativeRewardsByPoolLastStored[poolId] = currentEpoch_;
     }
 

contracts/staking/contracts/src/staking_pools/MixinStakingPool.sol

@@ -51,7 +51,7 @@ contract MixinStakingPool is
         returns (bytes32 poolId)
     {
         // note that an operator must be payable
-        address payable operator = msg.sender;
+        address operator = msg.sender;
 
         // compute unique id for this pool
         poolId = lastPoolId = bytes32(uint256(lastPoolId).safeAdd(1));
@@ -65,7 +65,6 @@ contract MixinStakingPool is
 
         // create and store pool
         IStructs.Pool memory pool = IStructs.Pool({
-            initialized: true,
             operator: operator,
             operatorShare: operatorShare
         });

contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol

@@ -139,12 +139,10 @@ contract MixinStakingPoolRewards is
         }
 
         // Add a cumulative reward entry for this epoch.
-        if (!_isCumulativeRewardSet(_cumulativeRewardsByPool[poolId][currentEpoch])) {
+        _forceSetCumulativeReward(
-            _forceSetCumulativeReward(
+            poolId,
-                poolId,
+            _getMostRecentCumulativeReward(poolId)
-                _getMostRecentCumulativeReward(poolId)
+        );
-            );
-        }
     }
 
     /// @dev Handles a pool's reward at the current epoch.

contracts/staking/contracts/src/sys/MixinAbstract.sol

@@ -35,12 +35,7 @@ contract MixinAbstract {
     /// @return membersStake The total stake for all non-operator members in
     ///         this pool.
     function finalizePool(bytes32 poolId)
-        public
+        public;
-        returns (
-            uint256 operatorReward,
-            uint256 membersReward,
-            uint256 membersStake
-        );
 
     /// @dev Computes the reward owed to a pool during finalization.
     ///      Does nothing if the pool is already finalized.

contracts/staking/contracts/src/sys/MixinFinalizer.sol

@@ -96,54 +96,61 @@ contract MixinFinalizer is
     ///      to finalize a pool immediately. Does nothing if the pool is already
     ///      finalized or was not active in the previous epoch.
     /// @param poolId The pool ID to finalize.
-    /// @return operatorReward The reward credited to the pool operator.
-    /// @return membersReward The reward credited to the pool members.
-    /// @return membersStake The total stake for all non-operator members in
-    ///         this pool.
     function finalizePool(bytes32 poolId)
         public
-        returns (
-            uint256 operatorReward,
-            uint256 membersReward,
-            uint256 membersStake
-        )
     {
-        uint256 epoch = currentEpoch;
+        // Noop on epoch 0
-        // There are no pools to finalize at epoch 0.
+        uint256 currentEpoch_ = currentEpoch;
-        if (epoch == 0) {
+        if (currentEpoch_ == 0) {
-            return (0, 0, 0);
+            return;
         }
-        uint256 prevEpoch = epoch - 1;
 
-        // Load the finalization state into memory.
+        // Load the finalization and pool state into memory.
         IStructs.UnfinalizedState memory state = unfinalizedState;
 
-        // If there are no more unfinalized pools remaining, there's nothing
+        // Noop if all active pools have been finalized.
-        // to do.
         if (state.poolsRemaining == 0) {
-            return (0, 0, 0);
+            return;
         }
 
+        uint256 prevEpoch = currentEpoch_.safeSub(1);
         IStructs.ActivePool memory pool = _getActivePoolFromEpoch(prevEpoch, poolId);
-        // Do nothing if the pool was not active or already finalized (has no fees).
+
+        // Noop if the pool was not active or already finalized (has no fees).
         if (pool.feesCollected == 0) {
-            return (operatorReward, membersReward, membersStake);
+            return;
         }
 
-        (operatorReward, membersReward) = _creditRewardsToPool(
+        // Clear the pool state so we don't finalize it again, and to recoup
-            epoch,
+        // some gas.
+        delete _getActivePoolsFromEpoch(prevEpoch)[poolId];
+
+        // Compute the rewards.
+        uint256 rewards = _getUnfinalizedPoolRewardsFromState(pool, state);
+
+        // Pay the operator and update rewards for the pool.
+        // Note that we credit at the CURRENT epoch even though these rewards
+        // were earned in the previous epoch.
+        (uint256 operatorReward, uint256 membersReward) = _syncPoolRewards(
             poolId,
-            pool,
+            rewards,
-            state
+            pool.membersStake
         );
+
+        // Emit an event.
+        emit RewardsPaid(
+            currentEpoch_,
+            poolId,
+            operatorReward,
+            membersReward
+        );
+
         uint256 totalReward = operatorReward.safeAdd(membersReward);
 
-        if (totalReward > 0) {
+        // Increase `totalRewardsFinalized`.
-            // Increase `totalRewardsFinalized`.
+        unfinalizedState.totalRewardsFinalized =
-            unfinalizedState.totalRewardsFinalized =
+            state.totalRewardsFinalized =
-                state.totalRewardsFinalized =
+            state.totalRewardsFinalized.safeAdd(totalReward);
-                state.totalRewardsFinalized.safeAdd(totalReward);
-        }
 
         // Decrease the number of unfinalized pools left.
         unfinalizedState.poolsRemaining =
@@ -159,8 +166,6 @@ contract MixinFinalizer is
                 state.rewardsAvailable.safeSub(state.totalRewardsFinalized)
             );
         }
-        membersStake = pool.membersStake;
-        return (operatorReward, membersReward, membersStake);
     }
 
     /// @dev Computes the reward owed to a pool during finalization.
@@ -279,46 +284,4 @@ contract MixinFinalizer is
             rewards = rewardsRemaining;
         }
     }
-
-    /// @dev Credits finalization rewards to a pool that was active in the
-    ///      last epoch.
-    /// @param epoch The current epoch.
-    /// @param poolId The pool ID to finalize.
-    /// @param pool The active pool to finalize.
-    /// @param state The current state of finalization.
-    /// @return operatorReward The reward credited to the pool operator.
-    /// @return membersReward The reward credited to the pool members.
-    function _creditRewardsToPool(
-        uint256 epoch,
-        bytes32 poolId,
-        IStructs.ActivePool memory pool,
-        IStructs.UnfinalizedState memory state
-    )
-        private
-        returns (uint256 operatorReward, uint256 membersReward)
-    {
-        // Clear the pool state so we don't finalize it again, and to recoup
-        // some gas.
-        delete _getActivePoolsFromEpoch(epoch.safeSub(1))[poolId];
-
-        // Compute the rewards.
-        uint256 rewards = _getUnfinalizedPoolRewardsFromState(pool, state);
-
-        // Pay the pool.
-        // Note that we credit at the CURRENT epoch even though these rewards
-        // were earned in the previous epoch.
-        (operatorReward, membersReward) = _syncPoolRewards(
-            poolId,
-            rewards,
-            pool.membersStake
-        );
-
-        // Emit an event.
-        emit RewardsPaid(
-            epoch,
-            poolId,
-            operatorReward,
-            membersReward
-        );
-    }
 }

contracts/staking/contracts/test/TestDelegatorRewards.sol

@@ -177,11 +177,6 @@ contract TestDelegatorRewards is
     ///      the current epoch and emit a event,
     function finalizePool(bytes32 poolId)
         public
-        returns (
-            uint256 operatorReward,
-            uint256 membersReward,
-            uint256 membersStake
-        )
     {
         UnfinalizedPoolReward memory reward = unfinalizedPoolRewardsByEpoch[currentEpoch][poolId];
         delete unfinalizedPoolRewardsByEpoch[currentEpoch][poolId];
@@ -189,8 +184,8 @@ contract TestDelegatorRewards is
         _setOperatorShare(poolId, reward.operatorReward, reward.membersReward);
 
         uint256 totalRewards = reward.operatorReward + reward.membersReward;
-        membersStake = reward.membersStake;
+        uint256 membersStake = reward.membersStake;
-        (operatorReward, membersReward) =
+        (uint256 operatorReward, uint256 membersReward) =
             _syncPoolRewards(poolId, totalRewards, membersStake);
         emit FinalizePool(poolId, operatorReward, membersReward, membersStake);
     }

contracts/staking/test/cumulative_reward_tracking_test.ts

@@ -53,7 +53,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 +68,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 +268,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 +303,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 () => {