feat: Refactor assumeNot* std cheats

[Sabnock01]

Excludes the vm address 0x7109709ECfa91a80626fF3989D68f67F5b1DD12D from the assumeNoPrecompiles cheat code.

Resolves #405

Note that address(vm) is used instead of address(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D) (such as in testAssumePayable) so that it fits squarely into the current implementation of the cheat code and respective test with minimal confusion. Open to changing this though.

[mds1]

Should we also exclude the console.log address here, which is 0x000000000000000000636F6e736F6c652e6c6f67?

Though, both vm and console aren't technically precompiles in the sense meant by this function, so perhaps we should instead go with something like this:

enum AddressType {
  ZeroAddress,
  Precompiles
  Payable
  NonPayable
  ForgeAddresses
  ...whatever else
}

function assumeAddressIsNot(AddressType addressType) internal { ... }
function assumeAddressIsNot(AddressType addressType1, AddressType addressType2) internal { ... }
// repeat for length of the enum

The latter would be my preference because it scales better and provides a consistent UX for all address types. There's some decisions though, e.g. is the default test contract address part of the ForgreAddresses type, with the vm and console address, or should it be a separate type that we split into ForgePrecompiles and ForgeAddresses or something like that

[Sabnock01]

I like this approach much better. Will rework.

[Sabnock01]

Wrote impl's for both the array and overloaded versions. Prefer the array version personally. Less code and easier to test. Either one is fine though.

Two questions though:

1. How do you propose to assert that an address is payable vs nonpayable and why include those in the enum exactly? Not positive if there's an optimal way to do this other than doing call on the address with some msg.value and checking success.
2. In both impl's, I have functions for each enum type that I check against in assumeAddressIsNot but both this and the overloaded version fail with too many vm.assume rejects when using a fuzz test.

What do you think of this method of testing where instead of calling assumeAddressIsNot all the derivative functions are called?

function testAssumeAddressIsNot(address addr) external {
    assumeNoZeroAddress(addr);
    assumeNoPrecompiles(addr, getChain("optimism_goerli").chainId);
    assumeNoForgeAddresses(addr);
    assertTrue(addr != address(0));
    assertTrue(
        addr < address(1) || (addr > address(9) && addr < address(0x4200000000000000000000000000000000000000))
            || addr > address(0x4200000000000000000000000000000000000800)        
    );
    assertTrue(addr != 0x7109709ECfa91a80626fF3989D68f67F5b1DD12D && addr != 0x000000000000000000636F6e736F6c652e6c6f67);
}

[mds1]

Prefer the array version personally. Less code and easier to test

How is the UX with the array version? Main reason I though overloads would be better is:

// This seems like good UX for devs
assumeAddressIsNot(1, 2, 3);

// This seems like bad UX (using a uint instead of enum for brevity in this example)
uint[] memory x = new uint[](3);
x[0] = 1;
x[1] = 2;
x[2] = 3;
assumeAddressIsNot(x);

How do you propose to assert that an address is payable vs nonpayable and why include those in the enum exactly? Not positive if there's an optimal way to do this other than doing call on the address with some msg.value and checking success.

In the forge-std assumePayable method we just check success on a zero value call. But on second thought I'm not sure if our tests are sufficiently robust, e.g. if a contract has a non-payable fallback and no receive, that contract is not payable, and I think our assumePayable would wrongly return true.

Better way is probably what you said, specifically:

• Return true if address has no code
• Otherwise, ensure recipient's current ETH balance is less than maxUint, deal 1 wei to test contract, send it to recipient and check success, reset test contract and recipient balances to their original values.

In both impl's, I have functions for each enum type that I check against in assumeAddressIsNot but both this and the overloaded version fail with too many vm.assume rejects when using a fuzz test. What do you think of this method of testing where instead of calling assumeAddressIsNot all the derivative functions are called?

If it helps, we can raise the assume limit for that test only using the inline config approach. I think the most robust way would be something like the below, so if a new address type is added it's automatically tested

function testAssumeAddressIsNot(address addr) external {
  for (uint8 i = 0; i < type(AddressType).max; i++) {
    assumeAddressIsNot(AddressType(i));
  }
  assertTrue(addr != address(0));
  assertTrue(
      addr < address(1) || (addr > address(9) && addr < address(0x4200000000000000000000000000000000000000))
            || addr > address(0x4200000000000000000000000000000000000800)        
    );
  assertTrue(addr != 0x7109709ECfa91a80626fF3989D68f67F5b1DD12D && addr != 0x000000000000000000636F6e736F6c652e6c6f67);

}

[Sabnock01]

Did not know of in-line config!

Also UX is certainly worse with the array impl but it's less code for StdCheats.sol is what I meant. But on second thought, we needn't concern ourselves about that so I am in favor of the overloaded one.

Few more q's if you don't mind:

1. That for loop is valid Solidity? Don't think there's a built-in way to get the length of an enum and it doesn't seem to work in either my forge test or chisel.
2. Payable and NonPayable are mutually exclusive right, so we wouldn't need to iterate through the whole enum but instead n - 1?
3. You still have to provide the address itself as an argument to assumeAddressIsNot right? That's the way assumeNoBlacklisted, assumeNoPrecompiles, and assumePayable are.

[mds1]

That for loop is valid Solidity? Don't think there's a built-in way to get the length of an enum and it doesn't seem to work in either my forge test or chisel.

Hm it should work, see the solidity docs:

Using type(NameOfEnum).min and type(NameOfEnum).max you can get the smallest and respectively largest value of the given enum.

---

Payable and NonPayable are mutually exclusive right, so we wouldn't need to iterate through the whole enum but instead n - 1?

Ah good point. So maybe we put payable/nonpayable as indexes 0 and 1 in the enum, then we have:

• main test that iterates through indicies 2-n for the non-mutually-exclusive cases
• second test for payable
• third test for nonpayable

Wdyt of that? This way new values added to the end of the enum are automatically tested

You still have to provide the address itself as an argument to assumeAddressIsNot right? That's the way assumeNoBlacklisted, assumeNoPrecompiles, and assumePayable are.

Oops yes, sorry forgot to include that 😅

[Sabnock01]

type(AddressType).max as it turns out returns an enum so it's trivial enough just to cast to uint8.

And I like the testing structure though even when setting max_fuzz_rejects to some absurdly high number like 10_000_000 which takes several minutes to complete, a fuzz test on assumeNoNonPayable still fails with too many rejects. Separately, was not able to get the in-line config to work for this even though it's documented. Will open issue.

So for both assumeNoPayable and assumeNonPayable I went with modified versions of the old test for the now deprecated assumePayable.

Latest impl pushed.

[mds1]

Awesome sounds great, thanks! Lmk if this is ready for review or if you still have changes left—will be tied up today/tomorrow but should be able to review this thursday

[Sabnock01]

It's ready for review!

[mds1]

Looks great overall, thank you! Lots of small comments around details / some nits

[Sabnock01]

Looks great overall, thank you! Lots of small comments around details / some nits

Cool. I like all of these suggestions. It's a big PR to be fair.

[Sabnock01]

Let me know what you make of these adjustments.

[mds1]

This looks great, thanks a lot @Sabnock01! I pushed one commit with a few small tweaks, lmk if you have any feedback/objections there, and afterwards I'll merge

[Sabnock01]

This looks great, thanks a lot @Sabnock01! I pushed one commit with a few small tweaks, lmk if you have any feedback/objections there, and afterwards I'll merge

These are good. Should have caught more of them honestly but was a pleasure working with you on this one.

[mds1]

Likewise, thanks for the quality PR! '🙌

I'll be putting out a new forge-std release with this shortly