Consolidate staking pool mixins #2190

abandeali1 · 2019-09-23T23:10:30Z

Description

This PR:

Adds codesize tests to the staking contracts (currently disabled)
Consolidates MixinStakingPoolMakers, MixinStakingPoolModifiers, and MixinStakingPool into a single mixin. These contracts had a lot of overlap. The resulting contract is larger than I would like, but I think less confusing overall.
Consolidates the modifiers into a single onlyStakingPoolOperatorOrMaker. Registered makers now essentially have the same rights as an operator. This includes admitting new makers into the pool, removing makers from the pool, or decreasing the operator's share. I like how this new behavior incentivizes operators to only allow makers that they control into a pool.
Fixes a bug where anyone could decrease an operator's share.
Addresses minor comments from @dorothy-zbornak on Delete StakingPoolRewardVault and EthVault #2186

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-23T23:29:42Z

Coverage remained the same at 75.804% when pulling 7f51822 on feat/3.0/consolidate-pool-mixins into c29a221 on 3.0.

moodlezoup

Looks good! Left a few comments and nits.
One thing to note is that I feel like with this change we need to be extra careful about communicating the fact that pools are intended only for makers controlled by a single entity, otherwise an operator might accidentally add an external maker who subsequently decreases the operator share to 0

moodlezoup · 2019-09-24T17:37:41Z

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

            senderAddress,
-            operator,
-            makerAddress
+            operator


idk if it still makes sense to use the operator address as a parameter for this error anymore –– maybe just the poolId instead? since you can get the operator from the poolId if you want

I like that change!

moodlezoup · 2019-09-24T17:41:15Z

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

        }

        if (membersReward > 0) {
            // Increment the balance of the pool


nit: update these comments too

moodlezoup · 2019-09-24T17:43:25Z

contracts/staking/test/codesize.ts

+import * as chai from 'chai';
+
+chaiSetup.configure();
+const expect = chai.expect;


nit: import expect from test-utils instead

moodlezoup · 2019-09-24T17:44:12Z

contracts/staking/test/pools_test.ts

        accounts = await env.getAccountAddressesAsync();
        owner = accounts[0];
-        users = accounts.slice(1);
+        users = accounts.slice(2);


nit: [owner, ...users] = accounts; or even [owner, operator, ...users] = accounts;

moodlezoup · 2019-09-24T17:46:07Z

contracts/staking/test/utils/api_wrapper.ts

-            const ethBalance = await this._web3Wrapper.getBalanceInWeiAsync(address);
-            const wethBalance = await this.wethContract.balanceOf.callAsync(address);
-            return BigNumber.sum(ethBalance, wethBalance);
+        getAvailableRewardsBalanceAsync: async (): Promise<BigNumber> => {


moodlezoup · 2019-09-24T17:51:35Z

contracts/staking/test/actors/maker_actor.ts

+import { PoolOperatorActor } from './pool_operator_actor';

-export class MakerActor extends BaseActor {
+export class MakerActor extends PoolOperatorActor {


I feel like the inheritance should go the other way around here, since (1) technically there's nothing stopping an operator from joining a staking pool, and (2) createStakingPool is kinda what differentiates a pool operator from a vanilla maker

There's nothing stopping a maker from creating a pool either, right? Maybe We should just combine these into one actor?

I guess my thought was that a maker "becomes" an operator by creating a pool (though I suppose the same can be said about an operator joining a pool). I think it's helpful to keep them as separate actors regardless, for clarity in e.g. rewards_test if nothing else; open to other opinions tho

moodlezoup · 2019-09-24T17:57:36Z

contracts/staking/test/pools_test.ts

+                new StakingRevertErrors.OnlyCallableByPoolOperatorOrMakerError(makerAddress, operatorAddress),
+            );
+        });
+        it('should fail to add a maker to a pool if not called by operator/registered maker', async () => {


This test does the same thing as 'Should fail to add a maker when called by someone other than the pool operator', so we can delete the latter

moodlezoup · 2019-09-24T18:06:35Z