Tests for new staking mechanics #2126

hysz · 2019-09-03T21:48:59Z

Description

These are the tests for #2118. I put them in this PR for readability.

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.

jalextowle

Nice job on this one! The tests are beautiful! A few nits and comments, and then it seems g2g

jalextowle · 2019-09-03T23:27:24Z

contracts/staking/contracts/test/LibFeeMathTest.sol

@@ -21,8 +21,6 @@
 pragma solidity ^0.5.5;


Nit: Update to 0.5.9.

jalextowle · 2019-09-03T23:30:15Z

contracts/staking/test/utils/Simulation.ts

@@ -1,9 +1,11 @@
+/*


Will this be updated in this PR, or another one?

The simulations will be the last to get updated, after the implementation is finalized and the unit tests are done. Likely, next week.

jalextowle · 2019-09-03T23:30:29Z

contracts/staking/test/simulations_test.ts

@@ -1,3 +1,6 @@
+/*


Same as below

(see above)

jalextowle · 2019-09-03T23:37:00Z

contracts/staking/test/stake_test.ts

@@ -1,47 +1,357 @@
 import { ERC20ProxyContract, ERC20Wrapper } from '@0x/contracts-asset-proxy';


(File Wide Comment) These tests are amazingly readable, and after reading through the StakerActor, they are also rigorous 😄. The actor pattern is sweet!

jalextowle · 2019-09-03T23:42:28Z

contracts/staking/test/stake_test.ts

+        });
+    });
+    describe('Simulations', () => {
+        it('Simulation from Staking Spec', async () => {


It would be nice to have a simulation that is a bit shorter for debugging purposes. If this simulation finds a bug, it may be a bit non-trivial to figure out where the bug lives.

That's a good point. I worked to include all of the edge cases individually, in the tests above. I wanted to keep this just to have some semblance of a legitimate sequence of interactions.

Fair enough! Before writing this comment, I had the thought that the Simulations would likely not get hit in the case of a failure (because of an earlier failure). Up to you about whether to include a shorter sequence.

Out of scope: Given the complexity of the staking logic, I think that some potential bugs could be found in pathologically long sequences (if there are any). I think it would really behoove us to do some fuzzing using the created actors.

jalextowle · 2019-09-03T23:44:31Z

contracts/staking/test/actors/staker_actor.ts

        expect(balances.stakeBalanceInVault, 'stake balance, recorded in vault').to.be.bignumber.equal(
            expectedBalances.stakeBalanceInVault,
        );
        expect(balances.withdrawableStakeBalance, 'withdrawable stake balance').to.be.bignumber.equal(
            expectedBalances.withdrawableStakeBalance,
        );
-        expect(balances.activatableStakeBalance, 'activatable stake balance').to.be.bignumber.equal(
-            expectedBalances.activatableStakeBalance,
+        expect(balances.activeStakeBalance.current, 'active stake balance (current)').to.be.bignumber.equal(


+1 for the named expect. This is super nice to read, and I'm sure it's nice to debug.

jalextowle · 2019-09-03T23:46:21Z

contracts/staking/test/actors/staker_actor.ts

        expectedStakerBalances.stakeBalanceInVault = initStakerBalances.stakeBalanceInVault.plus(amount);
-        expectedStakerBalances.activatedStakeBalance = initStakerBalances.activatedStakeBalance.plus(amount);
+        expectedStakerBalances.activeStakeBalance.current = initStakerBalances.activeStakeBalance.current.plus(amount);
+        expectedStakerBalances.activeStakeBalance.next = initStakerBalances.activeStakeBalance.next.plus(amount);
        await this.assertBalancesAsync(expectedStakerBalances);
        // check zrx balance of vault
        const finalZrxBalanceOfVault = await this._stakingWrapper.getZrxTokenBalanceOfZrxVaultAsync();
        expect(finalZrxBalanceOfVault).to.be.bignumber.equal(initZrxBalanceOfVault.plus(amount));


Nit: Add descriptions to expects across the whole file for consistency and easier debugging. It may even be good to pass the calling functions name into assertBalancesAsync so that the error messages are even nicer when assertBalancesAsync is called by other functions in the actor.

jalextowle

Looks good to me! Nice work, as always 🍻

jalextowle · 2019-09-04T17:21:53Z

contracts/staking/test/actors/finalizer_actor.ts

+        const operatorShareByPoolId = await this._getOperatorShareByPoolIdAsync(this._poolIds);
+        const rewardVaultBalanceByPoolId = await this._getRewardVaultBalanceByPoolIdAsync(this._poolIds);
+        const memberBalancesByPoolId = await this._getMemberBalancesByPoolIdAsync(this._membersByPoolId);
+        // compute expecnted changes


Nit:

Suggested change

// compute expecnted changes

// compute expected changes

jalextowle · 2019-09-04T17:35:32Z

contracts/staking/test/rewards_test.ts

+                operatorRewardVaultBalance: rewardNotForDelegator,
+            });
+        });
+        it('Should stop collecting rewards after undelegating, after several epochs', async () => {


+1 for test coverage

dorothy-zbornak · 2019-09-04T18:28:30Z

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

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

        // sanity check - this is a gas optimization that can be used because we assume a non-zero
        // split between stake and fees generated in the cobb-douglas computation (see below).
-        if (totalFeesCollected == 0 || totalWeightedStake == 0) {


What was the purpose of this? Seems to just add more logic.

We may no longer need this with the new math. But these were denominators in the old cobb-douglas implementation, so this avoided any division by zero.

I mean, why remove the check?

Ah ~ if there's no stake in a given pool then the entire reward goes to the operator. Similarly, if no pools are staked then the reward goes entirely to operators. Note that it's still in a pool's best interest to stake since alpha != 0. So if all the pools are unstaked, then one pool could come along with relatively low volume but non-zero stake and take a good portion of the total reward.

dorothy-zbornak

Smash them together!

hysz requested a review from abandeali1 as a code owner September 3, 2019 21:48

hysz assigned jalextowle Sep 3, 2019

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed-Tests-Squashed branch from 653941d to 3363852 Compare September 3, 2019 22:01

hysz assigned dorothy-zbornak Sep 3, 2019

jalextowle suggested changes Sep 3, 2019

View reviewed changes

hysz mentioned this pull request Sep 4, 2019

New staking mechanics #2118

Merged

4 tasks

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed-Tests-Squashed branch from 3363852 to a6add55 Compare September 4, 2019 05:50

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

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed-Tests-Squashed branch from a6add55 to eca8607 Compare September 4, 2019 06:46

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

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed-Tests-Squashed branch from eca8607 to 313bb5b Compare September 4, 2019 07:21

jalextowle approved these changes Sep 4, 2019

View reviewed changes

dorothy-zbornak reviewed Sep 4, 2019

View reviewed changes

dorothy-zbornak approved these changes Sep 4, 2019

View reviewed changes

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed-Tests-Squashed branch 3 times, most recently from f84df45 to 5c9cebc Compare September 5, 2019 13:37

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

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed-Tests-Squashed branch from 5c9cebc to 07fce98 Compare September 5, 2019 19:12

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

hysz added 6 commits September 5, 2019 12:27

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

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed-Tests-Squashed branch from 07fce98 to 04f2665 Compare September 5, 2019 19:29

updated changelogs

49baafa

hysz force-pushed the feature/staking/NewMechanicsSolidityOnly-Squashed-Tests-Squashed branch from 04f2665 to 49baafa Compare September 5, 2019 19:38

hysz merged commit fc7f2e7 into feature/staking/NewMechanicsSolidityOnly-Squashed Sep 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for new staking mechanics #2126

Tests for new staking mechanics #2126

hysz commented Sep 3, 2019 •

edited

Loading

jalextowle left a comment

jalextowle Sep 3, 2019

jalextowle Sep 3, 2019

hysz Sep 4, 2019

jalextowle Sep 3, 2019

hysz Sep 4, 2019

jalextowle Sep 3, 2019

jalextowle Sep 3, 2019

hysz Sep 4, 2019

jalextowle Sep 4, 2019

jalextowle Sep 3, 2019

jalextowle Sep 3, 2019

jalextowle left a comment

jalextowle Sep 4, 2019

jalextowle Sep 4, 2019

dorothy-zbornak Sep 4, 2019

hysz Sep 4, 2019

dorothy-zbornak Sep 4, 2019

hysz Sep 5, 2019

dorothy-zbornak left a comment

		@@ -1,47 +1,357 @@
		import { ERC20ProxyContract, ERC20Wrapper } from '@0x/contracts-asset-proxy';

Tests for new staking mechanics #2126

Tests for new staking mechanics #2126

Conversation

hysz commented Sep 3, 2019 • edited Loading

Description

Testing instructions

Types of changes

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

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

dorothy-zbornak left a comment

Choose a reason for hiding this comment

hysz commented Sep 3, 2019 •

edited

Loading