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

[WIP]: Adding foundry fuzzer for libraries processing bitmaps #641

Closed
wants to merge 15 commits into from

Conversation

LHerskind
Copy link
Contributor

Work in progress to add foundry fuzz test of libraries that perform low-level operations on bitmaps etc. Needs #632.

@LHerskind LHerskind changed the base branch from feat/cross-borrowing to master February 28, 2022 10:21
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@ce90d8d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #641   +/-   ##
=========================================
  Coverage          ?   99.95%           
=========================================
  Files             ?       45           
  Lines             ?     2114           
  Branches          ?      281           
=========================================
  Hits              ?     2113           
  Misses            ?        1           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce90d8d...f8f43e7. Read the comment docs.

@@ -11,6 +11,8 @@
"node": ">=16.0.0"
},
"scripts": {
"setup:foundry": "foundryup && git submodule update --init",
Copy link

Choose a reason for hiding this comment

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

Suggested change
"setup:foundry": "foundryup && git submodule update --init",
"setup:foundry": "foundryup && forge install",

Comment on lines 50 to 57
function testDecodeSupplyWithPermitParams(
uint16 assetId,
uint128 amount,
uint16 referralCode,
uint256 deadLine,
uint8 permitV
) public {
VM.assume(assetId < 128 && deadLine < type(uint32).max);
Copy link

@mds1 mds1 Mar 4, 2022

Choose a reason for hiding this comment

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

assume should mainly be used for very narrow checks—broad checks will slow down tests as it will take a while to find valid values, and the test may fail if you hit the max number of rejects. (You can also configure fuzz-max-local-rejects and fuzz-max-global-rejects in the foundry.toml file).

I'd recommend changing this test (and similar for other assume usage in the repo) to either:

  function testDecodeSupplyWithPermitParams(
    uint8 assetId, // changed from uint16 to uint8 to reduce number of rejects
    uint128 amount,
    uint16 referralCode,
    uint32 deadLine, // changed to uint32 so fuzzer max value for this param is now type(uint32).max
    uint8 permitV
  ) public {
    VM.assume(assetId < 128);

OR, when solidity's types aren't a great fit, use a uint256 and wrap values manually with modulo, either using Solmate's bound method or just using modulo directly (you want uint256 to reduce modulo bias). A downside of this approach is that when a test fails, you need to take the mod of the failed input to see the actual value used in tests

  function testDecodeSupplyWithPermitParams(
    uint256 assetId, // changed to uint256
    uint128 amount,
    uint16 referralCode,
    uint256 deadLine, // changed to uint256
    uint8 permitV
  ) public {
    assetId = assetId % 128; // wrap assetId between 0 and 127
    deadLine = deadLine % 2**32; // wraps deadLine between 0 and 2**32 - 1
    // or use `bound(val, min, max)` to wrap between min and max, inclusive

contract CalldataLogicTest is TestHelper {
using ReserveConfiguration for DataTypes.ReserveConfigurationMap;

Vm constant VM = Vm(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
Copy link

Choose a reason for hiding this comment

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

DSTest has HEVM_ADDRESS as a constant so you can use that directly for readability. You can also declared this VM variable just once in TestHelper instead of in each individual test contract 🙂

Suggested change
Vm constant VM = Vm(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
Vm constant VM = Vm(HEVM_ADDRESS);

@LHerskind
Copy link
Contributor Author

Closing this one since a more updated version in in #967 and I do no longer have credentials to push on top of this one 🫡

@LHerskind LHerskind closed this Aug 31, 2024
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.

4 participants