-
Notifications
You must be signed in to change notification settings - Fork 45
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
Prevent setting end time in the past #396
Conversation
fe70acd
to
c92a945
Compare
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.
Feedback below.
test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol
Outdated
Show resolved
Hide resolved
test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
c92a945
to
82e0095
Compare
I have improved the wording in my last commit. Can you please confirm that everything is good now? |
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 approved the changes because I have already resolved the comments with my latest commits.
But please have a look at all comments, @andreivladbrg.
the warp cheatcode doesn't work for the invariant tests
You are right. What we had to was to bound the start time and the cliff time like this:
uint40 currentTime = getBlockTimestamp();
params.broker.fee = bound(params.broker.fee, 0, DEFAULT_MAX_FEE);
params.range.start = boundUint40(params.range.start, 0, currentTime);
params.range.cliff = boundUint40(params.range.cliff, currentTime, currentTime + 52 weeks);
params.range.end = boundUint40(params.range.end, params.range.cliff + 1, MAX_UNIX_TIMESTAMP);
Another pre-requisite to make this work was to increase the default block.timestamp
used in our tests. I ended up setting it to March 1, 2023:
DEFAULT_START_TIME = uint40(1_677_632_400);
vm.warp({ timestamp: DEFAULT_START_TIME });
This proved to be a very good decision, since it has led me to identify several minor issues in the tests (e.g. using DEFAULT_CLIFF_TIME
instead of DEFAULT_CLIFF_DURATION
in the bounding of timeWarp
).
In addition, setting the block timestamp to a value in the present creates a more realistic testing environment (as I have argued here).
test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol
Outdated
Show resolved
Hide resolved
test/unit/lockup/linear/create-with-range/createWithRange.t.sol
Outdated
Show resolved
Hide resolved
test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol
Show resolved
Hide resolved
test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol
Outdated
Show resolved
Hide resolved
test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol
Show resolved
Hide resolved
test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol
Show resolved
Hide resolved
Good catch
TBH, I don't have an opinion here . Feel free to proceed as you see want. |
9fd0ff8
to
9f874e4
Compare
docs: specify that the end time can't be in the past test: when the current time is not less than end time
test: ensure the last milestone is greater than the current time in fuzzSegments test: bound end time between current time + 1 and MAX_UNIX_TIMESTAMP
chore: improve wording in explanatory comments test: improve naming for tests test: remove unnecessary casting
chore: fix explanations in comments test: use named arguments for "vm.warp"
test: add "MAX_SEGMENT_DURATION" test: delete stale comments about fuzzing scenarios test: improve "fuzzSegmentMilestones" test: fix "min" for cliff duration bounds test: simplify bounding for end time
There is an invariant test run that failed, but it is unrelated to this PR (see my explanation here). |
4b575a4
to
2311ea5
Compare
050677c
to
ef43dd1
Compare
Addresses https://github.com/cantinasec/sablier/issues/7