From 68e0322186b30004da04e79a0c71367abf24e5d4 Mon Sep 17 00:00:00 2001 From: DrZoltanFazekas Date: Fri, 22 Nov 2024 19:54:45 +0100 Subject: [PATCH] Fix issue 9, remove copy of deposit contract --- README.md | 4 + remappings.txt | 3 +- src/Deposit.sol | 637 --------------------------------- src/LiquidDelegationV2.sol | 1 - src/NonLiquidDelegationV2.sol | 122 ++++--- test/LiquidDelegation.t.sol | 12 +- test/NonLiquidDelegation.t.sol | 36 +- 7 files changed, 125 insertions(+), 690 deletions(-) delete mode 100644 src/Deposit.sol diff --git a/README.md b/README.md index c661fc8..8176bc2 100644 --- a/README.md +++ b/README.md @@ -7,6 +7,10 @@ Install Foundry (https://book.getfoundry.sh/getting-started/installation) and th forge install OpenZeppelin/openzeppelin-contracts-upgradeable --no-commit forge install OpenZeppelin/openzeppelin-contracts --no-commit ``` +The Zilliqa 2.0 deposit contract must be compiled for the tests in this repository to work. Specify the folder containing the `deposit.sol` file in `remappings.txt`: +``` +@zilliqa/zq2/=/home/user/zq2/zilliqa/src/contracts/ +``` ## Contract Deployment The delegation contract is used by delegators to stake and unstake ZIL with the respective validator. It acts as the validator node's control address and interacts with the `Deposit` system contract. diff --git a/remappings.txt b/remappings.txt index 5e817ed..6b6aa79 100644 --- a/remappings.txt +++ b/remappings.txt @@ -1,2 +1,3 @@ @openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/ -@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/ \ No newline at end of file +@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/ +@zilliqa/zq2/=/home/zoltan/Github/zq2/zilliqa/src/contracts/ \ No newline at end of file diff --git a/src/Deposit.sol b/src/Deposit.sol deleted file mode 100644 index 6dd543b..0000000 --- a/src/Deposit.sol +++ /dev/null @@ -1,637 +0,0 @@ -// SPDX-License-Identifier: MIT OR Apache-2.0 -pragma solidity ^0.8.20; - -struct Withdrawal { - uint256 startedAt; - uint256 amount; -} - -// Implementation of a double-ended queue of `Withdrawal`s, backed by a circular buffer. -library Deque { - struct Withdrawals { - Withdrawal[] values; - // The physical index of the first element, if it exists. If `len == 0`, the value of `head` is unimportant. - uint256 head; - // The number of elements in the queue. - uint256 len; - } - - // Returns the physical index of an element, given its logical index. - function physicalIdx( - Withdrawals storage deque, - uint256 idx - ) internal view returns (uint256) { - uint256 physical = deque.head + idx; - // Wrap the physical index in case it is out-of-bounds of the buffer. - if (physical >= deque.values.length) { - return physical - deque.values.length; - } else { - return physical; - } - } - - function length(Withdrawals storage deque) internal view returns (uint256) { - return deque.len; - } - - // Get the element at the given logical index. Reverts if `idx >= queue.length()`. - function get( - Withdrawals storage deque, - uint256 idx - ) internal view returns (Withdrawal storage) { - if (idx >= deque.len) { - revert("element does not exist"); - } - - uint256 pIdx = physicalIdx(deque, idx); - return deque.values[pIdx]; - } - - // Push an empty element to the back of the queue. Returns a reference to the new element. - function pushBack( - Withdrawals storage deque - ) internal returns (Withdrawal storage) { - // Add more space in the buffer if it is full. - if (deque.len == deque.values.length) { - deque.values.push(); - } - - uint256 idx = physicalIdx(deque, deque.len); - deque.len += 1; - - return deque.values[idx]; - } - - // Pop an element from the front of the queue. Note that this returns a reference to the element in storage. This - // means that further mutations of the queue may invalidate the returned element. Do not use this return value - // after calling any other mutations on the queue. - function popFront( - Withdrawals storage deque - ) internal returns (Withdrawal storage) { - if (deque.len == 0) { - revert("queue is empty"); - } - - uint256 oldHead = deque.head; - deque.head = physicalIdx(deque, 1); - deque.len -= 1; - return deque.values[oldHead]; - } - - // Peeks the element at the back of the queue. Note that this returns a reference to the element in storage. This - // means that further mutations of the queue may invalidate the returned element. Do not use this return value - // after calling any other mutations on the queue. - function back( - Withdrawals storage deque - ) internal view returns (Withdrawal storage) { - if (deque.len == 0) { - revert("queue is empty"); - } - - return get(deque, deque.len - 1); - } - - // Peeks the element at the front of the queue. Note that this returns a reference to the element in storage. This - // means that further mutations of the queue may invalidate the returned element. Do not use this return value - // after calling any other mutations on the queue. - function front( - Withdrawals storage deque - ) internal view returns (Withdrawal storage) { - if (deque.len == 0) { - revert("queue is empty"); - } - - return get(deque, 0); - } -} - -using Deque for Deque.Withdrawals; - -struct CommitteeStakerEntry { - // The index of the value in the `stakers` array plus 1. - // Index 0 is used to mean a value is not present. - uint256 index; - // Invariant: `balance >= minimumStake` - uint256 balance; -} - -struct Committee { - // Invariant: Equal to the sum of `balances` in `stakers`. - uint256 totalStake; - bytes[] stakerKeys; - mapping(bytes => CommitteeStakerEntry) stakers; -} - -struct Staker { - // The address used for authenticating requests from this staker to the deposit contract. - // Invariant: `controlAddress != address(0)`. - address controlAddress; - // The address which rewards for this staker will be sent to. - address rewardAddress; - // libp2p peer ID, corresponding to the staker's `blsPubKey` - bytes peerId; - // Invariants: Items are always sorted by `startedAt`. No two items have the same value of `startedAt`. - Deque.Withdrawals withdrawals; -} - -// Parameters passed to the deposit contract constructor, for each staker who should be in the initial committee. -struct InitialStaker { - bytes blsPubKey; - bytes peerId; - address rewardAddress; - address controlAddress; - uint256 amount; -} - -contract Deposit { - // The committee in the current epoch and the 2 epochs following it. The value for the current epoch - // is stored at index (currentEpoch() % 3). - Committee[3] _committee; - - // All stakers. Keys into this map are stored by the `Committee`. - mapping(bytes => Staker) _stakersMap; - // Mapping from `controlAddress` to `blsPubKey` for each staker. - mapping(address => bytes) _stakerKeys; - - // The latest epoch for which the committee was calculated. It is implied that no changes have (yet) occurred in - // future epochs, either because those epochs haven't happened yet or because they have happened, but no deposits - // or withdrawals were made. - uint64 latestComputedEpoch; - - uint256 public immutable minimumStake; - uint256 public immutable maximumStakers; - - uint64 public immutable blocksPerEpoch; - - modifier onlyControlAddress(bytes calldata blsPubKey) { - require(blsPubKey.length == 48); - require( - _stakersMap[blsPubKey].controlAddress == msg.sender, - "sender is not the control address" - ); - _; - } - - constructor( - uint256 _minimumStake, - uint256 _maximumStakers, - uint64 _blocksPerEpoch, - InitialStaker[] memory initialStakers - ) { - minimumStake = _minimumStake; - maximumStakers = _maximumStakers; - blocksPerEpoch = _blocksPerEpoch; - latestComputedEpoch = currentEpoch(); - - for (uint i = 0; i < initialStakers.length; i++) { - InitialStaker memory initialStaker = initialStakers[i]; - bytes memory blsPubKey = initialStaker.blsPubKey; - bytes memory peerId = initialStaker.peerId; - address rewardAddress = initialStaker.rewardAddress; - address controlAddress = initialStaker.controlAddress; - uint256 amount = initialStaker.amount; - - require(blsPubKey.length == 48); - require(peerId.length == 38); - require( - controlAddress != address(0), - "control address cannot be zero" - ); - - Committee storage currentCommittee = committee(); - require( - currentCommittee.stakerKeys.length < maximumStakers, - "too many stakers" - ); - - Staker storage staker = _stakersMap[blsPubKey]; - // This must be a new staker, meaning the control address must be zero. - require( - staker.controlAddress == address(0), - "staker already exists" - ); - - if (amount < minimumStake) { - revert("stake is less than minimum stake"); - } - - _stakerKeys[controlAddress] = blsPubKey; - staker.peerId = peerId; - staker.rewardAddress = rewardAddress; - staker.controlAddress = controlAddress; - - currentCommittee.totalStake += amount; - currentCommittee.stakers[blsPubKey].balance = amount; - currentCommittee.stakers[blsPubKey].index = - currentCommittee.stakerKeys.length + - 1; - currentCommittee.stakerKeys.push(blsPubKey); - } - } - - function currentEpoch() public view returns (uint64) { - return uint64(block.number / blocksPerEpoch); - } - - function committee() private view returns (Committee storage) { - if (latestComputedEpoch <= currentEpoch()) { - // If the current epoch is after the latest computed epoch, it is implied that no changes have happened to - // the committee since the latest computed epoch. Therefore, it suffices to return the committee at that - // latest computed epoch. - return _committee[latestComputedEpoch % 3]; - } else { - // Otherwise, the committee has been changed. The caller who made the change will have pre-computed the - // result for us, so we can just return it. - return _committee[currentEpoch() % 3]; - } - } - - function leaderFromRandomness( - uint256 randomness - ) private view returns (bytes memory) { - Committee storage currentCommittee = committee(); - // Get a random number in the inclusive range of 0 to (totalStake - 1) - uint256 position = randomness % currentCommittee.totalStake; - uint256 cummulative_stake = 0; - - // TODO: Consider binary search for performance. Or consider an alias method for O(1) performance. - for (uint256 i = 0; i < currentCommittee.stakerKeys.length; i++) { - bytes memory stakerKey = currentCommittee.stakerKeys[i]; - uint256 stakedBalance = currentCommittee.stakers[stakerKey].balance; - - cummulative_stake += stakedBalance; - - if (position < cummulative_stake) { - return stakerKey; - } - } - - revert("Unable to select next leader"); - } - - function leaderAtView( - uint256 viewNumber - ) public view returns (bytes memory) { - uint256 randomness = uint256( - keccak256(bytes.concat(bytes32(viewNumber))) - ); - return leaderFromRandomness(randomness); - } - - function getStakers() public view returns (bytes[] memory) { - return committee().stakerKeys; - } - - function getTotalStake() public view returns (uint256) { - return committee().totalStake; - } - - function getStakersData() - public - view - returns ( - bytes[] memory stakerKeys, - uint256[] memory balances, - Staker[] memory stakers - ) - { - Committee storage currentCommittee = committee(); - stakerKeys = currentCommittee.stakerKeys; - balances = new uint256[](stakerKeys.length); - stakers = new Staker[](stakerKeys.length); - for (uint i = 0; i < stakerKeys.length; i++) { - bytes memory key = stakerKeys[i]; - balances[i] = currentCommittee.stakers[key].balance; - stakers[i] = _stakersMap[key]; - } - } - - function getStake(bytes calldata blsPubKey) public view returns (uint256) { - require(blsPubKey.length == 48); - - // We don't need to check if `blsPubKey` is in `stakerKeys` here. If the `blsPubKey` is not a staker, the - // balance will default to zero. - return committee().stakers[blsPubKey].balance; - } - - function getFutureStake( - bytes calldata blsPubKey - ) public view returns (uint256) { - require(blsPubKey.length == 48); - - // if `latestComputedEpoch > currentEpoch()` - // then `latestComputedEpoch` determines the future committee we need - // otherwise there are no committee changes after `currentEpoch()` - // i.e. `latestComputedEpoch` determines the most recent committee - Committee storage latestCommittee = _committee[latestComputedEpoch % 3]; - - // We don't need to check if `blsPubKey` is in `stakerKeys` here. If the `blsPubKey` is not a staker, the - // balance will default to zero. - return latestCommittee.stakers[blsPubKey].balance; - } - - function getRewardAddress( - bytes calldata blsPubKey - ) public view returns (address) { - require(blsPubKey.length == 48); - if (_stakersMap[blsPubKey].controlAddress == address(0)) { - revert("not staked"); - } - return _stakersMap[blsPubKey].rewardAddress; - } - - function getControlAddress( - bytes calldata blsPubKey - ) public view returns (address) { - require(blsPubKey.length == 48); - if (_stakersMap[blsPubKey].controlAddress == address(0)) { - revert("not staked"); - } - return _stakersMap[blsPubKey].controlAddress; - } - - function setRewardAddress( - bytes calldata blsPubKey, - address rewardAddress - ) public onlyControlAddress(blsPubKey) { - _stakersMap[blsPubKey].rewardAddress = rewardAddress; - } - - function setControlAddress( - bytes calldata blsPubKey, - address controlAddress - ) public onlyControlAddress(blsPubKey) { - _stakersMap[blsPubKey].controlAddress = controlAddress; - } - - function getPeerId( - bytes calldata blsPubKey - ) public view returns (bytes memory) { - require(blsPubKey.length == 48); - if (_stakersMap[blsPubKey].controlAddress == address(0)) { - revert("not staked"); - } - return _stakersMap[blsPubKey].peerId; - } - - function updateLatestComputedEpoch() internal { - // If the latest computed epoch is less than two epochs ahead of the current one, we must fill in the missing - // epochs. This just involves copying the committee from the previous epoch to the next one. It is assumed that - // the caller will then want to update the future epochs. - if (latestComputedEpoch < currentEpoch() + 2) { - Committee storage latestComputedCommittee = _committee[ - latestComputedEpoch % 3 - ]; - // Note the early exit condition if `latestComputedEpoch + 3` which ensures this loop will not run more - // than twice. This is acceptable because we only store 3 committees at a time, so once we have updated two - // of them to the latest computed committee, there is no more work to do. - for ( - uint64 i = latestComputedEpoch + 1; - i <= currentEpoch() + 2 && i < latestComputedEpoch + 3; - i++ - ) { - // The operation we want to do is: `_committee[i % 3] = latestComputedCommittee` but we need to do it - // explicitly because `stakers` is a mapping. - - // Delete old keys from `_committee[i % 3].stakers`. - for (uint j = 0; j < _committee[i % 3].stakerKeys.length; j++) { - delete _committee[i % 3].stakers[ - _committee[i % 3].stakerKeys[j] - ]; - } - - _committee[i % 3].totalStake = latestComputedCommittee - .totalStake; - _committee[i % 3].stakerKeys = latestComputedCommittee - .stakerKeys; - for ( - uint j = 0; - j < latestComputedCommittee.stakerKeys.length; - j++ - ) { - bytes storage stakerKey = latestComputedCommittee - .stakerKeys[j]; - _committee[i % 3].stakers[ - stakerKey - ] = latestComputedCommittee.stakers[stakerKey]; - } - } - - latestComputedEpoch = currentEpoch() + 2; - } - } - - // keep in-sync with zilliqa/src/precompiles.rs - function _popVerify( - bytes memory pubkey, - bytes memory signature - ) internal view returns (bool) { - bytes memory input = abi.encodeWithSelector( - hex"bfd24965", // bytes4(keccak256("popVerify(bytes,bytes)")) - signature, - pubkey - ); - //TODO: don't remove the next line - return true; - uint inputLength = input.length; - bytes memory output = new bytes(32); - bool success; - assembly { - success := staticcall( - gas(), - 0x5a494c80, // "ZIL\x80" - add(input, 0x20), - inputLength, - add(output, 0x20), - 32 - ) - } - require(success, "popVerify"); - bool result = abi.decode(output, (bool)); - return result; - } - - function deposit( - bytes calldata blsPubKey, - bytes calldata peerId, - bytes calldata signature, - address rewardAddress - ) public payable { - require(blsPubKey.length == 48); - require(peerId.length == 38); - require(signature.length == 96); - - // Verify signature as a proof-of-possession of the private key. - bool pop = _popVerify(blsPubKey, signature); - require(pop, "rogue key check"); - - Staker storage staker = _stakersMap[blsPubKey]; - - if (msg.value < minimumStake) { - revert("stake is less than minimum stake"); - } - - _stakerKeys[msg.sender] = blsPubKey; - staker.peerId = peerId; - staker.rewardAddress = rewardAddress; - staker.controlAddress = msg.sender; - - updateLatestComputedEpoch(); - - Committee storage futureCommittee = _committee[ - (currentEpoch() + 2) % 3 - ]; - - require( - futureCommittee.stakerKeys.length < maximumStakers, - "too many stakers" - ); - require( - futureCommittee.stakers[blsPubKey].index == 0, - "staker already exists" - ); - - futureCommittee.totalStake += msg.value; - futureCommittee.stakers[blsPubKey].balance = msg.value; - futureCommittee.stakers[blsPubKey].index = - futureCommittee.stakerKeys.length + - 1; - futureCommittee.stakerKeys.push(blsPubKey); - } - - function depositTopup() public payable { - bytes storage stakerKey = _stakerKeys[msg.sender]; - require(stakerKey.length != 0, "staker does not exist"); - - updateLatestComputedEpoch(); - - Committee storage futureCommittee = _committee[ - (currentEpoch() + 2) % 3 - ]; - require( - futureCommittee.stakers[stakerKey].index != 0, - "staker does not exist" - ); - futureCommittee.totalStake += msg.value; - futureCommittee.stakers[stakerKey].balance += msg.value; - } - - function unstake(uint256 amount) public { - bytes storage stakerKey = _stakerKeys[msg.sender]; - require(stakerKey.length != 0, "staker does not exist"); - Staker storage staker = _stakersMap[stakerKey]; - - updateLatestComputedEpoch(); - - Committee storage futureCommittee = _committee[ - (currentEpoch() + 2) % 3 - ]; - - require( - futureCommittee.stakers[stakerKey].index != 0, - "staker does not exist" - ); - //TODO: keep the next line commented out - //require(futureCommittee.stakerKeys.length > 1, "too few stakers"); - require( - futureCommittee.stakers[stakerKey].balance >= amount, - "amount is greater than staked balance" - ); - - if (futureCommittee.stakers[stakerKey].balance - amount == 0) { - // Remove the staker from the future committee, because their staked amount has gone to zero. - futureCommittee.totalStake -= amount; - - uint256 deleteIndex = futureCommittee.stakers[stakerKey].index - 1; - uint256 lastIndex = futureCommittee.stakerKeys.length - 1; - - if (deleteIndex != lastIndex) { - // Move the last staker in `stakerKeys` to the position of the staker we want to delete. - bytes storage lastStakerKey = futureCommittee.stakerKeys[ - lastIndex - ]; - futureCommittee.stakerKeys[deleteIndex] = lastStakerKey; - // We need to remember to update the moved staker's `index` too. - futureCommittee.stakers[lastStakerKey].index = futureCommittee - .stakers[stakerKey] - .index; - } - - // It is now safe to delete the final staker in the list. - futureCommittee.stakerKeys.pop(); - delete futureCommittee.stakers[stakerKey]; - - // Note that we leave the staker in `_stakersMap` forever. - } else { - require( - futureCommittee.stakers[stakerKey].balance - amount >= - minimumStake, - "unstaking this amount would take the validator below the minimum stake" - ); - - // Partial unstake. The staker stays in the committee, but with a reduced stake. - futureCommittee.totalStake -= amount; - futureCommittee.stakers[stakerKey].balance -= amount; - } - - // Enqueue the withdrawal for this staker. - Deque.Withdrawals storage withdrawals = staker.withdrawals; - Withdrawal storage currentWithdrawal; - // We know `withdrawals` is sorted by `startedAt`. We also know `block.timestamp` is monotonically - // non-decreasing. Therefore if there is an existing entry with a `startedAt = block.timestamp`, it must be - // at the end of the queue. - if ( - withdrawals.length() != 0 && - withdrawals.back().startedAt == block.timestamp - ) { - // They have already made a withdrawal at this time, so grab a reference to the existing one. - currentWithdrawal = withdrawals.back(); - } else { - // Add a new withdrawal to the end of the queue. - currentWithdrawal = withdrawals.pushBack(); - currentWithdrawal.startedAt = block.timestamp; - } - currentWithdrawal.amount += amount; - } - - function withdraw() public { - _withdraw(0); - } - - function withdraw(uint256 count) public { - _withdraw(count); - } - - function withdrawalPeriod() public view returns (uint256) { - // shorter unbonding period for testing deposit withdrawals - if (block.chainid == 33469) return 5 minutes; - return 2 weeks; - } - - function _withdraw(uint256 count) internal { - uint256 releasedAmount = 0; - - Staker storage staker = _stakersMap[_stakerKeys[msg.sender]]; - - Deque.Withdrawals storage withdrawals = staker.withdrawals; - count = (count == 0 || count > withdrawals.length()) - ? withdrawals.length() - : count; - - while (count > 0) { - Withdrawal storage withdrawal = withdrawals.front(); - if (withdrawal.startedAt + withdrawalPeriod() <= block.timestamp) { - releasedAmount += withdrawal.amount; - withdrawals.popFront(); - } else { - // Thanks to the invariant on `withdrawals`, we know the elements are ordered by `startedAt`, so we can - // break early when we encounter any withdrawal that isn't ready to be released yet. - break; - } - count -= 1; - } - - (bool sent, ) = msg.sender.call{value: releasedAmount}(""); - require(sent, "failed to send"); - } -} diff --git a/src/LiquidDelegationV2.sol b/src/LiquidDelegationV2.sol index cb89ab5..910d0ae 100644 --- a/src/LiquidDelegationV2.sol +++ b/src/LiquidDelegationV2.sol @@ -192,7 +192,6 @@ contract LiquidDelegationV2 is BaseDelegation, ILiquidDelegation { emit Claimed(_msgSender(), total, ""); } - //TODO: make it onlyOwnerOrContract and call it every time someone stakes, unstakes or claims? function stakeRewards() public onlyOwner { // before the balance changes deduct the commission from the yet untaxed rewards taxRewards(); diff --git a/src/NonLiquidDelegationV2.sol b/src/NonLiquidDelegationV2.sol index 3cd0e6c..6d3a3da 100644 --- a/src/NonLiquidDelegationV2.sol +++ b/src/NonLiquidDelegationV2.sol @@ -37,10 +37,13 @@ contract NonLiquidDelegationV2 is BaseDelegation, INonLiquidDelegation { // respective staker that can be fully/partially // transferred to the staker mapping(address => uint256) allWithdrawnRewards; - // the last staking index up to which the rewards + // the last staking nextStakingIndex up to which the rewards // of the respective staker have been calculated // and added to allWithdrawnRewards - mapping(address => uint64) lastWithdrawnRewardIndex; + mapping(address => uint64) lastWithdrawnStakingIndex; + // the amount that has already been withdrawn from the + // constantly growing rewards accrued since the last staking + mapping(address => uint256) withdrawnAfterLastStaking; // balance of the reward address minus the // rewards accrued since the last staking int256 totalRewards; @@ -91,13 +94,15 @@ contract NonLiquidDelegationV2 is BaseDelegation, INonLiquidDelegation { uint64[] memory stakingIndices, uint64 firstStakingIndex, uint256 allWithdrawnRewards, - uint64 lastWithdrawnRewardIndex + uint64 lastWithdrawnStakingIndex, + uint256 withdrawnAfterLastStaking ) { NonLiquidDelegationStorage storage $ = _getNonLiquidDelegationStorage(); stakingIndices = $.stakingIndices[_msgSender()]; firstStakingIndex = $.firstStakingIndex[_msgSender()]; allWithdrawnRewards = $.allWithdrawnRewards[_msgSender()]; - lastWithdrawnRewardIndex = $.lastWithdrawnRewardIndex[_msgSender()]; + lastWithdrawnStakingIndex = $.lastWithdrawnStakingIndex[_msgSender()]; + withdrawnAfterLastStaking = $.withdrawnAfterLastStaking[_msgSender()]; } function getDelegatedStake() public view returns(uint256 result) { @@ -196,14 +201,16 @@ contract NonLiquidDelegationV2 is BaseDelegation, INonLiquidDelegation { function rewards(uint64 additionalSteps) public view returns(uint256) { NonLiquidDelegationStorage storage $ = _getNonLiquidDelegationStorage(); - (uint256 result, , ) = _rewards(additionalSteps); - return result - result * getCommissionNumerator() / DENOMINATOR + $.allWithdrawnRewards[_msgSender()]; + (uint256 resultInTotal, , , ) = _rewards(additionalSteps); + resultInTotal -= $.withdrawnAfterLastStaking[_msgSender()]; + return resultInTotal - resultInTotal * getCommissionNumerator() / DENOMINATOR + $.allWithdrawnRewards[_msgSender()]; } function rewards() public view returns(uint256) { NonLiquidDelegationStorage storage $ = _getNonLiquidDelegationStorage(); - (uint256 result, , ) = _rewards(); - return result - result * getCommissionNumerator() / DENOMINATOR + $.allWithdrawnRewards[_msgSender()]; + (uint256 resultInTotal, , , ) = _rewards(); + resultInTotal -= $.withdrawnAfterLastStaking[_msgSender()]; + return resultInTotal - resultInTotal * getCommissionNumerator() / DENOMINATOR + $.allWithdrawnRewards[_msgSender()]; } function taxRewards(uint256 untaxedRewards) internal returns (uint256) { @@ -239,16 +246,28 @@ contract NonLiquidDelegationV2 is BaseDelegation, INonLiquidDelegation { // are only withdrawn from the first staking from which they have not been withdrawn yet function withdrawRewards(uint256 amount, uint64 additionalSteps) public whenNotPaused returns(uint256) { NonLiquidDelegationStorage storage $ = _getNonLiquidDelegationStorage(); - (uint256 result, uint64 i, uint64 index) = additionalSteps == type(uint64).max ? + ( + uint256 resultInTotal, + uint256 resultAfterLastStaking, + uint64 posInStakingIndices, + uint64 nextStakingIndex + ) = additionalSteps == type(uint64).max ? _rewards() : _rewards(additionalSteps); // the caller has not delegated any stake - if (index == 0) + if (nextStakingIndex == 0) return 0; - uint256 taxedRewards = taxRewards(result); + // store the rewards accrued since the last staking (`resultAfterLastStaking`) + // in order to know next time how much the caller has already withdrawn, and + // reduce the current withdrawal (`resultInTotal`) by the amount that was stored + // last time (`withdrawnAfterLastStaking`) - this is essential because the reward + // amount since the last staking is growing all the time, but only the delta accrued + // since the last withdrawal shall be taken into account in the current withdrawal + ($.withdrawnAfterLastStaking[_msgSender()], resultInTotal) = (resultAfterLastStaking, resultInTotal - $.withdrawnAfterLastStaking[_msgSender()]); + uint256 taxedRewards = taxRewards(resultInTotal); $.allWithdrawnRewards[_msgSender()] += taxedRewards; - $.firstStakingIndex[_msgSender()] = i; - $.lastWithdrawnRewardIndex[_msgSender()] = index - 1; + $.firstStakingIndex[_msgSender()] = posInStakingIndices; + $.lastWithdrawnStakingIndex[_msgSender()] = nextStakingIndex - 1; if (amount == type(uint256).max) amount = $.allWithdrawnRewards[_msgSender()]; require(amount <= $.allWithdrawnRewards[_msgSender()], "can not withdraw more than accrued"); @@ -257,53 +276,72 @@ contract NonLiquidDelegationV2 is BaseDelegation, INonLiquidDelegation { (bool success, ) = _msgSender().call{value: amount}(""); require(success, "transfer of rewards failed"); emit RewardPaid(_msgSender(), amount); - //TODO: shouldn't we return amount instead? return taxedRewards; } - function _rewards() internal view returns(uint256 result, uint64 i, uint64 index) { + function _rewards() internal view returns ( + uint256 resultInTotal, + uint256 resultAfterLastStaking, + uint64 posInStakingIndices, + uint64 nextStakingIndex + ) { return _rewards(type(uint64).max); } - function _rewards(uint64 additionalSteps) internal view returns(uint256 result, uint64 i, uint64 index) { + function _rewards(uint64 additionalSteps) internal view returns ( + uint256 resultInTotal, + uint256 resultAfterLastStaking, + uint64 posInStakingIndices, + uint64 nextStakingIndex + ) { NonLiquidDelegationStorage storage $ = _getNonLiquidDelegationStorage(); - uint64 firstIndex; - for (i = $.firstStakingIndex[_msgSender()]; i < $.stakingIndices[_msgSender()].length; i++) { - index = $.stakingIndices[_msgSender()][i]; - uint256 amount = $.stakings[index].amount; - if (index < $.lastWithdrawnRewardIndex[_msgSender()]) - index = $.lastWithdrawnRewardIndex[_msgSender()]; - uint256 total = $.stakings[index].total; - index++; - if (firstIndex == 0) - firstIndex = index; - while (i == $.stakingIndices[_msgSender()].length - 1 ? index < $.stakings.length : index <= $.stakingIndices[_msgSender()][i+1]) { + uint64 firstStakingIndex; + for ( + posInStakingIndices = $.firstStakingIndex[_msgSender()]; + posInStakingIndices < $.stakingIndices[_msgSender()].length; + posInStakingIndices++ + ) { + nextStakingIndex = $.stakingIndices[_msgSender()][posInStakingIndices]; + uint256 amount = $.stakings[nextStakingIndex].amount; + if (nextStakingIndex < $.lastWithdrawnStakingIndex[_msgSender()]) + nextStakingIndex = $.lastWithdrawnStakingIndex[_msgSender()]; + uint256 total = $.stakings[nextStakingIndex].total; + nextStakingIndex++; + if (firstStakingIndex == 0) + firstStakingIndex = nextStakingIndex; + while ( + posInStakingIndices == $.stakingIndices[_msgSender()].length - 1 ? + nextStakingIndex < $.stakings.length : + nextStakingIndex <= $.stakingIndices[_msgSender()][posInStakingIndices+1] + ) { if (total > 0) - result += $.stakings[index].rewards * amount / total; - total = $.stakings[index].total; - index++; - if (index - firstIndex > additionalSteps) - return (result, i, index); + resultInTotal += $.stakings[nextStakingIndex].rewards * amount / total; + total = $.stakings[nextStakingIndex].total; + nextStakingIndex++; + if (nextStakingIndex - firstStakingIndex > additionalSteps) + return (resultInTotal, resultAfterLastStaking, posInStakingIndices, nextStakingIndex); } // all rewards recorded in the stakings were taken into account - if (index == $.stakings.length) { - // ensure that the next time we call withdrawRewards() the last index + if (nextStakingIndex == $.stakings.length) { + // ensure that the next time we call withdrawRewards() the last nextStakingIndex // representing the rewards accrued since the last staking are not // included in the result any more - however, what if there have - // been no stakings i.e. the last index remains the same, but there + // been no stakings i.e. the last nextStakingIndex remains the same, but there // have been additional rewards - how can we determine the amount of // rewards added since we called withdrawRewards() last time? - // index++; + // nextStakingIndex++; // the last step is to add the rewards accrued since the last staking - if (total > 0) - result += (int256(getRewards()) - $.totalRewards).toUint256() * amount / total; + if (total > 0) { + resultAfterLastStaking = (int256(getRewards()) - $.totalRewards).toUint256() * amount / total; + resultInTotal += resultAfterLastStaking; + } } } - // ensure that the next time the function is called the initial value of i refers - // to the last amount and total among the stakingIndices of the staker that already + // ensure that the next time the function is called the initial value of posInStakingIndices + // refers to the last amount and total among the stakingIndices of the staker that already // existed during the current call of the function so that we can continue from there - if (i > 0) - i--; + if (posInStakingIndices > 0) + posInStakingIndices--; } function collectCommission() public override {} diff --git a/test/LiquidDelegation.t.sol b/test/LiquidDelegation.t.sol index 147f8ce..8b05133 100644 --- a/test/LiquidDelegation.t.sol +++ b/test/LiquidDelegation.t.sol @@ -6,12 +6,18 @@ import {LiquidDelegationV2} from "src/LiquidDelegationV2.sol"; import {NonRebasingLST} from "src/NonRebasingLST.sol"; import {WithdrawalQueue} from "src/BaseDelegation.sol"; import {Delegation} from "src/Delegation.sol"; -import {Deposit, InitialStaker} from "src/Deposit.sol"; +import {Deposit, InitialStaker} from "@zilliqa/zq2/deposit.sol"; import {Console} from "src/Console.sol"; import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {Test, Vm} from "forge-std/Test.sol"; import "forge-std/console.sol"; +contract PopVerifyPrecompile { + function popVerify(bytes memory, bytes memory) public pure returns(bool) { + return true; + } +} + contract LiquidDelegationTest is Test { address payable proxy; LiquidDelegationV2 delegation; @@ -118,6 +124,10 @@ contract LiquidDelegationTest is Test { console.log("Deposit.maximumStakers() =", Deposit(delegation.DEPOSIT_CONTRACT()).maximumStakers()); console.log("Deposit.blocksPerEpoch() =", Deposit(delegation.DEPOSIT_CONTRACT()).blocksPerEpoch()); //*/ + + vm.etch(address(0x5a494c80), address(new PopVerifyPrecompile()).code); + + vm.stopPrank(); } function run( diff --git a/test/NonLiquidDelegation.t.sol b/test/NonLiquidDelegation.t.sol index ee0a185..6a5282f 100644 --- a/test/NonLiquidDelegation.t.sol +++ b/test/NonLiquidDelegation.t.sol @@ -5,13 +5,19 @@ import {NonLiquidDelegation} from "src/NonLiquidDelegation.sol"; import {NonLiquidDelegationV2} from "src/NonLiquidDelegationV2.sol"; import {WithdrawalQueue} from "src/BaseDelegation.sol"; import {Delegation} from "src/Delegation.sol"; -import {Deposit, InitialStaker} from "src/Deposit.sol"; +import {Deposit, InitialStaker} from "@zilliqa/zq2/deposit.sol"; import {Console} from "src/Console.sol"; import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {Test, Vm} from "forge-std/Test.sol"; import "forge-std/console.sol"; +contract PopVerifyPrecompile { + function popVerify(bytes memory, bytes memory) public pure returns(bool) { + return true; + } +} + contract NonLiquidDelegationTest is Test { address payable proxy; address owner; @@ -122,6 +128,9 @@ contract NonLiquidDelegationTest is Test { console.log("Deposit.maximumStakers() =", Deposit(delegation.DEPOSIT_CONTRACT()).maximumStakers()); console.log("Deposit.blocksPerEpoch() =", Deposit(delegation.DEPOSIT_CONTRACT()).blocksPerEpoch()); //*/ + + vm.etch(address(0x5a494c80), address(new PopVerifyPrecompile()).code); + vm.stopPrank(); } @@ -217,9 +226,16 @@ contract NonLiquidDelegationTest is Test { s = string.concat(s, "0.0%\t\t"); console.log(s); } - (uint64[] memory stakingIndices, uint64 firstStakingIndex, uint256 allWithdrawnRewards, uint64 lastWithdrawnRewardIndex) = delegation.getStakingData(); - Console.log("stakingIndices: %s", stakingIndices); - console.log("firstStakingIndex: %s lastWithdrawnRewardIndex: %s allWithdrawnRewards: %s", firstStakingIndex, lastWithdrawnRewardIndex, allWithdrawnRewards); + ( + uint64[] memory stakingIndices, + uint64 firstStakingIndex, + uint256 allWithdrawnRewards, + uint64 lastWithdrawnRewardIndex, + uint256 withdrawnAfterLastStaking + ) = delegation.getStakingData(); + Console.log("stakingIndices = [ %s]", stakingIndices); + console.log("firstStakingIndex = %s lastWithdrawnRewardIndex = %s", uint(firstStakingIndex), uint(lastWithdrawnRewardIndex)); + console.log("allWithdrawnRewards = %s withdrawnAfterLastStaking = %s", allWithdrawnRewards, withdrawnAfterLastStaking); } //TODO: add assertions @@ -673,10 +689,12 @@ contract NonLiquidDelegationTest is Test { uint64[] memory stakingIndices, uint64 firstStakingIndex, uint256 allWithdrawnRewards, - uint64 lastWithdrawnRewardIndex + uint64 lastWithdrawnRewardIndex, + uint256 withdrawnAfterLastStaking ) = delegation.getStakingData(); Console.log("stakingIndices = [ %s]", stakingIndices); - console.log("firstStakingIndex = %s allWithdrawnRewards = %s lastWithdrawnRewardIndex = %s", uint(firstStakingIndex), allWithdrawnRewards, uint(lastWithdrawnRewardIndex)); + console.log("firstStakingIndex = %s lastWithdrawnRewardIndex = %s", uint(firstStakingIndex), uint(lastWithdrawnRewardIndex)); + console.log("allWithdrawnRewards = %s withdrawnAfterLastStaking = %s", allWithdrawnRewards, withdrawnAfterLastStaking); vm.recordLogs(); vm.expectEmit( @@ -696,10 +714,12 @@ contract NonLiquidDelegationTest is Test { stakingIndices, firstStakingIndex, allWithdrawnRewards, - lastWithdrawnRewardIndex + lastWithdrawnRewardIndex, + withdrawnAfterLastStaking ) = delegation.getStakingData(); Console.log("stakingIndices = [ %s]", stakingIndices); - console.log("firstStakingIndex = %s allWithdrawnRewards = %s lastWithdrawnRewardIndex = %s", uint(firstStakingIndex), allWithdrawnRewards, uint(lastWithdrawnRewardIndex)); + console.log("firstStakingIndex = %s lastWithdrawnRewardIndex = %s", uint(firstStakingIndex), uint(lastWithdrawnRewardIndex)); + console.log("allWithdrawnRewards = %s withdrawnAfterLastStaking = %s", allWithdrawnRewards, withdrawnAfterLastStaking); Console.log("contract balance: %s.%s%s", address(delegation).balance); Console.log("staker balance: %s.%s%s", staker[i-1].balance);