Skip to content
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

test: use equality in the withdraw multiple test and in invariant #320

Merged
merged 7 commits into from
Oct 25, 2024

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Oct 24, 2024

As we changed the sd to 18 decimals, I discovered that the delay explained in the technical doc no longer holds true. The issue that prevented us from identifying the fix earlier was that these tests were giving false positives due to descaling the ongoing debt back to the token's decimals.

For example, previously, if the withdrawable amount was 1.99 tokens, the recipient would withdraw 1 token, and the snapshot debt would remain 0, causing a delay for the remaining 0.99 tokens. Now, with the snapshot updated to 0.99, thus there is no delay which is proven by the new test in this branch:

function test_Withdraw_NoDelay() external {
// 0.001e6 USDC per day
uint128 rps = 0.000000011574e18;
vm.warp(OCT_1_2024);
uint256 streamId = flow.createAndDeposit(users.sender, users.recipient, ud21x18(rps), usdc, true, 0.001e6);
uint40 initialSnapshotTime = OCT_1_2024;
assertEq(flow.getSnapshotTime(streamId), initialSnapshotTime, "snapshot time");
// Assert that there is still only one token unlocked.
vm.warp(initialSnapshotTime + 172 seconds);
assertEq(flow.withdrawableAmountOf(streamId), 1);
// Warp to the second when the second token gets unlocked.
vm.warp(initialSnapshotTime + 173 seconds);
assertEq(flow.withdrawableAmountOf(streamId), 2);
// Warp one second back in time, to test if there is a delay.
vm.warp(initialSnapshotTime + 172 seconds);
// Withdraw the token.
(uint128 withdrawnAmount,) = flow.withdrawMax(streamId, users.recipient);
assertEq(withdrawnAmount, 1, "withdrawn amount");
// Test the second gets unlocked, which proves there is no delay.
vm.warp(initialSnapshotTime + 173 seconds);
assertEq(withdrawnAmount + flow.withdrawableAmountOf(streamId), 2);
// Warp to a second before the third token gets unlocked.
vm.warp(initialSnapshotTime + 259 seconds);
assertEq(withdrawnAmount + flow.withdrawableAmountOf(streamId), 2);
// Warp to the second when the third token gets unlocked.
vm.warp(initialSnapshotTime + 260 seconds);
assertEq(withdrawnAmount + flow.withdrawableAmountOf(streamId), 3);
}

@andreivladbrg andreivladbrg marked this pull request as draft October 24, 2024 13:41
@andreivladbrg andreivladbrg marked this pull request as ready for review October 24, 2024 16:07
@andreivladbrg andreivladbrg changed the title test: use equality in the withdraw multiple test test: use equality in the withdraw multiple test and in invariant Oct 24, 2024
Copy link
Member

@smol-ninja smol-ninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent observation @andreivladbrg. Left some comments below.

tests/integration/fuzz/withdrawMultiple.t.sol Show resolved Hide resolved
tests/integration/fuzz/withdrawMultiple.t.sol Outdated Show resolved Hide resolved
tests/invariant/Flow.t.sol Outdated Show resolved Hide resolved
tests/invariant/Flow.t.sol Show resolved Hide resolved
tests/integration/fuzz/withdrawMultiple.t.sol Outdated Show resolved Hide resolved
tests/invariant/Flow.t.sol Show resolved Hide resolved
Copy link
Member

@smol-ninja smol-ninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go. I left a comment below in case you want to address it.

tests/integration/fuzz/withdrawMultiple.t.sol Outdated Show resolved Hide resolved
@andreivladbrg andreivladbrg merged commit 6786126 into main Oct 25, 2024
@smol-ninja smol-ninja deleted the test/delay-fix branch October 25, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants