-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Add initial fuzz targets #262
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
Conversation
Installed near-sandbox into /home/runner/work/contracts/contracts/target/debug/build/near-sandbox-utils-2ad3b5a56a37a3e2/out/.near/near-sandbox-2.8.0/near-sandbox Gas ReportSnapshot Limits
Estimated snapshot limit: 9635
Estimated snapshot limit: 11505 Action Gas Descriptors
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
peer2f00l
left a comment
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.
Is this something that we should add to our Github workflows?
peer2f00l
left a comment
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.
Ah, I see that the gas report is broken. Just that, I suppose.
| } | ||
|
|
||
| // Test different operations based on selector | ||
| match op_selector % 8 { |
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 would consider making these their own tests and fuzzing the inputs for each one, in this case we're watering down potential fuzz paths
| let _ = dec_a.fractional_part_as_u128_dividend(); | ||
|
|
||
| // Test edge cases based on operation selector | ||
| match op_selector % 10 { |
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.
Same as above for these switchlike statements
| for precision in [0, 1, 5, 10, 20, 38] { | ||
| let fixed = decimal.to_fixed(precision); | ||
| let _ = Decimal::from_str(&fixed); | ||
| } |
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 would fuzz everything up to a known fixed decimal here
| let edge_cases = [ | ||
| "0", | ||
| "1", | ||
| "0.0", | ||
| "1.0", | ||
| "0.1", | ||
| "0.00000001", | ||
| "999999999999999", | ||
| ]; |
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 think also here - fuzzing inputs should be the edge cases and countertests for them
| // Test malformed strings don't panic | ||
| let malformed = [ | ||
| ".", ".0", "0.", "abc", "1.2.3", "-1", "1e10", "NaN", "Infinity", "", | ||
| ]; |
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.
Also here, i think probably a few different fuzz tests for these multiple assertions, since finding a negative input would require running the entire suite again
| Decimal::from(x) / Decimal::from(u64::MAX) | ||
| } | ||
|
|
||
| fuzz_target!(|data: (u64, u64, u64, u64, u64, u64, u64)| { |
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.
Ditto here
| // Test 2: Compound interest calculation | ||
| // A = P(1 + r)^n | ||
| // For small rates, use binomial approximation to avoid overflow | ||
| // if rate_bps <= 1000 && periods <= 365 { | ||
| // let rate_per_period = rate_bps as f64 / 10000.0; | ||
| // let compound_multiplier = (1.0 + rate_per_period).powi(periods as i32); | ||
| // | ||
| // if compound_multiplier.is_finite() && compound_multiplier > 0.0 { | ||
| // let result = (principal as f64 * compound_multiplier) as u128; | ||
| // | ||
| // // Invariant: Result should always be >= principal | ||
| // assert!( | ||
| // result >= principal, | ||
| // "Compound interest resulted in less than principal" | ||
| // ); | ||
| // | ||
| // // Invariant: Result shouldn't be absurdly large | ||
| // assert!( | ||
| // result <= principal.saturating_mul(100), | ||
| // "Compound interest grew unreasonably" | ||
| // ); | ||
| // } | ||
| // } |
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.
Is this still needed?
carrion256
left a comment
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 would consider splitting up a bunch of the giant tests into more composable tests - right now if we meet any type of counter pattern from fuzzing we'd need to fuzz a lot to get to our desired state.
|
@carrion256 Thanks for the thorough review! I'll update this over the weekend to allow for more granular fuzzing and a faster iteration cycle. |
24c7c36 to
17134a2
Compare
17134a2 to
9d79b03
Compare
|
All tests pass locally; must be an issue with the runner again. |
This PR adds initial support for code fuzzing using
cargo fuzz.Currently the fuzz targets are working off of some assumed flows and should be improved for proper usage.
The limitations of existing fuzzing tooling require us to add some feature flag gating in order to have the code we want to fuzz compile in the fuzz target environment.
Additionally, some fields have been made public, but those could be reverted with preferred usage patterns without polluting our intended public API.
This change is