Escrow contract unit tests - 2 #176
Conversation
| return; | ||
| } | ||
|
|
||
| sendTo(ETHValues.from(_claimableAmount), payable(msg.sender)); |
There was a problem hiding this comment.
sendTo is available globally for the EthValue type, allowing it to be called as ETHValues.from(_claimableAmount).sendTo(payable(msg.sender))
| @@ -380,6 +424,13 @@ contract WithdrawalsBatchesQueueTest is UnitTest { | |||
| } | |||
| } | |||
|
|
|||
| function test_calcRequestAmounts_RevertOn_DivisionByZero() external { | |||
There was a problem hiding this comment.
It's better to rename the test to show the revert reason, for example: test_calcRequestAmounts_RevertOn_MaxRequestAmountIsZero()
| ) internal returns (Escrow) { | ||
| Escrow instance = createEscrowProxy(minWithdrawalsBatchSize); | ||
|
|
||
| vm.startPrank(_dualGovernance); |
There was a problem hiding this comment.
For one-time "pranking" vm.prank() method is more convenient
| // helper methods | ||
| // --- | ||
|
|
||
| function createEscrow(uint256 size) internal returns (Escrow) { |
There was a problem hiding this comment.
Internal methods should start from the _ prefix
| _random = Random.create(block.timestamp); | ||
|
|
||
| _stETH = new StETHMock(); | ||
| _stETH.__setShareRate(1); |
There was a problem hiding this comment.
It would be nice if Escrow tests used a share rate other than 1. However, this would require changes in the current StETHMock() contract.
| assertEq(escrowBalanceAfter, escrowBalanceBefore + amount); | ||
| } | ||
|
|
||
| function test_lockStETH_RevertOn_UnexpectedEscrowState() external { |
There was a problem hiding this comment.
Also, it would be nice to check that the implementation of the Escrow contract can't be used to lock tokens.
| uint256 vetoerBalanceAfter = ERC20Mock(address(_wstETH)).balanceOf(_vetoer); | ||
| uint256 escrowBalanceAfter = ERC20Mock(address(_wstETH)).balanceOf(address(_escrow)); | ||
|
|
||
| assertEq(vetoerBalanceAfter, vetoerBalanceBefore - amount); |
There was a problem hiding this comment.
For the wstETH, there also may be rounding errors, as the wstETH is unwrapped to stETH in the lockWstETH() method
| uint256 escrowBalanceAfter = ERC20Mock(address(_stETH)).balanceOf(address(_escrow)); | ||
|
|
||
| assertEq(vetoerBalanceAfter, vetoerBalanceBefore - amount); | ||
| assertEq(escrowBalanceAfter, escrowBalanceBefore + amount); |
There was a problem hiding this comment.
Shouldn't there also be checks that the state of the Escrow contract changed expectedly (getVetoerState() and getLockedAssetsTotals())
| function test_unlockWstETH_HappyPath() external { | ||
| uint256 amount = 1 ether; | ||
|
|
||
| vm.mockCall(address(_wstETH), abi.encodeWithSelector(IWstETH.wrap.selector), abi.encode(amount)); |
There was a problem hiding this comment.
Consider updating the tests to exclusively use vm.mockCall() (which appears to be the preferred approach, as it simplifies simulating stETH rounding errors) or to exclusively mock contracts. Combining both methods makes the tests more complex and harder to follow.
| vm.expectCall(address(_dualGovernance), abi.encodeWithSelector(IDualGovernance.activateNextState.selector)); | ||
|
|
||
| vm.prank(_vetoer); | ||
| _escrow.lockStETH(amount); |
There was a problem hiding this comment.
The returning value check is missing
No description provided.