New staking mechanics #2118

hysz · 2019-08-31T02:25:13Z

Description

This PR introduces a new mechanism for managing stake and tracking rewards. It is more flexible than the original implementation, allowing for a better user experience.

Note - Skip the Typescript in this review. There is a separate PR (#2126) that updates client-side code and adds tests. It will be merged into this PR before merging into 3.0.

Design Principles

Freshly minted stake is active.
Delegating, un-delegating and re-delegating stake comes into effect next epoch.
Users can freely adjust the distribution of their stake for the next epoch.
Stake can be withdrawn after it is inactive for one epoch.

Stake State Algorithm

There are three states that stake can exist in: Active, Inactive or Delegated. Each state has three fields:

How much stake is currently in this state (cur)
How much stake is in this state next epoch (next)
The last time this state was stored
1. This allows us to compute the correct values at any given epoch without user intervention

Inactive stake includes a Withdrawable field (W) that reflects how much stake can be withdrawn at any given time.

Example

Reward Tracking Mechanics

Reward Vault vs Eth Vault

The reward vault tracks the balance of each pool: how much belongs to the operator and how much is to be split between the delegators. ETH is deposited into this vault when an epoch is finalized.

The ETH Vault stores balances on a per-address basis. Value is moved from the reward vault to the ETH vault when a delegator changes their stake in a given pool.

Testing instructions

Checklist:

Prefix PR title with [WIP] if necessary.
Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

jalextowle

The logic seems solid for the most part. Really nice work on the redesigned logic, and all things considered, you got this out quickly!

I had some nits and comments, which I think should be addressed before merging this PR.

jalextowle · 2019-09-03T17:53:11Z

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

-            uint256 totalStakeDelegatedToPool = getTotalStakeDelegatedToPool(poolId);
-            uint256 stakeHeldByPoolOperator = getActivatedAndUndelegatedStake(getStakingPoolOperator(poolId));
+            uint256 totalStakeDelegatedToPool = getTotalStakeDelegatedToPool(poolId).current;
+            uint256 stakeHeldByPoolOperator = getActiveStake(getStakingPoolOperator(poolId)).current; // @TODO Update


What needs to be updated here? Can this "TODO" be removed?

jalextowle · 2019-09-03T18:02:59Z

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

@@ -218,9 +223,20 @@ contract MixinExchangeFees is
            );

            // record reward in vault
-            _recordDepositInStakingPoolRewardVault(activePools[i].poolId, reward);
+            bool rewardForOperatorOnly = activePools[i].delegatedStake == 0;


Is this used anywhere else? I like the fact that we're naming this boolean, but I think a comment in the call to recordDepositFor would be sufficient and it would save us from needing to use another stack slot. Alternatively, I think it could make sense to start making use of Solidity's block scoping features to make sure that the stack stays clean.

Ex.

uint256 poolPortion; { bool rewardForOperatorOnly = activePools[i].delegatedStake == 0; // This will get dropped after being used. (, poolPortion) = rewardVault.recordDepositFor(activePools[i].poolId, reward, rewardForOperatorOnly); }

The reason I call this out is that I think that reducing the number of stack variables in the top of a function's scope can be a boon to code maintenance since it reduces the likelihood that adding a stack variable will push the function over the stack limit.

Ah I actually thought that Solidity would stop managing stack variables once they were finished being used without needing an explicit scope ~ just verified this is not the case. I'm cool with inlining.

jalextowle · 2019-09-03T18:07:35Z

contracts/staking/contracts/src/immutable/MixinConstants.sol

@@ -42,4 +42,6 @@ contract MixinConstants is
    uint64 constant internal INITIAL_EPOCH = 0;

    uint64 constant internal INITIAL_TIMELOCK_PERIOD = INITIAL_EPOCH;
+
+    uint256 constant internal MIN_TOKEN_VALUE = 1000000000000000000; // 10**18


It turns out that 10 ** 18 will not be inlined by the Solidity compiler. I've verified this by debugging a transaction through the Remix debugger that uses this constant. With this in mind:

Nit: Use 10 ** 18 for readability.

jalextowle · 2019-09-03T18:09:38Z

contracts/staking/contracts/src/immutable/MixinStorage.sol


    // registered 0x Exchange contracts
    mapping (address => bool) internal validExchanges;

    // ZRX vault
    IZrxVault internal zrxVault;

+    // Rebate Vault


Nit: Can you update this comment? It isn't immediately clear to me what the difference between this "Rebate Vault" and the below "Rebate Vault", aside from the fact that I assume that this vault stores ETH.

jalextowle · 2019-09-03T18:14:53Z

contracts/staking/contracts/src/interfaces/IEthVault.sol

@@ -0,0 +1,71 @@
+/*
+
+  Copyright 2018 ZeroEx Intl.


Nit: Update to 2019 :)

jalextowle · 2019-09-03T21:17:43Z

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

    )
-        internal
+        private


I understand why this is being made private, but it may be wise to leave this as internal for the purposes of unit testing. Thoughts?

Yeah, I think you're right. For unit testing these will have to be made internal =\

jalextowle · 2019-09-03T21:20:02Z

contracts/staking/contracts/src/stake/MixinZrxVault.sol

+        internal
+    {
+        IZrxVault _zrxVault = zrxVault;
+        require(


Is there a reason not to use Rich Errors in this contract?

This function doesn't do much other than call _zrxVault.depositFrom(owner, amount). I'd be in favor of removing it completely.

jalextowle · 2019-09-03T21:20:41Z

contracts/staking/contracts/src/stake/MixinStakeStorage.sol

@@ -0,0 +1,196 @@
+/*
+
+  Copyright 2018 ZeroEx Intl.


Nit:

Suggested change

Copyright 2018 ZeroEx Intl.

Copyright 2019 ZeroEx Intl.

jalextowle · 2019-09-03T21:21:39Z

contracts/staking/contracts/src/stake/MixinStakeStorage.sol

+    /// @param balancePtrA first storage pointer.
+    /// @param balancePtrB second storage pointer.
+    /// @return true iff pointers are equal.
+    function _arePointersEqual(


jalextowle · 2019-09-03T21:24:58Z

contracts/staking/contracts/src/stake/MixinStakeStorage.sol

+        IStructs.DelayedBalance storage balancePtr,
+        IStructs.DelayedBalance memory balance
+    )
+        private


Nit: Same as above re testing

(see above)

abandeali1

I'm probably about 1/4 - 1/3 through this review but won't be able to get to the rest tonight. Will add the rest tomorrow morning!

abandeali1 · 2019-09-04T03:18:22Z

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

@@ -218,9 +223,20 @@ contract MixinExchangeFees is
            );

            // record reward in vault
-            _recordDepositInStakingPoolRewardVault(activePools[i].poolId, reward);
+            bool rewardForOperatorOnly = activePools[i].delegatedStake == 0;
+            (, uint256 poolPortion) = rewardVault.recordDepositFor(activePools[i].poolId, reward, rewardForOperatorOnly);


This is a pretty long line. We can break it down into multiple lines, which would also make it easier to inline the rewardForOperatorOnly calculation with a comment to the right.

abandeali1 · 2019-09-04T03:22:36Z

contracts/staking/contracts/src/immutable/MixinStorage.sol

    // fees collected this epoch
    mapping (bytes32 => uint256) internal protocolFeesThisEpochByPool;

    // pools that were active in the current epoch
    bytes32[] internal activePoolsThisEpoch;

-    // mapping from POol Id to Shadow Rewards
-    mapping (bytes32 => uint256) internal shadowRewardsByPoolId;
+    // reward ratios by epoch


Nit: spell out mapping from Pool Id to Epoch to Reward Ratio

abandeali1 · 2019-09-04T03:25:32Z

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



 /// @dev This mixin contains logic for managing ZRX tokens and Stake.
-/// Stake is minted when ZRX is deposited and burned when ZRX is withdrawn.


The diagrams will live in the spec ideally.

abandeali1 · 2019-09-04T03:27:47Z

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

+        uint256 currentWithdrawableStake = getWithdrawableStake(owner);
+        require(
+            amount <= currentWithdrawableStake,
+            "INSUFFICIENT_FUNDS"


Rich revert?

abandeali1 · 2019-09-04T03:56:07Z

contracts/staking/contracts/test/TestStorageLayout.sol

@@ -25,6 +25,9 @@ import "../src/interfaces/IStructs.sol";


 contract TestStorageLayout is
+    MixinDeploymentConstants,
+    Ownable,
+    MixinConstants,


These are already inherited by MixinStorage (I suspect you are using a script to generate these).

Yeah, to remove any ambiguities during linearization it will print out the full ancestral tree. Ugly, but it works. I'll create a task to remove frivolous imports for readability to see if we can make this a bit prettier. In the meantime I'll cleanup the ones in this PR.

FWIW this is an example demonstrating why explicitly inheriting the ancestral tree works:

Ex: contract A {} contract B {} contract C {} contract D is A, B {} Suppose now that there is a contract E that will inherit from B{} and D{} The following follows "most base-like" to "most-derived": contract E is B, D {} However, it unfolds to: B{}, A{}, B{}, D{}, E{} - which is wrong because B{} appears both before and after A{}. The correct contract inheritance for E{} should be: contract E is A{}, B{}, D{} So, even though contract E{} does not directly depend on A{} - it still must be included in the dependency list.

abandeali1 · 2019-09-04T04:00:20Z

contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol

+        uint256 newMembersBalance = uint256(balance.membersBalance).safeAdd(poolPortion);
+
+        // save new balances
+        balance.operatorBalance = LibSafeDowncast.downcastToUint96(newOperatorBalance);


We can use newOperatorBalance.downcastToUint96 here

abandeali1 · 2019-09-04T04:01:01Z

contracts/staking/contracts/src/vaults/EthVault.sol

+    using LibSafeMath for uint256;
+
+    // mapping from Owner to ETH balance
+    mapping (address => uint256) internal balances;


Nit: _balances

Note: we chatted offline. This will be done in a separate PR.

abandeali1 · 2019-09-04T04:03:17Z

contracts/staking/contracts/src/vaults/EthVault.sol

+
+/// @dev This vault manages ETH.
+contract EthVault is
+    Authorizable,


All of these are unnecessary exccept IEthVault and MixinVaultCore

abandeali1 · 2019-09-04T04:04:29Z

contracts/staking/contracts/src/vaults/EthVault.sol

+    /// Note that only the Staking contract can call this.
+    /// Note that this can only be called when *not* in Catostrophic Failure mode.
+    /// @param amount of ETH to withdraw.
+    function withdraw(uint256 amount)


A lot of these functions are exactly the same as in the ZrxVault. Maybe the repeat functions should live in MixinVaultCore?

abandeali1 · 2019-09-04T04:05:02Z

contracts/staking/contracts/src/sys/MixinScheduler.sol

@@ -36,6 +36,7 @@ import "../interfaces/IStakingEvents.sol";
 contract MixinScheduler is
    IStakingEvents,
    MixinDeploymentConstants,
+    Ownable,


This can be removed.

abandeali1 · 2019-09-04T15:44:39Z

contracts/staking/contracts/src/libs/LibStakingRichErrors.sol

@@ -443,4 +455,35 @@ library LibStakingRichErrors {
            poolId
        );
    }
+
+    function EthVaultNotSet()


All of these rich reverts should end with Error.

abandeali1 · 2019-09-04T15:58:53Z

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

+    /// @dev Stake ZRX tokens. Tokens are deposited into the ZRX Vault. Unstake to retrieve the ZRX.
+    ///      Stake is in the 'Active' state.
+    /// @param amount of ZRX to stake.
+    function stake(uint256 amount)


Didn't we want newly minted stake to only become active at the next epoch as well? Otherwise one can stake before the very end of an epoch and dilute rewards for everyone else.

Note: we chatted about this offline, but basically this won't dilute rewards because the stake isn't delegated.

abandeali1 · 2019-09-04T16:00:35Z

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

        external
    {
-        _mintStake(msg.sender, amount);
+        address payable owner = msg.sender;


This probably happens elsewhere too if I had to guess. We might want to do a search for owner usage throughout the staking contracts.

abandeali1 · 2019-09-04T16:08:47Z

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

+    }
+
+    /// @dev States that stake can exist in.
+    enum StakeState {


I personally like Status > State here. I feel like State implies storage to some degree (also consistent with other enums).

Hmm yeah I like that better too. Although a "status machine" doesn't quite have the same ring as a "state machine" 🌝

abandeali1 · 2019-09-04T16:09:23Z

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

+    /// @dev Info used to describe a state.
+    /// @param state of the stake.
+    /// @param poolId Unique Id of pool. This is set when state=DELEGATED.
+    struct StakeStateInfo {


This could probably just be StakeInfo

abandeali1 · 2019-09-04T19:58:31Z

contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol

+    {
+        // sanity check on eth vault
+        IEthVault _ethVault = ethVault;
+        if (address(_ethVault) == address(0)) {


Hoping we can remove this check too if _ethVault is set in the constructor.

abandeali1 · 2019-09-04T20:01:08Z

contracts/utils/contracts/src/LibSafeMath.sol

@@ -6,6 +6,8 @@ import "./LibSafeMathRichErrors.sol";

 library LibSafeMath {

+    using LibSafeMath for uint256;


I don't think this is used anywhere.

abandeali1 · 2019-09-04T20:22:48Z

contracts/staking/contracts/test/LibFeeMathTest.sol

@@ -2,7 +2,7 @@

 /*

-  Copyright 2018 ZeroEx Intl.


Do we still need this file at all?

It's removed by #2109

abandeali1 · 2019-09-04T20:24:38Z

contracts/staking/contracts/src/immutable/MixinStorage.sol

-    // mapping from Owner to Amount Staked
-    mapping (address => uint256) internal stakeByOwner;
+    // mapping from Owner to Amount of Active Stake
+    mapping (address => IStructs.DelayedBalance) internal activeStakeByOwner;


Nit: all of these variables should be prefixed with _. Probably cleaner to do this in another PR.

Agreed, I've created a task for this.

abandeali1 · 2019-09-04T20:28:36Z

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

+        // value is always zero in these two scenarios:
+        //   1. The current epoch is zero: delegation begins at epoch 1
+        //   2. The owner's delegated is current as of this epoch: their rewards have been moved to the ETH vault.
+        if (currentEpoch == 0 || delegatedStake.lastStored == currentEpoch) return 0;


These checks should probably be reversed, since we don't need currentEpoch == 0 to be checked after epoch 0.

Very good eye! I agree!

packages/order-utils/src/staking_revert_errors.ts

jalextowle · 2019-09-05T04:44:13Z

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

+            (, uint256 poolPortion) = rewardVault.recordDepositFor(
+                activePools[i].poolId,
+                reward,
+                activePools[i].delegatedStake == 0 // true -> reward is for operator only


Nice, this comment makes up for the variable IMHO 👍

jalextowle · 2019-09-05T04:56:51Z

contracts/staking/contracts/src/stake/MixinStakeStorage.sol

+    MixinStorage,
+    MixinScheduler
+{
+


Nit: I don't think this line should be here.

Interesting, yeah I think you're right. Seems to be 50/50 whether we have a newline here or not lol.

jalextowle · 2019-09-05T17:35:58Z

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

-                amount,
-                getActivatedStake(owner)
-            ));
+        // cache the current withdrawal amoiunt, which may change if we're moving out of the inactive status.


Suggested change

// cache the current withdrawal amoiunt, which may change if we're moving out of the inactive status.

// cache the current withdrawal amount, which may change if we're moving out of the inactive status.

jalextowle · 2019-09-05T17:42:14Z

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

+        address payable owner,
+        uint256 amount
+    )
+        private


A lot of these functions have been made private. We should make them internal for unit testing.

Note: we chatted about this offline. These will be set to internal on as needed, unless a better alternative arises in the meantime.

jalextowle · 2019-09-05T18:31:59Z

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

+        IStructs.Fraction memory mostRecentCumulativeRewards = cumulativeRewardsByPoolPtr[cumulativeRewardsLastStored];
+
+        // compute new cumulative reward
+        (uint256 numerator, uint256 denominator) = LibFractions.addFractions(


Would it make more sense for addFractions to return a fraction? It seems like we create a Fraction object a significant amount of the time after calling this.

Mm idk, since this is a general util library I'd prefer to not force a struct on future users. We actually only convert it to the Fraction struct in this fn for storage.

…olidityOnly-Squashed-Tests-Squashed Tests for new staking mechanics

coveralls · 2019-09-05T20:41:01Z

Coverage remained the same at 75.804% when pulling fc7f2e7 on feature/staking/NewMechanicsSolidityOnly-Squashed into 88e5635 on 3.0.

abandeali1

Nice, some awesome readability improvements in here!

jalextowle

Nice work bud! You definitely deserve a Chimay after this one 👍

Just a couple of comments, but they are not merge-blocking.

jalextowle · 2019-09-06T21:00:20Z

contracts/staking/contracts/src/vaults/EthVault.sol

+    {
+        // update balance
+        uint256 amount = msg.value;
+        balances[owner] = balances[owner].safeAdd(msg.value);


We might as well use amount here since msg.value was cached there, right? This said, CALLVALUE is typically actually cheaper than using a stack variable (it's a Gbase opcode), so I'd be in favor of just using msg.value for both uses and removing amount entirely, unless there is a readability concern.

jalextowle · 2019-09-06T21:01:35Z

contracts/staking/contracts/src/vaults/EthVault.sol

+        // update balance
+        // note that this call will revert if trying to withdraw more
+        // than the current balance
+        balances[owner] = balances[owner].safeSub(amount);


Nice reentrancy protection 👍

hysz requested a review from abandeali1 as a code owner August 31, 2019 02:25

hysz changed the title ~~New staking mechanics: delay on entry opposed to exit~~ New staking mechanics Aug 31, 2019

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed branch 2 times, most recently from bdd3d14 to d355356 Compare August 31, 2019 20:19

hysz assigned abandeali1 and jalextowle Sep 3, 2019

jalextowle suggested changes Sep 3, 2019

View reviewed changes

hysz mentioned this pull request Sep 3, 2019

Tests for new staking mechanics #2126

Merged

4 tasks

abandeali1 suggested changes Sep 4, 2019

View reviewed changes

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed branch from e6d4dbf to dbbcf66 Compare September 4, 2019 04:14

hysz requested a review from fabioberger as a code owner September 4, 2019 05:53

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed branch from 68c2ab6 to 20cabc1 Compare September 4, 2019 07:19

abandeali1 suggested changes Sep 4, 2019

View reviewed changes

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed branch 2 times, most recently from cd23a16 to b202a74 Compare September 5, 2019 13:37

jalextowle reviewed Sep 5, 2019

View reviewed changes

hysz added 5 commits September 5, 2019 11:45

New staking mechanics: delay on entry opposed to exit

eb6ad7d

Improving readability of staking

c0cb78b

Rich Reverts in new staking mechanics

7e5e224

Removed frivolous imports to improve readability

b5be162

Readability Improvements

da83f75

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed branch from b202a74 to da83f75 Compare September 5, 2019 19:12

hysz added 8 commits September 5, 2019 12:26

Some readability plus updated changelogs

d1b004f

Tests for new staking mechanics

293c428

Rich reverts in tests plus readability

4bc84cd

Stake State -> Status in tests

deceed3

current -> currentEpochBalance and next -> nextEpochBalance

5ac4c72

Got build to work after rebase

67079d9

Fixed linter

24e62fe

updated changelogs

49baafa

Merge pull request #2126 from 0xProject/feature/staking/NewMechanicsS…

fc7f2e7

…olidityOnly-Squashed-Tests-Squashed Tests for new staking mechanics

abandeali1 approved these changes Sep 5, 2019

View reviewed changes

jalextowle approved these changes Sep 6, 2019

View reviewed changes

hysz merged commit f477c0f into 3.0 Sep 6, 2019



		/// @dev This mixin contains logic for managing ZRX tokens and Stake.
		/// Stake is minted when ZRX is deposited and burned when ZRX is withdrawn.

		@@ -6,6 +6,8 @@ import "./LibSafeMathRichErrors.sol";

		library LibSafeMath {

		using LibSafeMath for uint256;

	// cache the current withdrawal amoiunt, which may change if we're moving out of the inactive status.
	// cache the current withdrawal amount, which may change if we're moving out of the inactive status.

New staking mechanics #2118

New staking mechanics #2118

Conversation

hysz commented Aug 31, 2019 • edited Loading

Description

Stake State Algorithm

Reward Tracking Mechanics

Reward Vault vs Eth Vault

Testing instructions

Checklist:

jalextowle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hysz Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

abandeali1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 5, 2019

abandeali1 left a comment

Choose a reason for hiding this comment

jalextowle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hysz commented Aug 31, 2019 •

edited

Loading

hysz Sep 4, 2019 •

edited

Loading