Cyfrin / foundry-defi-stablecoin-cu

250 stars 117 forks source link

Update StopOnRevertHandler.t.sol #14

Closed 0xPublicGoods closed 1 year ago

0xPublicGoods commented 1 year ago

In DSCEngine.sol we have the params with user first, then collateral

    function getCollateralBalanceOfUser(address user, address token) external view returns (uint256) {
        return s_collateralDeposited[user][token];
    }
    function redeemCollateral(uint256 collateralSeed, uint256 amountCollateral) public {
        ERC20Mock collateral = _getCollateralFromSeed(collateralSeed);
        uint256 maxCollateral = dscEngine.getCollateralBalanceOfUser(address(collateral), msg.sender);

        amountCollateral = bound(amountCollateral, 0, maxCollateral);
        if (amountCollateral == 0) {
            return;
        }
        dscEngine.redeemCollateral(address(collateral), amountCollateral);
    }

In the current Handler file above (which I am suggesting needs editing) we are calling collateral first and user second, I found this whilst running Handler.t.sol locally at this point in the video course (https://youtu.be/wUjYK5gwNZs?t=14989). The video tests run perfectly, my local tests however, have under/overflow arithmetic errors.

I have the params in the correct order,

dsce.getCollateralBalanceOfUser(msg.sender, address(collateral));

and these are the errors from my invariant test

Encountered 1 failing test in test/fuzz/InvariantsTest.t.sol:InvariantsTest
[FAIL. Reason: Arithmetic over/underflow]
        [Sequence]
                sender=0x62084110fd5fc65ce932bd6425016f4d01abec6c addr=[test/fuzz/Handler.t.sol:Handler]0x2e234dae75c793f67a35089c9d99245e1c58470b calldata=depositCollateral(uint256,uint256), args=[966458702 [9.664e8], 3]

This has been edited in place, no tests have been run on the repository - only my local version which is not yet finished

0xPublicGoods commented 1 year ago

I should point out that with my Handler.t.sol file and foundry.toml settings as follows:

[invariant]
runs = 1000
depth = 128
fail_on_revert = false

These are my results, reverts were minimal:

Running 1 test for test/fuzz/InvariantsTest.t.sol:InvariantsTest
[PASS] invariant_protocolMustHaveMoreCollateralValueThankDscSupply() (runs: 1000, calls: 128000, reverts: 4)

But caught quickly with

fail_on_revert = true
invariant_protocolMustHaveMoreCollateralValueThankDscSupply() (runs: 2, calls: 173, reverts: 1)
PatrickAlphaC commented 1 year ago

Thank you!

I fixed it here: https://github.com/Cyfrin/foundry-defi-stablecoin-f23/commit/4b9f59ba57b77793fdbffb9ab23c34e88626c2c2

There were some formatting issues with your PR I wanted to get out of the way.

PatrickAlphaC commented 1 year ago

The invariant tests will ultimately fail, as we've sort of set them up in this project as such.