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.

Sign up for GitHub

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

Jump to bottom

test: fuzz segments #315

Merged
merged 7 commits into from
Feb 10, 2023
Merged

test: fuzz segments #315

merged 7 commits into from
Feb 10, 2023

Conversation

PaulRBerg
Copy link
Member

@PaulRBerg PaulRBerg commented Feb 4, 2023 •
edited
Loading

Description

Closes #256 and #326.

Fuzzing the segment amounts and the exponents was easy, as there are no ordering rules that apply in their case. However, deltas and milestones were trickier.

What I did was to generate the milestones using the solidity-generators library, specifically using the arange function, and then I bounded each milestone to get a more uneven distribution.

Whereas the deltas I generated by iterating over the fuzzed segments and bounding each segment milestone (you read that right, I used the fuzzed segment milestones as the source of entropy for the bounded deltas).

@PaulRBerg PaulRBerg force-pushed the prb/fuzz-segments branch 2 times, most recently from 52254ed to 1f6c21c Compare February 4, 2023 20:03
@PaulRBerg PaulRBerg force-pushed the main branch 2 times, most recently from 1968f5a to 82c69c2 Compare February 6, 2023 19:41
@PaulRBerg PaulRBerg force-pushed the prb/fuzz-segments branch 2 times, most recently from 100c7b9 to ddec70f Compare February 7, 2023 20:46
@PaulRBerg
test: fuzz segments and deltas
0ed2b96 
chore: improve wording in code comments
chore: say "has been" instead of "was"
chore: say "have been" instead of "were"
test: load sender from "users" struct
@PaulRBerg
refactor: rename helper function
2affa24 
refactor: rename "checkDeltasAndAdjustSegments" to "checkDeltasAndCalculateMilestones"
@PaulRBerg
test: dryify e2e user assumptions checks
6286e65 
test: delete duplicate range assumptions check
test: rename "E2eTest" to "E2e_Test"
@PaulRBerg
test: fix "createWithDeltas" fuzz tests
8e4ba0e
@sablier-labs sablier-labs deleted a comment from codecov bot Feb 9, 2023
andreivladbrg
andreivladbrg reviewed Feb 9, 2023
View reviewed changes
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

Is there any reason why you chose this step?
https://github.com/sablierhq/v2-core/blob/8e4ba0e2b1cc3e8b32498b2aa9cf549d2462b6ea/test/shared/helpers/Calculations.t.sol#L196

@andreivladbrg
test: add one more param in "checkUsers"
76fb8f8 
test: order correctly the functions in "Calculations"
test: move assume cheatcodes from test function to "checkUsers"
test: remove unnecessary inherits in "Base_Test"
chore: remove unused imports
@PaulRBerg
Copy link
Member Author

Is there any reason why you chose this step?

Assuming that by "step" you meant segmentCount - 1, that's what arange uses under the hood (arange calls linspace):

https://github.com/mds1/solidity-generators/blob/60e485e0076ccf62d69ef79e1e2aeaba1fee3ef5/src/Generators.sol#L48

@andreivladbrg
Copy link
Member

andreivladbrg commented Feb 9, 2023 •
edited
Loading

uint step = num == 1 ? 0 : size / (num - 1);

Ok, in this case, they are not really "fuzzed". Only the startTime is fuzzed, and the segments length. It's still better than nothing 🫡 .

@PaulRBerg
Copy link
Member Author

Actually, the milestones are fuzzed:

  1. The step value depends upon the segmentCount, which it is fuzzed (up to Foundry's max array len 256).
  2. Each milestone is bounded between it and milestone - step / 2 and milestone + step / 2.

@sablier-labs sablier-labs deleted a comment from codecov bot Feb 9, 2023
@PaulRBerg
Copy link
Member Author

But even if the milestones had not been fuzzed (which is not the case), there would still be value in fuzzing the segment amounts, exponents, and the lengths of the segments array itself.

@PaulRBerg
test: run "vm.assume" before "bound"
d4c5fc3 
test: warp closer to the start of the test functions
@PaulRBerg PaulRBerg mentioned this pull request Feb 9, 2023
Closed
@PaulRBerg PaulRBerg force-pushed the prb/fuzz-segments branch 2 times, most recently from cc4f1e3 to 38c735e Compare February 9, 2023 18:56
@PaulRBerg
chore: improve wording in code comments
c37fc5f 
test: add "fuzzSegmentAmountsAndCalculateCreateAmounts" with defaults
test: fuzz segments in pro withdraw tests
test: fix names of shared withdraw test contracts
test: warp in "current milestone 1st" test
@PaulRBerg
Copy link
Member Author

@andreivladbrg besides the point above (which I said I will fix in #322), is this PR good to merge?

@andreivladbrg
Copy link
Member

Yes

@PaulRBerg
Copy link
Member Author

Awesome, thanks.

@PaulRBerg PaulRBerg merged commit 0893a99 into main Feb 10, 2023
@PaulRBerg PaulRBerg deleted the prb/fuzz-segments branch February 10, 2023 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreivladbrg andreivladbrg andreivladbrg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fuzz segments in pro tests
2 participants
@PaulRBerg @andreivladbrg