search

sablier-labs / flow

🍃 Smart contracts of the Sablier Flow protocol.
Other
8 stars 0 forks source link

test: add fuzz tests for create and deposit #155

Closed smol-ninja closed 4 months ago

smol-ninja commented 4 months ago

Linked issues

Additional changelog

  1. Constants.sol
    • Refactored it by adding spaces to improve readability.
    • Added TRANSFER_VALUE which is default value of 50,000. Used it to derive TRANSFER_AMOUNT for different
  2. Utils.sol
    • New bound functions.
    • Import Constants so that I can define UINT128_MAX in Constants and use it in `Utils.
  3. Base.t.sol
    • Define createAsset, useful to create assets on the run.
    • Replaced type(uint256).max with UINT256_MAX. I am using UINT128_MAX in fuzz to bound deposit amount so for consistency, I changed it here too.
    • Define getDefaultAmountWithDecimals - useful to calculate default transfer amount for a random asset. Useful in fuzz tests.
  4. Integration.t.sol
    • Refactors - tried to come up with bespoke names for helper functions. decimals.

  5. Use solidity >=0.8.22 everywhere. Let me know if its a problem.

    I have only covered create and deposit in this PR because before proceeding further, I want both of us to agree with the proposed changes. Once this PR is merged, I will proceed with the remaining tests in a separate PR.

This PR uses a different technique to write fuzz tests. For deposit function, to test all the possibilities, I have written a shared fuzz contract that creates some streams with all possible decimals and store them into fixture variables. Without fixtures and using the same approach as in lockup contracts, fuzz runs only test the initial deposit into the contract which imo is not sufficient. With this new approach, both initial deposits as well as subsequent deposits are tested.

40% of the fuzz run would load input values from the fixtures. 60% would be random.

smol-ninja commented 4 months ago

I want to mention that we should use bound over assume

Very useful reminder. Thank you.

I have replied to your feedback above. I will review the feedback PR once we have discussed the comments.

smol-ninja commented 4 months ago

@andreivladbrg other than resolving conflicts, I added two new commits: https://github.com/sablier-labs/flow/pull/155/commits/21f1d6c94f8d07aa79d388b27ec6177c88b9a0c3 and https://github.com/sablier-labs/flow/pull/155/commits/7c99bd36d7d0f16e33eb9c7a84c31c9ef5f020a3.

The explanation for removing ratePerSecond = boundUint128(ratePerSecond, 1, UINT128_MAX - 1); is that ratePerSecond is already uint128 type so vm.assume(ratePerSecond > 0) is all we need.

lmk if all looks good.

andreivladbrg commented 4 months ago

The explanation for removing ratePerSecond = boundUint128(ratePerSecond, 1, UINT128_MAX - 1); is that ratePerSecond is already uint128 type so vm.assume(ratePerSecond > 0) is all we need.

why do you want to use assume over bound?

even if there is only one value (0) that can be fuzzed to not be in the specified range, i am still of the opinion we should not discard that test

once again pasting this here:

want to mention that we should use bound over assume. When using assume, the test is discarded if the condition is not met, resulting in fewer test runs and potentially slowing down the testing process. On the other hand, bound adjusts the values to fit within a specified range, ensuring the test still runs. This approach leads to a higher number of test executions and ultimately a more thoroughly tested environment. See here.

smol-ninja commented 4 months ago

I now understand your point. I agree that we should not let it go waste even if its just a single test.

smol-ninja commented 4 months ago

Reverted it back.