Rich Reverts in new staking mechanics

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

@@ -118,6 +118,18 @@ library LibStakingRichErrors {
     bytes4 internal constant POOL_ALREADY_EXISTS_ERROR_SELECTOR =
         0x2a5e4dcf;
 
+    // bytes4(keccak256("EthVaultNotSet()"))
+    bytes4 internal constant ETH_VAULT_NOT_SET =
+        0xdb3f0be8;
+
+    // bytes4(keccak256("RewardVaultNotSet()"))
+    bytes4 internal constant REWARD_VAULT_NOT_SET =
+        0xfcb260f7;
+
+    // bytes4(keccak256("InvalidStakeState(uint256)"))
+    bytes4 internal constant INVALID_STAKE_STATE =
+        0xe6586728;
+
     // solhint-disable func-name-mixedcase
     function MiscalculatedRewardsError(
         uint256 totalRewardsPaid,
@@ -443,4 +455,35 @@ library LibStakingRichErrors {
             poolId
         );
     }
+
+    function EthVaultNotSet()
+        internal
+        pure
+        returns (bytes memory)
+    {
+        return abi.encodeWithSelector(
+            ETH_VAULT_NOT_SET
+        );
+    }
+
+    function RewardVaultNotSet()
+        internal
+        pure
+        returns (bytes memory)
+    {
+        return abi.encodeWithSelector(
+            REWARD_VAULT_NOT_SET
+        );
+    }
+
+    function InvalidStakeState(uint256 state)
+        internal
+        pure
+        returns (bytes memory)
+    {
+        return abi.encodeWithSelector(
+            INVALID_STAKE_STATE,
+            state
+        );
+    }
 }

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

@@ -27,6 +27,7 @@ import "./MixinZrxVault.sol";
 import "../staking_pools/MixinStakingPoolRewardVault.sol";
 import "../staking_pools/MixinStakingPoolRewards.sol";
 import "../sys/MixinScheduler.sol";
+import "../libs/LibStakingRichErrors.sol";
 import "./MixinStakeBalances.sol";
 import "./MixinStakeStorage.sol";
 
@@ -78,10 +79,14 @@ contract MixinStake is
 
         // sanity check
         uint256 currentWithdrawableStake = getWithdrawableStake(owner);
-        require(
-            amount <= currentWithdrawableStake,
-            "INSUFFICIENT_FUNDS"
-        );
+        if (amount > currentWithdrawableStake) {
+            LibRichErrors.rrevert(
+                LibStakingRichErrors.InsufficientBalanceError(
+                    amount,
+                    currentWithdrawableStake
+                )
+            );
+        }
 
         // burn inactive stake
         _burnBalance(inactiveStakeByOwner[owner], amount);
@@ -219,6 +224,7 @@ contract MixinStake is
     /// @return a storage pointer to the corresponding stake stake
     function _getBalancePtrFromState(IStructs.StakeState state)
         private
+        view
         returns (IStructs.DelayedBalance storage)
     {
         // lookup state
@@ -231,7 +237,12 @@ contract MixinStake is
             return delegatedStakeByOwner[owner];
         }
 
-        // not found
-        revert("Unrecognized State");
+        // invalid state
+        LibRichErrors.rrevert(
+            LibStakingRichErrors.InvalidStakeState(uint256(state))
+        );
+
+        // required to compile ~ we should never hit this.
+        revert("INVALID_STATE");
     }
 }

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

@@ -63,7 +63,12 @@ contract MixinStakeStorage is
 
         // sanity check on balance
         if (amount > from.next) {
-            revert("Insufficient Balance");
+            LibRichErrors.rrevert(
+                LibStakingRichErrors.InsufficientBalanceError(
+                    amount,
+                    from.next
+                )
+            );
         }
 
         // move stake for next epoch
@@ -183,6 +188,7 @@ contract MixinStakeStorage is
         IStructs.DelayedBalance storage balancePtrB
     )
         private
+        pure
         returns (bool areEqual)
     {
         assembly {

contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewardVault.sol

@@ -21,6 +21,7 @@ pragma solidity ^0.5.9;
 import "../interfaces/IStakingEvents.sol";
 import "../interfaces/IStakingPoolRewardVault.sol";
 import "../immutable/MixinStorage.sol";
+import "../libs/LibStakingRichErrors.sol";
 
 
 /// @dev This mixin contains logic for interfacing with the Staking Pool Reward Vault (vaults/StakingPoolRewardVault.sol)
@@ -71,7 +72,15 @@ contract MixinStakingPoolRewardVault is
     function _depositIntoStakingPoolRewardVault(uint256 amount)
         internal
     {
+        // cast to payable and sanity check
         address payable rewardVaultAddress = address(uint160(address(rewardVault)));
+        if (rewardVaultAddress == NIL_ADDRESS) {
+             LibRichErrors.rrevert(
+                LibStakingRichErrors.RewardVaultNotSet()
+            );
+        }
+
+        // perform transfer
         rewardVaultAddress.transfer(amount);
     }
 
@@ -86,11 +95,15 @@ contract MixinStakingPoolRewardVault is
     )
         internal
     {
+        // sanity check
         IStakingPoolRewardVault _rewardVault = rewardVault;
-        require(
-            address(_rewardVault) != NIL_ADDRESS,
-            "REWARD_VAULT_NOT_SET"
-        );
+        if (address(_rewardVault) == NIL_ADDRESS) {
+             LibRichErrors.rrevert(
+                LibStakingRichErrors.RewardVaultNotSet()
+            );
+        }
+
+        // perform transfer
         _rewardVault.transferMemberBalanceToEthVault(poolId, member, amount);
     }
 }

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

@@ -232,6 +232,7 @@ contract MixinStakingPoolRewards is
     /// @dev returns true iff Cumulative Rewards are set
     function _isCumulativeRewardSet(IStructs.Fraction memory cumulativeReward)
         private
+        pure
         returns (bool)
     {
         // we use the denominator as a proxy for whether the cumulative

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

@@ -120,13 +120,6 @@ contract StakingPoolRewardVault is
             return;
         }
 
-        // sanity check on eth vault
-        IEthVault _ethVault = ethVault;
-        require(
-            address(_ethVault) != address(0),
-            "ETH_VAULT_NOT_SET"
-        );
-
         // sanity check - sufficient balance?
         uint256 operatorBalance = uint256(balanceByPoolId[poolId].operatorBalance);
         if (amount > operatorBalance) {
@@ -138,7 +131,7 @@ contract StakingPoolRewardVault is
 
         // update balance and transfer `amount` in ETH to staking contract
         balanceByPoolId[poolId].operatorBalance = operatorBalance.safeSub(amount).downcastToUint96();
-        _ethVault.depositFor.value(amount)(operator);
+        _transferToEthVault(operator, amount);
 
         // notify
         emit RewardWithdrawnForOperator(poolId, amount);
@@ -157,12 +150,9 @@ contract StakingPoolRewardVault is
         external
         onlyStakingContract
     {
-        // sanity check on eth vault
-        IEthVault _ethVault = ethVault;
-        require(
-            address(_ethVault) != address(0),
-            "ETH_VAULT_NOT_SET"
-        );
+        if (amount == 0) {
+            return;
+        }
 
         // sanity check - sufficient balance?
         uint256 membersBalance = uint256(balanceByPoolId[poolId].membersBalance);
@@ -175,7 +165,7 @@ contract StakingPoolRewardVault is
 
         // update balance and transfer `amount` in ETH to staking contract
         balanceByPoolId[poolId].membersBalance = membersBalance.safeSub(amount).downcastToUint96();
-        _ethVault.depositFor.value(amount)(member);
+        _transferToEthVault(member, amount);
 
         // notify
         emit RewardWithdrawnForMember(poolId, amount);
@@ -296,4 +286,19 @@ contract StakingPoolRewardVault is
             poolPortion
         );
     }
+
+    function _transferToEthVault(address from, uint256 amount)
+        private
+    {
+        // sanity check on eth vault
+        IEthVault _ethVault = ethVault;
+        if (address(_ethVault) == address(0)) {
+            LibRichErrors.rrevert(
+                LibStakingRichErrors.EthVaultNotSet()
+            );
+        }
+
+        // perform xfer
+        _ethVault.depositFor.value(amount)(from);
+    }
 }

contracts/staking/test/stake_test.ts

@@ -1,6 +1,7 @@
 import { ERC20ProxyContract, ERC20Wrapper } from '@0x/contracts-asset-proxy';
 import { DummyERC20TokenContract } from '@0x/contracts-erc20';
 import { blockchainTests, describe, provider, web3Wrapper } from '@0x/contracts-test-utils';
+import { StakingRevertErrors } from '@0x/order-utils';
 import { BigNumber, StringRevertError } from '@0x/utils';
 import * as _ from 'lodash';
 
@@ -94,7 +95,7 @@ blockchainTests.resets.only('Stake States', () => {
                 { state: StakeState.Active },
                 { state: StakeState.Inactive },
                 amount,
-                new StringRevertError('Insufficient Balance'),
+                new StakingRevertErrors.InsufficientBalanceError(amount, ZERO),
             );
         });
         it('should fail to reassign stake', async () => {
@@ -113,7 +114,7 @@ blockchainTests.resets.only('Stake States', () => {
                 { state: StakeState.Inactive },
                 { state: StakeState.Active },
                 amount,
-                new StringRevertError('Insufficient Balance'),
+                new StakingRevertErrors.InsufficientBalanceError(amount, ZERO),
             );
         });
     });
@@ -254,20 +255,20 @@ blockchainTests.resets.only('Stake States', () => {
         it('should fail to unstake with insufficient balance', async () => {
             const amount = StakingWrapper.toBaseUnitAmount(10);
             await staker.stakeAsync(amount);
-            await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS'));
+            await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO));
         });
         it('should fail to unstake in the same epoch as stake was set to inactive', async () => {
             const amount = StakingWrapper.toBaseUnitAmount(10);
             await staker.stakeAsync(amount);
             await staker.moveStakeAsync({ state: StakeState.Active }, { state: StakeState.Inactive }, amount);
-            await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS'));
+            await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO));
         });
         it('should fail to unstake after being inactive for <1 epoch', async () => {
             const amount = StakingWrapper.toBaseUnitAmount(10);
             await staker.stakeAsync(amount);
             await staker.moveStakeAsync({ state: StakeState.Active }, { state: StakeState.Inactive }, amount);
             await staker.goToNextEpochAsync();
-            await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS'));
+            await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO));
         });
         it('should fail to unstake in same epoch that inactive/withdrawable stake has been reactivated', async () => {
             const amount = StakingWrapper.toBaseUnitAmount(10);
@@ -276,7 +277,7 @@ blockchainTests.resets.only('Stake States', () => {
             await staker.goToNextEpochAsync(); // stake is now inactive
             await staker.goToNextEpochAsync(); // stake is now withdrawable
             await staker.moveStakeAsync({ state: StakeState.Inactive }, { state: StakeState.Active }, amount);
-            await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS'));
+            await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO));
         });
         it('should fail to unstake one epoch after inactive/withdrawable stake has been reactivated', async () => {
             const amount = StakingWrapper.toBaseUnitAmount(10);
@@ -286,7 +287,7 @@ blockchainTests.resets.only('Stake States', () => {
             await staker.goToNextEpochAsync(); // stake is now withdrawable
             await staker.moveStakeAsync({ state: StakeState.Inactive }, { state: StakeState.Active }, amount);
             await staker.goToNextEpochAsync(); // stake is active and not withdrawable
-            await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS'));
+            await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO));
         });
         it('should fail to unstake >1 epoch after inactive/withdrawable stake has been reactivated', async () => {
             const amount = StakingWrapper.toBaseUnitAmount(10);
@@ -297,7 +298,7 @@ blockchainTests.resets.only('Stake States', () => {
             await staker.moveStakeAsync({ state: StakeState.Inactive }, { state: StakeState.Active }, amount);
             await staker.goToNextEpochAsync(); // stake is active and not withdrawable
             await staker.goToNextEpochAsync(); // stake is active and not withdrawable
-            await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS'));
+            await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO));
         });
     });
     describe('Simulations', () => {

packages/order-utils/src/staking_revert_errors.ts

@@ -195,6 +195,24 @@ export class PoolAlreadyExistsError extends RevertError {
     }
 }
 
+export class EthVaultNotSet extends RevertError {
+    constructor() {
+        super('EthVaultNotSet', 'EthVaultNotSet()');
+    }
+}
+
+export class RewardVaultNotSet extends RevertError {
+    constructor() {
+        super('RewardVaultNotSet', 'RewardVaultNotSet()');
+    }
+}
+
+export class InvalidStakeState extends RevertError {
+    constructor(state?: BigNumber) {
+        super('InvalidStakeState', 'InvalidStakeState(uint256 state)', { state });
+    }
+}
+
 const types = [
     MiscalculatedRewardsError,
     OnlyCallableByExchangeError,
@@ -220,6 +238,9 @@ const types = [
     AmountExceedsBalanceOfPoolError,
     OperatorShareMustBeBetween0And100Error,
     PoolAlreadyExistsError,
+    EthVaultNotSet,
+    RewardVaultNotSet,
+    InvalidStakeState
 ];
 
 // Register the types we've defined.