Bare bones for differential fuzzing #421
Conversation
Codecov Report
@@ Coverage Diff @@
## master #421 +/- ##
==========================================
- Coverage 86.64% 86.43% -0.21%
==========================================
Files 68 69 +1
Lines 4147 4431 +284
==========================================
+ Hits 3593 3830 +237
- Misses 554 601 +47
Continue to review full report at Codecov.
|
Ha! Somehow I didn't think of that, despite using |
| # toolchain: stable | ||
| # override: true | ||
| # - name: Run WASM tests | ||
| # run: wasm-pack test --node -- --workspace |
There was a problem hiding this comment.
Guess that was temporary disabled during development for some reason. Can we roll it back now?
There was a problem hiding this comment.
This is intentional because now tests are living in a separate library which needs to go under workspace so new changes lead to compile the solc bindings to wasm as well which is not possible right now as wasm32-unknown-unknown is not supporting c++ bindings right now. We did decided to comment the wasm-pack test cases on the CI and to make it work for now and for the releases we will manually run the wasm-pack test cases locally to verify whether everything is good or not.
| #[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Copy)] | ||
| pub struct TransferInfo<'a> { | ||
| pub to: &'a str, | ||
| pub value: u128, |
There was a problem hiding this comment.
I think this is limiting the set of possible test cases since in reality the value will be a U256. I guess u128 was chosen for convenience since it is the biggest rust integer but I think we should use the U256 type here that we also use elsewhere.
There was a problem hiding this comment.
We probably need to implement this for U256 support:
https://docs.rs/arbitrary/1.0.1/arbitrary/trait.Arbitrary.html
fuzz/src/erc20.rs
Outdated
| } | ||
|
|
||
| pub fn fe_erc20_transfer(info: TransferInfo) { | ||
| if info.value >= 1000000000000000000000000 as u128 { |
There was a problem hiding this comment.
What's the reasoning behind that value check?
There was a problem hiding this comment.
Actually, it should be <= so use shouldn't be able to transfer more than the total supply.
There was a problem hiding this comment.
The expected behavior is that the tx reverts, correct? I think we should be validating this.
| &mut executor, | ||
| "test-utils/fuzz/fixtures/fe/erc20.fe", // TODO: Need to fix the path | ||
| "ERC20", | ||
| &[string_token("Fe Coin"), string_token("FE")], |
There was a problem hiding this comment.
This is a good start, but it doesn't test much.
I think what we eventually want is to have two separate deployed contracts/executors that we perform many state-modifying transactions on in a random order. This will get us into edge cases where we might find bugs.
Of course this doesn't need to be included in this PR, I'm just wondering if you've been thinking about this or had a different direction in mind.
There was a problem hiding this comment.
Yeah, I was thinking about this to have some kind of random order testing. I have a process in mind where we can come up with some mutation testing where we can randomly mutate the contract as well if we want but that is I think one step forward for now, I am thinking to have some kind of prop testing with random ordering.
| if info.value | ||
| <= TOTAL_SUPPLY | ||
| .parse::<u128>() | ||
| .unwrap_or(1000000000000000000000000) |
There was a problem hiding this comment.
We should just expect here or make TOTAL_SUPPLY a u128.
There was a problem hiding this comment.
Okay, will add that change in another PR.
What was wrong?
Needs fuzzing of contracts.
How was it fixed?
It consists of the fuzzing setup for the erc20.fe and erc20.sol and has the rearrangements for the compiler tests as an independent directory to avoid circular dependency and it will also decrease the test execution time as it doesn't require rebuilding the dependencies again.
To run the fuzzer -
cd fuzz && cargo +nightly fuzz run erc20_target --features solc-backendTo-Do
OPTIONAL: Update Spec if applicable
Add entry to the release notes (may forgo for trivial changes)
Clean up commit history