Sync Rewards + Refactored Reward Vault #2156

hysz · 2019-09-14T07:10:21Z

Description

Reward Vault now only stores ETH for the delegators. The operator's reward goes directly to the ETH Vault. The reward vault has been stripped down and made much more lean. All pool logic has been moved to the staking contract.

Testing instructions

Types of changes

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.

hysz · 2019-09-17T02:42:01Z

contracts/staking/contracts/src/staking_pools/MixinStakingPoolMakers.sol

@@ -0,0 +1,217 @@
+/*


This was all existing code that was refactored out of MixinStakingPool.sol

hysz · 2019-09-17T02:42:43Z

contracts/staking/contracts/src/staking_pools/MixinStakingPoolModifiers.sol

@@ -0,0 +1,81 @@
+/*


The code in this file already existed but was refactored into its own Mixin.

hysz · 2019-09-17T02:44:47Z

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

        bytes32 poolId,
        uint256 reward,
        uint256 amountOfDelegatedStake,
        uint256 epoch
    )
        internal
    {
+
+        IStructs.Pool memory pool = poolById[poolId];


This logic was copied from the Staking Pool Vault.

coveralls · 2019-09-17T05:25:57Z

Coverage remained the same at 75.804% when pulling 2869dd3 on feature/staking/syncingRewards into b631fc6 on 3.0.

dorothy-zbornak

This is lovely! 🥂

dorothy-zbornak · 2019-09-17T18:00:47Z

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

@@ -237,7 +237,7 @@ contract MixinExchangeFees is

            // compute weighted stake
            uint256 totalStakeDelegatedToPool = getTotalStakeDelegatedToPool(poolId).currentEpochBalance;
-            uint256 stakeHeldByPoolOperator = getStakeDelegatedToPoolByOwner(rewardVault.operatorOf(poolId), poolId).currentEpochBalance;
+            uint256 stakeHeldByPoolOperator = getStakeDelegatedToPoolByOwner(poolById[poolId].operator, poolId).currentEpochBalance;


dorothy-zbornak · 2019-09-17T18:22:44Z

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

    /// @param poolId Unique Id of pool.
    /// @param amount Amount in ETH to transfer.
-    function transferMemberBalanceToEthVault(
+    function transferToEthVault(
        bytes32 poolId,


What do we think about just passing in the ethVault address here? Then we wouldn't have to set up _ethVault, etc.

I like it a lot. Then they can't get out of sync either.

moodlezoup

looks good to me! just a couple nits and then 🚀

moodlezoup · 2019-09-17T19:01:36Z

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

@@ -95,4 +95,16 @@ interface IStructs {
        uint256 cumulativeRewardEpoch;
        IStructs.Fraction cumulativeReward;
    }
+
+    /// @dev Holds the balances and other data for a staking pool.


Suggested change

/// @dev Holds the balances and other data for a staking pool.

/// @dev Holds metadata for a staking pool.

moodlezoup · 2019-09-17T19:32:15Z

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

-        // Maker has joined to the pool, awaiting operator confirmation
-        emit PendingAddMakerToPool(
+        // load pool and assert that we can decrease
+        IStructs.Pool memory pool = poolById[poolId];


nit: we only need poolById[poolId].operatorShare

abandeali1 · 2019-09-17T19:10:03Z

contracts/staking/contracts/src/staking_pools/MixinEthVault.sol

+    /// @param poolId Unique Id of pool to transfer reward for,
+    /// @param operator of the pool.
+    /// @param amount of ETH to transfer.
+    function _transferOperatorRewardToEthVault(


This entire contract was deleted in #2158 (it was not being used at all). We also no longer need to check if the vault addresses are null after #2166. I'd vote for keeping this contract deleted and moving this logic to wherever it is being used.

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

abandeali1 · 2019-09-17T19:32:33Z

contracts/staking/contracts/src/staking_pools/MixinStakingPoolMakers.sol

+    Ownable,
+    MixinStorage,
+    MixinZrxVault,
+    MixinStakingPoolRewardVault,


Going forward let's make sure that there is no redundant inheritance in new contracts. This has been cleaned up in #2158 .

These will get added back every time we run the linearize script. I'll remove them here but we'll probably have to do this again after MBF is merged.

abandeali1 · 2019-09-17T19:33:00Z

contracts/staking/contracts/src/staking_pools/MixinStakingPoolModifiers.sol

+    MixinConstants,
+    Ownable,
+    MixinStorage,
+    MixinZrxVault,


Let's cleanup these inherited contracts as well.

abandeali1 · 2019-09-17T20:05:33Z

contracts/staking/contracts/src/interfaces/IStakingEvents.sol

+    /// @param oldOperatorShare Previous share of rewards owned by operator.
+    /// @param newOperatorShare Newly decreased share of rewards owned by operator.
+    event OperatorShareDecreased(
+        bytes32 poolId,


Let's index poolId in any events where it is used.

abandeali1 · 2019-09-17T20:15:30Z

contracts/staking/contracts/src/staking_pools/MixinStakingPoolModifiers.sol

+    /// @dev Asserts that the sender is the operator of the input pool.
+    /// @param poolId Pool sender must be operator of.
+    modifier onlyStakingPoolOperator(bytes32 poolId) {
+        address poolOperator = poolById[poolId].operator;


Looks like we use poolOperator, operator, and operatorAddress in various places throughout these contracts.

Note: we chatted offline. Dropped the Address suffix and updated instances of operatorAddress and poolOperator to operator.

abandeali1 · 2019-09-17T20:19:15Z

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

+    /// @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.
+    function syncDelegatorRewards(bytes32 poolId)


Under what circumstances is this called? Who is expected to call it? It feels strange to have an external function callable by anyone that modifies state.

This is used by a delegator when they want to sync their rewards without delegating/undelegating. It's effectively the same as delegating zero stake. I've updated the description.

Note also that this uses msg.sender and can't force a sync for an arbitrary delegator.

abandeali1 · 2019-09-17T20:20:42Z

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


    // mapping from poolId to Pool metadata
-    mapping (bytes32 => Pool) internal poolById;
+    mapping (bytes32 => uint256) internal balanceByPoolId;


Nit: prefix with _

abandeali1 · 2019-09-17T20:24:36Z

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

    /// @param poolId Unique Id of pool.
    /// @param amount Amount in ETH to transfer.
-    function transferMemberBalanceToEthVault(
+    function transferToEthVault(
        bytes32 poolId,


I like it a lot. Then they can't get out of sync either.

abandeali1 · 2019-09-17T20:27:31Z

contracts/staking/contracts/src/interfaces/IStakingEvents.sol

+    /// @param amount The amount in ETH withdrawn.
+    /// @param operator of the pool.
+    /// @param poolId The pool the reward was deposited for.
+    event OperatorRewardTransferredToEthVault(


Do we need this event? We already log all of this information on any deposit into the EthVault.

Yeah, I see what you mean. Basically, the goal was to document the flow of value as it enters and leaves a contract. That said, we can probably nix now that the vaults are set during initialization (and they also emit both ingress/egress value flow).

abandeali1

A few lingering nits, then looks good to go! Nice job on this one, it is much cleaner than before.

abandeali1 · 2019-09-18T01:31:10Z

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

-    function getStakingPoolIdOfMaker(address makerAddress)
+    /// @dev Returns the unique id that will be assigned to the next pool that is created.
+    /// @return Pool id.
+    function getNextStakingPoolId()


This function can be removed since nextPoolId is now public (reminder to remove from any interfaces as well).

abandeali1 · 2019-09-18T01:41:42Z

contracts/staking/contracts/src/staking_pools/MixinStakingPoolMakers.sol

+    /// @dev Returns the current number of makers in a given pool.
+    /// @param poolId Unique id of pool.
+    /// @return Size of pool.
+    function getNumberOfMakersInStakingPool(bytes32 poolId)


I'm kind of meh on having all of these getters. The rest of them are actually pretty frequently used so I'm fine leaving them in, but we should remove this one. When we're using getters for individual properties, it is easy to incur extra costs from sloads (example below).

abandeali1 · 2019-09-18T01:42:32Z

contracts/staking/contracts/src/staking_pools/MixinStakingPoolMakers.sol

+        }
+
+        // Is the pool already full?
+        if (getNumberOfMakersInStakingPool(poolId) == maximumMakersInPool) {


We already reference poolById[poolId] above, so this call is using an extra sload. We can load the pool in memory and reference that instead.

abandeali1 · 2019-09-18T01:45:40Z

contracts/staking/contracts/src/staking_pools/MixinStakingPoolModifiers.sol

+        address operator;
+        if (
+            msg.sender != makerAddress &&
+            msg.sender != (operator = poolById[poolId].operator)


This pattern seems a bit strange to me. Why not just set operator when we declare it above?

I like this actually. Short-circuits the sload if the first condition fails. I think in practice makers will be passive accounts so we'll pretty much always hit the second condition, though, so maybe it's better to prioritize readability here.

This code isn't going to run very often, and I agree this will almost always be called by the operator. I think readability is more important here, although I wouldn't be opposed to using this pattern in code that gets hit frequently.

hysz requested a review from abandeali1 as a code owner September 14, 2019 07:10

hysz assigned abandeali1 and dorothy-zbornak Sep 16, 2019

hysz force-pushed the feature/staking/syncingRewards branch from 04a253e to 58be544 Compare September 17, 2019 02:25

hysz changed the base branch from feature/staking/refCountRewards to 3.0 September 17, 2019 02:26

hysz assigned moodlezoup and unassigned abandeali1 Sep 17, 2019

hysz commented Sep 17, 2019

View reviewed changes

hysz force-pushed the feature/staking/syncingRewards branch 2 times, most recently from a8fbfeb to b89c595 Compare September 17, 2019 04:18

hysz changed the title ~~[WIP] Sync Rewards + Refactored Reward Vault~~ Sync Rewards + Refactored Reward Vault Sep 17, 2019

hysz force-pushed the feature/staking/syncingRewards branch from b89c595 to ef2331b Compare September 17, 2019 05:02

dorothy-zbornak approved these changes Sep 17, 2019

View reviewed changes

moodlezoup approved these changes Sep 17, 2019

View reviewed changes

abandeali1 suggested changes Sep 17, 2019

View reviewed changes

hysz force-pushed the feature/staking/syncingRewards branch from ef2331b to 019df1e Compare September 18, 2019 00:26

hysz added 4 commits September 17, 2019 17:36

refactored the staking vault

be1a70c

updated changelog

db97fe8

Rebased and addressed PR comments

768387f

Removed MixinStakingPoolRewards

877abed

hysz force-pushed the feature/staking/syncingRewards branch 2 times, most recently from aa6eef4 to b54e499 Compare September 18, 2019 00:47

Updated changelog and ran linter

5a22579

hysz force-pushed the feature/staking/syncingRewards branch from b54e499 to 5a22579 Compare September 18, 2019 00:51

abandeali1 approved these changes Sep 18, 2019

View reviewed changes

Removed unnecessary sloads in MixinStakingPoolMakers

2869dd3

hysz merged commit 549697d into 3.0 Sep 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync Rewards + Refactored Reward Vault #2156

Sync Rewards + Refactored Reward Vault #2156

hysz commented Sep 14, 2019 •

edited

Loading

hysz Sep 17, 2019

hysz Sep 17, 2019

hysz Sep 17, 2019

coveralls commented Sep 17, 2019 •

edited

Loading

dorothy-zbornak left a comment

dorothy-zbornak Sep 17, 2019

dorothy-zbornak Sep 17, 2019

abandeali1 Sep 17, 2019

moodlezoup left a comment

moodlezoup Sep 17, 2019

moodlezoup Sep 17, 2019

abandeali1 Sep 17, 2019

hysz Sep 17, 2019

abandeali1 Sep 17, 2019

hysz Sep 17, 2019

abandeali1 Sep 17, 2019

abandeali1 Sep 17, 2019

abandeali1 Sep 17, 2019

hysz Sep 18, 2019

abandeali1 Sep 17, 2019

hysz Sep 17, 2019 •

edited

Loading

abandeali1 Sep 17, 2019

abandeali1 Sep 17, 2019

abandeali1 Sep 17, 2019

hysz Sep 17, 2019

hysz Sep 18, 2019

abandeali1 left a comment

abandeali1 Sep 18, 2019

abandeali1 Sep 18, 2019

abandeali1 Sep 18, 2019

abandeali1 Sep 18, 2019

dorothy-zbornak Sep 18, 2019

hysz Sep 18, 2019

	/// @dev Holds the balances and other data for a staking pool.
	/// @dev Holds metadata for a staking pool.

Sync Rewards + Refactored Reward Vault #2156

Sync Rewards + Refactored Reward Vault #2156

Conversation

hysz commented Sep 14, 2019 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 17, 2019 • edited Loading

dorothy-zbornak 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

moodlezoup 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

hysz Sep 17, 2019 • edited Loading

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

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

hysz commented Sep 14, 2019 •

edited

Loading

coveralls commented Sep 17, 2019 •

edited

Loading

hysz Sep 17, 2019 •

edited

Loading