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: Investigate Fuzzing #477

Merged
merged 50 commits into from
Feb 5, 2025
Merged

test: Investigate Fuzzing #477

merged 50 commits into from
Feb 5, 2025

Conversation

0xNeshi
Copy link
Collaborator

@0xNeshi 0xNeshi commented Jan 9, 2025

As per our agreement, this PR includes a basic fuzz tests setup and some missing unit/property tests for openzeppelin-crypto crate.

I created a couple of related issues to address missing property tests (see #501 #502 #503).
Adding property tests for contract will be easily possible after motsu is refactored (see https://github.com/OpenZeppelin/stylus-test-helpers/pull/14/files#r1919897820)

Resolves #458

PR Checklist

  • Tests
  • Documentation
  • Changelog

@0xNeshi 0xNeshi requested a review from ggonzalez94 January 9, 2025 10:09
@0xNeshi 0xNeshi self-assigned this Jan 9, 2025
@0xNeshi 0xNeshi marked this pull request as draft January 9, 2025 10:09
Copy link

netlify bot commented Jan 9, 2025

Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit ee69c65
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/67a39abc8f08e60008bb27b4

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.1%. Comparing base (a283748) to head (ee69c65).
Report is 1 commits behind head on main.

Additional details and impacted files
Files with missing lines Coverage Δ
lib/crypto/src/bits.rs 100.0% <100.0%> (ø)
lib/crypto/src/hash.rs 61.7% <100.0%> (-14.2%) ⬇️
lib/crypto/src/keccak.rs 100.0% <100.0%> (ø)
lib/crypto/src/merkle.rs 97.8% <100.0%> (-0.1%) ⬇️

@0xNeshi 0xNeshi changed the title test: Fuzz Test for openzeppelin_crypto::merkle::Verify test: Investigate Fuzzing Jan 15, 2025
@0xNeshi 0xNeshi marked this pull request as ready for review January 21, 2025 10:49
Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

I like the scenarios, great job @0xNeshi!
Left some comments.

fuzz/.gitignore Outdated Show resolved Hide resolved
fuzz/Cargo.toml Outdated Show resolved Hide resolved
fuzz/Cargo.toml Show resolved Hide resolved
fuzz/Cargo.toml Outdated Show resolved Hide resolved
fuzz/Cargo.toml Show resolved Hide resolved
fuzz/fuzz_targets/openzeppelin_stylus_utils.rs Outdated Show resolved Hide resolved
@0xNeshi 0xNeshi requested a review from bidzyyys January 22, 2025 10:06
@bidzyyys
Copy link
Collaborator

@0xNeshi
Copy link
Collaborator Author

0xNeshi commented Jan 22, 2025

@0xNeshi we should consider fuzzing for this folder -> https://github.com/OpenZeppelin/rust-contracts-stylus/tree/main/contracts/src/utils/math

Definitely 💯 We will need shims for this to work though, I'd suggest we hold off on this for now

Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@qalisander qalisander left a comment

Choose a reason for hiding this comment

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

Good job on fuzzing!
I think we should remember once we write many tests, it can make design changes difficult as well as support.


use super::*;

proptest! {
Copy link
Member

Choose a reason for hiding this comment

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

I see, that we are mostly testing here how keccak is implemented in the other crate (tiny-keccak). That it can produce the same hash, meets consistency and distribution requirement. Shouldn't these tests be implemented there already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's remarkably low on tests for such a popular crate

lib/crypto/src/bits.rs Outdated Show resolved Hide resolved
@0xNeshi 0xNeshi requested a review from qalisander January 30, 2025 19:28
Comment on lines 40 to 56
fn trimmed_iter_starts_with_one() {
proptest!(|(value in 1u64..)| {
let bits: Vec<bool> = value.bit_be_trimmed_iter().collect();
prop_assert!(!bits.is_empty());
prop_assert!(bits[0]);
})
}

#[test]
fn trimmed_is_subset_of_full() {
proptest!(|(value: u64)| {
let full: Vec<bool> = value.bit_be_iter().collect();
let trimmed: Vec<bool> = value.bit_be_trimmed_iter().collect();
let start_idx = value.leading_zeros() as usize;
prop_assert_eq!(&full[start_idx..], trimmed);
})
}
Copy link
Collaborator Author

@0xNeshi 0xNeshi Feb 5, 2025

Choose a reason for hiding this comment

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

@qalisander is this way of writing proptests as standalone better for your IDE than when all tests are grouped in one proptest!? You mentioned that this way you're able to run individual tests from within IDE

Copy link
Member

@qalisander qalisander left a comment

Choose a reason for hiding this comment

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

LGTM!

@0xNeshi 0xNeshi merged commit 1c86069 into main Feb 5, 2025
25 checks passed
@0xNeshi 0xNeshi deleted the fuzz-lib-crypto branch February 5, 2025 17:30
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.

[Feature]: Investigate Fuzzing
3 participants