Add remaining fuzzing tests and remove pseudo-random fuzzing #5095
Conversation
Removing Hardhat test as it's tested and fuzzed with Foundry via `test_transfer`
Remove `average` and `abs` unit tests which attempt to do fuzzing in favor of Foundry native-fuzzing, already found in `SignedMath.t.sol`
v1.8.2
Remove in favor of fuzzed unit test with Foundry
|
There was a problem hiding this comment.
We need to check that this doesn't reduce coverage. It may be necessary to keep at least one test.
(foundry tests are not registered when doing coverage)
There was a problem hiding this comment.
Our guideline so far was:
- unit test first, with as close to 100% coverage as possible
- fuzzing and formal verification where that is relevant, as a complement to unit tests (not as a replacement).
I still think this applies.
A direct consequence is that even when there is some fuzzing, we still want to have unit test that cover the code. In particular the corner cases whe have identified (and that we are not 100% sure fuzzing will explore).
The idea being the issue to avoid unit tests that loop over (large amount of) randomly generated values. However, that doesn't mean all loops in the tests should be removed. Following the swiss chesse model, fuzzing should be an extra layer, but we should not add holes in the unit test layers with the hope that fuzzing covers them.
My initial impression reading this PR is that the added foundry tests are great ... but I wouldn't necessarily remove (all) the corresponding unit tests. For example, there may to too many different input for testing SignedMath.average, but I still think we should test the combinaison of MinInt256 and MaxInt256 with each other, and with some smaller values.
| function testRecoverWithValidSignature(string calldata seed, string calldata message) public { | ||
| (address signer, uint256 key) = makeAddrAndKey(seed); | ||
|
|
||
| bytes32 hash = keccak256(abi.encode(message)); | ||
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(key, hash); | ||
|
|
||
| bytes memory signature = abi.encodePacked(r, s, v); | ||
| assertEq(signer, ECDSA.recover(hash, signature)); | ||
| } |
There was a problem hiding this comment.
| function testRecoverWithValidSignature(string calldata seed, string calldata message) public { | |
| (address signer, uint256 key) = makeAddrAndKey(seed); | |
| bytes32 hash = keccak256(abi.encode(message)); | |
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(key, hash); | |
| bytes memory signature = abi.encodePacked(r, s, v); | |
| assertEq(signer, ECDSA.recover(hash, signature)); | |
| } | |
| function testRecoverWithValidSignature(string calldata seed, bytes32 digest) public { | |
| (address signer, uint256 key) = makeAddrAndKey(seed); | |
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(key, digest); | |
| bytes memory signature = abi.encodePacked(r, s, v); | |
| assertEq(signer, ECDSA.recover(digest, signature)); | |
| } |
| function testRecoverWithWrongMessage(string calldata seed, string calldata message, bytes32 digest) public { | ||
| (address signer, uint256 key) = makeAddrAndKey(seed); | ||
|
|
||
| bytes32 validDigest = keccak256(abi.encode(message)); | ||
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(key, validDigest); | ||
|
|
||
| bytes memory signature = abi.encodePacked(r, s, v); | ||
| assertTrue(signer != ECDSA.recover(digest, signature)); | ||
| } |
There was a problem hiding this comment.
| function testRecoverWithWrongMessage(string calldata seed, string calldata message, bytes32 digest) public { | |
| (address signer, uint256 key) = makeAddrAndKey(seed); | |
| bytes32 validDigest = keccak256(abi.encode(message)); | |
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(key, validDigest); | |
| bytes memory signature = abi.encodePacked(r, s, v); | |
| assertTrue(signer != ECDSA.recover(digest, signature)); | |
| } | |
| function testRecoverWithWrongMessage(string calldata seed, bytes32 digest, bytes32 otherDigest) public { | |
| vm.assume(digest != otherDigest); | |
| (address signer, uint256 key) = makeAddrAndKey(seed); | |
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(key, digest); | |
| bytes memory signature = abi.encodePacked(r, s, v); | |
| assertFalse(signer == ECDSA.recover(otherDigest, signature)); | |
| } |
There was a problem hiding this comment.
We need to check that this doesn't reduce coverage. It may be necessary to keep at least one test.
(foundry tests are not registered when doing coverage)
Note: I like that unit test cover the EIP-55 references specifically
| it('core takes over ownership on transfer', async function () { | ||
| await this.token.connect(this.alice).transferFrom(this.alice, this.receiver, tokenId); | ||
|
|
||
| expect(await this.token.ownerOf(tokenId)).to.equal(this.receiver); | ||
| }); | ||
|
|
| describe('when authority replies with a delay', function () { | ||
| beforeEach(async function () { | ||
| this.authority = this.authorityDelayMock; | ||
| }); | ||
|
|
||
| for (const immediate of [true, false]) { | ||
| for (const delay of [0n, 42n]) { | ||
| it(`returns (immediate=${immediate}, delay=${delay})`, async function () { | ||
| await this.authority._setImmediate(immediate); | ||
| await this.authority._setDelay(delay); | ||
| const result = await this.mock.$canCallWithDelay(this.authority, this.user, this.other, '0x12345678'); | ||
| expect(result.immediate).to.equal(immediate); | ||
| expect(result.delay).to.equal(delay); | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test is not so much fuzzing as it is testing 4 clearly different cases. Not saying fuzzing would not cover them, but we don't really need 5000 fuzzing runs to cover the same code.
Also, I'm not sure how that change would affect coverage.
Fixes #4894 by removing unit tests that are already equivalently running with fuzzing via Foundry and adding remaining fuzzing unit tests. WIP.
PR Checklist
npx changeset add)