-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests for EscrowState library #92
Conversation
7ba81ea
to
a1dc2c7
Compare
// initialize() | ||
// --- | ||
|
||
function testFuzz_initialize_happyPath(uint32 minAssetsLockSeconds) external { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Duration
type may be used as a parameter to avoid later casting from uint32
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
EscrowState.initialize(_context, minAssetsLockDuration); | ||
|
||
checkContext(State.SignallingEscrow, minAssetsLockDuration, D0, D0, T0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use named parameters for the better readability:
checkContext({
state: State.SignallingEscrow,
minAssetsLockDuration: minAssetsLockDuration,
rageQuitExtensionDelay: D0,
rageQuitWithdrawalsTimelock: D0,
rageQuitExtensionDelayStartedAt: T0
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
_context.rageQuitExtensionDelay = rageQuitExtensionDelay; | ||
_context.rageQuitWithdrawalsTimelock = rageQuitWithdrawalsTimelock; | ||
|
||
vm.warp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UnitTest._wait()
method may be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that it will make the test easier to understand, take a look pls at these lines
https://github.com/lidofinance/dual-governance/pull/92/files#diff-94006bb4d7eaa8960b0a0a1fd4de0d02c0d50140ca8857d0ba68052b5aeee11fR229-R234
However I replaced all vm.warp()
calls with _wait()
as you requested.
{ | ||
_context.rageQuitExtensionDelayStartedAt = Timestamps.from(rageQuitExtensionDelayStartedAtSeconds); | ||
bool res = EscrowState.isRageQuitExtensionDelayStarted(_context); | ||
assert(res == _context.rageQuitExtensionDelayStartedAt.isNotZero()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use Solidity's native assert()
method in tests. Instead, use Foundry's assertEq()
, assertTrue()
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
a1dc2c7
to
3260959
Compare
106a529
to
8c2f6e3
Compare
8c2f6e3
to
3b9eb9d
Compare
No description provided.