Escrow unit tests improvements #241
Conversation
sandstone-ag
force-pushed
the
feature/escrow-contract-unit-tests-3
branch
from
73785a2 to
4ac6af1
Compare
| assertEq(escrowBalanceAfter, escrowBalanceBefore + amount); | ||
| assertApproxEqAbs( | ||
| vetoerBalanceAfter, | ||
| _stETH.getPooledEthByShares(_stETH.getSharesByPooledEth(vetoerBalanceBefore) - sharesAmount), |
There was a problem hiding this comment.
Additional conversions between stETH and shares may introduce extra rounding errors. In this case, it should be enough to compare: assertEq(vetoerBalanceAfter, vetoerBalanceBefore - amount);. Then, the accuracy will always fit the 2 wei for the rebase factor used in the test.
| ); | ||
| vm.expectCall(address(_dualGovernance), abi.encodeCall(IDualGovernance.activateNextState, ()), 2); | ||
|
|
||
| vm.prank(_vetoer); | ||
| uint256 lockedStETHShares = _escrow.lockStETH(amount); | ||
|
|
||
| assertEq(lockedStETHShares, sharesAmount); | ||
| assertApproxEqAbs(lockedStETHShares, sharesAmount, ACCURACY); |
There was a problem hiding this comment.
It's better to compare shares strictly here. When stETH is transferred using transferShares(), the expected and actually transferred shares amount should match exactly.
| assertEq(state.unstETHIdsCount, 2); | ||
| assertEq(state.stETHLockedShares.toUint256(), 0); | ||
| assertApproxEqAbs(state.unstETHLockedShares.toUint256(), unstethShares1 + unstethShares2, 2 * ACCURACY); | ||
| assertEq( |
There was a problem hiding this comment.
This assertion should not be strict, as in the previous check, as we assume that unstethShares1 and usntethShares2 may differ from the actual values. However, strict assertions will fail only on tiny withdrawal requests (dozens of WEIs).
There was a problem hiding this comment.
Or the method _vetoerLockedUnstEth should also return the actual amount of locked shares to use them together with the strict assertions
|
|
||
| _withdrawalQueue.setClaimableEtherResult(responses); | ||
| vm.expectCall( | ||
| address(_withdrawalQueue), abi.encodeCall(IWithdrawalQueue.getClaimableEther, (unstethIds, hints)) | ||
| ); | ||
|
|
||
| assertTrue(_escrow.getEscrowState() == EscrowState.SignallingEscrow); | ||
| assertEq(_escrow.getRageQuitSupport().toUint256(), 0); | ||
|
|
||
| _escrow.markUnstETHFinalized(unstethIds, hints); |
There was a problem hiding this comment.
It would also be nice to test events here, as it should contain the correct number of finalized shares
| assertEq(signallingEscrowDetails.totalStETHLockedShares.toUint256(), stethAmount); | ||
| assertEq(signallingEscrowDetails.totalStETHClaimedETH.toUint256(), stethAmount); | ||
| assertEq(signallingEscrowDetails.totalStETHLockedShares.toUint256(), _stETH.getSharesByPooledEth(ethAmount)); | ||
| assertEq(signallingEscrowDetails.totalStETHClaimedETH.toUint256(), ethAmount); |
There was a problem hiding this comment.
Also, it would be nice to check that after the withdrawal of ETH, other attempts of the vetoer to withdraw ETH will fail
| @@ -1108,29 +1273,32 @@ contract EscrowUnitTests is UnitTest { | |||
| } | |||
|
|
|||
| function test_withdrawETH_RevertOn_InvalidSharesValue() external { | |||
There was a problem hiding this comment.
It may be worth changing the name of the test to make it more explicit which case is tested, for example, test_withdrawETH_RevertOn_ZeroSharesWithdrawAttempt()
| @@ -1240,8 +1412,8 @@ contract EscrowUnitTests is UnitTest { | |||
| function test_withdrawETH_2_RevertOn_InvalidUnstETHStatus() external { | |||
There was a problem hiding this comment.
Maybe more descriptive to title it test_withdrawETH_2_RevertOn_UnstETHNotClaimed
| @@ -47,9 +53,15 @@ contract EscrowUnitTests is UnitTest { | |||
| Duration private _minLockAssetDuration = Durations.from(1 days); | |||
| Duration private _maxMinAssetsLockDuration = Durations.from(100 days); | |||
| uint256 private stethAmount = 100 ether; | |||
There was a problem hiding this comment.
The name stethSharesAmount seems more appropriate, as it’s referred to as shares in the tests
No description provided.