Delete `StakingPoolRewardVault` and `EthVault` #2186

abandeali1 · 2019-09-22T23:34:49Z

Description

Removes StakingPoolRewardVault. These balances are now tracked internally in the StakingProxy state.
Removes EthVault. Deposits to the EthVault have been completely replaced with WETH withdrawals directly to the user's account.
Renames a bunch of functions.
Removes storage layout tests (these assertions will be added back in the actual staking contracts in a separate PR).

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.

coveralls · 2019-09-22T23:54:27Z

Coverage remained the same at 75.804% when pulling abb2b46 on feat/3.0/delete-vaults into 2d77fce on 3.0.

hysz

This is fantastic! Dropped a few nits but then g2g!

hysz · 2019-09-23T22:58:29Z

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

+    /// @dev Returns the total balance of this contract, including WETH,
+    ///      minus any WETH that has been reserved for rewards.
+    /// @return totalBalance Total balance.
+    function getAvailableBalance()


Nit - could we disambiguate this from other balances (like, stake)?

I'll change this to getAvailableRewardsBalance 👍

hysz · 2019-09-23T22:59:19Z

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

@@ -84,6 +82,9 @@ contract MixinStorage is
    // mapping from Pool Id to Pool
    mapping (bytes32 => IStructs.Pool) internal _poolById;

+    // mapping from PoolId to balance of members
+    mapping (bytes32 => uint256) public balanceByPoolId;


Could we also disambiguate this from other balances? Is this the weth balance?

wethRewardsByPoolId?

hysz · 2019-09-23T23:00:34Z

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

@@ -151,6 +146,9 @@ contract MixinStorage is
    /// @dev State for unfinalized rewards.
    IStructs.UnfinalizedState public unfinalizedState;

+    /// @dev The WETH balance of this contract that is reserved for pool reward payouts.
+    uint256 _reservedWethBalance;


Nit - we may be able to communicate what this is reserved for in the name, like _wethReservedForPools or something.

hysz · 2019-09-23T23:00:47Z

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

@@ -1,70 +0,0 @@
-/*
-
-  Copyright 2019 ZeroEx Intl.


hysz · 2019-09-23T23:00:56Z

contracts/staking/contracts/src/interfaces/IStakingPoolRewardVault.sol

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


hysz · 2019-09-23T23:02:56Z

contracts/staking/test/storage_layout_test.ts

-
-import { artifacts, TestStorageLayoutContract } from '../src';
-
-blockchainTests.resets('Storage layout tests', env => {


Is this superseded by a similar test?

Note- spoke offline, this will be handled by #2186

hysz · 2019-09-23T23:11:05Z

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

-        // Transfer to EthVault.
-        ethVault.depositFor(member, balance);
+        // Decrement the balance of the pool
+        balanceByPoolId[poolId] = balanceByPoolId[poolId].safeSub(balance);


It looks like balanceByPoolId and reservedWethBalance must always move together.

A pattern we've used in similar scenarios is to encapsulate this behavior in a single function, then document in MixinStorage that these variables should be modified via the functions opposed to directly.

hysz

Looks great! https://merge.it (~‾▿‾)~

moodlezoup

nice rips, just a few nits and g2g!

moodlezoup · 2019-09-23T23:38:56Z

contracts/staking/test/params.ts

-            expect(actual[7]).to.eq(_params.ethVaultAddress);
-            expect(actual[8]).to.eq(_params.rewardVaultAddress);
-            expect(actual[9]).to.eq(_params.zrxVaultAddress);
+            expect(actual[7]).to.eq(_params.zrxVaultAddress);


While we're here, we should consider using this pattern so that we can use .deep.equal

moodlezoup · 2019-09-23T23:42:55Z

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

@@ -84,6 +82,9 @@ contract MixinStorage is
    // mapping from Pool Id to Pool
    mapping (bytes32 => IStructs.Pool) internal _poolById;

+    // mapping from PoolId to balance of members
+    mapping (bytes32 => uint256) public rewardsByPoolId;


any reason to not lump this into _poolById?

This would bump _poolById to 2 words, which means we would pay for an extra sload whenever we access it.

Turns out this isn't always the case. @hysz has some examples that show solidity will only load the fields that are used.

I guess I don't see a ton of benefit of doing this 🤷‍♂

moodlezoup · 2019-09-23T23:57:53Z

contracts/staking/test/actors/finalizer_actor.ts

@@ -159,21 +159,21 @@ export class FinalizerActor extends BaseActor {
        return delegatorBalancesByPoolId;
    }

-    private async _computeExpectedRewardVaultBalanceAsyncByPoolIdAsync(
+    private _computeExpectedRewardVaultBalanceAsyncByPoolId(


Suggested change

private _computeExpectedRewardVaultBalanceAsyncByPoolId(

private _computeExpectedRewardVaultBalanceByPoolId(

moodlezoup · 2019-09-23T23:59:02Z

contracts/staking/test/migration.ts

-                expect(params[7]).to.eq(ethVaultAddress);
-                expect(params[8]).to.eq(rewardVaultAddress);
-                expect(params[9]).to.eq(zrxVaultAddress);
+                expect(params[7]).to.eq(zrxVaultAddress);


While we're here, we should consider using this pattern so that we can use .deep.equal

same

I'm going to leave these for now because of some quirks with some params being numbers/BigNumbers (the event types won't match whatever wrapper we write), and we're not doing this too frequently.

I think @mzhu25 is on to something. I added it to the backlog.

dorothy-zbornak

REVIEIWING ANYWAYYY

dorothy-zbornak · 2019-09-24T01:03:09Z

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

+    /// @dev Returns the total balance of this contract, including WETH,
+    ///      minus any WETH that has been reserved for rewards.
+    /// @return totalBalance Total balance.
+    function getAvailableRewardsBalance()


In the interest of saving codesize, should we remove this function? It wouldn't be difficult to figure this out off-chain if we made _wethReservedForRewards public.

I like having getters for things that use multiple storage slots, since we can reduce the number of RPC calls we need to make.

Won't we have devutils for staking?

Alright you got me, removed it in #2190 😎

dorothy-zbornak · 2019-09-24T01:05:11Z

contracts/staking/contracts/src/immutable/MixinDeploymentConstants.sol

+    function _getWethContract()
+        internal
+        view
+        returns (IEtherToken wethContract)


dorothy-zbornak · 2019-09-24T01:09:23Z

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

@@ -84,6 +82,9 @@ contract MixinStorage is
    // mapping from Pool Id to Pool
    mapping (bytes32 => IStructs.Pool) internal _poolById;

+    // mapping from PoolId to balance of members
+    mapping (bytes32 => uint256) public rewardsByPoolId;


Turns out this isn't always the case. @hysz has some examples that show solidity will only load the fields that are used.

dorothy-zbornak · 2019-09-24T01:20:34Z

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

+    /// @dev Increments rewards for a pool.
+    /// @param poolId Unique id of pool.
+    /// @param amount Amount to increment rewards by.
+    function _incrementPoolRewards(bytes32 poolId, uint256 amount)


Nit: _increasePoolRewards

dorothy-zbornak · 2019-09-24T01:20:54Z

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

+    /// @dev Decrements rewards for a pool.
+    /// @param poolId Unique id of pool.
+    /// @param amount Amount to decrement rewards by.
+    function _decrementPoolRewards(bytes32 poolId, uint256 amount)


Nit: _decreasePoolRewards.

dorothy-zbornak · 2019-09-24T01:27:52Z

contracts/staking/contracts/test/TestStakingNoWETH.sol

+        view
+        returns (IEtherToken)
+    {
+        return IEtherToken(address(this));


dorothy-zbornak · 2019-09-24T01:37:28Z

contracts/staking/test/migration.ts

-                expect(params[7]).to.eq(ethVaultAddress);
-                expect(params[8]).to.eq(rewardVaultAddress);
-                expect(params[9]).to.eq(zrxVaultAddress);
+                expect(params[7]).to.eq(zrxVaultAddress);


I think @mzhu25 is on to something. I added it to the backlog.

abandeali1 force-pushed the feat/3.0/delete-vaults branch from 3ff1aee to b4cb9c8 Compare September 22, 2019 23:36

abandeali1 changed the title ~~Delete both vaults~~ Delete StakingPoolRewardVault and EthVault Sep 23, 2019

abandeali1 marked this pull request as ready for review September 23, 2019 21:45

abandeali1 requested review from fabioberger and hysz as code owners September 23, 2019 21:45

abandeali1 added 9 commits September 23, 2019 15:29

Delete vaults and fix contract build

639026e

Rename variables and functions

7de23c6

Fix all tests except for reward tests (skipped)

ef645e6

Fix incorrect return value in _syncPoolRewards

1710f13

Fix delegator reward unit tests

ee687a7

Rename overloaded function

156560a

Track WETH reserved for rewards

fd35249

Separate wrapping ETH and querying available WETH balance

3965d8f

Fix rewards tests

62663ed

abandeali1 force-pushed the feat/3.0/delete-vaults branch from 943d4eb to 62663ed Compare September 23, 2019 22:34

Fix tests

5ce9889

abandeali1 assigned moodlezoup and dorothy-zbornak Sep 23, 2019

abandeali1 changed the base branch from feat/staking/hurts-like-a-mbf to 3.0 September 23, 2019 23:04

hysz reviewed Sep 23, 2019

View reviewed changes

Rename functions and variables for clarity

e9f0f4a

hysz approved these changes Sep 23, 2019

View reviewed changes

moodlezoup approved these changes Sep 24, 2019

View reviewed changes

abandeali1 added 2 commits September 23, 2019 17:28

Delete unused rich reverts

5bbd57d

Remove vaults from variable names and comments

abb2b46

abandeali1 merged commit 2587cd3 into 3.0 Sep 24, 2019

dorothy-zbornak approved these changes Sep 24, 2019

View reviewed changes

abandeali1 mentioned this pull request Sep 24, 2019

Consolidate staking pool mixins #2190

Merged

4 tasks

mintcloud mentioned this pull request Sep 25, 2019

Initial table schema 0xProject/0x-event-pipeline#1

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete `StakingPoolRewardVault` and `EthVault` #2186

Delete `StakingPoolRewardVault` and `EthVault` #2186

abandeali1 commented Sep 22, 2019 •

edited

Loading

coveralls commented Sep 22, 2019 •

edited

Loading

hysz left a comment

hysz Sep 23, 2019

abandeali1 Sep 23, 2019

hysz Sep 23, 2019

abandeali1 Sep 23, 2019

hysz Sep 23, 2019

hysz Sep 23, 2019

hysz Sep 23, 2019

hysz Sep 23, 2019

hysz Sep 23, 2019

hysz Sep 23, 2019

hysz left a comment

moodlezoup left a comment

moodlezoup Sep 23, 2019 •

edited

Loading

moodlezoup Sep 23, 2019

abandeali1 Sep 24, 2019

dorothy-zbornak Sep 24, 2019

abandeali1 Sep 24, 2019

moodlezoup Sep 23, 2019

moodlezoup Sep 23, 2019 •

edited

Loading

abandeali1 Sep 24, 2019

dorothy-zbornak Sep 24, 2019

dorothy-zbornak left a comment

dorothy-zbornak Sep 24, 2019

abandeali1 Sep 24, 2019

dorothy-zbornak Sep 24, 2019

abandeali1 Sep 24, 2019

dorothy-zbornak Sep 24, 2019

dorothy-zbornak Sep 24, 2019

dorothy-zbornak Sep 24, 2019

dorothy-zbornak Sep 24, 2019

dorothy-zbornak Sep 24, 2019

dorothy-zbornak Sep 24, 2019


		import { artifacts, TestStorageLayoutContract } from '../src';

		blockchainTests.resets('Storage layout tests', env => {

	private _computeExpectedRewardVaultBalanceAsyncByPoolId(
	private _computeExpectedRewardVaultBalanceByPoolId(

Delete StakingPoolRewardVault and EthVault #2186

Delete StakingPoolRewardVault and EthVault #2186

Conversation

abandeali1 commented Sep 22, 2019 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

coveralls commented Sep 22, 2019 • edited Loading

hysz 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

hysz left a comment

Choose a reason for hiding this comment

moodlezoup left a comment

Choose a reason for hiding this comment

moodlezoup Sep 23, 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

moodlezoup Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

Delete `StakingPoolRewardVault` and `EthVault` #2186

Delete `StakingPoolRewardVault` and `EthVault` #2186

abandeali1 commented Sep 22, 2019 •

edited

Loading

coveralls commented Sep 22, 2019 •

edited

Loading

moodlezoup Sep 23, 2019 •

edited

Loading

moodlezoup Sep 23, 2019 •

edited

Loading