From 410b9c50d3ed8d71cac28936820e6542e7b598b8 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 24 Sep 2019 23:01:31 -0700 Subject: [PATCH 1/9] Flatten _withdrawAndSyncDelegatorRewards function, fix bug where cumulative rewards are always reset --- .../contracts/src/stake/MixinStake.sol | 16 +-- .../staking_pools/MixinCumulativeRewards.sol | 57 ++-------- .../staking_pools/MixinStakingPoolRewards.sol | 100 +++++++----------- 3 files changed, 60 insertions(+), 113 deletions(-) diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index 05329b1806..013aa9167e 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -188,7 +188,7 @@ contract MixinStake is _assertStakingPoolExists(poolId); // Cache amount delegated to pool by staker. - IStructs.StoredBalance memory initDelegatedStakeToPoolByOwner = + IStructs.StoredBalance memory initDelegatedStake = _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); // Increment how much stake the staker has delegated to the input pool. @@ -202,14 +202,14 @@ contract MixinStake is // Synchronizes reward state in the pool that the owner is delegating // to. - IStructs.StoredBalance memory finalDelegatedStakeToPoolByOwner = + IStructs.StoredBalance memory finalDelegatedStake = _loadSyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); _withdrawAndSyncDelegatorRewards( poolId, staker, - initDelegatedStakeToPoolByOwner, - finalDelegatedStakeToPoolByOwner + initDelegatedStake, + finalDelegatedStake ); } @@ -228,7 +228,7 @@ contract MixinStake is _assertStakingPoolExists(poolId); // cache amount delegated to pool by staker - IStructs.StoredBalance memory initDelegatedStakeToPoolByOwner = + IStructs.StoredBalance memory initDelegatedStake = _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); // decrement how much stake the staker has delegated to the input pool @@ -242,14 +242,14 @@ contract MixinStake is // synchronizes reward state in the pool that the owner is undelegating // from - IStructs.StoredBalance memory finalDelegatedStakeToPoolByOwner = + IStructs.StoredBalance memory finalDelegatedStake = _loadSyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); _withdrawAndSyncDelegatorRewards( poolId, staker, - initDelegatedStakeToPoolByOwner, - finalDelegatedStakeToPoolByOwner + initDelegatedStake, + finalDelegatedStake ); } diff --git a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol index 8df5fba0b6..45d1bb1b12 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol @@ -57,27 +57,6 @@ contract MixinCumulativeRewards is return cumulativeReward.denominator != 0; } - /// @dev Tries to set a cumulative reward for `poolId` at `epoch`. - /// @param poolId Unique Id of pool. - /// @param epoch Epoch of cumulative reward. - /// @param value Value of cumulative reward. - function _trySetCumulativeReward( - bytes32 poolId, - uint256 epoch, - IStructs.Fraction memory value - ) - internal - { - // Do nothing if it's in the past since we don't want to - // rewrite history. - if (epoch < currentEpoch - && _isCumulativeRewardSet(_cumulativeRewardsByPool[poolId][epoch])) - { - return; - } - _forceSetCumulativeReward(poolId, epoch, value); - } - /// @dev Sets a cumulative reward for `poolId` at `epoch`. /// This can be used to overwrite an existing value. /// @param poolId Unique Id of pool. @@ -95,33 +74,19 @@ contract MixinCumulativeRewards is // Never set the most recent reward epoch to one in the future, because // it may get removed if there are no more dependencies on it. if (epoch <= currentEpoch) { - _trySetMostRecentCumulativeRewardEpoch(poolId, epoch); - } - } + // Check if we should do any work + uint256 currentMostRecentEpoch = _cumulativeRewardsByPoolLastStored[poolId]; + if (epoch == currentMostRecentEpoch) { + return; + } - /// @dev Tries to set the epoch of the most recent cumulative reward. - /// The value will only be set if the input epoch is greater than the - /// current most recent value. - /// @param poolId Unique Id of pool. - /// @param epoch Epoch of the most recent cumulative reward. - function _trySetMostRecentCumulativeRewardEpoch( - bytes32 poolId, - uint256 epoch - ) - internal - { - // Check if we should do any work - uint256 currentMostRecentEpoch = _cumulativeRewardsByPoolLastStored[poolId]; - if (epoch == currentMostRecentEpoch) { - return; + // Update state to reflect the most recent cumulative reward + _forceSetMostRecentCumulativeRewardEpoch( + poolId, + currentMostRecentEpoch, + epoch + ); } - - // Update state to reflect the most recent cumulative reward - _forceSetMostRecentCumulativeRewardEpoch( - poolId, - currentMostRecentEpoch, - epoch - ); } /// @dev Forcefully sets the epoch of the most recent cumulative reward. diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index e03c10d386..5aba12522a 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -44,21 +44,23 @@ contract MixinStakingPoolRewards is { address member = msg.sender; - IStructs.StoredBalance memory finalDelegatedStakeToPoolByOwner = + IStructs.StoredBalance memory finalDelegatedStake = _loadSyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]); + IStructs.StoredBalance memory initDelegatedStake = + _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]); + _withdrawAndSyncDelegatorRewards( poolId, member, - // Initial balance - _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]), - finalDelegatedStakeToPoolByOwner + initDelegatedStake, + finalDelegatedStake ); // Update stored balance with synchronized version; this prevents // redundant withdrawals. _delegatedStakeToPoolByOwner[member][poolId] = - finalDelegatedStakeToPoolByOwner; + finalDelegatedStake; } /// @dev Computes the reward balance in ETH of the operator of a pool. @@ -118,32 +120,45 @@ contract MixinStakingPoolRewards is /// withdrawing rewards and adding/removing dependencies on cumulative rewards. /// @param poolId Unique id of pool. /// @param member of the pool. - /// @param initialDelegatedStakeToPoolByOwner The member's delegated + /// @param initDelegatedStake The member's delegated /// balance at the beginning of this transaction. - /// @param finalDelegatedStakeToPoolByOwner The member's delegated balance + /// @param finalDelegatedStake The member's delegated balance /// at the end of this transaction. function _withdrawAndSyncDelegatorRewards( bytes32 poolId, address member, - IStructs.StoredBalance memory initialDelegatedStakeToPoolByOwner, - IStructs.StoredBalance memory finalDelegatedStakeToPoolByOwner + IStructs.StoredBalance memory initDelegatedStake, + IStructs.StoredBalance memory finalDelegatedStake ) internal { - // Withdraw any rewards from. - // this must be done before we can modify the staker's portion of the - // delegator pool. - _finalizePoolAndWithdrawDelegatorRewards( + // Ensure the pool is finalized. + finalizePool(poolId); + + // Compute balance owed to delegator + uint256 balance = _computeDelegatorReward( poolId, - member, - initialDelegatedStakeToPoolByOwner + initDelegatedStake, + // No unfinalized values because we ensured the pool is already + // finalized. + 0, + 0 ); + // Withdraw non-0 balance + if (balance != 0) { + // Decrease the balance of the pool + _decreasePoolRewards(poolId, balance); + + // Withdraw the member's WETH balance + getWethContract().transfer(member, balance); + } + // Add dependencies on cumulative rewards for this epoch and the next // epoch, if necessary. _setCumulativeRewardDependenciesForDelegator( poolId, - finalDelegatedStakeToPoolByOwner + finalDelegatedStake ); } @@ -244,41 +259,6 @@ contract MixinStakingPoolRewards is return (operatorReward, membersReward); } - /// @dev Transfers a delegators accumulated rewards to the delegator. - /// This is required before the member's stake in the pool can be modified. - /// @param poolId Unique id of pool. - /// @param member The member of the pool. - /// @param unsyncedStake Unsynced stake of the delegator to the pool. - function _finalizePoolAndWithdrawDelegatorRewards( - bytes32 poolId, - address member, - IStructs.StoredBalance memory unsyncedStake - ) - private - { - // Ensure the pool is finalized. - finalizePool(poolId); - - // Compute balance owed to delegator - uint256 balance = _computeDelegatorReward( - poolId, - unsyncedStake, - // No unfinalized values because we ensured the pool is already - // finalized. - 0, - 0 - ); - if (balance == 0) { - return; - } - - // Decrease the balance of the pool - _decreasePoolRewards(poolId, balance); - - // Withdraw the member's WETH balance - getWethContract().transfer(member, balance); - } - /// @dev Computes the reward balance in ETH of a specific member of a pool. /// @param poolId Unique id of pool. /// @param unsyncedStake Unsynced delegated stake to pool by staker @@ -388,11 +368,11 @@ contract MixinStakingPoolRewards is /// 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 _delegatedStakeToPoolByOwner Amount of stake the member has + /// @param finalDelegatedStake Amount of stake the member has /// delegated to the pool. function _setCumulativeRewardDependenciesForDelegator( bytes32 poolId, - IStructs.StoredBalance memory _delegatedStakeToPoolByOwner + IStructs.StoredBalance memory finalDelegatedStake ) private { @@ -402,16 +382,18 @@ contract MixinStakingPoolRewards is // The delegator depends on the Cumulative Reward for this epoch // only if they are currently staked or will be staked next epoch. - if (_delegatedStakeToPoolByOwner.currentEpochBalance == 0 && _delegatedStakeToPoolByOwner.nextEpochBalance == 0) { + if (finalDelegatedStake.currentEpochBalance == 0 && finalDelegatedStake.nextEpochBalance == 0) { return; } // Delegator depends on the Cumulative Reward for this epoch - ensure it is set. - _trySetCumulativeReward( - poolId, - _delegatedStakeToPoolByOwner.currentEpoch, - mostRecentCumulativeReward - ); + if (!_isCumulativeRewardSet(_cumulativeRewardsByPool[poolId][finalDelegatedStake.currentEpoch])) { + _forceSetCumulativeReward( + poolId, + finalDelegatedStake.currentEpoch, + mostRecentCumulativeReward + ); + } } /// @dev Increases rewards for a pool. From c1871b5bca204926a6547042e59676b6aa655bbd Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 24 Sep 2019 23:01:44 -0700 Subject: [PATCH 2/9] Fix xumulative reward tests --- contracts/staking/test/cumulative_reward_tracking_test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/staking/test/cumulative_reward_tracking_test.ts b/contracts/staking/test/cumulative_reward_tracking_test.ts index e23191ee6d..5e1df454f4 100644 --- a/contracts/staking/test/cumulative_reward_tracking_test.ts +++ b/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 () => { From d07005dcbe7d5187e232dd0a08a06a983612d6d6 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 24 Sep 2019 23:04:37 -0700 Subject: [PATCH 3/9] Change increment -> increase and decrement -> decrease --- .../staking/contracts/src/stake/MixinStake.sol | 16 ++++++++-------- .../contracts/src/stake/MixinStakeStorage.sol | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index 013aa9167e..0ed5fe3f5d 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -42,10 +42,10 @@ contract MixinStake is getZrxVault().depositFrom(staker, amount); // mint stake - _incrementCurrentAndNextBalance(_activeStakeByOwner[staker], amount); + _increaseCurrentAndNextBalance(_activeStakeByOwner[staker], amount); // update global total of active stake - _incrementCurrentAndNextBalance(globalStakeByStatus[uint8(IStructs.StakeStatus.ACTIVE)], amount); + _increaseCurrentAndNextBalance(globalStakeByStatus[uint8(IStructs.StakeStatus.ACTIVE)], amount); // notify emit Stake( @@ -75,10 +75,10 @@ contract MixinStake is } // burn inactive stake - _decrementCurrentAndNextBalance(_inactiveStakeByOwner[staker], amount); + _decreaseCurrentAndNextBalance(_inactiveStakeByOwner[staker], amount); // update global total of inactive stake - _decrementCurrentAndNextBalance(globalStakeByStatus[uint8(IStructs.StakeStatus.INACTIVE)], amount); + _decreaseCurrentAndNextBalance(globalStakeByStatus[uint8(IStructs.StakeStatus.INACTIVE)], amount); // update withdrawable field _withdrawableStakeByOwner[staker] = @@ -192,13 +192,13 @@ contract MixinStake is _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); // Increment how much stake the staker has delegated to the input pool. - _incrementNextBalance( + _increaseNextBalance( _delegatedStakeToPoolByOwner[staker][poolId], amount ); // Increment how much stake has been delegated to pool. - _incrementNextBalance(_delegatedStakeByPoolId[poolId], amount); + _increaseNextBalance(_delegatedStakeByPoolId[poolId], amount); // Synchronizes reward state in the pool that the owner is delegating // to. @@ -232,13 +232,13 @@ contract MixinStake is _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); // decrement how much stake the staker has delegated to the input pool - _decrementNextBalance( + _decreaseNextBalance( _delegatedStakeToPoolByOwner[staker][poolId], amount ); // decrement how much stake has been delegated to pool - _decrementNextBalance(_delegatedStakeByPoolId[poolId], amount); + _decreaseNextBalance(_delegatedStakeByPoolId[poolId], amount); // synchronizes reward state in the pool that the owner is undelegating // from diff --git a/contracts/staking/contracts/src/stake/MixinStakeStorage.sol b/contracts/staking/contracts/src/stake/MixinStakeStorage.sol index 23ea0c7404..6588a0cc89 100644 --- a/contracts/staking/contracts/src/stake/MixinStakeStorage.sol +++ b/contracts/staking/contracts/src/stake/MixinStakeStorage.sol @@ -111,7 +111,7 @@ contract MixinStakeStorage is /// @dev Increments both the `current` and `next` fields. /// @param balancePtr storage pointer to balance. /// @param amount to mint. - function _incrementCurrentAndNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) + function _increaseCurrentAndNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) internal { // Remove stake from balance @@ -126,7 +126,7 @@ contract MixinStakeStorage is /// @dev Decrements both the `current` and `next` fields. /// @param balancePtr storage pointer to balance. /// @param amount to mint. - function _decrementCurrentAndNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) + function _decreaseCurrentAndNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) internal { // Remove stake from balance @@ -141,7 +141,7 @@ contract MixinStakeStorage is /// @dev Increments the `next` field (but not the `current` field). /// @param balancePtr storage pointer to balance. /// @param amount to increment by. - function _incrementNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) + function _increaseNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) internal { // Add stake to balance @@ -155,7 +155,7 @@ contract MixinStakeStorage is /// @dev Decrements the `next` field (but not the `current` field). /// @param balancePtr storage pointer to balance. /// @param amount to decrement by. - function _decrementNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) + function _decreaseNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) internal { // Remove stake from balance From 22a6de48ae3bc133f7d3f21ad506f7794127e8bb Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 25 Sep 2019 10:43:14 -0700 Subject: [PATCH 4/9] Remove arguments that are unnecessarily passed around --- .../contracts/src/stake/MixinStake.sol | 26 +--------- .../staking_pools/MixinStakingPoolRewards.sol | 47 ++++++++----------- .../contracts/test/TestDelegatorRewards.sol | 15 ++---- 3 files changed, 24 insertions(+), 64 deletions(-) diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index 0ed5fe3f5d..6dc3079500 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -187,10 +187,6 @@ contract MixinStake is // Sanity check the pool we're delegating to exists. _assertStakingPoolExists(poolId); - // Cache amount delegated to pool by staker. - IStructs.StoredBalance memory initDelegatedStake = - _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); - // Increment how much stake the staker has delegated to the input pool. _increaseNextBalance( _delegatedStakeToPoolByOwner[staker][poolId], @@ -200,16 +196,9 @@ contract MixinStake is // Increment how much stake has been delegated to pool. _increaseNextBalance(_delegatedStakeByPoolId[poolId], amount); - // Synchronizes reward state in the pool that the owner is delegating - // to. - IStructs.StoredBalance memory finalDelegatedStake = - _loadSyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); - _withdrawAndSyncDelegatorRewards( poolId, - staker, - initDelegatedStake, - finalDelegatedStake + staker ); } @@ -227,10 +216,6 @@ contract MixinStake is // sanity check the pool we're undelegating from exists _assertStakingPoolExists(poolId); - // cache amount delegated to pool by staker - IStructs.StoredBalance memory initDelegatedStake = - _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); - // decrement how much stake the staker has delegated to the input pool _decreaseNextBalance( _delegatedStakeToPoolByOwner[staker][poolId], @@ -240,16 +225,9 @@ contract MixinStake is // decrement how much stake has been delegated to pool _decreaseNextBalance(_delegatedStakeByPoolId[poolId], amount); - // synchronizes reward state in the pool that the owner is undelegating - // from - IStructs.StoredBalance memory finalDelegatedStake = - _loadSyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); - _withdrawAndSyncDelegatorRewards( poolId, - staker, - initDelegatedStake, - finalDelegatedStake + staker ); } diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index 5aba12522a..f9e3c0a943 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -44,23 +44,15 @@ contract MixinStakingPoolRewards is { address member = msg.sender; - IStructs.StoredBalance memory finalDelegatedStake = - _loadSyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]); - - IStructs.StoredBalance memory initDelegatedStake = - _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]); - _withdrawAndSyncDelegatorRewards( poolId, - member, - initDelegatedStake, - finalDelegatedStake + member ); // Update stored balance with synchronized version; this prevents // redundant withdrawals. _delegatedStakeToPoolByOwner[member][poolId] = - finalDelegatedStake; + _loadSyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]); } /// @dev Computes the reward balance in ETH of the operator of a pool. @@ -110,7 +102,7 @@ contract MixinStakingPoolRewards is ); return _computeDelegatorReward( poolId, - _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]), + member, unfinalizedMembersReward, unfinalizedMembersStake ); @@ -120,15 +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 initDelegatedStake The member's delegated - /// balance at the beginning of this transaction. - /// @param finalDelegatedStake The member's delegated balance - /// at the end of this transaction. function _withdrawAndSyncDelegatorRewards( bytes32 poolId, - address member, - IStructs.StoredBalance memory initDelegatedStake, - IStructs.StoredBalance memory finalDelegatedStake + address member ) internal { @@ -138,7 +124,7 @@ contract MixinStakingPoolRewards is // Compute balance owed to delegator uint256 balance = _computeDelegatorReward( poolId, - initDelegatedStake, + member, // No unfinalized values because we ensured the pool is already // finalized. 0, @@ -158,7 +144,7 @@ contract MixinStakingPoolRewards is // epoch, if necessary. _setCumulativeRewardDependenciesForDelegator( poolId, - finalDelegatedStake + member ); } @@ -261,13 +247,13 @@ contract MixinStakingPoolRewards is /// @dev Computes the reward balance in ETH of a specific member of a pool. /// @param poolId Unique id of pool. - /// @param unsyncedStake Unsynced delegated stake to pool by staker + /// @param member of the pool. /// @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, - IStructs.StoredBalance memory unsyncedStake, + address member, uint256 unfinalizedMembersReward, uint256 unfinalizedMembersStake ) @@ -282,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. @@ -368,17 +357,15 @@ contract MixinStakingPoolRewards is /// 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 finalDelegatedStake Amount of stake the member has - /// delegated to the pool. + /// @param member of the pool. function _setCumulativeRewardDependenciesForDelegator( bytes32 poolId, - IStructs.StoredBalance memory finalDelegatedStake + address member ) private { - // Get the most recent cumulative reward, which will serve as a - // reference point when updating dependencies - IStructs.Fraction memory mostRecentCumulativeReward = _getMostRecentCumulativeReward(poolId); + 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. @@ -386,6 +373,10 @@ contract MixinStakingPoolRewards is 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( diff --git a/contracts/staking/contracts/test/TestDelegatorRewards.sol b/contracts/staking/contracts/test/TestDelegatorRewards.sol index 5648c97f9e..2c4aa80c7a 100644 --- a/contracts/staking/contracts/test/TestDelegatorRewards.sol +++ b/contracts/staking/contracts/test/TestDelegatorRewards.sol @@ -110,7 +110,6 @@ contract TestDelegatorRewards is external { _initGenesisCumulativeRewards(poolId); - IStructs.StoredBalance memory initialStake = _delegatedStakeToPoolByOwner[delegator][poolId]; IStructs.StoredBalance storage _stake = _delegatedStakeToPoolByOwner[delegator][poolId]; _stake.isInitialized = true; _stake.currentEpochBalance += uint96(stake); @@ -118,9 +117,7 @@ contract TestDelegatorRewards is _stake.currentEpoch = uint32(currentEpoch); _withdrawAndSyncDelegatorRewards( poolId, - delegator, - initialStake, - _stake + delegator ); } @@ -135,7 +132,6 @@ contract TestDelegatorRewards is external { _initGenesisCumulativeRewards(poolId); - IStructs.StoredBalance memory initialStake = _delegatedStakeToPoolByOwner[delegator][poolId]; IStructs.StoredBalance storage _stake = _delegatedStakeToPoolByOwner[delegator][poolId]; if (_stake.currentEpoch < currentEpoch) { _stake.currentEpochBalance = _stake.nextEpochBalance; @@ -145,9 +141,7 @@ contract TestDelegatorRewards is _stake.currentEpoch = uint32(currentEpoch); _withdrawAndSyncDelegatorRewards( poolId, - delegator, - initialStake, - _stake + delegator ); } @@ -162,7 +156,6 @@ contract TestDelegatorRewards is external { _initGenesisCumulativeRewards(poolId); - IStructs.StoredBalance memory initialStake = _delegatedStakeToPoolByOwner[delegator][poolId]; IStructs.StoredBalance storage _stake = _delegatedStakeToPoolByOwner[delegator][poolId]; if (_stake.currentEpoch < currentEpoch) { _stake.currentEpochBalance = _stake.nextEpochBalance; @@ -172,9 +165,7 @@ contract TestDelegatorRewards is _stake.currentEpoch = uint32(currentEpoch); _withdrawAndSyncDelegatorRewards( poolId, - delegator, - initialStake, - _stake + delegator ); } From 25cb1c11388abe73cabcde095ae26e29864f23b0 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 25 Sep 2019 10:58:16 -0700 Subject: [PATCH 5/9] Remove epoch param that is always equivalent to currentEpoch --- .../staking_pools/MixinCumulativeRewards.sol | 32 ++++++++----------- .../staking_pools/MixinStakingPoolRewards.sol | 2 -- .../test/TestCumulativeRewardTracking.sol | 4 +-- 3 files changed, 14 insertions(+), 24 deletions(-) diff --git a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol index 45d1bb1b12..755f706ced 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol @@ -37,7 +37,6 @@ contract MixinCumulativeRewards is // Sets the default cumulative reward _forceSetCumulativeReward( poolId, - currentEpoch, IStructs.Fraction({ numerator: 0, denominator: MIN_TOKEN_VALUE @@ -60,33 +59,28 @@ contract MixinCumulativeRewards is /// @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 epoch Epoch of cumulative reward. /// @param value Value of cumulative reward. function _forceSetCumulativeReward( bytes32 poolId, - uint256 epoch, IStructs.Fraction memory value ) internal { - _cumulativeRewardsByPool[poolId][epoch] = value; + uint256 currentEpoch_ = currentEpoch; + _cumulativeRewardsByPool[poolId][currentEpoch_] = value; - // Never set the most recent reward epoch to one in the future, because - // it may get removed if there are no more dependencies on it. - if (epoch <= currentEpoch) { - // Check if we should do any work - uint256 currentMostRecentEpoch = _cumulativeRewardsByPoolLastStored[poolId]; - if (epoch == currentMostRecentEpoch) { - return; - } - - // Update state to reflect the most recent cumulative reward - _forceSetMostRecentCumulativeRewardEpoch( - poolId, - currentMostRecentEpoch, - epoch - ); + // Check if we should do any work + uint256 currentMostRecentEpoch = _cumulativeRewardsByPoolLastStored[poolId]; + if (currentEpoch_ == currentMostRecentEpoch) { + return; } + + // Update state to reflect the most recent cumulative reward + _forceSetMostRecentCumulativeRewardEpoch( + poolId, + currentMostRecentEpoch, + currentEpoch_ + ); } /// @dev Forcefully sets the epoch of the most recent cumulative reward. diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index f9e3c0a943..99274b54e8 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -206,7 +206,6 @@ contract MixinStakingPoolRewards is // Store cumulative rewards for this epoch. _forceSetCumulativeReward( poolId, - currentEpoch, cumulativeReward ); } @@ -381,7 +380,6 @@ contract MixinStakingPoolRewards is if (!_isCumulativeRewardSet(_cumulativeRewardsByPool[poolId][finalDelegatedStake.currentEpoch])) { _forceSetCumulativeReward( poolId, - finalDelegatedStake.currentEpoch, mostRecentCumulativeReward ); } diff --git a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol index 6409ada717..a57f6c329e 100644 --- a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol +++ b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol @@ -51,15 +51,13 @@ contract TestCumulativeRewardTracking is function _forceSetCumulativeReward( bytes32 poolId, - uint256 epoch, IStructs.Fraction memory value ) internal { - emit SetCumulativeReward(poolId, epoch); + emit SetCumulativeReward(poolId, currentEpoch); MixinCumulativeRewards._forceSetCumulativeReward( poolId, - epoch, value ); } From 44400754255ee55271b2469fba9ff6aaf45ba1f0 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 25 Sep 2019 11:03:26 -0700 Subject: [PATCH 6/9] Inline _forceSetMostRecentCumulativeRewardEpoch --- .../staking_pools/MixinCumulativeRewards.sol | 25 ++----------------- .../test/TestCumulativeRewardTracking.sol | 20 --------------- .../cumulative_reward_tracking_simulation.ts | 7 +----- 3 files changed, 3 insertions(+), 49 deletions(-) diff --git a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol index 755f706ced..21368fc093 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol @@ -76,29 +76,8 @@ contract MixinCumulativeRewards is } // Update state to reflect the most recent cumulative reward - _forceSetMostRecentCumulativeRewardEpoch( - poolId, - currentMostRecentEpoch, - currentEpoch_ - ); - } - - /// @dev Forcefully sets the epoch of the most recent cumulative reward. - /// @param poolId Unique Id of pool. - /// @param currentMostRecentEpoch Epoch of the most recent cumulative - /// reward. - /// @param newMostRecentEpoch Epoch of the new most recent cumulative - /// reward. - function _forceSetMostRecentCumulativeRewardEpoch( - bytes32 poolId, - uint256 currentMostRecentEpoch, - uint256 newMostRecentEpoch - ) - internal - { - // Sanity check that we're not trying to go back in time - assert(newMostRecentEpoch >= currentMostRecentEpoch); - _cumulativeRewardsByPoolLastStored[poolId] = newMostRecentEpoch; + assert(currentEpoch_ > currentMostRecentEpoch); + _cumulativeRewardsByPoolLastStored[poolId] = currentEpoch_; } /// @dev Computes a member's reward over a given epoch interval. diff --git a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol index a57f6c329e..8e74842989 100644 --- a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol +++ b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol @@ -30,11 +30,6 @@ contract TestCumulativeRewardTracking is uint256 epoch ); - event SetMostRecentCumulativeReward( - bytes32 poolId, - uint256 epoch - ); - constructor( address wethAddress, address zrxVaultAddress @@ -61,19 +56,4 @@ contract TestCumulativeRewardTracking is value ); } - - function _forceSetMostRecentCumulativeRewardEpoch( - bytes32 poolId, - uint256 currentMostRecentEpoch, - uint256 newMostRecentEpoch - ) - internal - { - emit SetMostRecentCumulativeReward(poolId, newMostRecentEpoch); - MixinCumulativeRewards._forceSetMostRecentCumulativeRewardEpoch( - poolId, - currentMostRecentEpoch, - newMostRecentEpoch - ); - } } diff --git a/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts b/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts index 516c0ffd31..675f079f25 100644 --- a/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts +++ b/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts @@ -36,12 +36,7 @@ export class CumulativeRewardTrackingSimulation { private static _extractTestLogs(txReceiptLogs: DecodedLogs): TestLog[] { const logs = []; for (const log of txReceiptLogs) { - if (log.event === TestCumulativeRewardTrackingEvents.SetMostRecentCumulativeReward) { - logs.push({ - event: log.event, - epoch: log.args.epoch.toNumber(), - }); - } else if (log.event === TestCumulativeRewardTrackingEvents.SetCumulativeReward) { + if (log.event === TestCumulativeRewardTrackingEvents.SetCumulativeReward) { logs.push({ event: log.event, epoch: log.args.epoch.toNumber(), From 9e3331d0188714eaec1c6f0553fc04841eb79f1f Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 25 Sep 2019 11:31:30 -0700 Subject: [PATCH 7/9] Fix ordering of function calls, remove optimization in _forceSetCumulativeReward --- .../contracts/src/stake/MixinStake.sol | 20 +++++++++---------- .../staking_pools/MixinCumulativeRewards.sol | 11 +++------- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index 6dc3079500..a77350fbd5 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -187,6 +187,11 @@ contract MixinStake is // Sanity check the pool we're delegating to exists. _assertStakingPoolExists(poolId); + _withdrawAndSyncDelegatorRewards( + poolId, + staker + ); + // Increment how much stake the staker has delegated to the input pool. _increaseNextBalance( _delegatedStakeToPoolByOwner[staker][poolId], @@ -195,11 +200,6 @@ contract MixinStake is // Increment how much stake has been delegated to pool. _increaseNextBalance(_delegatedStakeByPoolId[poolId], amount); - - _withdrawAndSyncDelegatorRewards( - poolId, - staker - ); } /// @dev Un-Delegates a owners stake from a staking pool. @@ -216,6 +216,11 @@ contract MixinStake is // sanity check the pool we're undelegating from exists _assertStakingPoolExists(poolId); + _withdrawAndSyncDelegatorRewards( + poolId, + staker + ); + // decrement how much stake the staker has delegated to the input pool _decreaseNextBalance( _delegatedStakeToPoolByOwner[staker][poolId], @@ -224,11 +229,6 @@ contract MixinStake is // decrement how much stake has been delegated to pool _decreaseNextBalance(_delegatedStakeByPoolId[poolId], amount); - - _withdrawAndSyncDelegatorRewards( - poolId, - staker - ); } /// @dev Returns a storage pointer to a user's stake in a given status. diff --git a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol index 21368fc093..ed5626049d 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol @@ -68,15 +68,10 @@ contract MixinCumulativeRewards is { uint256 currentEpoch_ = currentEpoch; _cumulativeRewardsByPool[poolId][currentEpoch_] = value; - - // Check if we should do any work - uint256 currentMostRecentEpoch = _cumulativeRewardsByPoolLastStored[poolId]; - if (currentEpoch_ == currentMostRecentEpoch) { - return; - } - + // Update state to reflect the most recent cumulative reward - assert(currentEpoch_ > currentMostRecentEpoch); + uint256 currentMostRecentEpoch = _cumulativeRewardsByPoolLastStored[poolId]; + assert(currentEpoch_ >= currentMostRecentEpoch); _cumulativeRewardsByPoolLastStored[poolId] = currentEpoch_; } From 5b77e2c8aca435d6f9aa52ab1157f807f812a775 Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Wed, 25 Sep 2019 15:29:22 -0400 Subject: [PATCH 8/9] `@0x/contracts-staking`: Fix stake accounting. --- .../contracts/src/stake/MixinStake.sol | 26 +++++++++++++------ .../staking_pools/MixinStakingPoolRewards.sol | 15 +++++++---- .../contracts/test/TestDelegatorRewards.sol | 14 +++++++--- .../test/cumulative_reward_tracking_test.ts | 24 ++++++++--------- 4 files changed, 50 insertions(+), 29 deletions(-) diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index a77350fbd5..930003161a 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -187,10 +187,9 @@ contract MixinStake is // Sanity check the pool we're delegating to exists. _assertStakingPoolExists(poolId); - _withdrawAndSyncDelegatorRewards( - poolId, - staker - ); + // Cache amount delegated to pool by staker. + IStructs.StoredBalance memory initDelegatedStakeToPoolByOwner = + _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); // Increment how much stake the staker has delegated to the input pool. _increaseNextBalance( @@ -200,6 +199,12 @@ 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. @@ -216,10 +221,9 @@ contract MixinStake is // sanity check the pool we're undelegating from exists _assertStakingPoolExists(poolId); - _withdrawAndSyncDelegatorRewards( - poolId, - staker - ); + // Cache amount delegated to pool by staker. + IStructs.StoredBalance memory initDelegatedStakeToPoolByOwner = + _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); // decrement how much stake the staker has delegated to the input pool _decreaseNextBalance( @@ -229,6 +233,12 @@ 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. diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index 99274b54e8..7d70b50d72 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -46,7 +46,8 @@ contract MixinStakingPoolRewards is _withdrawAndSyncDelegatorRewards( poolId, - member + member, + _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]) ); // Update stored balance with synchronized version; this prevents @@ -103,6 +104,7 @@ contract MixinStakingPoolRewards is return _computeDelegatorReward( poolId, member, + _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]), unfinalizedMembersReward, unfinalizedMembersStake ); @@ -112,9 +114,12 @@ 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 + address member, + IStructs.StoredBalance memory unsyncedStake ) internal { @@ -125,6 +130,7 @@ contract MixinStakingPoolRewards is uint256 balance = _computeDelegatorReward( poolId, member, + unsyncedStake, // No unfinalized values because we ensured the pool is already // finalized. 0, @@ -247,12 +253,14 @@ 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 ) @@ -267,9 +275,6 @@ 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. diff --git a/contracts/staking/contracts/test/TestDelegatorRewards.sol b/contracts/staking/contracts/test/TestDelegatorRewards.sol index 2c4aa80c7a..a782d63a4f 100644 --- a/contracts/staking/contracts/test/TestDelegatorRewards.sol +++ b/contracts/staking/contracts/test/TestDelegatorRewards.sol @@ -86,7 +86,7 @@ contract TestDelegatorRewards is _poolById[poolId].operator = operatorAddress; _setOperatorShare(poolId, operatorReward, membersReward); _initGenesisCumulativeRewards(poolId); - + _syncPoolRewards( poolId, operatorReward + membersReward, @@ -110,6 +110,7 @@ contract TestDelegatorRewards is external { _initGenesisCumulativeRewards(poolId); + IStructs.StoredBalance memory initialStake = _delegatedStakeToPoolByOwner[delegator][poolId]; IStructs.StoredBalance storage _stake = _delegatedStakeToPoolByOwner[delegator][poolId]; _stake.isInitialized = true; _stake.currentEpochBalance += uint96(stake); @@ -117,7 +118,8 @@ contract TestDelegatorRewards is _stake.currentEpoch = uint32(currentEpoch); _withdrawAndSyncDelegatorRewards( poolId, - delegator + delegator, + initialStake ); } @@ -132,6 +134,7 @@ contract TestDelegatorRewards is external { _initGenesisCumulativeRewards(poolId); + IStructs.StoredBalance memory initialStake = _delegatedStakeToPoolByOwner[delegator][poolId]; IStructs.StoredBalance storage _stake = _delegatedStakeToPoolByOwner[delegator][poolId]; if (_stake.currentEpoch < currentEpoch) { _stake.currentEpochBalance = _stake.nextEpochBalance; @@ -141,7 +144,8 @@ contract TestDelegatorRewards is _stake.currentEpoch = uint32(currentEpoch); _withdrawAndSyncDelegatorRewards( poolId, - delegator + delegator, + initialStake ); } @@ -156,6 +160,7 @@ contract TestDelegatorRewards is external { _initGenesisCumulativeRewards(poolId); + IStructs.StoredBalance memory initialStake = _delegatedStakeToPoolByOwner[delegator][poolId]; IStructs.StoredBalance storage _stake = _delegatedStakeToPoolByOwner[delegator][poolId]; if (_stake.currentEpoch < currentEpoch) { _stake.currentEpochBalance = _stake.nextEpochBalance; @@ -165,7 +170,8 @@ contract TestDelegatorRewards is _stake.currentEpoch = uint32(currentEpoch); _withdrawAndSyncDelegatorRewards( poolId, - delegator + delegator, + initialStake ); } diff --git a/contracts/staking/test/cumulative_reward_tracking_test.ts b/contracts/staking/test/cumulative_reward_tracking_test.ts index 5e1df454f4..c4ba1d3235 100644 --- a/contracts/staking/test/cumulative_reward_tracking_test.ts +++ b/contracts/staking/test/cumulative_reward_tracking_test.ts @@ -39,7 +39,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { await simulation.runTestAsync( [TestAction.Finalize], [TestAction.CreatePool], - [{ event: 'SetCumulativeReward', epoch: 1 }, { event: 'SetMostRecentCumulativeReward', epoch: 1 }], + [{ event: 'SetCumulativeReward', epoch: 1 }], ); }); it('delegating in the same epoch pool is created', async () => { @@ -86,7 +86,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { // Unsets the CR for epoch 0 TestAction.Delegate, ], - [{ event: 'SetCumulativeReward', epoch: 1 }, { event: 'SetMostRecentCumulativeReward', epoch: 1 }], + [{ event: 'SetCumulativeReward', epoch: 1 }], ); }); it('re-delegating in a new epoch', async () => { @@ -105,7 +105,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { // Sets MRCR to epoch 1 TestAction.Delegate, ], - [{ event: 'SetCumulativeReward', epoch: 1 }, { event: 'SetMostRecentCumulativeReward', epoch: 1 }], + [{ event: 'SetCumulativeReward', epoch: 1 }], ); }); it('delegating in epoch 1 then again in epoch 2', async () => { @@ -128,7 +128,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { // Clears CR for epoch 1 TestAction.Delegate, ], - [{ event: 'SetCumulativeReward', epoch: 2 }, { event: 'SetMostRecentCumulativeReward', 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: 'SetMostRecentCumulativeReward', 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: 'SetMostRecentCumulativeReward', 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: 'SetMostRecentCumulativeReward', 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: 'SetMostRecentCumulativeReward', 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: 'SetMostRecentCumulativeReward', epoch: 4 }], + [{ event: 'SetCumulativeReward', epoch: 4 } ], ); }); it('earn reward in epoch 1 with no stake, then delegate', async () => { @@ -362,7 +362,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { // Creates CR for epoch 2 TestAction.Delegate, ], - [{ event: 'SetCumulativeReward', epoch: 1 }, { event: 'SetMostRecentCumulativeReward', epoch: 1 }], + [{ event: 'SetCumulativeReward', epoch: 1 }], ); }); it('delegate in epoch 1, 3, then delegate in epoch 4', async () => { @@ -397,7 +397,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { // Creates CR for epoch 5 TestAction.Delegate, ], - [{ event: 'SetCumulativeReward', epoch: 4 }, { event: 'SetMostRecentCumulativeReward', 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: 'SetMostRecentCumulativeReward', epoch: 3 }], + [{ event: 'SetCumulativeReward', epoch: 3 } ], ); }); }); From f925c353447a5b82df07e0cb2ada65b3fe11941e Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Wed, 25 Sep 2019 16:34:06 -0400 Subject: [PATCH 9/9] `@0x/contracts-staking: Always do `_withdrawAndSyncDelegatorRewards()` before staking operations and always add a CR (if unset) in `_withdrawSyncDelegatorRewards()`. --- .../contracts/src/stake/MixinStake.sol | 26 +++----- .../staking_pools/MixinStakingPoolRewards.sol | 61 ++++--------------- .../contracts/test/TestDelegatorRewards.sol | 28 ++++----- .../contracts/test/TestStakingNoWETH.sol | 2 +- .../test/cumulative_reward_tracking_test.ts | 16 ++--- 5 files changed, 42 insertions(+), 91 deletions(-) diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index 930003161a..a77350fbd5 100644 --- 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. diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index 7d70b50d72..153481bfa0 100644 --- 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. diff --git a/contracts/staking/contracts/test/TestDelegatorRewards.sol b/contracts/staking/contracts/test/TestDelegatorRewards.sol index a782d63a4f..5e86e001bd 100644 --- 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 diff --git a/contracts/staking/contracts/test/TestStakingNoWETH.sol b/contracts/staking/contracts/test/TestStakingNoWETH.sol index 77375f59b9..e176f326aa 100644 --- 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 diff --git a/contracts/staking/test/cumulative_reward_tracking_test.ts b/contracts/staking/test/cumulative_reward_tracking_test.ts index c4ba1d3235..b2b4e360fc 100644 --- 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 }], ); }); });