From da83f75a138650aa4200515dc533333edb900f0a Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Thu, 5 Sep 2019 03:35:03 -0700 Subject: [PATCH] Readability Improvements --- .../contracts/src/fees/MixinExchangeFees.sol | 4 +- .../contracts/src/immutable/MixinStorage.sol | 15 ++- .../src/interfaces/IStakingEvents.sol | 4 +- .../contracts/src/interfaces/IStructs.sol | 42 ++++--- .../src/libs/LibStakingRichErrors.sol | 32 +++--- .../contracts/src/stake/MixinStake.sol | 76 ++++++------- .../src/stake/MixinStakeBalances.sol | 49 ++++----- .../contracts/src/stake/MixinStakeStorage.sol | 103 ++++++++++-------- .../MixinStakingPoolRewardVault.sol | 4 +- .../staking_pools/MixinStakingPoolRewards.sol | 29 +++-- .../contracts/src/vaults/MixinVaultCore.sol | 2 +- .../src/vaults/StakingPoolRewardVault.sol | 2 +- contracts/staking/test/utils/Simulation.ts | 2 +- .../staking/test/utils/staking_wrapper.ts | 24 ++-- contracts/staking/test/utils/types.ts | 6 +- contracts/utils/contracts/src/LibSafeMath.sol | 2 - .../order-utils/src/exchange_revert_errors.ts | 1 + .../order-utils/src/staking_revert_errors.ts | 20 ++-- 18 files changed, 220 insertions(+), 197 deletions(-) diff --git a/contracts/staking/contracts/src/fees/MixinExchangeFees.sol b/contracts/staking/contracts/src/fees/MixinExchangeFees.sol index c11261d879..366b64ac3e 100644 --- a/contracts/staking/contracts/src/fees/MixinExchangeFees.sol +++ b/contracts/staking/contracts/src/fees/MixinExchangeFees.sol @@ -207,8 +207,8 @@ contract MixinExchangeFees is bytes32 poolId = activePoolsThisEpoch[i]; // compute weighted stake - uint256 totalStakeDelegatedToPool = getTotalStakeDelegatedToPool(poolId).current; - uint256 stakeHeldByPoolOperator = getStakeDelegatedToPoolByOwner(getStakingPoolOperator(poolId), poolId).current; // @TODO Update + uint256 totalStakeDelegatedToPool = getTotalStakeDelegatedToPool(poolId).currentEpochBalance; + uint256 stakeHeldByPoolOperator = getStakeDelegatedToPoolByOwner(getStakingPoolOperator(poolId), poolId).currentEpochBalance; uint256 weightedStake = stakeHeldByPoolOperator.safeAdd( totalStakeDelegatedToPool .safeSub(stakeHeldByPoolOperator) diff --git a/contracts/staking/contracts/src/immutable/MixinStorage.sol b/contracts/staking/contracts/src/immutable/MixinStorage.sol index 8c0b7b46b4..6d388e100d 100644 --- a/contracts/staking/contracts/src/immutable/MixinStorage.sol +++ b/contracts/staking/contracts/src/immutable/MixinStorage.sol @@ -42,19 +42,24 @@ contract MixinStorage is address internal stakingContract; // mapping from Owner to Amount of Active Stake - mapping (address => IStructs.DelayedBalance) internal activeStakeByOwner; + // (access using _loadAndSyncBalance or _loadUnsyncedBalance) + mapping (address => IStructs.StoredBalance) internal activeStakeByOwner; // mapping from Owner to Amount of Inactive Stake - mapping (address => IStructs.DelayedBalance) internal inactiveStakeByOwner; + // (access using _loadAndSyncBalance or _loadUnsyncedBalance) + mapping (address => IStructs.StoredBalance) internal inactiveStakeByOwner; // mapping from Owner to Amount Delegated - mapping (address => IStructs.DelayedBalance) internal delegatedStakeByOwner; + // (access using _loadAndSyncBalance or _loadUnsyncedBalance) + mapping (address => IStructs.StoredBalance) internal delegatedStakeByOwner; // mapping from Owner to Pool Id to Amount Delegated - mapping (address => mapping (bytes32 => IStructs.DelayedBalance)) internal delegatedStakeToPoolByOwner; + // (access using _loadAndSyncBalance or _loadUnsyncedBalance) + mapping (address => mapping (bytes32 => IStructs.StoredBalance)) internal delegatedStakeToPoolByOwner; // mapping from Pool Id to Amount Delegated - mapping (bytes32 => IStructs.DelayedBalance) internal delegatedStakeByPoolId; + // (access using _loadAndSyncBalance or _loadUnsyncedBalance) + mapping (bytes32 => IStructs.StoredBalance) internal delegatedStakeByPoolId; // mapping from Owner to Amount of Withdrawable Stake mapping (address => uint256) internal withdrawableStakeByOwner; diff --git a/contracts/staking/contracts/src/interfaces/IStakingEvents.sol b/contracts/staking/contracts/src/interfaces/IStakingEvents.sol index 3f956e8924..e9a4861c9d 100644 --- a/contracts/staking/contracts/src/interfaces/IStakingEvents.sol +++ b/contracts/staking/contracts/src/interfaces/IStakingEvents.sol @@ -25,9 +25,9 @@ interface IStakingEvents { event MoveStake( address indexed owner, uint256 amount, - uint8 fromState, + uint8 fromStatus, bytes32 indexed fromPool, - uint8 toState, + uint8 toStatus, bytes32 indexed toProol ); diff --git a/contracts/staking/contracts/src/interfaces/IStructs.sol b/contracts/staking/contracts/src/interfaces/IStructs.sol index fbae07e3a4..d39ae0855a 100644 --- a/contracts/staking/contracts/src/interfaces/IStructs.sol +++ b/contracts/staking/contracts/src/interfaces/IStructs.sol @@ -39,7 +39,7 @@ interface IStructs { address makerAddress; } - /// @dev State for Staking Pools (see MixinStakingPool). + /// @dev Status for Staking Pools (see MixinStakingPool). /// @param operatorAddress Address of pool operator. /// @param operatorShare Portion of pool rewards owned by operator, in ppm. struct Pool { @@ -47,7 +47,7 @@ interface IStructs { uint32 operatorShare; } - /// @dev State for a pool that actively traded during the current epoch. + /// @dev Status for a pool that actively traded during the current epoch. /// (see MixinExchangeFees). /// @param poolId Unique Id of staking pool. /// @param feesCollected Fees collected in ETH by this pool in the current epoch. @@ -59,33 +59,39 @@ interface IStructs { uint256 delegatedStake; } - /// @dev A delayed balance allows values to be computed - struct DelayedBalance { - uint96 current; - uint96 next; - uint64 lastStored; + /// @dev Encapsulates a balance for the current and next epochs. + /// Note that these balances may be stale if the current epoch + /// is greater than `currentEpoch`. + /// Always load this struct using _loadAndSyncBalance or _loadUnsyncedBalance. + /// @param currentEpoch the current epoch + /// @param currentEpochBalance balance in the current epoch. + /// @param nextEpochBalance balance in the next epoch. + struct StoredBalance { + uint64 currentEpoch; + uint96 currentEpochBalance; + uint96 nextEpochBalance; } /// @dev Balance struct for stake. - /// @param current Balance in the current epoch. - /// @param next Balance in the next epoch. + /// @param currentEpochBalance Balance in the current epoch. + /// @param nextEpochBalance Balance in the next epoch. struct StakeBalance { - uint256 current; - uint256 next; + uint256 currentEpochBalance; + uint256 nextEpochBalance; } - /// @dev States that stake can exist in. - enum StakeState { + /// @dev Statuses that stake can exist in. + enum StakeStatus { ACTIVE, INACTIVE, DELEGATED } - /// @dev Info used to describe a state. - /// @param state of the stake. - /// @param poolId Unique Id of pool. This is set when state=DELEGATED. - struct StakeStateInfo { - StakeState state; + /// @dev Info used to describe a status. + /// @param status of the stake. + /// @param poolId Unique Id of pool. This is set when status=DELEGATED. + struct StakeInfo { + StakeStatus status; bytes32 poolId; } diff --git a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol index 907ee9c8a6..67a195778f 100644 --- a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol +++ b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol @@ -122,17 +122,17 @@ library LibStakingRichErrors { bytes4 internal constant INVALID_COBB_DOUGLAS_ALPHA_ERROR_SELECTOR = 0x8f8e73de; - // bytes4(keccak256("EthVaultNotSet()")) - bytes4 internal constant ETH_VAULT_NOT_SET = - 0xdb3f0be8; + // bytes4(keccak256("EthVaultNotSetError()")) + bytes4 internal constant ETH_VAULT_NOT_SET_ERROR_SELECTOR = + 0xa067f596; - // bytes4(keccak256("RewardVaultNotSet()")) - bytes4 internal constant REWARD_VAULT_NOT_SET = - 0xfcb260f7; + // bytes4(keccak256("RewardVaultNotSetError()")) + bytes4 internal constant REWARD_VAULT_NOT_SET_ERROR_SELECTOR = + 0xe6976d70; - // bytes4(keccak256("InvalidStakeState(uint256)")) - bytes4 internal constant INVALID_STAKE_STATE = - 0xe6586728; + // bytes4(keccak256("InvalidStakeStatusError(uint256)")) + bytes4 internal constant INVALID_STAKE_STATUS_ERROR_SELECTOR = + 0xb7161acd; // solhint-disable func-name-mixedcase function MiscalculatedRewardsError( @@ -475,34 +475,34 @@ library LibStakingRichErrors { ); } - function EthVaultNotSet() + function EthVaultNotSetError() internal pure returns (bytes memory) { return abi.encodeWithSelector( - ETH_VAULT_NOT_SET + ETH_VAULT_NOT_SET_ERROR_SELECTOR ); } - function RewardVaultNotSet() + function RewardVaultNotSetError() internal pure returns (bytes memory) { return abi.encodeWithSelector( - REWARD_VAULT_NOT_SET + REWARD_VAULT_NOT_SET_ERROR_SELECTOR ); } - function InvalidStakeState(uint256 state) + function InvalidStakeStatusError(uint256 status) internal pure returns (bytes memory) { return abi.encodeWithSelector( - INVALID_STAKE_STATE, - state + INVALID_STAKE_STATUS_ERROR_SELECTOR, + status ); } } diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index ffdd9d3f9e..761a73fcc2 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -44,7 +44,7 @@ contract MixinStake is using LibSafeMath for uint256; /// @dev Stake ZRX tokens. Tokens are deposited into the ZRX Vault. Unstake to retrieve the ZRX. - /// Stake is in the 'Active' state. + /// Stake is in the 'Active' status. /// @param amount of ZRX to stake. function stake(uint256 amount) external @@ -55,7 +55,7 @@ contract MixinStake is _depositFromOwnerIntoZrxVault(owner, amount); // mint stake - _mintBalance(activeStakeByOwner[owner], amount); + _incrementCurrentAndNextBalance(activeStakeByOwner[owner], amount); // notify emit Stake( @@ -65,7 +65,7 @@ contract MixinStake is } /// @dev Unstake. Tokens are withdrawn from the ZRX Vault and returned to the owner. - /// Stake must be in the 'inactive' state for at least one full epoch to unstake. + /// Stake must be in the 'inactive' status for at least one full epoch to unstake. /// @param amount of ZRX to unstake. function unstake(uint256 amount) external @@ -84,7 +84,7 @@ contract MixinStake is } // burn inactive stake - _burnBalance(inactiveStakeByOwner[owner], amount); + _decrementCurrentAndNextBalance(inactiveStakeByOwner[owner], amount); // update withdrawable field withdrawableStakeByOwner[owner] = currentWithdrawableStake.safeSub(amount); @@ -99,30 +99,30 @@ contract MixinStake is ); } - /// @dev Moves stake between states: 'active', 'inactive' or 'delegated'. + /// @dev Moves stake between statuses: 'active', 'inactive' or 'delegated'. /// This change comes into effect next epoch. - /// @param from state to move stake out of. - /// @param to state to move stake into. + /// @param from status to move stake out of. + /// @param to status to move stake into. /// @param amount of stake to move. function moveStake( - IStructs.StakeStateInfo calldata from, - IStructs.StakeStateInfo calldata to, + IStructs.StakeInfo calldata from, + IStructs.StakeInfo calldata to, uint256 amount ) external { - // sanity check - do nothing if moving stake between the same state - if (from.state != IStructs.StakeState.DELEGATED && from.state == to.state) { + // sanity check - do nothing if moving stake between the same status + if (from.status != IStructs.StakeStatus.DELEGATED && from.status == to.status) { return; - } else if (from.state == IStructs.StakeState.DELEGATED && from.poolId == to.poolId) { + } else if (from.status == IStructs.StakeStatus.DELEGATED && from.poolId == to.poolId) { return; } address payable owner = msg.sender; // handle delegation; this must be done before moving stake as the current - // (out-of-sync) state is used during delegation. - if (from.state == IStructs.StakeState.DELEGATED) { + // (out-of-sync) status is used during delegation. + if (from.status == IStructs.StakeStatus.DELEGATED) { _undelegateStake( from.poolId, owner, @@ -130,7 +130,7 @@ contract MixinStake is ); } - if (to.state == IStructs.StakeState.DELEGATED) { + if (to.status == IStructs.StakeStatus.DELEGATED) { _delegateStake( to.poolId, owner, @@ -138,28 +138,28 @@ contract MixinStake is ); } - // cache the current withdrawal state if we're moving out of the inactive state. - uint256 cachedWithdrawableStakeByOwner = (from.state == IStructs.StakeState.INACTIVE) + // cache the current withdrawal amoiunt, which may change if we're moving out of the inactive status. + uint256 withdrawableStake = (from.status == IStructs.StakeStatus.INACTIVE) ? getWithdrawableStake(owner) : 0; // execute move - IStructs.DelayedBalance storage fromPtr = _getBalancePtrFromState(from.state); - IStructs.DelayedBalance storage toPtr = _getBalancePtrFromState(to.state); + IStructs.StoredBalance storage fromPtr = _getBalancePtrFromStatus(owner, from.status); + IStructs.StoredBalance storage toPtr = _getBalancePtrFromStatus(owner, to.status); _moveStake(fromPtr, toPtr, amount); // update withdrawable field, if necessary - if (from.state == IStructs.StakeState.INACTIVE) { - withdrawableStakeByOwner[owner] = _computeWithdrawableStake(owner, cachedWithdrawableStakeByOwner); + if (from.status == IStructs.StakeStatus.INACTIVE) { + withdrawableStakeByOwner[owner] = _computeWithdrawableStake(owner, withdrawableStake); } // notify emit MoveStake( owner, amount, - uint8(from.state), + uint8(from.status), from.poolId, - uint8(to.state), + uint8(to.status), to.poolId ); } @@ -183,10 +183,10 @@ contract MixinStake is _syncCumulativeRewardsNeededByDelegator(poolId, currentEpoch); // increment how much stake the owner has delegated to the input pool - _incrementBalance(delegatedStakeToPoolByOwner[owner][poolId], amount); + _incrementNextBalance(delegatedStakeToPoolByOwner[owner][poolId], amount); // increment how much stake has been delegated to pool - _incrementBalance(delegatedStakeByPoolId[poolId], amount); + _incrementNextBalance(delegatedStakeByPoolId[poolId], amount); } /// @dev Un-Delegates a owners stake from a staking pool. @@ -208,33 +208,33 @@ contract MixinStake is _syncCumulativeRewardsNeededByDelegator(poolId, currentEpoch); // decrement how much stake the owner has delegated to the input pool - _decrementBalance(delegatedStakeToPoolByOwner[owner][poolId], amount); + _decrementNextBalance(delegatedStakeToPoolByOwner[owner][poolId], amount); // decrement how much stake has been delegated to pool - _decrementBalance(delegatedStakeByPoolId[poolId], amount); + _decrementNextBalance(delegatedStakeByPoolId[poolId], amount); } - /// @dev Returns a storage pointer to a user's stake in a given state. - /// @param state of user's stake to lookup. + /// @dev Returns a storage pointer to a user's stake in a given status. + /// @param owner of stake to query. + /// @param status of user's stake to lookup. /// @return a storage pointer to the corresponding stake stake - function _getBalancePtrFromState(IStructs.StakeState state) + function _getBalancePtrFromStatus(address owner, IStructs.StakeStatus status) private view - returns (IStructs.DelayedBalance storage) + returns (IStructs.StoredBalance storage) { - // lookup state - address owner = msg.sender; - if (state == IStructs.StakeState.ACTIVE) { + // lookup status + if (status == IStructs.StakeStatus.ACTIVE) { return activeStakeByOwner[owner]; - } else if (state == IStructs.StakeState.INACTIVE) { + } else if (status == IStructs.StakeStatus.INACTIVE) { return inactiveStakeByOwner[owner]; - } else if (state == IStructs.StakeState.DELEGATED) { + } else if (status == IStructs.StakeStatus.DELEGATED) { return delegatedStakeByOwner[owner]; } - // invalid state + // invalid status LibRichErrors.rrevert( - LibStakingRichErrors.InvalidStakeState(uint256(state)) + LibStakingRichErrors.InvalidStakeStatusError(uint256(status)) ); // required to compile ~ we should never hit this. diff --git a/contracts/staking/contracts/src/stake/MixinStakeBalances.sol b/contracts/staking/contracts/src/stake/MixinStakeBalances.sol index 7bd82f8d5c..946352eb34 100644 --- a/contracts/staking/contracts/src/stake/MixinStakeBalances.sol +++ b/contracts/staking/contracts/src/stake/MixinStakeBalances.sol @@ -59,10 +59,10 @@ contract MixinStakeBalances is view returns (IStructs.StakeBalance memory balance) { - IStructs.DelayedBalance memory storedBalance = _syncBalanceDestructive(activeStakeByOwner[owner]); + IStructs.StoredBalance memory storedBalance = _loadAndSyncBalance(activeStakeByOwner[owner]); return IStructs.StakeBalance({ - current: storedBalance.current, - next: storedBalance.next + currentEpochBalance: storedBalance.currentEpochBalance, + nextEpochBalance: storedBalance.nextEpochBalance }); } @@ -74,10 +74,10 @@ contract MixinStakeBalances is view returns (IStructs.StakeBalance memory balance) { - IStructs.DelayedBalance memory storedBalance = _syncBalanceDestructive(inactiveStakeByOwner[owner]); + IStructs.StoredBalance memory storedBalance = _loadAndSyncBalance(inactiveStakeByOwner[owner]); return IStructs.StakeBalance({ - current: storedBalance.current, - next: storedBalance.next + currentEpochBalance: storedBalance.currentEpochBalance, + nextEpochBalance: storedBalance.nextEpochBalance }); } @@ -89,8 +89,7 @@ contract MixinStakeBalances is view returns (uint256) { - uint256 cachedWithdrawableStakeByOwner = withdrawableStakeByOwner[owner]; - return _computeWithdrawableStake(owner, cachedWithdrawableStakeByOwner); + return _computeWithdrawableStake(owner, withdrawableStakeByOwner[owner]); } /// @dev Returns the stake delegated by a given owner. @@ -101,10 +100,10 @@ contract MixinStakeBalances is view returns (IStructs.StakeBalance memory balance) { - IStructs.DelayedBalance memory storedBalance = _syncBalanceDestructive(delegatedStakeByOwner[owner]); + IStructs.StoredBalance memory storedBalance = _loadAndSyncBalance(delegatedStakeByOwner[owner]); return IStructs.StakeBalance({ - current: storedBalance.current, - next: storedBalance.next + currentEpochBalance: storedBalance.currentEpochBalance, + nextEpochBalance: storedBalance.nextEpochBalance }); } @@ -117,10 +116,10 @@ contract MixinStakeBalances is view returns (IStructs.StakeBalance memory balance) { - IStructs.DelayedBalance memory storedBalance = _syncBalanceDestructive(delegatedStakeToPoolByOwner[owner][poolId]); + IStructs.StoredBalance memory storedBalance = _loadAndSyncBalance(delegatedStakeToPoolByOwner[owner][poolId]); return IStructs.StakeBalance({ - current: storedBalance.current, - next: storedBalance.next + currentEpochBalance: storedBalance.currentEpochBalance, + nextEpochBalance: storedBalance.nextEpochBalance }); } @@ -132,31 +131,31 @@ contract MixinStakeBalances is view returns (IStructs.StakeBalance memory balance) { - IStructs.DelayedBalance memory storedBalance = _syncBalanceDestructive(delegatedStakeByPoolId[poolId]); + IStructs.StoredBalance memory storedBalance = _loadAndSyncBalance(delegatedStakeByPoolId[poolId]); return IStructs.StakeBalance({ - current: storedBalance.current, - next: storedBalance.next + currentEpochBalance: storedBalance.currentEpochBalance, + nextEpochBalance: storedBalance.nextEpochBalance }); } /// @dev Returns the stake that can be withdrawn for a given owner. - /// This stake is in the "Deactive & Withdrawable" state. /// @param owner to query. + /// @param lastStoredWithdrawableStake The amount of withdrawable stake that was last stored. /// @return Withdrawable stake for owner. - function _computeWithdrawableStake(address owner, uint256 cachedWithdrawableStakeByOwner) + function _computeWithdrawableStake(address owner, uint256 lastStoredWithdrawableStake) internal view returns (uint256) { // stake cannot be withdrawn if it has been reallocated for the `next` epoch; // so the upper bound of withdrawable stake is always limited by the value of `next`. - IStructs.DelayedBalance memory storedBalance = inactiveStakeByOwner[owner]; - if (storedBalance.lastStored == currentEpoch) { - return storedBalance.next < cachedWithdrawableStakeByOwner ? storedBalance.next : cachedWithdrawableStakeByOwner; - } else if (uint256(storedBalance.lastStored).safeAdd(1) == currentEpoch) { - return storedBalance.next < storedBalance.current ? storedBalance.next : storedBalance.current; + IStructs.StoredBalance memory storedBalance = _loadUnsyncedBalance(inactiveStakeByOwner[owner]); + if (storedBalance.currentEpoch == currentEpoch) { + return LibSafeMath.min256(storedBalance.nextEpochBalance, lastStoredWithdrawableStake); + } else if (uint256(storedBalance.currentEpoch).safeAdd(1) == currentEpoch) { + return LibSafeMath.min256(storedBalance.nextEpochBalance, storedBalance.currentEpochBalance); } else { - return storedBalance.next; + return storedBalance.nextEpochBalance; } } } diff --git a/contracts/staking/contracts/src/stake/MixinStakeStorage.sol b/contracts/staking/contracts/src/stake/MixinStakeStorage.sol index 3b757b85c1..92bb877157 100644 --- a/contracts/staking/contracts/src/stake/MixinStakeStorage.sol +++ b/contracts/staking/contracts/src/stake/MixinStakeStorage.sol @@ -36,6 +36,7 @@ contract MixinStakeStorage is { using LibSafeMath for uint256; + using LibSafeDowncast for uint256; /// @dev Moves stake between states: 'active', 'inactive' or 'delegated'. /// This change comes into effect next epoch. @@ -43,8 +44,8 @@ contract MixinStakeStorage is /// @param toPtr pointer to storage location of `to` stake. /// @param amount of stake to move. function _moveStake( - IStructs.DelayedBalance storage fromPtr, - IStructs.DelayedBalance storage toPtr, + IStructs.StoredBalance storage fromPtr, + IStructs.StoredBalance storage toPtr, uint256 amount ) internal @@ -55,104 +56,118 @@ contract MixinStakeStorage is } // load balance from storage and synchronize it - IStructs.DelayedBalance memory from = _syncBalanceDestructive(fromPtr); - IStructs.DelayedBalance memory to = _syncBalanceDestructive(toPtr); + IStructs.StoredBalance memory from = _loadAndSyncBalance(fromPtr); + IStructs.StoredBalance memory to = _loadAndSyncBalance(toPtr); // sanity check on balance - if (amount > from.next) { + if (amount > from.nextEpochBalance) { LibRichErrors.rrevert( LibStakingRichErrors.InsufficientBalanceError( amount, - from.next + from.nextEpochBalance ) ); } // move stake for next epoch - from.next = LibSafeDowncast.downcastToUint96(uint256(from.next).safeSub(amount)); - to.next = LibSafeDowncast.downcastToUint96(uint256(to.next).safeAdd(amount)); + from.nextEpochBalance = uint256(from.nextEpochBalance).safeSub(amount).downcastToUint96(); + to.nextEpochBalance = uint256(to.nextEpochBalance).safeAdd(amount).downcastToUint96(); // update state in storage _storeBalance(fromPtr, from); _storeBalance(toPtr, to); } - /// @dev Synchronizes the fields of a stored balance. + /// @dev Loads a balance from storage and synchronizes its current/next fields. /// The structs `current` field is set to `next` if the /// current epoch is greater than the epoch in which the struct /// was stored. - /// @param balance to update. This will be equal to the return value after calling. + /// @param balancePtr to load and sync. /// @return synchronized balance. - function _syncBalanceDestructive(IStructs.DelayedBalance memory balance) + function _loadAndSyncBalance(IStructs.StoredBalance storage balancePtr) internal view - returns (IStructs.DelayedBalance memory) + returns (IStructs.StoredBalance memory balance) { + // load from storage + balance = balancePtr; + // sync uint256 currentEpoch = getCurrentEpoch(); - if (currentEpoch > balance.lastStored) { - balance.lastStored = LibSafeDowncast.downcastToUint64(currentEpoch); - balance.current = balance.next; + if (currentEpoch > balance.currentEpoch) { + balance.currentEpoch = currentEpoch.downcastToUint64(); + balance.currentEpochBalance = balance.nextEpochBalance; } return balance; } - /// @dev Mints new value in a balance. - /// This causes both the `current` and `next` fields to increase immediately. + /// @dev Loads a balance from storage without synchronizing its fields. + /// This function exists so that developers will have to explicitly + /// communicate that they're loading a synchronized or unsynchronized balance. + /// These balances should never be accessed directly. + /// @param balancePtr to load. + /// @return unsynchronized balance. + function _loadUnsyncedBalance(IStructs.StoredBalance storage balancePtr) + internal + view + returns (IStructs.StoredBalance memory balance) + { + balance = balancePtr; + return balance; + } + + /// @dev Increments both the `current` and `next` fields. /// @param balancePtr storage pointer to balance. /// @param amount to mint. - function _mintBalance(IStructs.DelayedBalance storage balancePtr, uint256 amount) + function _incrementCurrentAndNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) internal { // Remove stake from balance - IStructs.DelayedBalance memory balance = _syncBalanceDestructive(balancePtr); - balance.next = LibSafeDowncast.downcastToUint96(uint256(balance.next).safeAdd(amount)); - balance.current = LibSafeDowncast.downcastToUint96(uint256(balance.current).safeAdd(amount)); + IStructs.StoredBalance memory balance = _loadAndSyncBalance(balancePtr); + balance.nextEpochBalance = uint256(balance.nextEpochBalance).safeAdd(amount).downcastToUint96(); + balance.currentEpochBalance = uint256(balance.currentEpochBalance).safeAdd(amount).downcastToUint96(); // update state _storeBalance(balancePtr, balance); } - /// @dev Burns existing value in a balance. - /// This causes both the `current` and `next` fields to decrease immediately. + /// @dev Decrements both the `current` and `next` fields. /// @param balancePtr storage pointer to balance. /// @param amount to mint. - function _burnBalance(IStructs.DelayedBalance storage balancePtr, uint256 amount) + function _decrementCurrentAndNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) internal { // Remove stake from balance - IStructs.DelayedBalance memory balance = _syncBalanceDestructive(balancePtr); - balance.next = LibSafeDowncast.downcastToUint96(uint256(balance.next).safeSub(amount)); - balance.current = LibSafeDowncast.downcastToUint96(uint256(balance.current).safeSub(amount)); + IStructs.StoredBalance memory balance = _loadAndSyncBalance(balancePtr); + balance.nextEpochBalance = uint256(balance.nextEpochBalance).safeSub(amount).downcastToUint96(); + balance.currentEpochBalance = uint256(balance.currentEpochBalance).safeSub(amount).downcastToUint96(); // update state _storeBalance(balancePtr, balance); } - /// @dev Increments a balance. - /// Ths updates the `next` field but not the `current` field. + /// @dev Increments the `next` field (but not the `current` field). /// @param balancePtr storage pointer to balance. /// @param amount to increment by. - function _incrementBalance(IStructs.DelayedBalance storage balancePtr, uint256 amount) + function _incrementNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) internal { // Add stake to balance - IStructs.DelayedBalance memory balance = _syncBalanceDestructive(balancePtr); - balance.next = LibSafeDowncast.downcastToUint96(uint256(balance.next).safeAdd(amount)); + IStructs.StoredBalance memory balance = _loadAndSyncBalance(balancePtr); + balance.nextEpochBalance = uint256(balance.nextEpochBalance).safeAdd(amount).downcastToUint96(); // update state _storeBalance(balancePtr, balance); } - /// @dev Decrements a balance. - /// Ths updates the `next` field but not the `current` field. + /// @dev Decrements the `next` field (but not the `current` field). /// @param balancePtr storage pointer to balance. /// @param amount to decrement by. - function _decrementBalance(IStructs.DelayedBalance storage balancePtr, uint256 amount) + function _decrementNextBalance(IStructs.StoredBalance storage balancePtr, uint256 amount) internal { // Remove stake from balance - IStructs.DelayedBalance memory balance = _syncBalanceDestructive(balancePtr); - balance.next = LibSafeDowncast.downcastToUint96(uint256(balance.next).safeSub(amount)); + IStructs.StoredBalance memory balance = _loadAndSyncBalance(balancePtr); + balance.nextEpochBalance = uint256(balance.nextEpochBalance).safeSub(amount).downcastToUint96(); // update state _storeBalance(balancePtr, balance); @@ -162,16 +177,16 @@ contract MixinStakeStorage is /// @param balancePtr points to where `balance` will be stored. /// @param balance to save to storage. function _storeBalance( - IStructs.DelayedBalance storage balancePtr, - IStructs.DelayedBalance memory balance + IStructs.StoredBalance storage balancePtr, + IStructs.StoredBalance memory balance ) private { // note - this compresses into a single `sstore` when optimizations are enabled, // since the StakeBalance struct occupies a single word of storage. - balancePtr.lastStored = balance.lastStored; - balancePtr.next = balance.next; - balancePtr.current = balance.current; + balancePtr.currentEpoch = balance.currentEpoch; + balancePtr.nextEpochBalance = balance.nextEpochBalance; + balancePtr.currentEpochBalance = balance.currentEpochBalance; } /// @dev Returns true iff storage pointers resolve to same storage location. @@ -180,9 +195,9 @@ contract MixinStakeStorage is /// @return true iff pointers are equal. function _arePointersEqual( // solhint-disable-next-line no-unused-vars - IStructs.DelayedBalance storage balancePtrA, + IStructs.StoredBalance storage balancePtrA, // solhint-disable-next-line no-unused-vars - IStructs.DelayedBalance storage balancePtrB + IStructs.StoredBalance storage balancePtrB ) private pure diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewardVault.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewardVault.sol index 113b9b49ed..264f580abd 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewardVault.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewardVault.sol @@ -73,7 +73,7 @@ contract MixinStakingPoolRewardVault is address payable rewardVaultAddress = address(uint160(address(rewardVault))); if (rewardVaultAddress == NIL_ADDRESS) { LibRichErrors.rrevert( - LibStakingRichErrors.RewardVaultNotSet() + LibStakingRichErrors.RewardVaultNotSetError() ); } @@ -96,7 +96,7 @@ contract MixinStakingPoolRewardVault is IStakingPoolRewardVault _rewardVault = rewardVault; if (address(_rewardVault) == NIL_ADDRESS) { LibRichErrors.rrevert( - LibStakingRichErrors.RewardVaultNotSet() + LibStakingRichErrors.RewardVaultNotSetError() ); } diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index c90170e6bb..b4b218e29d 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -48,33 +48,32 @@ contract MixinStakingPoolRewards is returns (uint256 totalReward) { // cache some values to reduce sloads - IStructs.DelayedBalance memory delegatedStake = delegatedStakeToPoolByOwner[member][poolId]; + IStructs.StoredBalance memory delegatedStake = _loadUnsyncedBalance(delegatedStakeToPoolByOwner[member][poolId]); uint256 currentEpoch = getCurrentEpoch(); // value is always zero in these two scenarios: - // 1. The current epoch is zero: delegation begins at epoch 1 - // 2. The owner's delegated is current as of this epoch: their rewards have been moved to the ETH vault. - if (currentEpoch == 0 || delegatedStake.lastStored == currentEpoch) return 0; + // 1. The owner's delegated is current as of this epoch: their rewards have been moved to the ETH vault. + // 2. The current epoch is zero: delegation begins at epoch 1 + if (delegatedStake.currentEpoch == currentEpoch || currentEpoch == 0) return 0; - // compute reward accumulated during `lastStored` epoch; - // the `current` balance describes how much stake was collecting rewards when `lastStored` was set. - uint256 rewardsAccumulatedDuringLastStoredEpoch = (delegatedStake.current != 0) + // compute reward accumulated during `delegatedStake.currentEpoch`; + uint256 rewardsAccumulatedDuringLastStoredEpoch = (delegatedStake.currentEpochBalance != 0) ? _computeMemberRewardOverInterval( poolId, - delegatedStake.current, - delegatedStake.lastStored - 1, - delegatedStake.lastStored + delegatedStake.currentEpochBalance, + delegatedStake.currentEpoch - 1, + delegatedStake.currentEpoch ) : 0; - // compute the rewards accumulated by the `next` balance; - // this starts at `lastStored + 1` and goes up until the last epoch, during which + // compute the reward accumulated by the `next` balance; + // this starts at `delegatedStake.currentEpoch + 1` and goes up until the last epoch, during which // rewards were accumulated. This is at most the most recently finalized epoch (current epoch - 1). - uint256 rewardsAccumulatedAfterLastStoredEpoch = (cumulativeRewardsByPoolLastStored[poolId] > delegatedStake.lastStored) + uint256 rewardsAccumulatedAfterLastStoredEpoch = (cumulativeRewardsByPoolLastStored[poolId] > delegatedStake.currentEpoch) ? _computeMemberRewardOverInterval( poolId, - delegatedStake.next, - delegatedStake.lastStored, + delegatedStake.nextEpochBalance, + delegatedStake.currentEpoch, cumulativeRewardsByPoolLastStored[poolId] ) : 0; diff --git a/contracts/staking/contracts/src/vaults/MixinVaultCore.sol b/contracts/staking/contracts/src/vaults/MixinVaultCore.sol index a50f578d9e..3c4a5d6dfa 100644 --- a/contracts/staking/contracts/src/vaults/MixinVaultCore.sol +++ b/contracts/staking/contracts/src/vaults/MixinVaultCore.sol @@ -34,7 +34,7 @@ import "../interfaces/IVaultCore.sol"; /// recoverable flaw/bug/vulnerability, simply detach the staking contract /// by setting its address to `address(0)`. Once in Catastrophic Failure Mode, /// a vault cannot be reset to normal mode; this prevents corruption of related -/// state in the staking contract. +/// status in the staking contract. contract MixinVaultCore is Authorizable, IVaultCore diff --git a/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol b/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol index 8977d70283..cb9c52e942 100644 --- a/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol +++ b/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol @@ -280,7 +280,7 @@ contract StakingPoolRewardVault is IEthVault _ethVault = ethVault; if (address(_ethVault) == address(0)) { LibRichErrors.rrevert( - LibStakingRichErrors.EthVaultNotSet() + LibStakingRichErrors.EthVaultNotSetError() ); } diff --git a/contracts/staking/test/utils/Simulation.ts b/contracts/staking/test/utils/Simulation.ts index 57a53d267e..68136c0728 100644 --- a/contracts/staking/test/utils/Simulation.ts +++ b/contracts/staking/test/utils/Simulation.ts @@ -64,7 +64,7 @@ export class Simulation { await this._withdrawRewardForStakingPoolMemberForDelegatorsByUndelegatingAsync(this._p); } - // @TODO cleanup state and verify the staking contract is empty + // @TODO cleanup status and verify the staking contract is empty } private async _withdrawRewardForStakingPoolMemberForDelegatorsByUndelegatingAsync( diff --git a/contracts/staking/test/utils/staking_wrapper.ts b/contracts/staking/test/utils/staking_wrapper.ts index 4d717d78d1..0506181d39 100644 --- a/contracts/staking/test/utils/staking_wrapper.ts +++ b/contracts/staking/test/utils/staking_wrapper.ts @@ -203,27 +203,27 @@ export class StakingWrapper { } public async moveStakeAsync( owner: string, - _fromState: { - state: number, + _fromStatus: { + status: number, poolId?: string }, - _toState: { - state: number, + _toStatus: { + status: number, poolId?: string }, amount: BigNumber, ): Promise { - const fromState = { - state: _fromState.state, - poolId: _fromState.poolId !== undefined ? _fromState.poolId : constants.NIL_POOL_ID + const fromStatus = { + status: _fromStatus.status, + poolId: _fromStatus.poolId !== undefined ? _fromStatus.poolId : constants.NIL_POOL_ID }; - const toState = { - state: _toState.state, - poolId: _toState.poolId !== undefined ? _toState.poolId : constants.NIL_POOL_ID + const toStatus = { + status: _toStatus.status, + poolId: _toStatus.poolId !== undefined ? _toStatus.poolId : constants.NIL_POOL_ID }; const calldata = this.getStakingContract().moveStake.getABIEncodedTransactionData( - fromState, - toState, + fromStatus, + toStatus, amount, ); const txReceipt = await this._executeTransactionAsync(calldata, owner); diff --git a/contracts/staking/test/utils/types.ts b/contracts/staking/test/utils/types.ts index afe3526617..383aa47695 100644 --- a/contracts/staking/test/utils/types.ts +++ b/contracts/staking/test/utils/types.ts @@ -50,11 +50,11 @@ export interface SimulationParams { } export interface StakeBalance { - current: BigNumber, - next: BigNumber, + currentEpochBalance: BigNumber, + nextEpochBalance: BigNumber, } -export enum StakeStateId { +export enum StakeStatus { ACTIVE, INACTIVE, DELEGATED diff --git a/contracts/utils/contracts/src/LibSafeMath.sol b/contracts/utils/contracts/src/LibSafeMath.sol index 4dea8bf10a..407e02eb45 100644 --- a/contracts/utils/contracts/src/LibSafeMath.sol +++ b/contracts/utils/contracts/src/LibSafeMath.sol @@ -6,8 +6,6 @@ import "./LibSafeMathRichErrors.sol"; library LibSafeMath { - using LibSafeMath for uint256; - function safeMul(uint256 a, uint256 b) internal pure diff --git a/packages/order-utils/src/exchange_revert_errors.ts b/packages/order-utils/src/exchange_revert_errors.ts index 6ae833d711..739f6a8fb0 100644 --- a/packages/order-utils/src/exchange_revert_errors.ts +++ b/packages/order-utils/src/exchange_revert_errors.ts @@ -293,6 +293,7 @@ const types = [ TransactionGasPriceError, TransactionInvalidContextError, TransactionSignatureError, + IncompleteFillError, ]; // Register the types we've defined. diff --git a/packages/order-utils/src/staking_revert_errors.ts b/packages/order-utils/src/staking_revert_errors.ts index 4997b346b9..a36dd044bf 100644 --- a/packages/order-utils/src/staking_revert_errors.ts +++ b/packages/order-utils/src/staking_revert_errors.ts @@ -204,21 +204,21 @@ export class InvalidCobbDouglasAlphaError extends RevertError { } } -export class EthVaultNotSet extends RevertError { +export class EthVaultNotSetError extends RevertError { constructor() { - super('EthVaultNotSet', 'EthVaultNotSet()'); + super('EthVaultNotSetError', 'EthVaultNotSetError()'); } } -export class RewardVaultNotSet extends RevertError { +export class RewardVaultNotSetError extends RevertError { constructor() { - super('RewardVaultNotSet', 'RewardVaultNotSet()'); + super('RewardVaultNotSetError', 'RewardVaultNotSetError()'); } } -export class InvalidStakeState extends RevertError { - constructor(state?: BigNumber) { - super('InvalidStakeState', 'InvalidStakeState(uint256 state)', { state }); +export class InvalidStakeStatusError extends RevertError { + constructor(status?: BigNumber) { + super('InvalidStakeStatusError', 'InvalidStakeStatusError(uint256 status)', { status }); } } @@ -248,9 +248,9 @@ const types = [ InvalidPoolOperatorShareError, PoolAlreadyExistsError, InvalidCobbDouglasAlphaError, - EthVaultNotSet, - RewardVaultNotSet, - InvalidStakeState + EthVaultNotSetError, + RewardVaultNotSetError, + InvalidStakeStatusError ]; // Register the types we've defined.