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.

Sign up for GitHub

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

Jump to bottom

feat: Refactor assumeNot* std cheats #407

Merged
merged 18 commits into from
Jul 11, 2023
Merged

feat: Refactor assumeNot* std cheats #407

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1730a9b
fix: exclude vm address from assumeNoPrecompiles
Sabnock01 Jun 23, 2023
934e0ae
forge fmt
Sabnock01 Jun 23, 2023
49bc2a6
oops
Sabnock01 Jun 23, 2023
0ab33b1
update assumePayableIsNot
Sabnock01 Jul 3, 2023
9ab202f
forge fmt
Sabnock01 Jul 3, 2023
5c869c9
Merge branch 'master' into fix/assume-no-precompiles
Sabnock01 Jul 3, 2023
0b25de5
forge fmt v2
Sabnock01 Jul 3, 2023
0b3b438
remove exposed_assumePayable test
Sabnock01 Jul 3, 2023
508abbb
modify _isPayable to be compiler agnostic
Sabnock01 Jul 3, 2023
cdecd83
compiler agnostic v2
Sabnock01 Jul 3, 2023
6e10c88
warnings
Sabnock01 Jul 3, 2023
73cca73
minor fixes
Sabnock01 Jul 4, 2023
c31747d
minor fixes v2
Sabnock01 Jul 4, 2023
ff9e33d
fix: make suggested changes
Sabnock01 Jul 10, 2023
53abaa4
forge fmt
Sabnock01 Jul 10, 2023
59d5726
fix: correct use of console address
Sabnock01 Jul 10, 2023
d9f25df
fix: remove extcodesize check
Sabnock01 Jul 10, 2023
3981afc
chore: tweak function arg order and other small cleanup
mds1 Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/StdCheats.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ abstract contract StdCheatsSafe {
// address), but the same rationale for excluding them applies so we include those too.

// These should be present on all EVM-compatible chains.
vm.assume(addr < address(0x1) || addr > address(0x9));
vm.assume((addr < address(0x1) || addr > address(0x9)) && addr != address(vm));
Copy link
Collaborator

@mds1 mds1 Jun 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this approach much better. Will rework.

Copy link
Contributor Author

@Sabnock01 Sabnock01 Jul 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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);

}

Copy link
Contributor Author

@Sabnock01 Sabnock01 Jul 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 😅

Copy link
Contributor Author

@Sabnock01 Sabnock01 Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ready for review!


// forgefmt: disable-start
if (chainId == 10 || chainId == 420) {
Expand Down
2 changes: 1 addition & 1 deletion test/StdCheats.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ contract StdCheatsTest is Test {
assumeNoPrecompiles(addr, getChain("optimism_goerli").chainId);
assertTrue(
addr < address(1) || (addr > address(9) && addr < address(0x4200000000000000000000000000000000000000))
|| addr > address(0x4200000000000000000000000000000000000800)
|| (addr > address(0x4200000000000000000000000000000000000800) && addr < address(vm)) || addr > address(vm)
);
}

Expand Down