Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Commit

Permalink
Merge pull request #2263 from 0xProject/fix/3.0-audit/staking/cumulat…
Browse files Browse the repository at this point in the history 
…ive-rewards-refactor

Staking: Refactor and slightly simplify rewards tracking
  • Loading branch information
@abandeali1
abandeali1 committed Oct 14, 2019
2 parents 43fa753 + a6603d6 commit 5128295
Show file tree
Hide file tree
Showing 7 changed files with 94 additions and 112 deletions.
108 changes: 67 additions & 41 deletions contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,21 +31,6 @@ contract MixinCumulativeRewards is
{
using LibSafeMath for uint256;

/// @dev Initializes Cumulative Rewards for a given pool.
/// @param poolId Unique id of pool.
function _initializeCumulativeRewards(bytes32 poolId)
internal
{
// Sets the default cumulative reward
_forceSetCumulativeReward(
poolId,
IStructs.Fraction({
numerator: 0,
denominator: MIN_TOKEN_VALUE
})
);
}

/// @dev returns true iff Cumulative Rewards are set
function _isCumulativeRewardSet(IStructs.Fraction memory cumulativeReward)
internal
Expand All @@ -58,22 +43,71 @@ contract MixinCumulativeRewards is
return cumulativeReward.denominator != 0;
}

/// @dev Sets a cumulative reward for `poolId` at `epoch`.
/// This can be used to overwrite an existing value.
/// @param poolId Unique Id of pool.
/// @param value Value of cumulative reward.
function _forceSetCumulativeReward(
/// @dev Sets a pool's cumulative delegator rewards for the current epoch,
/// given the rewards earned and stake from the last epoch, which will
/// be summed with the previous cumulative rewards for this pool.
/// If the last cumulative reward epoch is the current epoch, this is a
/// no-op.
/// @param poolId The pool ID.
/// @param reward The total reward earned by pool delegators from the last epoch.
/// @param stake The total delegated stake in the pool in the last epoch.
function _addCumulativeReward(
bytes32 poolId,
IStructs.Fraction memory value
uint256 reward,
uint256 stake
)
internal
{
// Fetch the last epoch at which we stored an entry for this pool;
// this is the most up-to-date cumulative rewards for this pool.
uint256 lastStoredEpoch = _cumulativeRewardsByPoolLastStored[poolId];
uint256 currentEpoch_ = currentEpoch;
_cumulativeRewardsByPool[poolId][currentEpoch_] = value;
// Update state to reflect the most recent cumulative reward

// If we already have a record for this epoch, don't overwrite it.
if (lastStoredEpoch == currentEpoch_) {
return;
}

IStructs.Fraction memory mostRecentCumulativeReward =
_cumulativeRewardsByPool[poolId][lastStoredEpoch];

// Compute new cumulative reward
IStructs.Fraction memory cumulativeReward;
if (_isCumulativeRewardSet(mostRecentCumulativeReward)) {
// If we have a prior cumulative reward entry, we sum them as fractions.
(cumulativeReward.numerator, cumulativeReward.denominator) = LibFractions.add(
mostRecentCumulativeReward.numerator,
mostRecentCumulativeReward.denominator,
reward,
stake
);
// Normalize to prevent overflows in future operations.
(cumulativeReward.numerator, cumulativeReward.denominator) = LibFractions.normalize(
cumulativeReward.numerator,
cumulativeReward.denominator
);
} else {
(cumulativeReward.numerator, cumulativeReward.denominator) = (reward, stake);
}

// Store cumulative rewards for this epoch.
_cumulativeRewardsByPool[poolId][currentEpoch_] = cumulativeReward;
_cumulativeRewardsByPoolLastStored[poolId] = currentEpoch_;
}

/// @dev Sets a pool's cumulative delegator rewards for the current epoch,
/// using the last stored cumulative rewards. If we've already set
/// a CR for this epoch, this is a no-op.
/// @param poolId The pool ID.
function _updateCumulativeReward(bytes32 poolId)
internal
{
// Just add empty rewards for this epoch, which will be added to
// the previous CR, so we end up with the previous CR being set for
// this epoch.
_addCumulativeReward(poolId, 0, 1);
}

/// @dev Computes a member's reward over a given epoch interval.
/// @param poolId Uniqud Id of pool.
/// @param memberStakeOverInterval Stake delegated to pool by member over
Expand All @@ -100,13 +134,8 @@ contract MixinCumulativeRewards is
require(beginEpoch < endEpoch, "CR_INTERVAL_INVALID");

// Sanity check begin reward
(IStructs.Fraction memory beginReward, uint256 beginRewardStoredAt) = _getCumulativeRewardAtEpoch(poolId, beginEpoch);
(IStructs.Fraction memory endReward, uint256 endRewardStoredAt) = _getCumulativeRewardAtEpoch(poolId, endEpoch);

// If the rewards were stored at the same epoch then the computation will result in zero.
if (beginRewardStoredAt == endRewardStoredAt) {
return 0;
}
IStructs.Fraction memory beginReward = _getCumulativeRewardAtEpoch(poolId, beginEpoch);
IStructs.Fraction memory endReward = _getCumulativeRewardAtEpoch(poolId, endEpoch);

// Compute reward
reward = LibFractions.scaleDifference(
Expand All @@ -122,7 +151,7 @@ contract MixinCumulativeRewards is
/// @param poolId Unique ID of pool.
/// @return cumulativeReward The most recent cumulative reward `poolId`.
function _getMostRecentCumulativeReward(bytes32 poolId)
internal
private
view
returns (IStructs.Fraction memory cumulativeReward)
{
Expand All @@ -138,36 +167,33 @@ contract MixinCumulativeRewards is
/// @return cumulativeReward The cumulative reward for `poolId` at `epoch`.
/// @return cumulativeRewardStoredAt Epoch that the `cumulativeReward` is stored at.
function _getCumulativeRewardAtEpoch(bytes32 poolId, uint256 epoch)
internal
private
view
returns (
IStructs.Fraction memory cumulativeReward,
uint256 cumulativeRewardStoredAt
)
returns (IStructs.Fraction memory cumulativeReward)
{
// Return CR at `epoch`, given it's set.
cumulativeReward = _cumulativeRewardsByPool[poolId][epoch];
if (_isCumulativeRewardSet(cumulativeReward)) {
return (cumulativeReward, epoch);
return cumulativeReward;
}

// Return CR at `epoch-1`, given it's set.
uint256 lastEpoch = epoch.safeSub(1);
cumulativeReward = _cumulativeRewardsByPool[poolId][lastEpoch];
if (_isCumulativeRewardSet(cumulativeReward)) {
return (cumulativeReward, lastEpoch);
return cumulativeReward;
}

// Return the most recent CR, given it's less than `epoch`.
uint256 mostRecentEpoch = _cumulativeRewardsByPoolLastStored[poolId];
if (mostRecentEpoch < epoch) {
cumulativeReward = _cumulativeRewardsByPool[poolId][mostRecentEpoch];
if (_isCumulativeRewardSet(cumulativeReward)) {
return (cumulativeReward, mostRecentEpoch);
return cumulativeReward;
}
}

// Could not find a CR for `epoch`
revert("CR_INVALID_EPOCH");
// Otherwise return an empty CR.
return IStructs.Fraction(0, 1);
}
}
4 changes: 0 additions & 4 deletions contracts/staking/contracts/src/staking_pools/MixinStakingPool.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,6 @@ contract MixinStakingPool is
});
_poolById[poolId] = pool;

// initialize cumulative rewards for this pool;
// this is used to track rewards earned by delegators.
_initializeCumulativeRewards(poolId);

// Staking pool has been created
emit StakingPoolCreated(poolId, operator, operatorShare);

Expand Down
35 changes: 5 additions & 30 deletions contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ pragma solidity ^0.5.9;
pragma experimental ABIEncoderV2;

import "@0x/contracts-exchange-libs/contracts/src/LibMath.sol";
import "@0x/contracts-utils/contracts/src/LibFractions.sol";
import "@0x/contracts-utils/contracts/src/LibSafeMath.sol";
import "./MixinCumulativeRewards.sol";
import "../sys/MixinAbstract.sol";
Expand Down Expand Up @@ -138,11 +137,9 @@ contract MixinStakingPoolRewards is
getWethContract().transfer(member, balance);
}

// Add a cumulative reward entry for this epoch.
_forceSetCumulativeReward(
poolId,
_getMostRecentCumulativeReward(poolId)
);
// Ensure a cumulative reward entry exists for this epoch,
// copying the previous epoch's CR if one doesn't exist already.
_updateCumulativeReward(poolId);
}

/// @dev Handles a pool's reward at the current epoch.
Expand Down Expand Up @@ -181,30 +178,8 @@ contract MixinStakingPoolRewards is
if (membersReward > 0) {
// Increase the balance of the pool
_increasePoolRewards(poolId, membersReward);

// Fetch the last epoch at which we stored an entry for this pool;
// this is the most up-to-date cumulative rewards for this pool.
IStructs.Fraction memory mostRecentCumulativeReward = _getMostRecentCumulativeReward(poolId);

// Compute new cumulative reward
IStructs.Fraction memory cumulativeReward;
(cumulativeReward.numerator, cumulativeReward.denominator) = LibFractions.add(
mostRecentCumulativeReward.numerator,
mostRecentCumulativeReward.denominator,
membersReward,
membersStake
);
// Normalize to prevent overflows.
(cumulativeReward.numerator, cumulativeReward.denominator) = LibFractions.normalize(
cumulativeReward.numerator,
cumulativeReward.denominator
);

// Store cumulative rewards for this epoch.
_forceSetCumulativeReward(
poolId,
cumulativeReward
);
// Create a cumulative reward entry at the current epoch.
_addCumulativeReward(poolId, membersReward, membersStake);
}

return (operatorReward, membersReward);
Expand Down
16 changes: 11 additions & 5 deletions contracts/staking/contracts/test/TestCumulativeRewardTracking.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,22 @@ contract TestCumulativeRewardTracking is

function init() public {}

function _forceSetCumulativeReward(
function _addCumulativeReward(
bytes32 poolId,
IStructs.Fraction memory value
uint256 reward,
uint256 stake
)
internal
{
emit SetCumulativeReward(poolId, currentEpoch);
MixinCumulativeRewards._forceSetCumulativeReward(
uint256 lastStoredEpoch = _cumulativeRewardsByPoolLastStored[poolId];
MixinCumulativeRewards._addCumulativeReward(
poolId,
value
reward,
stake
);
uint256 newLastStoredEpoch = _cumulativeRewardsByPoolLastStored[poolId];
if (newLastStoredEpoch != lastStoredEpoch) {
emit SetCumulativeReward(poolId, currentEpoch);
}
}
}
18 changes: 0 additions & 18 deletions contracts/staking/contracts/test/TestDelegatorRewards.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ contract TestDelegatorRewards is
// Lazily initialize this pool.
_poolById[poolId].operator = operatorAddress;
_setOperatorShare(poolId, operatorReward, membersReward);
_initGenesisCumulativeRewards(poolId);
}

/// @dev Expose/wrap `_syncPoolRewards`.
Expand All @@ -87,7 +86,6 @@ contract TestDelegatorRewards is
// Lazily initialize this pool.
_poolById[poolId].operator = operatorAddress;
_setOperatorShare(poolId, operatorReward, membersReward);
_initGenesisCumulativeRewards(poolId);

_syncPoolRewards(
poolId,
Expand All @@ -111,7 +109,6 @@ contract TestDelegatorRewards is
)
external
{
_initGenesisCumulativeRewards(poolId);
_withdrawAndSyncDelegatorRewards(
poolId,
delegator
Expand All @@ -136,7 +133,6 @@ contract TestDelegatorRewards is
)
external
{
_initGenesisCumulativeRewards(poolId);
_withdrawAndSyncDelegatorRewards(
poolId,
delegator
Expand All @@ -159,7 +155,6 @@ contract TestDelegatorRewards is
)
external
{
_initGenesisCumulativeRewards(poolId);
_withdrawAndSyncDelegatorRewards(
poolId,
delegator
Expand Down Expand Up @@ -204,19 +199,6 @@ contract TestDelegatorRewards is
membersStake = reward.membersStake;
}

/// @dev Create a cumulative rewards entry for a pool if one doesn't
/// already exist to get around having to create pools in advance.
function _initGenesisCumulativeRewards(bytes32 poolId)
private
{
uint256 lastRewardEpoch = _cumulativeRewardsByPoolLastStored[poolId];
IStructs.Fraction memory cumulativeReward =
_cumulativeRewardsByPool[poolId][lastRewardEpoch];
if (!_isCumulativeRewardSet(cumulativeReward)) {
_initializeCumulativeRewards(poolId);
}
}

/// @dev Set the operator share of a pool based on reward ratios.
function _setOperatorShare(
bytes32 poolId,
Expand Down
16 changes: 6 additions & 10 deletions contracts/staking/test/cumulative_reward_tracking_test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,10 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {

describe('Tracking Cumulative Rewards (CR)', () => {
it('pool created at epoch 0', async () => {
await simulation.runTestAsync([], [TestAction.CreatePool], [{ event: 'SetCumulativeReward', epoch: 0 }]);
await simulation.runTestAsync([], [TestAction.CreatePool], []);
});
it('pool created in epoch >0', async () => {
await simulation.runTestAsync(
[TestAction.Finalize],
[TestAction.CreatePool],
[{ event: 'SetCumulativeReward', epoch: 1 }],
);
await simulation.runTestAsync([TestAction.Finalize], [TestAction.CreatePool], []);
});
it('delegating in the same epoch pool is created', async () => {
await simulation.runTestAsync(
Expand All @@ -53,7 +49,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
// Creates CR for epoch 1
TestAction.Delegate,
],
[{ event: 'SetCumulativeReward', epoch: 0 }],
[],
);
});
it('re-delegating in the same epoch', async () => {
Expand All @@ -68,7 +64,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
// Updates CR for epoch 0
TestAction.Delegate,
],
[{ event: 'SetCumulativeReward', epoch: 0 }, { event: 'SetCumulativeReward', epoch: 0 }],
[],
);
});
it('delegating in new epoch', async () => {
Expand Down Expand Up @@ -268,7 +264,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
// Clears CR for epoch 2
TestAction.Delegate,
],
[{ event: 'SetCumulativeReward', epoch: 3 }],
[],
);
});
it('delegate in epoch 0 and 1, earn reward in epoch 3, then undelegate half', async () => {
Expand Down Expand Up @@ -303,7 +299,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => {
// Clears CR for epoch 2
TestAction.Undelegate,
],
[{ event: 'SetCumulativeReward', epoch: 3 }],
[],
);
});
it('delegate in epoch 1, 2, earn rewards in epoch 3, skip to epoch 4, then delegate', async () => {
Expand Down
Loading

0 comments on commit 5128295

Please sign in to comment.