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.

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

chore: enhance docs of expect revert #1428

Merged
Merged
Changes from all commits
Commits
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
20 changes: 17 additions & 3 deletions src/cheatcodes/expect-revert.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,20 @@ function expectPartialRevert(bytes4 message, address reverter) external;
>
> Carefully read the next sections!

To understand why you are getting this error you first have to understand what `call depth` means.

You can think of call depth in a similar way to function scoping. When you are entering an **external** call the call depth is increased by `1`.
When you exit the external call the call depth is decreased by `1`. If you have nested calls it looks as follows:

```ignore
0 → Contract A (calls B) → 1 → Contract B (calls C) → 2 → Contract C (returns) → 1 → Contract B (returns) → 0
```

**Internal** functions on the other hand do **NOT** increase the call depth. It is not actually making _calls_ but rather _jumping_ to the target location.

When testing **internal** functions with `vm.expectRevert` at the same call depth **ONLY** the **FIRST** `vm.expectRevert` is executed.

In the following example there are two `vm.expectRevert`'s that exist at the same call depth hence only the **FIRST** one is executed and
the test returns a **SUCCESS**. This is likely different behavior from what you may assume.
The following example shows where the footgun occurs. There are two `vm.expectRevert`'s that exist at the same call depth hence only the **FIRST** one is executed and the test returns a **SUCCESS**. This is likely different behavior from what you may assume.

```solidity
// DO NOT IMPLEMENT AS FOLLOWS! THIS IS AN INCORRECT USE.
Expand All @@ -97,10 +107,14 @@ If the **next call** does not revert with the expected data `message`, then `exp
>
> Add `/// forge-config: default.allow_internal_expect_revert = true` above the test function.
>
> Or globally, this is **STRONGLY** discouraged:
> Or globally, this is discouraged:
>
> Add `allow_internal_expect_revert = true` to `foundry.toml`.

As long as you are not using `vm.expectRevert` on multiple internal functions is a single test function body it is generally considered safe.

You are recommended to apply this rule in a similar manner one would when tagging assembly blocks as `memory-safe`.

> **Note**
>
> For a call like `stable.donate(sUSD.balanceOf(user))`, the next call expected to revert is `sUSD.balanceOf(user)` and not `stable.donate()`.
Expand Down