From 361576814c83fc1a3655f8419c863165af0eb1f2 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Thu, 10 Oct 2019 15:31:10 +0900 Subject: [PATCH] Removed loadSyncedBalance and loadUnsyncedBalance --- .../contracts/src/immutable/MixinStorage.sol | 4 -- .../contracts/src/interfaces/IStructs.sol | 1 - .../contracts/src/stake/MixinStake.sol | 5 +-- .../src/stake/MixinStakeBalances.sol | 8 ++-- .../contracts/src/stake/MixinStakeStorage.sol | 42 +++++-------------- .../staking_pools/MixinStakingPoolRewards.sol | 39 ++++++++--------- .../contracts/test/TestMixinStakeStorage.sol | 8 ++-- .../unit_tests/mixin_stake_storage_test.ts | 16 +++---- 8 files changed, 47 insertions(+), 76 deletions(-) diff --git a/contracts/staking/contracts/src/immutable/MixinStorage.sol b/contracts/staking/contracts/src/immutable/MixinStorage.sol index 0bb150a148..a8ee6ac7f9 100644 --- a/contracts/staking/contracts/src/immutable/MixinStorage.sol +++ b/contracts/staking/contracts/src/immutable/MixinStorage.sol @@ -44,21 +44,17 @@ contract MixinStorage is IStructs.ReadOnlyState public readOnlyState; // mapping from StakeStatus to global stored balance - // (access using _loadSyncedBalance or _loadUnsyncedBalance) // NOTE: only Status.DELEGATED is used to access this mapping, but this format // is used for extensibility mapping (uint8 => IStructs.StoredBalance) internal _globalStakeByStatus; // mapping from StakeStatus to address of staker to stored balance - // (access using _loadSyncedBalance or _loadUnsyncedBalance) mapping (uint8 => mapping (address => IStructs.StoredBalance)) internal _ownerStakeByStatus; // Mapping from Owner to Pool Id to Amount Delegated - // (access using _loadSyncedBalance or _loadUnsyncedBalance) mapping (address => mapping (bytes32 => IStructs.StoredBalance)) internal _delegatedStakeToPoolByOwner; // Mapping from Pool Id to Amount Delegated - // (access using _loadSyncedBalance or _loadUnsyncedBalance) mapping (bytes32 => IStructs.StoredBalance) internal _delegatedStakeByPoolId; // tracking Pool Id, a unique identifier for each staking pool. diff --git a/contracts/staking/contracts/src/interfaces/IStructs.sol b/contracts/staking/contracts/src/interfaces/IStructs.sol index 4857ec04d2..3c5ca8ecc4 100644 --- a/contracts/staking/contracts/src/interfaces/IStructs.sol +++ b/contracts/staking/contracts/src/interfaces/IStructs.sol @@ -60,7 +60,6 @@ interface IStructs { /// @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 _loadSyncedBalance or _loadUnsyncedBalance. /// @param currentEpoch the current epoch /// @param currentEpochBalance balance in the current epoch. /// @param nextEpochBalance balance in `currentEpoch+1`. diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index da9542b08f..08ead69be2 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -63,7 +63,7 @@ contract MixinStake is address staker = msg.sender; IStructs.StoredBalance memory undelegatedBalance = - _loadSyncedBalance(_ownerStakeByStatus[uint8(IStructs.StakeStatus.UNDELEGATED)][staker]); + _loadCurrentBalance(_ownerStakeByStatus[uint8(IStructs.StakeStatus.UNDELEGATED)][staker]); // stake must be undelegated in current and next epoch to be withdrawn uint256 currentWithdrawableStake = LibSafeMath.min256( @@ -111,8 +111,7 @@ contract MixinStake is { address payable staker = msg.sender; - // handle delegation; this must be done before moving stake as the - // current (out-of-sync) status is used during delegation. + // handle delegation if (from.status == IStructs.StakeStatus.DELEGATED) { _undelegateStake( from.poolId, diff --git a/contracts/staking/contracts/src/stake/MixinStakeBalances.sol b/contracts/staking/contracts/src/stake/MixinStakeBalances.sol index 37f0f73195..bc089d9651 100644 --- a/contracts/staking/contracts/src/stake/MixinStakeBalances.sol +++ b/contracts/staking/contracts/src/stake/MixinStakeBalances.sol @@ -39,7 +39,7 @@ contract MixinStakeBalances is view returns (IStructs.StoredBalance memory balance) { - balance = _loadSyncedBalance( + balance = _loadCurrentBalance( _globalStakeByStatus[uint8(IStructs.StakeStatus.DELEGATED)] ); if (stakeStatus == IStructs.StakeStatus.UNDELEGATED) { @@ -64,7 +64,7 @@ contract MixinStakeBalances is view returns (IStructs.StoredBalance memory balance) { - balance = _loadSyncedBalance( + balance = _loadCurrentBalance( _ownerStakeByStatus[uint8(stakeStatus)][staker] ); return balance; @@ -90,7 +90,7 @@ contract MixinStakeBalances is view returns (IStructs.StoredBalance memory balance) { - balance = _loadSyncedBalance(_delegatedStakeToPoolByOwner[staker][poolId]); + balance = _loadCurrentBalance(_delegatedStakeToPoolByOwner[staker][poolId]); return balance; } @@ -103,7 +103,7 @@ contract MixinStakeBalances is view returns (IStructs.StoredBalance memory balance) { - balance = _loadSyncedBalance(_delegatedStakeByPoolId[poolId]); + balance = _loadCurrentBalance(_delegatedStakeByPoolId[poolId]); return balance; } } diff --git a/contracts/staking/contracts/src/stake/MixinStakeStorage.sol b/contracts/staking/contracts/src/stake/MixinStakeStorage.sol index 6c815d0205..92d1a642ce 100644 --- a/contracts/staking/contracts/src/stake/MixinStakeStorage.sol +++ b/contracts/staking/contracts/src/stake/MixinStakeStorage.sol @@ -48,9 +48,9 @@ contract MixinStakeStorage is return; } - // load balance from storage and synchronize it - IStructs.StoredBalance memory from = _loadSyncedBalance(fromPtr); - IStructs.StoredBalance memory to = _loadSyncedBalance(toPtr); + // load current balances from storage + IStructs.StoredBalance memory from = _loadCurrentBalance(fromPtr); + IStructs.StoredBalance memory to = _loadCurrentBalance(toPtr); // sanity check on balance if (amount > from.nextEpochBalance) { @@ -71,20 +71,15 @@ contract MixinStakeStorage is _storeBalance(toPtr, to); } - /// @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 balancePtr to load and sync. - /// @return synchronized balance. - function _loadSyncedBalance(IStructs.StoredBalance storage balancePtr) + /// @dev Loads a balance from storage and updates its fields to reflect values for the current epoch. + /// @param balancePtr to load. + /// @return current balance. + function _loadCurrentBalance(IStructs.StoredBalance storage balancePtr) internal view returns (IStructs.StoredBalance memory balance) { - // load from storage balance = balancePtr; - // sync uint256 currentEpoch_ = currentEpoch; if (currentEpoch_ > balance.currentEpoch) { balance.currentEpoch = currentEpoch_.downcastToUint64(); @@ -93,21 +88,6 @@ contract MixinStakeStorage is return balance; } - /// @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 - pure - 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. @@ -115,7 +95,7 @@ contract MixinStakeStorage is internal { // Remove stake from balance - IStructs.StoredBalance memory balance = _loadSyncedBalance(balancePtr); + IStructs.StoredBalance memory balance = _loadCurrentBalance(balancePtr); balance.nextEpochBalance = uint256(balance.nextEpochBalance).safeAdd(amount).downcastToUint96(); balance.currentEpochBalance = uint256(balance.currentEpochBalance).safeAdd(amount).downcastToUint96(); @@ -130,7 +110,7 @@ contract MixinStakeStorage is internal { // Remove stake from balance - IStructs.StoredBalance memory balance = _loadSyncedBalance(balancePtr); + IStructs.StoredBalance memory balance = _loadCurrentBalance(balancePtr); balance.nextEpochBalance = uint256(balance.nextEpochBalance).safeSub(amount).downcastToUint96(); balance.currentEpochBalance = uint256(balance.currentEpochBalance).safeSub(amount).downcastToUint96(); @@ -145,7 +125,7 @@ contract MixinStakeStorage is internal { // Add stake to balance - IStructs.StoredBalance memory balance = _loadSyncedBalance(balancePtr); + IStructs.StoredBalance memory balance = _loadCurrentBalance(balancePtr); balance.nextEpochBalance = uint256(balance.nextEpochBalance).safeAdd(amount).downcastToUint96(); // update state @@ -159,7 +139,7 @@ contract MixinStakeStorage is internal { // Remove stake from balance - IStructs.StoredBalance memory balance = _loadSyncedBalance(balancePtr); + IStructs.StoredBalance memory balance = _loadCurrentBalance(balancePtr); balance.nextEpochBalance = uint256(balance.nextEpochBalance).safeSub(amount).downcastToUint96(); // update state diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index 282010d6a7..35de11be6f 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -49,10 +49,8 @@ contract MixinStakingPoolRewards is member ); - // Update stored balance with synchronized version; this prevents - // redundant withdrawals. _delegatedStakeToPoolByOwner[member][poolId] = - _loadSyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]); + _loadCurrentBalance(_delegatedStakeToPoolByOwner[member][poolId]); } /// @dev Computes the reward balance in ETH of the operator of a pool. @@ -268,13 +266,12 @@ contract MixinStakingPoolRewards is return 0; } - IStructs.StoredBalance memory unsyncedStake = - _loadUnsyncedBalance(_delegatedStakeToPoolByOwner[member][poolId]); + IStructs.StoredBalance memory delegatedStake = _delegatedStakeToPoolByOwner[member][poolId]; - // There can be no rewards if the last epoch when stake was synced is + // There can be no rewards if the last epoch when stake was stored is // equal to the current epoch, because all prior rewards, including // rewards finalized this epoch have been claimed. - if (unsyncedStake.currentEpoch == _currentEpoch) { + if (delegatedStake.currentEpoch == _currentEpoch) { return 0; } @@ -282,29 +279,29 @@ contract MixinStakingPoolRewards is // 1/3 Unfinalized rewards earned in `currentEpoch - 1`. reward = _computeUnfinalizedDelegatorReward( - unsyncedStake, + delegatedStake, _currentEpoch, unfinalizedMembersReward, unfinalizedMembersStake ); - // 2/3 Finalized rewards earned in epochs [`unsyncedStake.currentEpoch + 1` .. `currentEpoch - 1`] - uint256 unsyncedStakeNextEpoch = uint256(unsyncedStake.currentEpoch).safeAdd(1); + // 2/3 Finalized rewards earned in epochs [`delegatedStake.currentEpoch + 1` .. `currentEpoch - 1`] + uint256 delegatedStakeNextEpoch = uint256(delegatedStake.currentEpoch).safeAdd(1); reward = reward.safeAdd( _computeMemberRewardOverInterval( poolId, - unsyncedStake.currentEpochBalance, - unsyncedStake.currentEpoch, - unsyncedStakeNextEpoch + delegatedStake.currentEpochBalance, + delegatedStake.currentEpoch, + delegatedStakeNextEpoch ) ); - // 3/3 Finalized rewards earned in epoch `unsyncedStake.currentEpoch`. + // 3/3 Finalized rewards earned in epoch `delegatedStake.currentEpoch`. reward = reward.safeAdd( _computeMemberRewardOverInterval( poolId, - unsyncedStake.nextEpochBalance, - unsyncedStakeNextEpoch, + delegatedStake.nextEpochBalance, + delegatedStakeNextEpoch, _currentEpoch ) ); @@ -313,13 +310,13 @@ contract MixinStakingPoolRewards is } /// @dev Computes the unfinalized rewards earned by a delegator in the last epoch. - /// @param unsyncedStake Unsynced delegated stake to pool by staker + /// @param delegatedStake Amount of stake delegated to pool by a specific staker /// @param currentEpoch_ The epoch in which this call is executing /// @param unfinalizedMembersReward Unfinalized total members reward (if any). /// @param unfinalizedMembersStake Unfinalized total members stake (if any). /// @return reward Balance in WETH. function _computeUnfinalizedDelegatorReward( - IStructs.StoredBalance memory unsyncedStake, + IStructs.StoredBalance memory delegatedStake, uint256 currentEpoch_, uint256 unfinalizedMembersReward, uint256 unfinalizedMembersStake @@ -336,9 +333,9 @@ contract MixinStakingPoolRewards is // Unfinalized rewards are always earned from stake in // the prior epoch so we want the stake at `currentEpoch_-1`. - uint256 unfinalizedStakeBalance = unsyncedStake.currentEpoch >= currentEpoch_.safeSub(1) ? - unsyncedStake.currentEpochBalance : - unsyncedStake.nextEpochBalance; + uint256 unfinalizedStakeBalance = delegatedStake.currentEpoch >= currentEpoch_.safeSub(1) ? + delegatedStake.currentEpochBalance : + delegatedStake.nextEpochBalance; // Sanity check to save gas on computation if (unfinalizedStakeBalance == 0) { diff --git a/contracts/staking/contracts/test/TestMixinStakeStorage.sol b/contracts/staking/contracts/test/TestMixinStakeStorage.sol index e3eabbfa39..82882130f6 100644 --- a/contracts/staking/contracts/test/TestMixinStakeStorage.sol +++ b/contracts/staking/contracts/test/TestMixinStakeStorage.sol @@ -72,19 +72,19 @@ contract TestMixinStakeStorage is _decreaseNextBalance(testBalances[index], amount); } - function loadSyncedBalance(uint256 index) + function loadCurrentBalance(uint256 index) external returns (IStructs.StoredBalance memory balance) { - return _loadSyncedBalance(testBalances[index]); + return _loadCurrentBalance(testBalances[index]); } - function loadUnsyncedBalance(uint256 index) + function loadStaleBalance(uint256 index) external view returns (IStructs.StoredBalance memory balance) { - return _loadUnsyncedBalance(testBalances[index]); + return testBalances[index]; } function setStoredBalance( diff --git a/contracts/staking/test/unit_tests/mixin_stake_storage_test.ts b/contracts/staking/test/unit_tests/mixin_stake_storage_test.ts index 5e31f7934c..066e4077a4 100644 --- a/contracts/staking/test/unit_tests/mixin_stake_storage_test.ts +++ b/contracts/staking/test/unit_tests/mixin_stake_storage_test.ts @@ -121,24 +121,24 @@ blockchainTests.resets('MixinStakeStorage unit tests', env => { }); describe('Load balance', () => { - it('_loadSyncedBalance does not change state if balance was previously synced in the current epoch', async () => { + it('Balance does not change state if balance was previously synced in the current epoch', async () => { await testContract.setStoredBalance.awaitTransactionSuccessAsync(defaultSyncedBalance, INDEX_ZERO); - const actualBalance = await testContract.loadSyncedBalance.callAsync(INDEX_ZERO); + const actualBalance = await testContract.loadCurrentBalance.callAsync(INDEX_ZERO); expect(actualBalance).to.deep.equal(defaultSyncedBalance); }); - it('_loadSyncedBalance updates current epoch fields if the balance has not yet been synced in the current epoch', async () => { + it('Balance updates current epoch fields if the balance has not yet been synced in the current epoch', async () => { await testContract.setStoredBalance.awaitTransactionSuccessAsync(defaultUnsyncedBalance, INDEX_ZERO); - const actualBalance = await testContract.loadSyncedBalance.callAsync(INDEX_ZERO); + const actualBalance = await testContract.loadCurrentBalance.callAsync(INDEX_ZERO); expect(actualBalance).to.deep.equal(defaultSyncedBalance); }); - it('_loadUnsyncedBalance loads unsynced balance from storage without changing fields', async () => { + it('Balance loads unsynced balance from storage without changing fields', async () => { await testContract.setStoredBalance.awaitTransactionSuccessAsync(defaultUnsyncedBalance, INDEX_ZERO); - const actualBalance = await testContract.loadUnsyncedBalance.callAsync(INDEX_ZERO); + const actualBalance = await testContract.loadStaleBalance.callAsync(INDEX_ZERO); expect(actualBalance).to.deep.equal(defaultUnsyncedBalance); }); - it('_loadUnsyncedBalance loads synced balance from storage without changing fields', async () => { + it('Balance loads synced balance from storage without changing fields', async () => { await testContract.setStoredBalance.awaitTransactionSuccessAsync(defaultSyncedBalance, INDEX_ZERO); - const actualBalance = await testContract.loadUnsyncedBalance.callAsync(INDEX_ZERO); + const actualBalance = await testContract.loadStaleBalance.callAsync(INDEX_ZERO); expect(actualBalance).to.deep.equal(defaultSyncedBalance); }); });