💥 Add Property-Based Tests (Fuzz Testing) and Invariant Tests

[pcaversaccio]

The current test suite does not entail any fuzzing. We should implement appropriate fuzz tests, where it makes sense.

[hrik2001]

Hey! I have some experience with fuzzing in foundry (and foundry in general), for sure I can help. Although I don't have experience with vyper, but have coded in python so wouldn't take much time to pick up. Do u think I can help?

[pcaversaccio]

@hrik2001 thanks for your message. Sounds great - to be clear this won't be a fast task since we need to add to all tests the appropriate fuzz tests. I need to refactor something small this week within the tests but thereafter you could start. So wrt the workflow I would suggest the following workflow:

• I create a separate branch fuzz-tests against which you can open PRs;
• We should go step-by-step, i.e. contract by contract, and should add the fuzz tests at the end of the unit tests for each contract;
• We will need to adjust the CI as well and create a dedicated Foundry profile (i.e. profile.ci) with fuzz_runs = 10000.

Do we have a plan?

[hrik2001]

Hey @pcaversaccio, that sounds really great, and yup, didn't expect this to be a fast task.
We can have a general chit chat on discord or twitter about how the fuzzy tests need to be implemented for each smart contract so that we have a plan of action. And then we can get down to working on the PRs!

Also, since you are interested in fuzz testing, I think you might also be interested in fuzz invariant testing. Forge hasn't put the documentation for it in their book yet, but yeah you might want to take a look. If you like it, we can implement that too along with fuzz tests. I think invariants would be helpful because for auth stuff, invariants make a lot of sense.

References:
foundry-rs/book#497
https://github.com/foundry-rs/foundry/tree/master/testdata%2Ffuzz%2Finvariant
https://github.com/maple-labs/revenue-distribution-token/blob/add-forge-invariants/contracts/test/Invariants.t.sol

Do let me know if you like the idea or not. Till then, I will try learning vyper and also I will grok the codebase, so that by the time you end up refactoring, we can start implementing.

[pcaversaccio]

@hrik2001 concerning the invariants test - yes we can and should add them where it makes sense. The main difference to normal property-based testing is that the invariant tests do not assume a certain initial state for the contracts (while this is true for normal fuzz tests or even symbolic execution). Thus we should use it only when we want to test for uncertain initial states.

We should define the fuzz tests at the end of the unit tests for each contract and thereafter the invariants tests as a separate contract. When it comes to the exact unit tests, I would go step-by-step through each unit test and assess whether a fuzz test would make sense. An example:

/// The current unit test
function testGrantRoleSuccess() public {
    address admin = address(vyperDeployer);
    address account = vm.addr(1);
    vm.startPrank(admin);
    vm.expectEmit(true, true, true, false);
    emit RoleGranted(ADDITIONAL_ROLE_1, account, admin);
    accessControl.grantRole(ADDITIONAL_ROLE_1, account);
    assertTrue(accessControl.hasRole(ADDITIONAL_ROLE_1, account));
    vm.stopPrank();
}

/// How the fuzz test could look like
function testGrantRoleSuccess(address account) public {
    address admin = address(vyperDeployer);
    vm.startPrank(admin);
    vm.expectEmit(true, true, true, false);
    emit RoleGranted(ADDITIONAL_ROLE_1, account, admin);
    accessControl.grantRole(ADDITIONAL_ROLE_1, account);
    assertTrue(accessControl.hasRole(ADDITIONAL_ROLE_1, account));
    vm.stopPrank();
}

We should also look into a smart way to fuzz the constructor states - probably we will need a separate test that creates a new contract in itself to test the initial state, like that:

function testInitialSetup(string memory _BASE_URI) public {
    bytes memory args = abi.encode(_BASE_URI);
    ERC1155Extended = IERC1155Extended(
        vyperDeployer.deployContract("src/tokens/", "ERC1155", args)
    );
    assertTrue(ERC1155Extended.owner() == deployer);
    assertTrue(ERC1155Extended.is_minter(deployer));
    assertEq(
        ERC1155Extended.uri(0),
        string.concat(_BASE_URI, Strings.toString(uint256(0)))
    );
    assertEq(
        ERC1155Extended.uri(1),
        string.concat(_BASE_URI, Strings.toString(uint256(1)))
    );
}

What are your thoughts?

[hrik2001]

Hi @pcaversaccio, this looks like a solid plan, and I agree that a method to fuzz the initial constructor state needs to be looked into. What if we create a helper function, just for deployment. So suppose

struct deployERC1155Params {
    string _BASE_URI;
}

function deployERC1155(deployERC1155Params memory params) public {
    bytes memory args = abi.encode(params._BASE_URI);
    ERC1155Extended = IERC1155Extended(
        vyperDeployer.deployContract("src/tokens/", "ERC1155", args)
    );
}

Now suppose when we are writing a fuzz test, let's assume the test you had included, it can be written as

function testGrantRoleSuccess(address account, deployERC1155Params memory init) public {
    deployERC1155(init);
    address admin = address(vyperDeployer);
    vm.startPrank(admin);
    vm.expectEmit(true, true, true, false);
    emit RoleGranted(ADDITIONAL_ROLE_1, account, admin);
    accessControl.grantRole(ADDITIONAL_ROLE_1, account);
    assertTrue(accessControl.hasRole(ADDITIONAL_ROLE_1, account));
    vm.stopPrank();
}

This will ensure that in each iteration of the fuzz test you would have a fresh contract with fresh contructor args + the struct makes it elegant and also prevents "stack too deep" error (as contracts like ERC20 have a lot of contructor params)

[pcaversaccio]

That makes sense - generally, we should not overcomplicate the implementations tho and try to keep everything KISS. One peculiarity you will observe is that we can't fuzz for the entire type space since Vyper doesn't allow dynamically sized objects but you need to statically allocate memory to it, e.g. in the above case _BASE_URI: immutable(String[80]) where the string would allow for a max amount of 80 characters. So we need to vm.assume always what is possible otherwise the tests will fail..

I created a branch called feat/fuzz-tests and a draft PR #61 against we will work. I already committed the CI changes. If you disagree with the params lmk.