From 12f0797acebb36b91605d66cbd44e4c60ac19968 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Mon, 9 Sep 2019 18:18:21 -0700 Subject: [PATCH 1/5] Reference counting for rewards, plus unit tests for cumulative rewards. --- .../contracts/src/immutable/MixinStorage.sol | 3 + .../src/interfaces/IStakingEvents.sol | 2 +- .../contracts/src/interfaces/IStructs.sol | 12 +- .../contracts/src/libs/LibSafeDowncast.sol | 17 + .../contracts/src/stake/MixinStake.sol | 24 +- .../contracts/src/stake/MixinStakeStorage.sol | 3 +- .../staking_pools/MixinCumulativeRewards.sol | 230 ++++++++++ .../staking_pools/MixinStakingPoolRewards.sol | 284 ++++++------ .../test/TestCumulativeRewardTracking.sol | 63 +++ .../contracts/test/TestStorageLayout.sol | 3 + contracts/staking/package.json | 2 +- contracts/staking/src/artifacts.ts | 4 + contracts/staking/src/wrappers.ts | 2 + .../test/cumulative_reward_tracking_test.ts | 404 ++++++++++++++++++ contracts/staking/test/rewards_test.ts | 55 +++ .../cumulative_reward_tracking_simulation.ts | 172 ++++++++ contracts/staking/tsconfig.json | 2 + .../contracts/src/LibSafeMathRichErrors.sol | 1 + 18 files changed, 1136 insertions(+), 147 deletions(-) create mode 100644 contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol create mode 100644 contracts/staking/contracts/test/TestCumulativeRewardTracking.sol create mode 100644 contracts/staking/test/cumulative_reward_tracking_test.ts create mode 100644 contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts diff --git a/contracts/staking/contracts/src/immutable/MixinStorage.sol b/contracts/staking/contracts/src/immutable/MixinStorage.sol index 677ac62204..ff0b07d68c 100644 --- a/contracts/staking/contracts/src/immutable/MixinStorage.sol +++ b/contracts/staking/contracts/src/immutable/MixinStorage.sol @@ -95,6 +95,9 @@ contract MixinStorage is // mapping from Pool Id to Epoch to Reward Ratio mapping (bytes32 => mapping (uint256 => IStructs.Fraction)) internal cumulativeRewardsByPool; + // mapping from Pool Id to Epoch to Cumulative Rewards Reference Counter + mapping (bytes32 => mapping (uint256 => uint256)) internal cumulativeRewardsByPoolReferenceCounter; + // mapping from Pool Id to Epoch mapping (bytes32 => uint256) internal cumulativeRewardsByPoolLastStored; diff --git a/contracts/staking/contracts/src/interfaces/IStakingEvents.sol b/contracts/staking/contracts/src/interfaces/IStakingEvents.sol index 2cac7d812a..194656e4b6 100644 --- a/contracts/staking/contracts/src/interfaces/IStakingEvents.sol +++ b/contracts/staking/contracts/src/interfaces/IStakingEvents.sol @@ -28,7 +28,7 @@ interface IStakingEvents { uint8 fromStatus, bytes32 indexed fromPool, uint8 toStatus, - bytes32 indexed toProol + bytes32 indexed toPool ); /// @dev Emitted by MixinExchangeManager when an exchange is added. diff --git a/contracts/staking/contracts/src/interfaces/IStructs.sol b/contracts/staking/contracts/src/interfaces/IStructs.sol index 6c0a871cb4..8b6a731a1c 100644 --- a/contracts/staking/contracts/src/interfaces/IStructs.sol +++ b/contracts/staking/contracts/src/interfaces/IStructs.sol @@ -37,11 +37,13 @@ interface IStructs { /// Note that these balances may be stale if the current epoch /// is greater than `currentEpoch`. /// Always load this struct using _loadAndSyncBalance or _loadUnsyncedBalance. + /// @param isInitialized /// @param currentEpoch the current epoch /// @param currentEpochBalance balance in the current epoch. /// @param nextEpochBalance balance in the next epoch. struct StoredBalance { - uint64 currentEpoch; + bool isInitialized; + uint32 currentEpoch; uint96 currentEpochBalance; uint96 nextEpochBalance; } @@ -85,4 +87,12 @@ interface IStructs { bytes32 poolId; bool confirmed; } + + /// @dev Encapsulates the epoch and value of a cumulative reward. + /// @param cumulativeRewardEpoch Epoch of the reward. + /// @param cumulativeReward Value of the reward. + struct CumulativeRewardInfo { + uint256 cumulativeRewardEpoch; + IStructs.Fraction cumulativeReward; + } } diff --git a/contracts/staking/contracts/src/libs/LibSafeDowncast.sol b/contracts/staking/contracts/src/libs/LibSafeDowncast.sol index c7ce8793ba..145745ed27 100644 --- a/contracts/staking/contracts/src/libs/LibSafeDowncast.sol +++ b/contracts/staking/contracts/src/libs/LibSafeDowncast.sol @@ -57,4 +57,21 @@ library LibSafeDowncast { } return b; } + + /// @dev Safely downcasts to a uint32 + /// Note that this reverts if the input value is too large. + function downcastToUint32(uint256 a) + internal + pure + returns (uint32 b) + { + b = uint32(a); + if (uint256(b) != a) { + LibRichErrors.rrevert(LibSafeMathRichErrors.Uint256DowncastError( + LibSafeMathRichErrors.DowncastErrorCodes.VALUE_TOO_LARGE_TO_DOWNCAST_TO_UINT32, + a + )); + } + return b; + } } diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index 2ce61bb30e..ad32097915 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -181,18 +181,18 @@ contract MixinStake is ) private { - // transfer any rewards from the transient pool vault to the eth vault; - // this must be done before we can modify the owner's portion of the delegator pool. - _transferDelegatorsAccumulatedRewardsToEthVault(poolId, owner); - - // sync cumulative rewards that we'll need for future computations - _syncCumulativeRewardsNeededByDelegator(poolId, currentEpoch); + // cache amount delegated to pool by owner + IStructs.StoredBalance memory initDelegatedStakeToPoolByOwner = _loadUnsyncedBalance(delegatedStakeToPoolByOwner[owner][poolId]); // increment how much stake the owner has delegated to the input pool _incrementNextBalance(delegatedStakeToPoolByOwner[owner][poolId], amount); // increment how much stake has been delegated to pool _incrementNextBalance(delegatedStakeByPoolId[poolId], amount); + + // synchronizes reward state in the pool that the staker is undelegating from + IStructs.StoredBalance memory finalDelegatedStakeToPoolByOwner = _loadAndSyncBalance(delegatedStakeToPoolByOwner[owner][poolId]); + _syncRewardsForDelegator(poolId, owner, initDelegatedStakeToPoolByOwner, finalDelegatedStakeToPoolByOwner); } /// @dev Un-Delegates a owners stake from a staking pool. @@ -206,18 +206,18 @@ contract MixinStake is ) private { - // transfer any rewards from the transient pool vault to the eth vault; - // this must be done before we can modify the owner's portion of the delegator pool. - _transferDelegatorsAccumulatedRewardsToEthVault(poolId, owner); - - // sync cumulative rewards that we'll need for future computations - _syncCumulativeRewardsNeededByDelegator(poolId, currentEpoch); + // cache amount delegated to pool by owner + IStructs.StoredBalance memory initDelegatedStakeToPoolByOwner = _loadUnsyncedBalance(delegatedStakeToPoolByOwner[owner][poolId]); // decrement how much stake the owner has delegated to the input pool _decrementNextBalance(delegatedStakeToPoolByOwner[owner][poolId], amount); // decrement how much stake has been delegated to pool _decrementNextBalance(delegatedStakeByPoolId[poolId], amount); + + // synchronizes reward state in the pool that the staker is undelegating from + IStructs.StoredBalance memory finalDelegatedStakeToPoolByOwner = _loadAndSyncBalance(delegatedStakeToPoolByOwner[owner][poolId]); + _syncRewardsForDelegator(poolId, owner, initDelegatedStakeToPoolByOwner, finalDelegatedStakeToPoolByOwner); } /// @dev Returns a storage pointer to a user's stake in a given status. diff --git a/contracts/staking/contracts/src/stake/MixinStakeStorage.sol b/contracts/staking/contracts/src/stake/MixinStakeStorage.sol index c52967a948..ae2a557e5d 100644 --- a/contracts/staking/contracts/src/stake/MixinStakeStorage.sol +++ b/contracts/staking/contracts/src/stake/MixinStakeStorage.sol @@ -95,7 +95,7 @@ contract MixinStakeStorage is // sync uint256 currentEpoch = getCurrentEpoch(); if (currentEpoch > balance.currentEpoch) { - balance.currentEpoch = currentEpoch.downcastToUint64(); + balance.currentEpoch = currentEpoch.downcastToUint32(); balance.currentEpochBalance = balance.nextEpochBalance; } return balance; @@ -185,6 +185,7 @@ contract MixinStakeStorage is { // note - this compresses into a single `sstore` when optimizations are enabled, // since the StakeBalance struct occupies a single word of storage. + balancePtr.isInitialized = true; balancePtr.currentEpoch = balance.currentEpoch; balancePtr.nextEpochBalance = balance.nextEpochBalance; balancePtr.currentEpochBalance = balance.currentEpochBalance; diff --git a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol new file mode 100644 index 0000000000..bc1a6a514d --- /dev/null +++ b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol @@ -0,0 +1,230 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.9; +pragma experimental ABIEncoderV2; + +import "@0x/contracts-utils/contracts/src/LibFractions.sol"; +import "@0x/contracts-utils/contracts/src/LibSafeMath.sol"; +import "../immutable/MixinStorage.sol"; +import "../immutable/MixinConstants.sol"; +import "../stake/MixinStakeBalances.sol"; +import "./MixinStakingPoolRewardVault.sol"; + + +contract MixinCumulativeRewards is + IStakingEvents, + MixinConstants, + Ownable, + MixinStorage, + MixinZrxVault, + MixinStakingPoolRewardVault, + MixinScheduler, + MixinStakeStorage, + MixinStakeBalances +{ + using LibSafeMath for uint256; + + /// @dev Initializes Cumulative Rewards for a given pool. + function _initializeCumulativeRewards(bytes32 poolId) + internal + { + uint256 currentEpoch = getCurrentEpoch(); + _setCumulativeReward(poolId, currentEpoch, IStructs.Fraction({numerator: 0, denominator: MIN_TOKEN_VALUE})); + _setMostRecentCumulativeReward(poolId, currentEpoch); + } + + /// @dev returns true iff Cumulative Rewards are set + function _isCumulativeRewardSet(IStructs.Fraction memory cumulativeReward) + internal + pure + returns (bool) + { + // we use the denominator as a proxy for whether the cumulative + // reward is set, as setting the cumulative reward always sets this + // field to at least 1. + return cumulativeReward.denominator != 0; + } + + /// @dev Sets a cumulative reward for `poolId` at `epoch`. + /// @param poolId Unique Id of pool. + /// @param epoch of cumulative reward. + /// @param value of cumulative reward. + function _setCumulativeReward( + bytes32 poolId, + uint256 epoch, + IStructs.Fraction memory value + ) + internal + { + cumulativeRewardsByPool[poolId][epoch] = value; + } + + /// @dev Unsets the cumulative reward for `poolId` at `epoch`. + /// @param poolId Unique id of pool. + /// @param epoch of cumulative reward to unset. + function _unsetCumulativeReward(bytes32 poolId, uint256 epoch) + internal + { + assert(cumulativeRewardsByPoolReferenceCounter[poolId][epoch] == 0); + cumulativeRewardsByPool[poolId][epoch] = IStructs.Fraction({numerator: 0, denominator: 0}); + } + + /// @dev Returns info on most recent cumulative reward. + function _getMostRecentCumulativeRewardInfo(bytes32 poolId) + internal + returns (IStructs.CumulativeRewardInfo memory) + { + // fetch the last epoch at which we stored a cumulative reward for this pool + uint256 cumulativeRewardsLastStored = cumulativeRewardsByPoolLastStored[poolId]; + + // query and return cumulative reward info for this pool + return IStructs.CumulativeRewardInfo({ + cumulativeReward: cumulativeRewardsByPool[poolId][cumulativeRewardsLastStored], + cumulativeRewardEpoch: cumulativeRewardsLastStored + }); + } + + /// @dev Adds a dependency on a cumulative reward for a given epoch. + /// @param poolId Unique Id of pool. + /// @param epoch to remove dependency from. + /// @param mostRecentCumulativeRewardInfo Epoch of the most recent cumulative reward. + /// @param isDependent still FGREG EEGGEGREG + function _addOrRemoveDependencyOnCumulativeReward( + bytes32 poolId, + uint256 epoch, + IStructs.CumulativeRewardInfo memory mostRecentCumulativeRewardInfo, + bool isDependent + ) + internal + { + if (isDependent) { + _addDependencyOnCumulativeReward( + poolId, + epoch, + mostRecentCumulativeRewardInfo + ); + } else { + _removeDependencyOnCumulativeReward( + poolId, + epoch, + mostRecentCumulativeRewardInfo.cumulativeRewardEpoch + ); + } + } + + /// @dev Adds a dependency on a cumulative reward for a given epoch. + /// @param poolId Unique Id of pool. + /// @param epoch to remove dependency from. + /// @param mostRecentCumulativeRewardInfo Info on the most recent cumulative reward. + function _addDependencyOnCumulativeReward( + bytes32 poolId, + uint256 epoch, + IStructs.CumulativeRewardInfo memory mostRecentCumulativeRewardInfo + ) + internal + { + // add dependency by increasing the reference counter + cumulativeRewardsByPoolReferenceCounter[poolId][epoch] = cumulativeRewardsByPoolReferenceCounter[poolId][epoch].safeAdd(1); + + // ensure dependency has a value, otherwise copy it from the most recent reward + if (!_isCumulativeRewardSet(cumulativeRewardsByPool[poolId][epoch])) { + _setCumulativeReward(poolId, epoch, mostRecentCumulativeRewardInfo.cumulativeReward); + _setMostRecentCumulativeReward(poolId, epoch); + } + } + + /// @dev Removes a dependency on a cumulative reward for a given epoch. + /// @param poolId Unique Id of pool. + /// @param epoch to remove dependency from. + /// @param mostRecentCumulativeRewardEpoch Epoch of the most recent cumulative reward. + function _removeDependencyOnCumulativeReward( + bytes32 poolId, + uint256 epoch, + uint256 mostRecentCumulativeRewardEpoch + ) + internal + { + // remove dependency by decreasing reference counter + uint256 newReferenceCounter = cumulativeRewardsByPoolReferenceCounter[poolId][epoch].safeSub(1); + cumulativeRewardsByPoolReferenceCounter[poolId][epoch] = newReferenceCounter; + + // clear cumulative reward from state, if it is no longer needed + if (newReferenceCounter == 0 && epoch < mostRecentCumulativeRewardEpoch) { + _unsetCumulativeReward(poolId, epoch); + } + } + + /// @dev Computes a member's reward over a given epoch interval. + /// @param poolId Uniqud Id of pool. + /// @param memberStakeOverInterval Stake delegated to pool by meber over the interval. + /// @param beginEpoch beginning of interval. + /// @param endEpoch end of interval. + /// @return rewards accumulated over interval [beginEpoch, endEpoch] + function _computeMemberRewardOverInterval( + bytes32 poolId, + uint256 memberStakeOverInterval, + uint256 beginEpoch, + uint256 endEpoch + ) + internal + view + returns (uint256) + { + // sanity check + if (memberStakeOverInterval == 0) { + return 0; + } + + // compute reward + IStructs.Fraction memory beginRatio = cumulativeRewardsByPool[poolId][beginEpoch]; + IStructs.Fraction memory endRatio = cumulativeRewardsByPool[poolId][endEpoch]; + uint256 reward = LibFractions.scaleFractionalDifference( + endRatio.numerator, + endRatio.denominator, + beginRatio.numerator, + beginRatio.denominator, + memberStakeOverInterval + ); + return reward; + } + + /// @dev Sets the most recent cumulative reward for the pool. + /// @param poolId Unique Id of pool. + /// @param epoch The epoch of the most recent cumulative reward. + function _setMostRecentCumulativeReward(bytes32 poolId, uint256 epoch) + internal + { + // load the current value, sanity check that we're not trying to go back in time + uint256 currentMostRecentEpoch = cumulativeRewardsByPoolLastStored[poolId]; + assert(epoch >= currentMostRecentEpoch); + + // check if we should do any work + if (epoch == currentMostRecentEpoch) { + return; + } + + // unset the current most recent reward if it has no more references + if (cumulativeRewardsByPoolReferenceCounter[poolId][currentMostRecentEpoch] == 0) { + _unsetCumulativeReward(poolId, currentMostRecentEpoch); + } + + // update state to reflect the most recent cumulative reward + cumulativeRewardsByPoolLastStored[poolId] = epoch; + } +} diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index 12ded65743..fc43b2d5ed 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -25,6 +25,7 @@ import "../immutable/MixinStorage.sol"; import "../immutable/MixinConstants.sol"; import "../stake/MixinStakeBalances.sol"; import "./MixinStakingPoolRewardVault.sol"; +import "./MixinCumulativeRewards.sol"; contract MixinStakingPoolRewards is @@ -36,7 +37,8 @@ contract MixinStakingPoolRewards is MixinStakingPoolRewardVault, MixinScheduler, MixinStakeStorage, - MixinStakeBalances + MixinStakeBalances, + MixinCumulativeRewards { using LibSafeMath for uint256; @@ -50,110 +52,51 @@ contract MixinStakingPoolRewards is view returns (uint256 totalReward) { - // cache some values to reduce sloads - IStructs.StoredBalance memory delegatedStake = _loadUnsyncedBalance(delegatedStakeToPoolByOwner[member][poolId]); - uint256 currentEpoch = getCurrentEpoch(); - - // value is always zero in these two scenarios: - // 1. The owner's delegated is current as of this epoch: their rewards have been moved to the ETH vault. - // 2. The current epoch is zero: delegation begins at epoch 1 - if (delegatedStake.currentEpoch == currentEpoch || currentEpoch == 0) return 0; - - // compute reward accumulated during `delegatedStake.currentEpoch`; - uint256 rewardsAccumulatedDuringLastStoredEpoch = (delegatedStake.currentEpochBalance != 0) - ? _computeMemberRewardOverInterval( - poolId, - delegatedStake.currentEpochBalance, - delegatedStake.currentEpoch - 1, - delegatedStake.currentEpoch - ) - : 0; - - // compute the reward accumulated by the `next` balance; - // this starts at `delegatedStake.currentEpoch + 1` and goes up until the last epoch, during which - // rewards were accumulated. This is at most the most recently finalized epoch (current epoch - 1). - uint256 rewardsAccumulatedAfterLastStoredEpoch = (cumulativeRewardsByPoolLastStored[poolId] > delegatedStake.currentEpoch) - ? _computeMemberRewardOverInterval( - poolId, - delegatedStake.nextEpochBalance, - delegatedStake.currentEpoch, - cumulativeRewardsByPoolLastStored[poolId] - ) - : 0; - - // compute the total reward - totalReward = rewardsAccumulatedDuringLastStoredEpoch.safeAdd(rewardsAccumulatedAfterLastStoredEpoch); - return totalReward; + return _computeRewardBalanceOfDelegator( + poolId, + _loadUnsyncedBalance(delegatedStakeToPoolByOwner[member][poolId]), + getCurrentEpoch() + ); } - /// @dev Transfers a delegators accumulated rewards from the transient pool Reward Pool vault - /// to the Eth Vault. This is required before the member's stake in the pool can be - /// modified. + /// @dev Syncs rewards for a delegator. This includes transferring rewards from + /// the Reward Vault to the Eth Vault, and adding/removing dependencies on cumulative rewards. /// @param poolId Unique id of pool. - /// @param member The member of the pool. - function _transferDelegatorsAccumulatedRewardsToEthVault(bytes32 poolId, address member) - internal - { - // there are no delegators in the first epoch - uint256 currentEpoch = getCurrentEpoch(); - if (currentEpoch == 0) { - return; - } - - // compute balance owed to delegator - uint256 balance = computeRewardBalanceOfDelegator(poolId, member); - if (balance == 0) { - return; - } - - // transfer from transient Reward Pool vault to ETH Vault - _transferMemberBalanceToEthVault(poolId, member, balance); - } - - /// @dev Initializes Cumulative Rewards for a given pool. - function _initializeCumulativeRewards(bytes32 poolId) + /// @param member of the pool. + /// @param initialDelegatedStakeToPoolByOwner The member's delegated balance at the beginning of this transaction. + /// @param finalDelegatedStakeToPoolByOwner The member's delegated balance at the end of this transaction. + function _syncRewardsForDelegator( + bytes32 poolId, + address member, + IStructs.StoredBalance memory initialDelegatedStakeToPoolByOwner, + IStructs.StoredBalance memory finalDelegatedStakeToPoolByOwner + ) internal { uint256 currentEpoch = getCurrentEpoch(); - cumulativeRewardsByPool[poolId][currentEpoch] = IStructs.Fraction({numerator: 0, denominator: MIN_TOKEN_VALUE}); - cumulativeRewardsByPoolLastStored[poolId] = currentEpoch; - } - /// @dev To compute a delegator's reward we must know the cumulative reward - /// at the epoch before they delegated. If they were already delegated then - /// we also need to know the value at the epoch in which they modified - /// their delegated stake for this pool. See `computeRewardBalanceOfDelegator`. - /// @param poolId Unique Id of pool. - /// @param epoch at which the stake was delegated by the delegator. - function _syncCumulativeRewardsNeededByDelegator(bytes32 poolId, uint256 epoch) - internal - { - // set default value if staking at epoch 0 - if (epoch == 0) { - return; - } + // transfer any rewards from the transient pool vault to the eth vault; + // this must be done before we can modify the owner's portion of the delegator pool. + _transferDelegatorRewardsToEthVault( + poolId, + member, + initialDelegatedStakeToPoolByOwner, + currentEpoch + ); - // cache a storage pointer to the cumulative rewards for `poolId` indexed by epoch. - mapping (uint256 => IStructs.Fraction) storage cumulativeRewardsByPoolPtr = cumulativeRewardsByPool[poolId]; + // add dependencies on cumulative rewards for this epoch and the previous epoch, if necessary. + _setCumulativeRewardDependenciesForDelegator( + poolId, + finalDelegatedStakeToPoolByOwner, + true + ); - // 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 cumulativeRewardsLastStored = cumulativeRewardsByPoolLastStored[poolId]; - IStructs.Fraction memory mostRecentCumulativeRewards = cumulativeRewardsByPoolPtr[cumulativeRewardsLastStored]; - - // copy our most up-to-date cumulative rewards for last epoch, if necessary. - uint256 lastEpoch = currentEpoch.safeSub(1); - if (cumulativeRewardsLastStored != lastEpoch) { - cumulativeRewardsByPoolPtr[lastEpoch] = mostRecentCumulativeRewards; - cumulativeRewardsByPoolLastStored[poolId] = lastEpoch; - } - - // copy our most up-to-date cumulative rewards for last epoch, if necessary. - // this is necessary if the pool does not earn any rewards this epoch; - // if it does then this value may be overwritten when the epoch is finalized. - if (!_isCumulativeRewardSet(cumulativeRewardsByPoolPtr[epoch])) { - cumulativeRewardsByPoolPtr[epoch] = mostRecentCumulativeRewards; - } + // remove dependencies on previous cumulative rewards, if they are no longer needed. + _setCumulativeRewardDependenciesForDelegator( + poolId, + initialDelegatedStakeToPoolByOwner, + false + ); } /// @dev Records a reward for delegators. This adds to the `cumulativeRewardsByPool`. @@ -191,51 +134,130 @@ contract MixinStakingPoolRewards is denominator.safeDiv(MIN_TOKEN_VALUE) ); - // store cumulative rewards - cumulativeRewardsByPoolPtr[epoch] = IStructs.Fraction({ - numerator: numeratorNormalized, - denominator: denominatorNormalized - }); - cumulativeRewardsByPoolLastStored[poolId] = epoch; + // store cumulative rewards and set most recent + _setCumulativeReward( + poolId, + epoch, + IStructs.Fraction({ + numerator: numeratorNormalized, + denominator: denominatorNormalized + }) + ); + _setMostRecentCumulativeReward(poolId, epoch); } - /// @dev Computes a member's reward over a given epoch interval. - /// @param poolId Uniqud Id of pool. - /// @param memberStakeOverInterval Stake delegated to pool by meber over the interval. - /// @param beginEpoch beginning of interval. - /// @param endEpoch end of interval. - /// @return rewards accumulated over interval [beginEpoch, endEpoch] - function _computeMemberRewardOverInterval( + /// @dev Transfers a delegators accumulated rewards from the transient pool Reward Pool vault + /// to the Eth Vault. 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. + function _transferDelegatorRewardsToEthVault( bytes32 poolId, - uint256 memberStakeOverInterval, - uint256 beginEpoch, - uint256 endEpoch + address member, + IStructs.StoredBalance memory unsyncedDelegatedStakeToPoolByOwner, + uint256 currentEpoch + ) + private + { + // compute balance owed to delegator + uint256 balance = _computeRewardBalanceOfDelegator( + poolId, + unsyncedDelegatedStakeToPoolByOwner, + currentEpoch + ); + if (balance == 0) { + return; + } + + // transfer from transient Reward Pool vault to ETH Vault + _transferMemberBalanceToEthVault(poolId, member, balance); + } + + /// @dev Computes the reward balance in ETH of a specific member of a pool. + /// @param poolId Unique id of pool. + /// @param unsyncedDelegatedStakeToPoolByOwner Unsynced delegated stake to pool by owner + /// @param currentEpoch The epoch in which this call is executing + /// @return totalReward Balance in ETH. + function _computeRewardBalanceOfDelegator( + bytes32 poolId, + IStructs.StoredBalance memory unsyncedDelegatedStakeToPoolByOwner, + uint256 currentEpoch ) private view - returns (uint256) + returns (uint256 totalReward) { - IStructs.Fraction memory beginRatio = cumulativeRewardsByPool[poolId][beginEpoch]; - IStructs.Fraction memory endRatio = cumulativeRewardsByPool[poolId][endEpoch]; - uint256 reward = LibFractions.scaleFractionalDifference( - endRatio.numerator, - endRatio.denominator, - beginRatio.numerator, - beginRatio.denominator, - memberStakeOverInterval - ); - return reward; + // value is always zero in these two scenarios: + // 1. The owner's delegated is current as of this epoch: their rewards have been moved to the ETH vault. + // 2. The current epoch is zero: delegation begins at epoch 1 + if (unsyncedDelegatedStakeToPoolByOwner.currentEpoch == currentEpoch || currentEpoch == 0) return 0; + + // compute reward accumulated during `delegatedStake.currentEpoch`; + uint256 rewardsAccumulatedDuringLastStoredEpoch = (unsyncedDelegatedStakeToPoolByOwner.currentEpochBalance != 0) + ? _computeMemberRewardOverInterval( + poolId, + unsyncedDelegatedStakeToPoolByOwner.currentEpochBalance, + uint256(unsyncedDelegatedStakeToPoolByOwner.currentEpoch).safeSub(1), + unsyncedDelegatedStakeToPoolByOwner.currentEpoch + ) + : 0; + + // compute the reward accumulated by the `next` balance; + // this starts at `delegatedStake.currentEpoch + 1` and goes up until the last epoch, during which + // rewards were accumulated. This is at most the most recently finalized epoch (current epoch - 1). + uint256 rewardsAccumulatedAfterLastStoredEpoch = (cumulativeRewardsByPoolLastStored[poolId] > unsyncedDelegatedStakeToPoolByOwner.currentEpoch) + ? _computeMemberRewardOverInterval( + poolId, + unsyncedDelegatedStakeToPoolByOwner.nextEpochBalance, + unsyncedDelegatedStakeToPoolByOwner.currentEpoch, + cumulativeRewardsByPoolLastStored[poolId] + ) + : 0; + + // compute the total reward + totalReward = rewardsAccumulatedDuringLastStoredEpoch.safeAdd(rewardsAccumulatedAfterLastStoredEpoch); + return totalReward; } - /// @dev returns true iff Cumulative Rewards are set - function _isCumulativeRewardSet(IStructs.Fraction memory cumulativeReward) + /// @dev Adds or removes cumulative reward dependencies for a delegator. + /// A delegator always depends on the cumulative reward for the current epoch. + /// They will also depend on the previous epoch's reward, if they are already staked with the input pool. + /// @param poolId Unique id of pool. + /// @param delegatedStakeToPoolByOwner Amount of stake the member has delegated to the pool. + /// @param isDependent is true iff adding a dependency. False, otherwise. + function _setCumulativeRewardDependenciesForDelegator( + bytes32 poolId, + IStructs.StoredBalance memory delegatedStakeToPoolByOwner, + bool isDependent + ) private - pure - returns (bool) { - // we use the denominator as a proxy for whether the cumulative - // reward is set, as setting the cumulative reward always sets this - // field to at least 1. - return cumulativeReward.denominator != 0; + // if this delegator is not yet initialized then there's no dependency to unset. + if (!isDependent && !delegatedStakeToPoolByOwner.isInitialized) { + return; + } + + // get the most recent cumulative reward, which will serve as a reference point when updating dependencies + IStructs.CumulativeRewardInfo memory mostRecentCumulativeRewardInfo = _getMostRecentCumulativeRewardInfo(poolId); + + // record dependency on `lastEpoch` + if (delegatedStakeToPoolByOwner.currentEpoch > 0 && delegatedStakeToPoolByOwner.currentEpochBalance != 0) { + _addOrRemoveDependencyOnCumulativeReward( + poolId, + uint256(delegatedStakeToPoolByOwner.currentEpoch).safeSub(1), + mostRecentCumulativeRewardInfo, + isDependent + ); + } + + // record dependency on current epoch. + if (delegatedStakeToPoolByOwner.currentEpochBalance != 0 || delegatedStakeToPoolByOwner.nextEpochBalance != 0) { + _addOrRemoveDependencyOnCumulativeReward( + poolId, + delegatedStakeToPoolByOwner.currentEpoch, + mostRecentCumulativeRewardInfo, + isDependent + ); + } } } diff --git a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol new file mode 100644 index 0000000000..cd7a844378 --- /dev/null +++ b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol @@ -0,0 +1,63 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ +pragma solidity ^0.5.9; +pragma experimental ABIEncoderV2; + +import "./TestStaking.sol"; + + +contract TestCumulativeRewardTracking is + TestStaking +{ + + event SetMostRecentCumulativeReward( + bytes32 poolId, + uint256 epoch + ); + + event UnsetCumulativeReward( + bytes32 poolId, + uint256 epoch + ); + + event SetCumulativeReward( + bytes32 poolId, + uint256 epoch + ); + + function _setMostRecentCumulativeReward(bytes32 poolId, uint256 epoch) + internal + { + emit SetMostRecentCumulativeReward(poolId, epoch); + MixinCumulativeRewards._setMostRecentCumulativeReward(poolId, epoch); + } + + function _unsetCumulativeReward(bytes32 poolId, uint256 epoch) + internal + { + emit UnsetCumulativeReward(poolId, epoch); + MixinCumulativeRewards._unsetCumulativeReward(poolId, epoch); + } + + function _setCumulativeReward(bytes32 poolId, uint256 epoch, IStructs.Fraction memory value) + internal + { + emit SetCumulativeReward(poolId, epoch); + MixinCumulativeRewards._setCumulativeReward(poolId, epoch, value); + } +} diff --git a/contracts/staking/contracts/test/TestStorageLayout.sol b/contracts/staking/contracts/test/TestStorageLayout.sol index ba89e1f0d0..8b2d17c40f 100644 --- a/contracts/staking/contracts/test/TestStorageLayout.sol +++ b/contracts/staking/contracts/test/TestStorageLayout.sol @@ -97,6 +97,9 @@ contract TestStorageLayout is if sub(cumulativeRewardsByPool_slot, slot) { revertIncorrectStorageSlot() } slot := add(slot, 1) + if sub(cumulativeRewardsByPoolReferenceCounter_slot, slot) { revertIncorrectStorageSlot() } + slot := add(slot, 1) + if sub(cumulativeRewardsByPoolLastStored_slot, slot) { revertIncorrectStorageSlot() } slot := add(slot, 1) diff --git a/contracts/staking/package.json b/contracts/staking/package.json index 2f545b94d8..490419276e 100644 --- a/contracts/staking/package.json +++ b/contracts/staking/package.json @@ -37,7 +37,7 @@ }, "config": { "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.", - "abis": "./generated-artifacts/@(EthVault|IEthVault|IStaking|IStakingEvents|IStakingPoolRewardVault|IStakingProxy|IStorageInit|IStructs|IVaultCore|IZrxVault|LibFixedMath|LibFixedMathRichErrors|LibProxy|LibSafeDowncast|LibStakingRichErrors|MixinConstants|MixinDeploymentConstants|MixinEthVault|MixinExchangeFees|MixinExchangeManager|MixinParams|MixinScheduler|MixinStake|MixinStakeBalances|MixinStakeStorage|MixinStakingPool|MixinStakingPoolRewardVault|MixinStakingPoolRewards|MixinStorage|MixinVaultCore|MixinZrxVault|ReadOnlyProxy|Staking|StakingPoolRewardVault|StakingProxy|TestCobbDouglas|TestInitTarget|TestLibFixedMath|TestLibProxy|TestLibProxyReceiver|TestLibSafeDowncast|TestProtocolFees|TestProtocolFeesERC20Proxy|TestStaking|TestStakingProxy|TestStorageLayout|ZrxVault).json" + "abis": "./generated-artifacts/@(EthVault|IEthVault|IStaking|IStakingEvents|IStakingPoolRewardVault|IStakingProxy|IStorageInit|IStructs|IVaultCore|IZrxVault|LibFixedMath|LibFixedMathRichErrors|LibProxy|LibSafeDowncast|LibStakingRichErrors|MixinConstants|MixinCumulativeRewards|MixinDeploymentConstants|MixinEthVault|MixinExchangeFees|MixinExchangeManager|MixinParams|MixinScheduler|MixinStake|MixinStakeBalances|MixinStakeStorage|MixinStakingPool|MixinStakingPoolRewardVault|MixinStakingPoolRewards|MixinStorage|MixinVaultCore|MixinZrxVault|ReadOnlyProxy|Staking|StakingPoolRewardVault|StakingProxy|TestCobbDouglas|TestCumulativeRewardTracking|TestInitTarget|TestLibFixedMath|TestLibProxy|TestLibProxyReceiver|TestLibSafeDowncast|TestProtocolFees|TestProtocolFeesERC20Proxy|TestStaking|TestStakingProxy|TestStorageLayout|ZrxVault).json" }, "repository": { "type": "git", diff --git a/contracts/staking/src/artifacts.ts b/contracts/staking/src/artifacts.ts index 81d40d7bac..1c0bbe1114 100644 --- a/contracts/staking/src/artifacts.ts +++ b/contracts/staking/src/artifacts.ts @@ -21,6 +21,7 @@ import * as LibProxy from '../generated-artifacts/LibProxy.json'; import * as LibSafeDowncast from '../generated-artifacts/LibSafeDowncast.json'; import * as LibStakingRichErrors from '../generated-artifacts/LibStakingRichErrors.json'; import * as MixinConstants from '../generated-artifacts/MixinConstants.json'; +import * as MixinCumulativeRewards from '../generated-artifacts/MixinCumulativeRewards.json'; import * as MixinDeploymentConstants from '../generated-artifacts/MixinDeploymentConstants.json'; import * as MixinEthVault from '../generated-artifacts/MixinEthVault.json'; import * as MixinExchangeFees from '../generated-artifacts/MixinExchangeFees.json'; @@ -41,6 +42,7 @@ import * as Staking from '../generated-artifacts/Staking.json'; import * as StakingPoolRewardVault from '../generated-artifacts/StakingPoolRewardVault.json'; import * as StakingProxy from '../generated-artifacts/StakingProxy.json'; import * as TestCobbDouglas from '../generated-artifacts/TestCobbDouglas.json'; +import * as TestCumulativeRewardTracking from '../generated-artifacts/TestCumulativeRewardTracking.json'; import * as TestInitTarget from '../generated-artifacts/TestInitTarget.json'; import * as TestLibFixedMath from '../generated-artifacts/TestLibFixedMath.json'; import * as TestLibProxy from '../generated-artifacts/TestLibProxy.json'; @@ -79,6 +81,7 @@ export const artifacts = { MixinStakeBalances: MixinStakeBalances as ContractArtifact, MixinStakeStorage: MixinStakeStorage as ContractArtifact, MixinZrxVault: MixinZrxVault as ContractArtifact, + MixinCumulativeRewards: MixinCumulativeRewards as ContractArtifact, MixinEthVault: MixinEthVault as ContractArtifact, MixinStakingPool: MixinStakingPool as ContractArtifact, MixinStakingPoolRewardVault: MixinStakingPoolRewardVault as ContractArtifact, @@ -90,6 +93,7 @@ export const artifacts = { StakingPoolRewardVault: StakingPoolRewardVault as ContractArtifact, ZrxVault: ZrxVault as ContractArtifact, TestCobbDouglas: TestCobbDouglas as ContractArtifact, + TestCumulativeRewardTracking: TestCumulativeRewardTracking as ContractArtifact, TestInitTarget: TestInitTarget as ContractArtifact, TestLibFixedMath: TestLibFixedMath as ContractArtifact, TestLibProxy: TestLibProxy as ContractArtifact, diff --git a/contracts/staking/src/wrappers.ts b/contracts/staking/src/wrappers.ts index 7d5c94e6f7..19693381bc 100644 --- a/contracts/staking/src/wrappers.ts +++ b/contracts/staking/src/wrappers.ts @@ -19,6 +19,7 @@ export * from '../generated-wrappers/lib_proxy'; export * from '../generated-wrappers/lib_safe_downcast'; export * from '../generated-wrappers/lib_staking_rich_errors'; export * from '../generated-wrappers/mixin_constants'; +export * from '../generated-wrappers/mixin_cumulative_rewards'; export * from '../generated-wrappers/mixin_deployment_constants'; export * from '../generated-wrappers/mixin_eth_vault'; export * from '../generated-wrappers/mixin_exchange_fees'; @@ -39,6 +40,7 @@ export * from '../generated-wrappers/staking'; export * from '../generated-wrappers/staking_pool_reward_vault'; export * from '../generated-wrappers/staking_proxy'; export * from '../generated-wrappers/test_cobb_douglas'; +export * from '../generated-wrappers/test_cumulative_reward_tracking'; export * from '../generated-wrappers/test_init_target'; export * from '../generated-wrappers/test_lib_fixed_math'; export * from '../generated-wrappers/test_lib_proxy'; diff --git a/contracts/staking/test/cumulative_reward_tracking_test.ts b/contracts/staking/test/cumulative_reward_tracking_test.ts new file mode 100644 index 0000000000..184ef06d48 --- /dev/null +++ b/contracts/staking/test/cumulative_reward_tracking_test.ts @@ -0,0 +1,404 @@ +import { ERC20Wrapper } from '@0x/contracts-asset-proxy'; +import { blockchainTests, describe } from '@0x/contracts-test-utils'; +import * as _ from 'lodash'; + +import { artifacts } from '../src'; + +import { deployAndConfigureContractsAsync, StakingApiWrapper } from './utils/api_wrapper'; +import { CumulativeRewardTrackingSimulation, TestAction } from './utils/cumulative_reward_tracking_simulation'; + +// tslint:disable:no-unnecessary-type-assertion +// tslint:disable:max-file-line-count +blockchainTests.resets('Cumulative Reward Tracking', env => { + // tokens & addresses + let accounts: string[]; + let owner: string; + // wrappers + let stakingApiWrapper: StakingApiWrapper; + let simulation: CumulativeRewardTrackingSimulation; + // let testWrapper: TestRewardBalancesContract; + let erc20Wrapper: ERC20Wrapper; + + // tests + before(async () => { + // create accounts + accounts = await env.getAccountAddressesAsync(); + owner = accounts[0]; + const actors = accounts.slice(1); + // set up ERC20Wrapper + erc20Wrapper = new ERC20Wrapper(env.provider, accounts, owner); + // deploy staking contracts + stakingApiWrapper = await deployAndConfigureContractsAsync(env, owner, erc20Wrapper, artifacts.TestStaking); + simulation = new CumulativeRewardTrackingSimulation(stakingApiWrapper, actors); + await simulation.deployAndConfigureTestContractsAsync(env); + }); + + describe('Tracking Cumulative Rewards (CR)', () => { + it('should set CR and Most Recent CR when a pool is created', async () => { + await simulation.runTestAsync( + [], + [TestAction.CreatePool], + [{ event: 'SetCumulativeReward', epoch: 0 }, { event: 'SetMostRecentCumulativeReward', epoch: 0 }], + ); + }); + it('should not set CR or Most Recent CR when values already exist for the current epoch', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + ], + + [ + TestAction.Delegate, // does nothing wrt CR, as there is alread a CR set for this epoch. + ], + [], + ); + }); + it('should not set CR or Most Recent CR when user re-delegates and values already exist for the current epoch', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, // does nothing wrt CR, as there is alread a CR set for this epoch. + ], + [ + TestAction.Delegate, // does nothing wrt CR, as there is alread a CR set for this epoch. + ], + [], + ); + }); + it('should not set CR or Most Recent CR when user undelegagtes and values already exist for the current epoch', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, // does nothing wrt CR, as there is alread a CR set for this epoch. + ], + [ + TestAction.Undelegate, // does nothing wrt CR, as there is alread a CR set for this epoch. + ], + [], + ); + }); + it('should (i) set CR and Most Recent CR when delegating, and (ii) unset previous Most Recent CR if there are no dependencies', async () => { + // since there was no delegation in epoch 0 there is no longer a dependency on the CR for epoch 0 + await simulation.runTestAsync( + [TestAction.CreatePool, TestAction.Finalize], + [TestAction.Delegate], + [ + { event: 'SetCumulativeReward', epoch: 1 }, + { event: 'SetMostRecentCumulativeReward', epoch: 1 }, + { event: 'UnsetCumulativeReward', epoch: 0 }, + ], + ); + }); + it('should (i) set CR and Most Recent CR when delegating, and (ii) NOT unset previous Most Recent CR if there are dependencies', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, // does nothing wrt CR, as there is alread a CR set for this epoch. + TestAction.Finalize, // moves to epoch 1 + ], + [ + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + ], + [{ event: 'SetCumulativeReward', epoch: 1 }, { event: 'SetMostRecentCumulativeReward', epoch: 1 }], + ); + }); + it('should not unset the current Most Recent CR, even if there are no dependencies', async () => { + // note - we never unset the current Most Recent CR; only ever a previous value - given there are no depencies from delegators. + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + ], + [ + TestAction.Undelegate, // does nothing. This delegator no longer has dependency, but the most recent CR is 1 so we don't remove. + ], + [], + ); + }); + it('should set CR and update Most Recent CR when delegating more stake', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + TestAction.Finalize, // moves to epoch 2 + ], + [ + TestAction.Delegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [{ event: 'SetCumulativeReward', epoch: 2 }, { event: 'SetMostRecentCumulativeReward', epoch: 2 }], + ); + }); + it('should set CR and update Most Recent CR when undelegating', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + TestAction.Finalize, // moves to epoch 2 + ], + [ + TestAction.Undelegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [{ event: 'SetCumulativeReward', epoch: 2 }, { event: 'SetMostRecentCumulativeReward', epoch: 2 }], + ); + }); + it('should set CR and update Most Recent CR when undelegating, plus remove the CR that is no longer depends on.', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + TestAction.Finalize, // moves to epoch 2 + ], + [ + TestAction.Undelegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [ + { event: 'SetCumulativeReward', epoch: 2 }, + { event: 'SetMostRecentCumulativeReward', epoch: 2 }, + { event: 'UnsetCumulativeReward', epoch: 0 }, + ], + ); + }); + it('should set CR and update Most Recent CR when redelegating, plus remove the CR that is no longer depends on.', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, // does nothing wrt CR + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + TestAction.Finalize, // moves to epoch 2 + ], + [ + TestAction.Delegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [ + { event: 'SetCumulativeReward', epoch: 2 }, + { event: 'SetMostRecentCumulativeReward', epoch: 2 }, + { event: 'UnsetCumulativeReward', epoch: 0 }, + ], + ); + }); + it('should set CR and Most Recent CR when a reward is earned', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, // does nothing wrt CR, as there is alread a CR set for this epoch. + TestAction.Finalize, // moves to epoch 1 + TestAction.PayProtocolFee, + ], + [ + TestAction.Finalize, // adds a CR for epoch 1, plus updates most recent CR + ], + [{ event: 'SetCumulativeReward', epoch: 1 }, { event: 'SetMostRecentCumulativeReward', epoch: 1 }], + ); + }); + it('should set/unset CR and update Most Recent CR when redelegating, the epoch following a reward was earned', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, // does nothing wrt CR + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + TestAction.Finalize, // moves to epoch 2 + TestAction.PayProtocolFee, // this means a CR will be available upon finalization + TestAction.Finalize, // creates new CR for epoch 2; moves to epoch 3 + ], + [ + TestAction.Delegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [ + { event: 'SetCumulativeReward', epoch: 3 }, + { event: 'SetMostRecentCumulativeReward', epoch: 3 }, + { event: 'UnsetCumulativeReward', epoch: 0 }, + { event: 'UnsetCumulativeReward', epoch: 1 }, + ], + ); + }); + it('should set/unset CR and update Most Recent CR when redelegating, the epoch following a reward was earned', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, // does nothing wrt CR + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + TestAction.Finalize, // moves to epoch 2 + TestAction.PayProtocolFee, // this means a CR will be available upon finalization + TestAction.Finalize, // creates new CR for epoch 2; moves to epoch 3 + ], + [ + TestAction.Undelegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [ + { event: 'SetCumulativeReward', epoch: 3 }, + { event: 'SetMostRecentCumulativeReward', epoch: 3 }, + { event: 'UnsetCumulativeReward', epoch: 0 }, + { event: 'UnsetCumulativeReward', epoch: 1 }, + ], + ); + }); + it('should set/unset CR and update Most Recent CR when redelegating, one full epoch after a reward was earned', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, // does nothing wrt CR + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + TestAction.Finalize, // moves to epoch 2 + TestAction.PayProtocolFee, // this means a CR will be available upon finalization + TestAction.Finalize, // creates new CR for epoch 2; moves to epoch 3 + TestAction.Finalize, // moves to epoch 4 + ], + [ + TestAction.Delegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [ + { event: 'SetCumulativeReward', epoch: 3 }, + { event: 'SetMostRecentCumulativeReward', epoch: 3 }, + { event: 'UnsetCumulativeReward', epoch: 2 }, + { event: 'SetCumulativeReward', epoch: 4 }, + { event: 'SetMostRecentCumulativeReward', epoch: 4 }, + { event: 'UnsetCumulativeReward', epoch: 0 }, + { event: 'UnsetCumulativeReward', epoch: 1 }, + ], + ); + }); + it('should set/unset CR and update Most Recent CR when redelegating, one full epoch after a reward was earned', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, // does nothing wrt CR + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + TestAction.Finalize, // moves to epoch 2 + TestAction.PayProtocolFee, // this means a CR will be available upon finalization + TestAction.Finalize, // creates new CR for epoch 2; moves to epoch 3 + ], + [ + TestAction.Undelegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [ + { event: 'SetCumulativeReward', epoch: 3 }, + { event: 'SetMostRecentCumulativeReward', epoch: 3 }, + { event: 'UnsetCumulativeReward', epoch: 0 }, + { event: 'UnsetCumulativeReward', epoch: 1 }, + ], + ); + }); + it('should set/unset CR and update Most Recent CR when delegating for the first time in an epoch with no CR', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.PayProtocolFee, // this means a CR will be available upon finalization + TestAction.Finalize, // creates new CR for epoch 0; moves to epoch 1 + ], + [ + TestAction.Delegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [ + { event: 'SetCumulativeReward', epoch: 1 }, + { event: 'SetMostRecentCumulativeReward', epoch: 1 }, + { event: 'UnsetCumulativeReward', epoch: 0 }, + ], + ); + }); + it('should set/unset CR and update Most Recent CR when delegating for the first time in an epoch with no CR, after an epoch where a reward was earned', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Finalize, // creates new CR for epoch 0; moves to epoch 1 + ], + [ + TestAction.Delegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [ + { event: 'SetCumulativeReward', epoch: 1 }, + { event: 'SetMostRecentCumulativeReward', epoch: 1 }, + { event: 'UnsetCumulativeReward', epoch: 0 }, + ], + ); + }); + it('should set CR and update Most Recent CR when delegating in two subsequent epochs', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. + TestAction.Finalize, // moves to epoch 1 + ], + [ + TestAction.Delegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [{ event: 'SetCumulativeReward', epoch: 2 }, { event: 'SetMostRecentCumulativeReward', epoch: 2 }], + ); + }); + it('should set/unset CR and update Most Recent CR when delegating in two subsequent epochs, when there is an old CR to clear', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + TestAction.Finalize, // moves to epoch 2 + TestAction.Finalize, // moves to epoch 3 + TestAction.Delegate, // copies CR from epoch 1 to epoch 3. Sets most recent CR to epoch 3. + TestAction.Finalize, // moves to epoch 4 + ], + [ + TestAction.Delegate, // copies CR from epoch 1 to epoch 3. Sets most recent CR to epoch 3. + ], + [ + { event: 'SetCumulativeReward', epoch: 4 }, + { event: 'SetMostRecentCumulativeReward', epoch: 4 }, + { event: 'UnsetCumulativeReward', epoch: 2 }, + ], + ); + }); + it('should set/unset CR and update Most Recent CR re-delegating after one full epoch', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + TestAction.Finalize, // moves to epoch 2 + TestAction.Finalize, // moves to epoch 3 + ], + [ + TestAction.Delegate, // copies CR from epoch 1 to epoch 3. Sets most recent CR to epoch 3. + ], + [ + { event: 'SetCumulativeReward', epoch: 2 }, + { event: 'SetMostRecentCumulativeReward', epoch: 2 }, + { event: 'SetCumulativeReward', epoch: 3 }, + { event: 'SetMostRecentCumulativeReward', epoch: 3 }, + { event: 'UnsetCumulativeReward', epoch: 1 }, + ], + ); + }); + it('should set/unset CR and update Most Recent CR when redelegating after receiving a reward', async () => { + await simulation.runTestAsync( + [ + TestAction.CreatePool, // creates CR in epoch 0 + TestAction.Delegate, // does nothing wrt CR + TestAction.Finalize, // moves to epoch 1 + TestAction.Delegate, // copies CR from epoch 0 to epoch 1. Sets most recent CR to epoch 1. + TestAction.Finalize, // moves to epoch 2 + TestAction.PayProtocolFee, // this means a CR will be available upon finalization + TestAction.Finalize, // creates new CR for epoch 2; moves to epoch 3 + ], + [ + TestAction.Undelegate, // copies CR from epoch 1 to epoch 2. Sets most recent CR to epoch 2. + ], + [ + { event: 'SetCumulativeReward', epoch: 3 }, + { event: 'SetMostRecentCumulativeReward', epoch: 3 }, + { event: 'UnsetCumulativeReward', epoch: 0 }, + { event: 'UnsetCumulativeReward', epoch: 1 }, + ], + ); + }); + }); +}); +// tslint:enable:no-unnecessary-type-assertion diff --git a/contracts/staking/test/rewards_test.ts b/contracts/staking/test/rewards_test.ts index 62f74b0837..97e434706d 100644 --- a/contracts/staking/test/rewards_test.ts +++ b/contracts/staking/test/rewards_test.ts @@ -177,6 +177,15 @@ blockchainTests.resets('Testing Rewards', env => { await finalizer.finalizeAsync([{ reward: fee, poolId }]); }; const ZERO = new BigNumber(0); + it('Reward balance should be zero if not delegated', async () => { + // sanity balances - all zero + await validateEndBalances({}); + }); + it('Reward balance should be zero if not delegated, when epoch is greater than 0', async () => { + await payProtocolFeeAndFinalize(); + // sanity balances - all zero + await validateEndBalances({}); + }); it('Reward balance should be zero in same epoch as delegation', async () => { const amount = toBaseUnitAmount(4); await stakers[0].stakeAsync(amount); @@ -269,6 +278,7 @@ blockchainTests.resets('Testing Rewards', env => { }); it('Should split pool reward between delegators, when they join in different epochs', async () => { // first staker delegates (epoch 0) + const stakeAmounts = [toBaseUnitAmount(4), toBaseUnitAmount(6)]; const totalStakeAmount = toBaseUnitAmount(10); await stakers[0].stakeAsync(stakeAmounts[0]); @@ -277,8 +287,10 @@ blockchainTests.resets('Testing Rewards', env => { new StakeInfo(StakeStatus.Delegated, poolId), stakeAmounts[0], ); + // skip epoch, so staker can start earning rewards await payProtocolFeeAndFinalize(); + // second staker delegates (epoch 1) await stakers[1].stakeAsync(stakeAmounts[1]); await stakers[1].moveStakeAsync( @@ -286,9 +298,11 @@ blockchainTests.resets('Testing Rewards', env => { new StakeInfo(StakeStatus.Delegated, poolId), stakeAmounts[1], ); + // skip epoch, so staker can start earning rewards await payProtocolFeeAndFinalize(); // finalize + const reward = toBaseUnitAmount(10); await payProtocolFeeAndFinalize(reward); // sanity check final balances @@ -499,15 +513,19 @@ blockchainTests.resets('Testing Rewards', env => { await payProtocolFeeAndFinalize(); // earn reward await payProtocolFeeAndFinalize(rewardForDelegator); + // undelegate stake and finalize epoch await stakers[0].moveStakeAsync( new StakeInfo(StakeStatus.Delegated, poolId), new StakeInfo(StakeStatus.Active), stakeAmount, ); + await payProtocolFeeAndFinalize(); + // this should not go do the delegator await payProtocolFeeAndFinalize(rewardNotForDelegator); + // sanity check final balances await validateEndBalances({ stakerEthVaultBalance_1: rewardForDelegator, @@ -602,6 +620,43 @@ blockchainTests.resets('Testing Rewards', env => { membersRewardVaultBalance: rewardsForDelegator[1], }); }); + it('Should collect fees correctly when there is a payout for `currentEpochBalance` but not `nextEpochBalance`', async () => { + // first staker delegates (epoch 0) + const rewardForDelegator = toBaseUnitAmount(10); + const stakeAmount = toBaseUnitAmount(4); + await stakers[0].stakeAsync(stakeAmount); + await stakers[0].moveStakeAsync( + new StakeInfo(StakeStatus.Active), + new StakeInfo(StakeStatus.Delegated, poolId), + stakeAmount, + ); + // skip epoch, so staker can start earning rewards + await payProtocolFeeAndFinalize(); + // undelegate stake and finalize epoch + await stakers[0].moveStakeAsync( + new StakeInfo(StakeStatus.Delegated, poolId), + new StakeInfo(StakeStatus.Active), + stakeAmount, + ); + // this should go to the delegator + await payProtocolFeeAndFinalize(rewardForDelegator); + + // delegate stake ~ this will result in a payout where rewards are computed on + // the balance's `currentEpochBalance` field but not the `nextEpochBalance` field. + await stakers[0].moveStakeAsync( + new StakeInfo(StakeStatus.Active), + new StakeInfo(StakeStatus.Delegated, poolId), + stakeAmount, + ); + // sanity check final balances + await validateEndBalances({ + stakerRewardVaultBalance_1: ZERO, + stakerEthVaultBalance_1: rewardForDelegator, + operatorRewardVaultBalance: ZERO, + poolRewardVaultBalance: ZERO, + membersRewardVaultBalance: ZERO, + }); + }); }); }); // tslint:enable:no-unnecessary-type-assertion diff --git a/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts b/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts new file mode 100644 index 0000000000..0de040689a --- /dev/null +++ b/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts @@ -0,0 +1,172 @@ +import { BlockchainTestsEnvironment, expect, txDefaults } from '@0x/contracts-test-utils'; +import { BigNumber } from '@0x/utils'; +import { DecodedLogArgs, TransactionReceiptWithDecodedLogs } from 'ethereum-types'; +import * as _ from 'lodash'; + +import { TestCumulativeRewardTrackingContract } from '../../generated-wrappers/test_cumulative_reward_tracking'; +import { artifacts } from '../../src'; + +import { StakingApiWrapper } from './api_wrapper'; +import { toBaseUnitAmount } from './number_utils'; +import { StakeInfo, StakeStatus } from './types'; + +export enum TestAction { + Finalize, + Delegate, + Undelegate, + PayProtocolFee, + CreatePool, +} + +interface TestLog { + event: string; + epoch: number; +} + +export class CumulativeRewardTrackingSimulation { + private readonly _amountToStake = toBaseUnitAmount(100); + private readonly _protocolFeeAmount = new BigNumber(10); + private readonly _stakingApiWrapper: StakingApiWrapper; + private readonly _staker: string; + private readonly _poolOperator: string; + private readonly _takerAddress: string; + private readonly _exchangeAddress: string; + private _testCumulativeRewardTrackingContract?: TestCumulativeRewardTrackingContract; + private _poolId: string; + + private static _extractTestLogs(txReceiptLogs: DecodedLogArgs[]): TestLog[] { + const logs = []; + for (const log of txReceiptLogs) { + if ((log as any).event === 'SetMostRecentCumulativeReward') { + logs.push({ + event: 'SetMostRecentCumulativeReward', + epoch: (log as any).args.epoch, + }); + } else if ((log as any).event === 'SetCumulativeReward') { + logs.push({ + event: 'SetCumulativeReward', + epoch: (log as any).args.epoch, + }); + } else if ((log as any).event === 'UnsetCumulativeReward') { + logs.push({ + event: 'UnsetCumulativeReward', + epoch: (log as any).args.epoch, + }); + } + } + return logs; + } + + private static _assertTestLogs(expectedSequence: TestLog[], txReceiptLogs: DecodedLogArgs[]): void { + const logs = CumulativeRewardTrackingSimulation._extractTestLogs(txReceiptLogs); + expect(logs.length).to.be.equal(expectedSequence.length); + for (let i = 0; i < expectedSequence.length; i++) { + const expectedLog = expectedSequence[i]; + const actualLog = logs[i]; + expect(expectedLog.event, `testing event name of ${JSON.stringify(expectedLog)}`).to.be.equal( + actualLog.event, + ); + expect(expectedLog.epoch, `testing epoch of ${JSON.stringify(expectedLog)}`).to.be.equal(actualLog.epoch); + } + } + + constructor(stakingApiWrapper: StakingApiWrapper, actors: string[]) { + this._stakingApiWrapper = stakingApiWrapper; + // setup actors + this._staker = actors[0]; + this._poolOperator = actors[1]; + this._takerAddress = actors[2]; + this._exchangeAddress = actors[3]; + this._poolId = ''; + } + + public async deployAndConfigureTestContractsAsync(env: BlockchainTestsEnvironment): Promise { + // set exchange address + await this._stakingApiWrapper.stakingContract.addExchangeAddress.awaitTransactionSuccessAsync( + this._exchangeAddress, + ); + this._testCumulativeRewardTrackingContract = await TestCumulativeRewardTrackingContract.deployFrom0xArtifactAsync( + artifacts.TestCumulativeRewardTracking, + env.provider, + txDefaults, + artifacts, + ); + } + + public getTestCumulativeRewardTrackingContract(): TestCumulativeRewardTrackingContract { + if (this._testCumulativeRewardTrackingContract === undefined) { + throw new Error(`Contract has not been deployed. Run 'deployAndConfigureTestContractsAsync'.`); + } + return this._testCumulativeRewardTrackingContract; + } + + public async runTestAsync( + initActions: TestAction[], + testActions: TestAction[], + expectedTestLogs: TestLog[], + ): Promise { + await this._executeActionsAsync(initActions); + await this._stakingApiWrapper.stakingProxyContract.attachStakingContract.awaitTransactionSuccessAsync( + this.getTestCumulativeRewardTrackingContract().address, + ); + const testLogs = await this._executeActionsAsync(testActions); + CumulativeRewardTrackingSimulation._assertTestLogs(expectedTestLogs, testLogs); + } + + private async _executeActionsAsync(actions: TestAction[]): Promise { + let logs: DecodedLogArgs[] = []; + for (const action of actions) { + let txReceipt: TransactionReceiptWithDecodedLogs; + switch (action) { + case TestAction.Finalize: + txReceipt = await this._stakingApiWrapper.utils.skipToNextEpochAsync(); + break; + + case TestAction.Delegate: + await this._stakingApiWrapper.stakingContract.stake.sendTransactionAsync(this._amountToStake, { + from: this._staker, + }); + txReceipt = await this._stakingApiWrapper.stakingContract.moveStake.awaitTransactionSuccessAsync( + new StakeInfo(StakeStatus.Active), + new StakeInfo(StakeStatus.Delegated, this._poolId), + this._amountToStake, + { from: this._staker }, + ); + break; + + case TestAction.Undelegate: + txReceipt = await this._stakingApiWrapper.stakingContract.moveStake.awaitTransactionSuccessAsync( + new StakeInfo(StakeStatus.Delegated, this._poolId), + new StakeInfo(StakeStatus.Active), + this._amountToStake, + { from: this._staker }, + ); + break; + + case TestAction.PayProtocolFee: + txReceipt = await this._stakingApiWrapper.stakingContract.payProtocolFee.awaitTransactionSuccessAsync( + this._poolOperator, + this._takerAddress, + this._protocolFeeAmount, + { from: this._exchangeAddress, value: this._protocolFeeAmount }, + ); + break; + + case TestAction.CreatePool: + txReceipt = await this._stakingApiWrapper.stakingContract.createStakingPool.awaitTransactionSuccessAsync( + 0, + true, + { from: this._poolOperator }, + ); + const createStakingPoolLog = txReceipt.logs[0]; + this._poolId = (createStakingPoolLog as any).args.poolId; + break; + + default: + throw new Error('Unrecognized test action'); + } + logs = logs.concat(txReceipt.logs); + } + return logs; + } +} diff --git a/contracts/staking/tsconfig.json b/contracts/staking/tsconfig.json index fd993162f4..4c4584f4de 100644 --- a/contracts/staking/tsconfig.json +++ b/contracts/staking/tsconfig.json @@ -19,6 +19,7 @@ "generated-artifacts/LibSafeDowncast.json", "generated-artifacts/LibStakingRichErrors.json", "generated-artifacts/MixinConstants.json", + "generated-artifacts/MixinCumulativeRewards.json", "generated-artifacts/MixinDeploymentConstants.json", "generated-artifacts/MixinEthVault.json", "generated-artifacts/MixinExchangeFees.json", @@ -39,6 +40,7 @@ "generated-artifacts/StakingPoolRewardVault.json", "generated-artifacts/StakingProxy.json", "generated-artifacts/TestCobbDouglas.json", + "generated-artifacts/TestCumulativeRewardTracking.json", "generated-artifacts/TestInitTarget.json", "generated-artifacts/TestLibFixedMath.json", "generated-artifacts/TestLibProxy.json", diff --git a/contracts/utils/contracts/src/LibSafeMathRichErrors.sol b/contracts/utils/contracts/src/LibSafeMathRichErrors.sol index 618cae0dcc..4c215eab35 100644 --- a/contracts/utils/contracts/src/LibSafeMathRichErrors.sol +++ b/contracts/utils/contracts/src/LibSafeMathRichErrors.sol @@ -27,6 +27,7 @@ library LibSafeMathRichErrors { } enum DowncastErrorCodes { + VALUE_TOO_LARGE_TO_DOWNCAST_TO_UINT32, VALUE_TOO_LARGE_TO_DOWNCAST_TO_UINT64, VALUE_TOO_LARGE_TO_DOWNCAST_TO_UINT96 } From f9163ccc01308bc6060a525af46af3e27748a068 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Mon, 16 Sep 2019 14:30:26 -0700 Subject: [PATCH 2/5] Improved readability of API for cumulative rewards --- .../staking_pools/MixinCumulativeRewards.sol | 142 +++++++++++++----- .../staking_pools/MixinStakingPoolRewards.sol | 7 +- .../staking/contracts/src/sys/MixinParams.sol | 16 +- .../contracts/src/sys/MixinScheduler.sol | 16 +- .../test/TestCumulativeRewardTracking.sol | 48 +++--- .../test/cumulative_reward_tracking_test.ts | 11 +- .../cumulative_reward_tracking_simulation.ts | 14 +- 7 files changed, 178 insertions(+), 76 deletions(-) diff --git a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol index bc1a6a514d..be519b9c0b 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol @@ -41,12 +41,20 @@ 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 { uint256 currentEpoch = getCurrentEpoch(); - _setCumulativeReward(poolId, currentEpoch, IStructs.Fraction({numerator: 0, denominator: MIN_TOKEN_VALUE})); - _setMostRecentCumulativeReward(poolId, currentEpoch); + // sets the default cumulative reward + _forceSetCumulativeReward( + poolId, + currentEpoch, + IStructs.Fraction({ + numerator: 0, + denominator: MIN_TOKEN_VALUE + }) + ); } /// @dev returns true iff Cumulative Rewards are set @@ -61,11 +69,45 @@ contract MixinCumulativeRewards is return cumulativeReward.denominator != 0; } - /// @dev Sets a cumulative reward for `poolId` at `epoch`. + /// Returns true iff the cumulative reward for `poolId` at `epoch` can be unset. + /// @param poolId Unique id of pool. + /// @param epoch of the cumulative reward. + function _canUnsetCumulativeReward(bytes32 poolId, uint256 epoch) + internal + view + returns (bool) + { + return ( + _isCumulativeRewardSet(cumulativeRewardsByPool[poolId][epoch]) && // is there a value to unset + cumulativeRewardsByPoolReferenceCounter[poolId][epoch] == 0 && // no references to this CR + cumulativeRewardsByPoolLastStored[poolId] > epoch // this is *not* the most recent CR + ); + } + + /// @dev Tries to set a cumulative reward for `poolId` at `epoch`. /// @param poolId Unique Id of pool. /// @param epoch of cumulative reward. /// @param value of cumulative reward. - function _setCumulativeReward( + function _trySetCumulativeReward( + bytes32 poolId, + uint256 epoch, + IStructs.Fraction memory value + ) + internal + { + if (_isCumulativeRewardSet(cumulativeRewardsByPool[poolId][epoch])) { + // do nothing; we don't want to override the current value + 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. + /// @param epoch of cumulative reward. + /// @param value of cumulative reward. + function _forceSetCumulativeReward( bytes32 poolId, uint256 epoch, IStructs.Fraction memory value @@ -73,15 +115,27 @@ contract MixinCumulativeRewards is internal { cumulativeRewardsByPool[poolId][epoch] = value; + _trySetMostRecentCumulativeRewardEpoch(poolId, epoch); + } + + /// @dev Tries to unset the cumulative reward for `poolId` at `epoch`. + /// @param poolId Unique id of pool. + /// @param epoch of cumulative reward to unset. + function _tryUnsetCumulativeReward(bytes32 poolId, uint256 epoch) + internal + { + if (!_canUnsetCumulativeReward(poolId, epoch)) { + return; + } + _forceUnsetCumulativeReward(poolId, epoch); } /// @dev Unsets the cumulative reward for `poolId` at `epoch`. /// @param poolId Unique id of pool. /// @param epoch of cumulative reward to unset. - function _unsetCumulativeReward(bytes32 poolId, uint256 epoch) + function _forceUnsetCumulativeReward(bytes32 poolId, uint256 epoch) internal { - assert(cumulativeRewardsByPoolReferenceCounter[poolId][epoch] == 0); cumulativeRewardsByPool[poolId][epoch] = IStructs.Fraction({numerator: 0, denominator: 0}); } @@ -100,11 +154,44 @@ contract MixinCumulativeRewards is }); } + /// @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 of the most recent cumulative reward. + function _trySetMostRecentCumulativeRewardEpoch(bytes32 poolId, uint256 epoch) + internal + { + // load the current value, sanity check that we're not trying to go back in time + uint256 currentMostRecentEpoch = cumulativeRewardsByPoolLastStored[poolId]; + assert(epoch >= currentMostRecentEpoch); + + // check if we should do any work + if (epoch == currentMostRecentEpoch) { + return; + } + + // update state to reflect the most recent cumulative reward + _forceSetMostRecentCumulativeRewardEpoch(poolId, epoch); + + // unset the previous most recent reward, if it is no longer needed + _tryUnsetCumulativeReward(poolId, currentMostRecentEpoch); + } + + /// @dev Forcefully sets the epoch of the most recent cumulative reward. + /// @param poolId Unique Id of pool. + /// @param epoch of the most recent cumulative reward. + function _forceSetMostRecentCumulativeRewardEpoch(bytes32 poolId, uint256 epoch) + internal + { + cumulativeRewardsByPoolLastStored[poolId] = epoch; + } + /// @dev Adds a dependency on a cumulative reward for a given epoch. /// @param poolId Unique Id of pool. /// @param epoch to remove dependency from. - /// @param mostRecentCumulativeRewardInfo Epoch of the most recent cumulative reward. - /// @param isDependent still FGREG EEGGEGREG + /// @param mostRecentCumulativeRewardInfo Info for the most recent cumulative reward (value and epoch) + /// @param isDependent True iff there is a dependency on the cumulative reward for `poolId` at `epoch` function _addOrRemoveDependencyOnCumulativeReward( bytes32 poolId, uint256 epoch, @@ -142,11 +229,12 @@ contract MixinCumulativeRewards is // add dependency by increasing the reference counter cumulativeRewardsByPoolReferenceCounter[poolId][epoch] = cumulativeRewardsByPoolReferenceCounter[poolId][epoch].safeAdd(1); - // ensure dependency has a value, otherwise copy it from the most recent reward - if (!_isCumulativeRewardSet(cumulativeRewardsByPool[poolId][epoch])) { - _setCumulativeReward(poolId, epoch, mostRecentCumulativeRewardInfo.cumulativeReward); - _setMostRecentCumulativeReward(poolId, epoch); - } + // set CR to most recent reward (if it is not already set) + _trySetCumulativeReward( + poolId, + epoch, + mostRecentCumulativeRewardInfo.cumulativeReward + ); } /// @dev Removes a dependency on a cumulative reward for a given epoch. @@ -165,9 +253,7 @@ contract MixinCumulativeRewards is cumulativeRewardsByPoolReferenceCounter[poolId][epoch] = newReferenceCounter; // clear cumulative reward from state, if it is no longer needed - if (newReferenceCounter == 0 && epoch < mostRecentCumulativeRewardEpoch) { - _unsetCumulativeReward(poolId, epoch); - } + _tryUnsetCumulativeReward(poolId, epoch); } /// @dev Computes a member's reward over a given epoch interval. @@ -203,28 +289,4 @@ contract MixinCumulativeRewards is ); return reward; } - - /// @dev Sets the most recent cumulative reward for the pool. - /// @param poolId Unique Id of pool. - /// @param epoch The epoch of the most recent cumulative reward. - function _setMostRecentCumulativeReward(bytes32 poolId, uint256 epoch) - internal - { - // load the current value, sanity check that we're not trying to go back in time - uint256 currentMostRecentEpoch = cumulativeRewardsByPoolLastStored[poolId]; - assert(epoch >= currentMostRecentEpoch); - - // check if we should do any work - if (epoch == currentMostRecentEpoch) { - return; - } - - // unset the current most recent reward if it has no more references - if (cumulativeRewardsByPoolReferenceCounter[poolId][currentMostRecentEpoch] == 0) { - _unsetCumulativeReward(poolId, currentMostRecentEpoch); - } - - // update state to reflect the most recent cumulative reward - cumulativeRewardsByPoolLastStored[poolId] = epoch; - } } diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index fc43b2d5ed..158578f027 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -135,7 +135,7 @@ contract MixinStakingPoolRewards is ); // store cumulative rewards and set most recent - _setCumulativeReward( + _trySetCumulativeReward( poolId, epoch, IStructs.Fraction({ @@ -143,7 +143,6 @@ contract MixinStakingPoolRewards is denominator: denominatorNormalized }) ); - _setMostRecentCumulativeReward(poolId, epoch); } /// @dev Transfers a delegators accumulated rewards from the transient pool Reward Pool vault @@ -187,8 +186,8 @@ contract MixinStakingPoolRewards is view returns (uint256 totalReward) { - // value is always zero in these two scenarios: - // 1. The owner's delegated is current as of this epoch: their rewards have been moved to the ETH vault. + // reward balance is always zero in these two scenarios: + // 1. The owner's delegated stake is current as of this epoch: their rewards have been moved to the ETH vault. // 2. The current epoch is zero: delegation begins at epoch 1 if (unsyncedDelegatedStakeToPoolByOwner.currentEpoch == currentEpoch || currentEpoch == 0) return 0; diff --git a/contracts/staking/contracts/src/sys/MixinParams.sol b/contracts/staking/contracts/src/sys/MixinParams.sol index 91d35c42c2..3a4a373b25 100644 --- a/contracts/staking/contracts/src/sys/MixinParams.sol +++ b/contracts/staking/contracts/src/sys/MixinParams.sol @@ -102,8 +102,11 @@ contract MixinParams is _cobbDouglasAlphaDenomintor = cobbDouglasAlphaDenomintor; } - /// @dev Initialzize storage belonging to this mixin. - function _initMixinParams() internal { + /// @dev Assert param values before initializing them. + /// This must be updated for each migration. + function _assertMixinParamsBeforeInit() + internal + { // Ensure state is uninitialized. if (epochDurationInSeconds != 0 && rewardDelegatedStakeWeight != 0 && @@ -118,6 +121,15 @@ contract MixinParams is ) ); } + } + + /// @dev Initialize storage belonging to this mixin. + function _initMixinParams() + internal + { + // assert the current values before overwriting them. + _assertMixinParamsBeforeInit(); + // Set up defaults. epochDurationInSeconds = 2 weeks; rewardDelegatedStakeWeight = (90 * PPM_DENOMINATOR) / 100; // 90% diff --git a/contracts/staking/contracts/src/sys/MixinScheduler.sol b/contracts/staking/contracts/src/sys/MixinScheduler.sol index baebd5b656..481a965996 100644 --- a/contracts/staking/contracts/src/sys/MixinScheduler.sol +++ b/contracts/staking/contracts/src/sys/MixinScheduler.sol @@ -75,9 +75,9 @@ contract MixinScheduler is return getCurrentEpochStartTimeInSeconds().safeAdd(epochDurationInSeconds); } - /// @dev Initializes state owned by this mixin. - /// Fails if state was already initialized. - function _initMixinScheduler() + /// @dev Assert scheduler state before initializing it. + /// This must be updated for each migration. + function _assertMixinSchedulerBeforeInit() internal { if (currentEpochStartTimeInSeconds != 0) { @@ -87,6 +87,16 @@ contract MixinScheduler is ) ); } + } + + /// @dev Initializes state owned by this mixin. + /// Fails if state was already initialized. + function _initMixinScheduler() + internal + { + // assert the current values before overwriting them. + _assertMixinSchedulerBeforeInit(); + // solhint-disable-next-line currentEpochStartTimeInSeconds = block.timestamp; } diff --git a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol index cd7a844378..2747a0dcba 100644 --- a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol +++ b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol @@ -25,7 +25,7 @@ contract TestCumulativeRewardTracking is TestStaking { - event SetMostRecentCumulativeReward( + event SetCumulativeReward( bytes32 poolId, uint256 epoch ); @@ -35,29 +35,41 @@ contract TestCumulativeRewardTracking is uint256 epoch ); - event SetCumulativeReward( + event SetMostRecentCumulativeReward( bytes32 poolId, uint256 epoch ); - function _setMostRecentCumulativeReward(bytes32 poolId, uint256 epoch) - internal - { - emit SetMostRecentCumulativeReward(poolId, epoch); - MixinCumulativeRewards._setMostRecentCumulativeReward(poolId, epoch); - } - - function _unsetCumulativeReward(bytes32 poolId, uint256 epoch) - internal - { - emit UnsetCumulativeReward(poolId, epoch); - MixinCumulativeRewards._unsetCumulativeReward(poolId, epoch); - } - - function _setCumulativeReward(bytes32 poolId, uint256 epoch, IStructs.Fraction memory value) + function _forceSetCumulativeReward( + bytes32 poolId, + uint256 epoch, + IStructs.Fraction memory value + ) internal { emit SetCumulativeReward(poolId, epoch); - MixinCumulativeRewards._setCumulativeReward(poolId, epoch, value); + MixinCumulativeRewards._forceSetCumulativeReward(poolId, epoch, value); } + + function _forceUnsetCumulativeReward(bytes32 poolId, uint256 epoch) + internal + { + emit UnsetCumulativeReward(poolId, epoch); + MixinCumulativeRewards._forceUnsetCumulativeReward(poolId, epoch); + } + + function _forceSetMostRecentCumulativeRewardEpoch(bytes32 poolId, uint256 epoch) + internal + { + emit SetMostRecentCumulativeReward(poolId, epoch); + MixinCumulativeRewards._forceSetMostRecentCumulativeRewardEpoch(poolId, epoch); + } + + function _assertMixinParamsBeforeInit() + internal + {} + + function _assertMixinSchedulerBeforeInit() + internal + {} } diff --git a/contracts/staking/test/cumulative_reward_tracking_test.ts b/contracts/staking/test/cumulative_reward_tracking_test.ts index 184ef06d48..83429ceed4 100644 --- a/contracts/staking/test/cumulative_reward_tracking_test.ts +++ b/contracts/staking/test/cumulative_reward_tracking_test.ts @@ -34,11 +34,18 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { }); describe('Tracking Cumulative Rewards (CR)', () => { - it('should set CR and Most Recent CR when a pool is created', async () => { + it('should set CR hen a pool is created is epoch 0', async () => { await simulation.runTestAsync( [], [TestAction.CreatePool], - [{ event: 'SetCumulativeReward', epoch: 0 }, { event: 'SetMostRecentCumulativeReward', epoch: 0 }], + [{ event: 'SetCumulativeReward', epoch: 0 }], + ); + }); + it('should set CR and Most Recent CR when a pool is created in epoch >0', async () => { + await simulation.runTestAsync( + [TestAction.Finalize], + [TestAction.CreatePool], + [{ event: 'SetCumulativeReward', epoch: 1 }, { event: 'SetMostRecentCumulativeReward', epoch: 1 }], ); }); it('should not set CR or Most Recent CR when values already exist for the current epoch', async () => { diff --git a/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts b/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts index 0de040689a..70d30aa6c3 100644 --- a/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts +++ b/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts @@ -37,20 +37,20 @@ export class CumulativeRewardTrackingSimulation { private static _extractTestLogs(txReceiptLogs: DecodedLogArgs[]): TestLog[] { const logs = []; for (const log of txReceiptLogs) { - if ((log as any).event === 'SetMostRecentCumulativeReward') { + if ((log as DecodedLogArgs).event === 'SetMostRecentCumulativeReward') { logs.push({ event: 'SetMostRecentCumulativeReward', - epoch: (log as any).args.epoch, + epoch: (log as DecodedLogArgs).args.epoch.toNumber(), }); - } else if ((log as any).event === 'SetCumulativeReward') { + } else if ((log as DecodedLogArgs).event === 'SetCumulativeReward') { logs.push({ event: 'SetCumulativeReward', - epoch: (log as any).args.epoch, + epoch: (log as DecodedLogArgs).args.epoch.toNumber(), }); - } else if ((log as any).event === 'UnsetCumulativeReward') { + } else if ((log as DecodedLogArgs).event === 'UnsetCumulativeReward') { logs.push({ event: 'UnsetCumulativeReward', - epoch: (log as any).args.epoch, + epoch: (log as DecodedLogArgs).args.epoch.toNumber(), }); } } @@ -159,7 +159,7 @@ export class CumulativeRewardTrackingSimulation { { from: this._poolOperator }, ); const createStakingPoolLog = txReceipt.logs[0]; - this._poolId = (createStakingPoolLog as any).args.poolId; + this._poolId = (createStakingPoolLog as DecodedLogArgs).args.poolId; break; default: From e1d51bae73a6545e0ca0a15516d49cdd785642f3 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Mon, 16 Sep 2019 16:02:50 -0700 Subject: [PATCH 3/5] Tests are passing and ran linter --- .../staking_pools/MixinCumulativeRewards.sol | 37 +++++++++++-------- .../staking_pools/MixinStakingPoolRewards.sol | 2 +- .../test/TestCumulativeRewardTracking.sol | 24 +++++++++--- .../test/cumulative_reward_tracking_test.ts | 6 +-- .../cumulative_reward_tracking_simulation.ts | 13 ++++--- 5 files changed, 48 insertions(+), 34 deletions(-) diff --git a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol index be519b9c0b..c5a8494f1c 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol @@ -162,29 +162,37 @@ contract MixinCumulativeRewards is function _trySetMostRecentCumulativeRewardEpoch(bytes32 poolId, uint256 epoch) internal { - // load the current value, sanity check that we're not trying to go back in time - uint256 currentMostRecentEpoch = cumulativeRewardsByPoolLastStored[poolId]; - assert(epoch >= currentMostRecentEpoch); - // 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, epoch); - - // unset the previous most recent reward, if it is no longer needed - _tryUnsetCumulativeReward(poolId, currentMostRecentEpoch); + _forceSetMostRecentCumulativeRewardEpoch( + poolId, + currentMostRecentEpoch, + epoch + ); } /// @dev Forcefully sets the epoch of the most recent cumulative reward. /// @param poolId Unique Id of pool. - /// @param epoch of the most recent cumulative reward. - function _forceSetMostRecentCumulativeRewardEpoch(bytes32 poolId, uint256 epoch) + /// @param currentMostRecentEpoch of the most recent cumulative reward. + /// @param newMostRecentEpoch of the new most recent cumulative reward. + function _forceSetMostRecentCumulativeRewardEpoch( + bytes32 poolId, + uint256 currentMostRecentEpoch, + uint256 newMostRecentEpoch + ) internal { - cumulativeRewardsByPoolLastStored[poolId] = epoch; + // sanity check that we're not trying to go back in time + assert(newMostRecentEpoch >= currentMostRecentEpoch); + cumulativeRewardsByPoolLastStored[poolId] = newMostRecentEpoch; + + // unset the previous most recent reward, if it is no longer needed + _tryUnsetCumulativeReward(poolId, currentMostRecentEpoch); } /// @dev Adds a dependency on a cumulative reward for a given epoch. @@ -209,8 +217,7 @@ contract MixinCumulativeRewards is } else { _removeDependencyOnCumulativeReward( poolId, - epoch, - mostRecentCumulativeRewardInfo.cumulativeRewardEpoch + epoch ); } } @@ -240,11 +247,9 @@ contract MixinCumulativeRewards is /// @dev Removes a dependency on a cumulative reward for a given epoch. /// @param poolId Unique Id of pool. /// @param epoch to remove dependency from. - /// @param mostRecentCumulativeRewardEpoch Epoch of the most recent cumulative reward. function _removeDependencyOnCumulativeReward( bytes32 poolId, - uint256 epoch, - uint256 mostRecentCumulativeRewardEpoch + uint256 epoch ) internal { diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index 158578f027..6fae9bf65c 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -135,7 +135,7 @@ contract MixinStakingPoolRewards is ); // store cumulative rewards and set most recent - _trySetCumulativeReward( + _forceSetCumulativeReward( poolId, epoch, IStructs.Fraction({ diff --git a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol index 2747a0dcba..90583db8cb 100644 --- a/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol +++ b/contracts/staking/contracts/test/TestCumulativeRewardTracking.sol @@ -48,7 +48,11 @@ contract TestCumulativeRewardTracking is internal { emit SetCumulativeReward(poolId, epoch); - MixinCumulativeRewards._forceSetCumulativeReward(poolId, epoch, value); + MixinCumulativeRewards._forceSetCumulativeReward( + poolId, + epoch, + value + ); } function _forceUnsetCumulativeReward(bytes32 poolId, uint256 epoch) @@ -58,18 +62,26 @@ contract TestCumulativeRewardTracking is MixinCumulativeRewards._forceUnsetCumulativeReward(poolId, epoch); } - function _forceSetMostRecentCumulativeRewardEpoch(bytes32 poolId, uint256 epoch) + function _forceSetMostRecentCumulativeRewardEpoch( + bytes32 poolId, + uint256 currentMostRecentEpoch, + uint256 newMostRecentEpoch + ) internal { - emit SetMostRecentCumulativeReward(poolId, epoch); - MixinCumulativeRewards._forceSetMostRecentCumulativeRewardEpoch(poolId, epoch); + emit SetMostRecentCumulativeReward(poolId, newMostRecentEpoch); + MixinCumulativeRewards._forceSetMostRecentCumulativeRewardEpoch( + poolId, + currentMostRecentEpoch, + newMostRecentEpoch + ); } function _assertMixinParamsBeforeInit() internal - {} + {} // solhint-disable-line no-empty-blocks function _assertMixinSchedulerBeforeInit() internal - {} + {} // solhint-disable-line no-empty-blocks } diff --git a/contracts/staking/test/cumulative_reward_tracking_test.ts b/contracts/staking/test/cumulative_reward_tracking_test.ts index 83429ceed4..59bc6d6ed0 100644 --- a/contracts/staking/test/cumulative_reward_tracking_test.ts +++ b/contracts/staking/test/cumulative_reward_tracking_test.ts @@ -35,11 +35,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { describe('Tracking Cumulative Rewards (CR)', () => { it('should set CR hen a pool is created is epoch 0', async () => { - await simulation.runTestAsync( - [], - [TestAction.CreatePool], - [{ event: 'SetCumulativeReward', epoch: 0 }], - ); + await simulation.runTestAsync([], [TestAction.CreatePool], [{ event: 'SetCumulativeReward', epoch: 0 }]); }); it('should set CR and Most Recent CR when a pool is created in epoch >0', async () => { await simulation.runTestAsync( diff --git a/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts b/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts index 70d30aa6c3..1df1d47c10 100644 --- a/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts +++ b/contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts @@ -37,20 +37,20 @@ export class CumulativeRewardTrackingSimulation { private static _extractTestLogs(txReceiptLogs: DecodedLogArgs[]): TestLog[] { const logs = []; for (const log of txReceiptLogs) { - if ((log as DecodedLogArgs).event === 'SetMostRecentCumulativeReward') { + if (log.event === 'SetMostRecentCumulativeReward') { logs.push({ event: 'SetMostRecentCumulativeReward', - epoch: (log as DecodedLogArgs).args.epoch.toNumber(), + epoch: log.args.epoch.toNumber(), }); - } else if ((log as DecodedLogArgs).event === 'SetCumulativeReward') { + } else if (log.event === 'SetCumulativeReward') { logs.push({ event: 'SetCumulativeReward', - epoch: (log as DecodedLogArgs).args.epoch.toNumber(), + epoch: log.args.epoch.toNumber(), }); - } else if ((log as DecodedLogArgs).event === 'UnsetCumulativeReward') { + } else if (log.event === 'UnsetCumulativeReward') { logs.push({ event: 'UnsetCumulativeReward', - epoch: (log as DecodedLogArgs).args.epoch.toNumber(), + epoch: log.args.epoch.toNumber(), }); } } @@ -159,6 +159,7 @@ export class CumulativeRewardTrackingSimulation { { from: this._poolOperator }, ); const createStakingPoolLog = txReceipt.logs[0]; + // tslint:disable-next-line no-unnecessary-type-assertion this._poolId = (createStakingPoolLog as DecodedLogArgs).args.poolId; break; From e224e6cde5d5236030c370253e09b29531d8d26c Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Mon, 16 Sep 2019 16:03:40 -0700 Subject: [PATCH 4/5] updated changelog --- contracts/staking/CHANGELOG.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/staking/CHANGELOG.json b/contracts/staking/CHANGELOG.json index 63f1ceefa4..5272f915f6 100644 --- a/contracts/staking/CHANGELOG.json +++ b/contracts/staking/CHANGELOG.json @@ -41,6 +41,10 @@ { "note": "Replace `MixinDeploymentConstants` with `MixinParams`.", "pr": 2131 + }, + { + "note": "Reference counting for cumulative rewards.", + "pr": 2154 } ] } From 43d1d0b2171b6cc76d0dc7e33177f0b6a3c263ad Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Mon, 16 Sep 2019 17:04:16 -0700 Subject: [PATCH 5/5] more explicit sanity checks for computing balance in interval (previously all failed with div-by-zero) typos --- .../src/libs/LibStakingRichErrors.sol | 29 +++++++++++ .../contracts/src/stake/MixinStake.sol | 2 +- .../staking_pools/MixinCumulativeRewards.sol | 52 ++++++++++++++++--- .../test/cumulative_reward_tracking_test.ts | 4 +- contracts/staking/test/rewards_test.ts | 5 +- packages/order-utils/CHANGELOG.json | 4 ++ .../order-utils/src/staking_revert_errors.ts | 22 ++++++++ packages/utils/src/safe_math_revert_errors.ts | 1 + 8 files changed, 106 insertions(+), 13 deletions(-) diff --git a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol index 4e14b81f7b..3daf14c8cb 100644 --- a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol +++ b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol @@ -51,6 +51,12 @@ library LibStakingRichErrors { PoolIsFull } + enum CumulativeRewardIntervalErrorCode { + BeginEpochMustBeLessThanEndEpoch, + BeginEpochDoesNotHaveReward, + EndEpochDoesNotHaveReward + } + // bytes4(keccak256("MiscalculatedRewardsError(uint256,uint256)")) bytes4 internal constant MISCALCULATED_REWARDS_ERROR_SELECTOR = 0xf7806c4e; @@ -147,6 +153,10 @@ library LibStakingRichErrors { bytes internal constant INVALID_WETH_ASSET_DATA_ERROR = hex"24bf322c"; + // bytes4(keccak256("CumulativeRewardIntervalError(uint8,bytes32,uint256,uint256)")) + bytes4 internal constant CUMULATIVE_REWARD_INTERVAL_ERROR_SELECTOR = + 0x1f806d55; + // solhint-disable func-name-mixedcase function MiscalculatedRewardsError( uint256 totalRewardsPaid, @@ -455,4 +465,23 @@ library LibStakingRichErrors { { return INVALID_WETH_ASSET_DATA_ERROR; } + + function CumulativeRewardIntervalError( + CumulativeRewardIntervalErrorCode errorCode, + bytes32 poolId, + uint256 beginEpoch, + uint256 endEpoch + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + CUMULATIVE_REWARD_INTERVAL_ERROR_SELECTOR, + errorCode, + poolId, + beginEpoch, + endEpoch + ); + } } diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index ad32097915..0603d780a2 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -190,7 +190,7 @@ contract MixinStake is // increment how much stake has been delegated to pool _incrementNextBalance(delegatedStakeByPoolId[poolId], amount); - // synchronizes reward state in the pool that the staker is undelegating from + // synchronizes reward state in the pool that the staker is delegating to IStructs.StoredBalance memory finalDelegatedStakeToPoolByOwner = _loadAndSyncBalance(delegatedStakeToPoolByOwner[owner][poolId]); _syncRewardsForDelegator(poolId, owner, initDelegatedStakeToPoolByOwner, finalDelegatedStakeToPoolByOwner); } diff --git a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol index c5a8494f1c..8e19176efc 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol @@ -263,7 +263,7 @@ contract MixinCumulativeRewards is /// @dev Computes a member's reward over a given epoch interval. /// @param poolId Uniqud Id of pool. - /// @param memberStakeOverInterval Stake delegated to pool by meber over the interval. + /// @param memberStakeOverInterval Stake delegated to pool by member over the interval. /// @param beginEpoch beginning of interval. /// @param endEpoch end of interval. /// @return rewards accumulated over interval [beginEpoch, endEpoch] @@ -277,19 +277,55 @@ contract MixinCumulativeRewards is view returns (uint256) { - // sanity check + // sanity check inputs if (memberStakeOverInterval == 0) { return 0; } + // sanity check interval + if (beginEpoch >= endEpoch) { + LibRichErrors.rrevert( + LibStakingRichErrors.CumulativeRewardIntervalError( + LibStakingRichErrors.CumulativeRewardIntervalErrorCode.BeginEpochMustBeLessThanEndEpoch, + poolId, + beginEpoch, + endEpoch + ) + ); + } + + // sanity check begin reward + IStructs.Fraction memory beginReward = cumulativeRewardsByPool[poolId][beginEpoch]; + if (!_isCumulativeRewardSet(beginReward)) { + LibRichErrors.rrevert( + LibStakingRichErrors.CumulativeRewardIntervalError( + LibStakingRichErrors.CumulativeRewardIntervalErrorCode.BeginEpochDoesNotHaveReward, + poolId, + beginEpoch, + endEpoch + ) + ); + } + + // sanity check end reward + IStructs.Fraction memory endReward = cumulativeRewardsByPool[poolId][endEpoch]; + if (!_isCumulativeRewardSet(endReward)) { + LibRichErrors.rrevert( + LibStakingRichErrors.CumulativeRewardIntervalError( + LibStakingRichErrors.CumulativeRewardIntervalErrorCode.EndEpochDoesNotHaveReward, + poolId, + beginEpoch, + endEpoch + ) + ); + } + // compute reward - IStructs.Fraction memory beginRatio = cumulativeRewardsByPool[poolId][beginEpoch]; - IStructs.Fraction memory endRatio = cumulativeRewardsByPool[poolId][endEpoch]; uint256 reward = LibFractions.scaleFractionalDifference( - endRatio.numerator, - endRatio.denominator, - beginRatio.numerator, - beginRatio.denominator, + endReward.numerator, + endReward.denominator, + beginReward.numerator, + beginReward.denominator, memberStakeOverInterval ); return reward; diff --git a/contracts/staking/test/cumulative_reward_tracking_test.ts b/contracts/staking/test/cumulative_reward_tracking_test.ts index 59bc6d6ed0..e7e2f6f6cc 100644 --- a/contracts/staking/test/cumulative_reward_tracking_test.ts +++ b/contracts/staking/test/cumulative_reward_tracking_test.ts @@ -34,7 +34,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { }); describe('Tracking Cumulative Rewards (CR)', () => { - it('should set CR hen a pool is created is epoch 0', async () => { + it('should set CR when a pool is created at epoch 0', async () => { await simulation.runTestAsync([], [TestAction.CreatePool], [{ event: 'SetCumulativeReward', epoch: 0 }]); }); it('should set CR and Most Recent CR when a pool is created in epoch >0', async () => { @@ -166,7 +166,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { ], ); }); - it('should set CR and update Most Recent CR when redelegating, plus remove the CR that is no longer depends on.', async () => { + it('should set CR and update Most Recent CR when redelegating, plus remove the CR that it no longer depends on.', async () => { await simulation.runTestAsync( [ TestAction.CreatePool, // creates CR in epoch 0 diff --git a/contracts/staking/test/rewards_test.ts b/contracts/staking/test/rewards_test.ts index 97e434706d..7822daac27 100644 --- a/contracts/staking/test/rewards_test.ts +++ b/contracts/staking/test/rewards_test.ts @@ -620,7 +620,9 @@ blockchainTests.resets('Testing Rewards', env => { membersRewardVaultBalance: rewardsForDelegator[1], }); }); - it('Should collect fees correctly when there is a payout for `currentEpochBalance` but not `nextEpochBalance`', async () => { + it('Should collect fees correctly when re-delegating after un-delegating', async () => { + // Note - there are two ranges over which payouts are computed (see _computeRewardBalanceOfDelegator). + // This triggers the first range (rewards for `delegatedStake.currentEpoch`), but not the second. // first staker delegates (epoch 0) const rewardForDelegator = toBaseUnitAmount(10); const stakeAmount = toBaseUnitAmount(4); @@ -640,7 +642,6 @@ blockchainTests.resets('Testing Rewards', env => { ); // this should go to the delegator await payProtocolFeeAndFinalize(rewardForDelegator); - // delegate stake ~ this will result in a payout where rewards are computed on // the balance's `currentEpochBalance` field but not the `nextEpochBalance` field. await stakers[0].moveStakeAsync( diff --git a/packages/order-utils/CHANGELOG.json b/packages/order-utils/CHANGELOG.json index 105be7fd17..acd5110299 100644 --- a/packages/order-utils/CHANGELOG.json +++ b/packages/order-utils/CHANGELOG.json @@ -93,6 +93,10 @@ { "note": "Add `InitializationError`, `InvalidParamValue` to `StakingRevertErrors`.", "pr": 2131 + }, + { + "note": "Add `CumulativeRewardIntervalError`.", + "pr": 2154 } ] }, diff --git a/packages/order-utils/src/staking_revert_errors.ts b/packages/order-utils/src/staking_revert_errors.ts index 7ac661781a..9955d5675e 100644 --- a/packages/order-utils/src/staking_revert_errors.ts +++ b/packages/order-utils/src/staking_revert_errors.ts @@ -30,6 +30,12 @@ export enum InitializationErrorCode { MixinParamsAlreadyInitialized, } +export enum CumulativeRewardIntervalErrorCode { + BeginEpochMustBeLessThanEndEpoch, + BeginEpochDoesNotHaveReward, + EndEpochDoesNotHaveReward, +} + export class MiscalculatedRewardsError extends RevertError { constructor(totalRewardsPaid?: BigNumber | number | string, initialContractBalance?: BigNumber | number | string) { super( @@ -225,6 +231,21 @@ export class ProxyDestinationCannotBeNilError extends RevertError { } } +export class CumulativeRewardIntervalError extends RevertError { + constructor( + errorCode?: CumulativeRewardIntervalErrorCode, + poolId?: string, + beginEpoch?: BigNumber | number | string, + endEpoch?: BigNumber | number | string, + ) { + super( + 'CumulativeRewardIntervalError', + 'CumulativeRewardIntervalError(uint8 errorCode, bytes32 poolId, uint256 beginEpoch, uint256 endEpoch)', + { errorCode, poolId, beginEpoch, endEpoch }, + ); + } +} + const types = [ AmountExceedsBalanceOfPoolError, BlockTimestampTooLowError, @@ -249,6 +270,7 @@ const types = [ RewardVaultNotSetError, WithdrawAmountExceedsMemberBalanceError, ProxyDestinationCannotBeNilError, + CumulativeRewardIntervalError, ]; // Register the types we've defined. diff --git a/packages/utils/src/safe_math_revert_errors.ts b/packages/utils/src/safe_math_revert_errors.ts index 874e9bdaf5..2d0bce62ea 100644 --- a/packages/utils/src/safe_math_revert_errors.ts +++ b/packages/utils/src/safe_math_revert_errors.ts @@ -11,6 +11,7 @@ export enum BinOpErrorCodes { } export enum DowncastErrorCodes { + ValueTooLargeToDowncastToUint32, ValueTooLargeToDowncastToUint64, ValueTooLargeToDowncastToUint96, }