Add mock contracts for StEth, WstEth, WithdrawalQueue for testing the DG setup on Holesky #143
Conversation
… setup on Holesky
sandstone-ag
force-pushed
the
feature/lido-contracts-mock
branch
from
96f8a8c
to
6aaef31
Compare
scripts/lido-mocks/StETHMock.sol
Outdated
| import {PercentD16, HUNDRED_PERCENT_D16} from "contracts/types/PercentD16.sol"; | ||
|
|
||
| /* solhint-disable no-unused-vars,custom-errors */ | ||
| contract StETHMock is ERC20, IStETH { |
There was a problem hiding this comment.
I propose using the StETH contract as the base for the mock. It’s straightforward, with minimal dependencies, and migrating from version 0.4.24 shouldn’t be difficult. This approach will also add clarity to the implementation. The current mock is somewhat misleading because it stores share information in the balances mapping.
| address private deployer; | ||
|
|
||
| function run() external { | ||
| if (17000 != block.chainid) { |
There was a problem hiding this comment.
Is it important which chain the mocks are deployed on? It seems they aren’t tied to any specific contracts on a particular chain.
There was a problem hiding this comment.
This script and contracts are not intended for use on mainnet, but it can be used on any testnet if needed. So I changed the check and error message.
| /// | ||
|
|
||
| IStETH public ST_ETH; | ||
| address payable private refundAddress; |
There was a problem hiding this comment.
Private and internal variables and methods of the contracts (in libraries only private) should use an underscore (_) prefix, as recommended in the Solidity style guide.
scripts/lido-mocks/StETHMock.sol
Outdated
| } | ||
|
|
||
| function rebaseTotalPooledEther(PercentD16 rebaseFactor) public { | ||
| totalPooledEther = PercentD16.unwrap(rebaseFactor) * totalPooledEther / HUNDRED_PERCENT_D16; |
There was a problem hiding this comment.
It's better to use PercentD16.toUint256() instead of unwrap()
| return lastCheckpointIndex; | ||
| } | ||
|
|
||
| function requestWithdrawals( |
There was a problem hiding this comment.
To match the original contract, it would be nice also to revert if the contract is paused.
|
|
||
| if (request.claimed) revert RequestAlreadyClaimed(_requestId); | ||
|
|
||
| // NB! This allows claim for anyone! // if (request.owner != msg.sender) revert NotOwner(msg.sender, request.owner); |
There was a problem hiding this comment.
Maybe it's better to preserve the original behavior. Why do we need to remove this check from the mock?
| assert(requestsByOwner[request.owner].remove(_requestId)); | ||
|
|
||
| uint256 ethWithDiscount = _calculateClaimableEther(request, _requestId, _hint); | ||
| lockedEtherAmount = lockedEtherAmount - ethWithDiscount; |
There was a problem hiding this comment.
nitpick: lockedEtherAmount -= ethWithDiscount;
| WithdrawalRequest storage request = queue[_requestId]; | ||
| if (request.claimed) revert RequestAlreadyClaimed(_requestId); | ||
|
|
||
| // NB! This allows transfer for anyone // if (_from != request.owner) revert TransferFromIncorrectOwner(_from, request.owner); |
There was a problem hiding this comment.
Why should transferring to itself be allowed in the mock?
| WithdrawalRequest memory lastFinalizedRequest = queue[lastFinalizedRequestId]; | ||
| WithdrawalRequest memory requestToFinalize = queue[_lastRequestIdToBeFinalized]; | ||
|
|
||
| uint128 stETHToFinalize = requestToFinalize.cumulativeStETH - lastFinalizedRequest.cumulativeStETH; |
There was a problem hiding this comment.
The original contract has a check that _amountOfETH <= stETHToFinalize. Is it safe to remove it?
There was a problem hiding this comment.
Yes, it's safe IMO. This implementation allows to receive an arbitrary amount of ether for finalization, and it allows to claim only previously requested amount of ether. The excessive (not claimable) ether amount stored in the UnsafeWithdrawalQueueMock can be returned to a special refund address using the method refundEth(). I think this logic is much simpler for further usage in manual tests.
scripts/lido-mocks/WstETHMock.sol
Outdated
| stETH = _stETH; | ||
| } | ||
|
|
||
| function mint(address account, uint256 amount) external { |
There was a problem hiding this comment.
burn and mint are not needed for the wstETH token, the wrap and unwrap methods should be used
Add mock contracts for StEth, WstEth, WithdrawalQueue for testing the DG setup on Holesky