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

Expect revert from address #5430

Closed
wants to merge 23 commits into from

Conversation

xeno097
Copy link
Contributor

@xeno097 xeno097 commented Jul 18, 2023

Motivation

As requested in #5299, this pr adds the following expectRevert cheat code overloads to verify that the specified address reverted:

function expectRevert(bytes calldata revertData, address reverter) external;
function expectRevert(bytes4 revertData, address reverter) external;
function expectRevert(address reverter) external;

Solution

Track the reverts that happen during contract execution and store them in reverts_by_address.
The process to check if an address reverted is as follows:

  • Check that there is an entry in reverts_by_address for the selected address:
    • If there isn't an entry for the address it is a failure because the address did not revert.
    • If the is an entry, check if the user is expecting any revert or a specific one:
      • If the user is expecting any revert (expected_revert is None) it is a success.
      • If the user is expecting a revert with specific data check if that data is in the set associated with expected_revert_address and if it is then it is a success, otherwise it is a failure.

Note: This new logic does not enforce that the current call should end with a revert (but it stays the same for the old expectRevert only with data). The only thing that matters is that anywhere in the call stack expected_revert_address reverted. For example, if we have 2 contracts A and B, and B calls A which reverts, even if B catches the error but does not revert, if we are expecting A to revert then the assertion will be successful.

Ps: I'm super sorry for the super delayed pr 🙏🏼

Closes: #5299

@mds1
Copy link
Collaborator

mds1 commented Jul 18, 2023

Thanks! I haven't reviewed but just two questions on the behavior:

  1. How does this handle the options in Add vm.expectRevert overloads that take an address arg, to distinguish between error selectors that are identical but in different contracts. #5299 (comment), e.g. does it care who originated the error? For example, if we have contract A which calls B and B calls C, and expect B to revert. Say C reverts, so then B reverts, then A reverts. Does that (a) count as a match because B reverted with the right data, or (b) not count as a match because C originated the revert? I can see arguments for both so not sure what the best way to handle it is
  2. It says "even if B catches the error but does not revert, if we are expecting A to revert then the assertion will be successful". Is this how the current expectRevert behaves? I don't think it is, and I think the behaviors should be consistent with the current behavior.

@xeno097
Copy link
Contributor Author

xeno097 commented Jul 18, 2023

Thanks! I haven't reviewed but just two questions on the behavior:

  1. How does this handle the options in Add vm.expectRevert overloads that take an address arg, to distinguish between error selectors that are identical but in different contracts. #5299 (comment), e.g. does it care who originated the error? For example, if we have contract A which calls B and B calls C, and expect B to revert. Say C reverts, so then B reverts, then A reverts. Does that (a) count as a match because B reverted with the right data, or (b) not count as a match because C originated the revert? I can see arguments for both so not sure what the best way to handle it is
  2. It says "even if B catches the error but does not revert, if we are expecting A to revert then the assertion will be successful". Is this how the current expectRevert behaves? I don't think it is, and I think the behaviors should be consistent with the current behavior.

Hey @mds1 about this

(a) count as a match because B reverted with the right data, or (b) not count as a match because C originated the revert? or (b) not count as a match because C originated the revert? I can see arguments for both so not sure what the best way to handle it is

The current implementation does not care who originated the revert, the only thing that matters if the contract at address expected_revert_address reverted with the data specified by the user. Initially, I was implementing this taking into account who originated the revert but then thought that it could be tricky to determine who originated the error because the user could do something like this:

error SomeError();

contract B {
    function reverts() external pure {
        revert SomeError();
    }

    function doesNotRevert() external pure {}
}

contract A {
    B b;

    constructor(B _b) {
        b = _b;
    }

    function revertFromB() external view {
        b.reverts();
    }

    function revertFromA() external view {
        try b.doesNotRevert() {
            revert SomeError();
        } catch {}
    }

    function revertRethrowingRevertFromB() external view {
        try b.reverts() {} catch {
            revert SomeError();
        }
    }
}

In revertRethrowingRevertFromB how do I know if A is throwing the same revert originated from B or is another revert with the same data from the EVM point of view? I tried to do it but didn't manage to find a way to answer this question. So in the end I decided to ignore the source of the revert and focus only on answering the question: did the contract revert with the specified user data?

About

It says "even if B catches the error but does not revert, if we are expecting A to revert then the assertion will be successful". Is this how the current expectRevert behaves? I don't think it is, and I think the behaviors should be consistent with the current behavior.

Currently expectRevert expects the immediate call after expectRevert as a whole to revert, and that the revert data matches the one provided by the user (unless none is given).
I thought to not enforce the same behavior when using an address because technically speaking in contract B catches a revert from A but B itself does not revert and we are expecting A to revert then our assertion is correct because A did revert even if B didn't. For me, it would be strange to require the call as a whole to revert if I only want to assert that one (or some of them) reverted in the call stack.

@Evalir Evalir requested review from mattsse and Evalir and removed request for mattsse July 18, 2023 17:58
@mds1
Copy link
Collaborator

mds1 commented Jul 18, 2023

In revertRethrowingRevertFromB how do I know if A is throwing the same revert originated from B or is another revert with the same data from the EVM point of view?

Yep this is a great point and was my concern there too, so I'm good with the current behavior of "does not care who originated the revert". Thanks for the clear example

Currently expectRevert expects the immediate call after expectRevert as a whole to revert, and that the revert data matches the one provided by the user (unless none is given).
I thought to not enforce the same behavior when using an address because technically speaking in contract B catches a revert from A but B itself does not revert and we are expecting A to revert then our assertion is correct because A did revert even if B didn't. For me, it would be strange to require the call as a whole to revert if I only want to assert that one (or some of them) reverted in the call stack.

Makes sense, I'm also ok with this as long as @Evalir is also. Only comment would be to make sure we document this difference in the book

@xeno097
Copy link
Contributor Author

xeno097 commented Jul 18, 2023

Makes sense, I'm also ok with this as long as @Evalir is also. Only comment would be to make sure we document this difference in the book

Nice 🔥 I'll make it as clear as possible!

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

This is a hefty PR so will review again a tad later—but I have some comments we can begin tackling. I think this is moving in the right direction! let's just make sure to document things properly

};
}

fn stringify(data: &[u8]) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

I think Address or H160 has an utility to convert from a hex string? we should use this instead of having a custom func

.unwrap_or_else(|| format!("0x{}", hex::encode(data)))
}

fn actual_error_from_bytes(data: Bytes) -> Bytes {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use decode_revert from utils here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with decode_revert is that it parses the error to human readable string what I want is to preserve the error and then have the hex string version of it to compare it with the revert data

Copy link
Member

Choose a reason for hiding this comment

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

Right, wdyt about renaming this to something a bit more suitable like get_raw_err?

}
// Empty revert data
else if address_reverts.contains("0x") {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this only checks if the 0x string exists, not if it's exactly 0x which would mean invalid data. Was this how we did it previously?

Copy link
Contributor Author

@xeno097 xeno097 Jul 20, 2023

Choose a reason for hiding this comment

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

In the logic without the address the data is compared as bytes and the .is_empty is called. Here I use 0x because the stringified version of an empty byte data is 0x but if you think that there is a better way to achieve this I'd be welcome.

Also to add to the context of why I did it this way if I use bytes as the datatype for the parsed hashset clippy complains that I'm using a type with interior mutability as a key to get a value from the hash set so between 1) use #[allow(clippy::mutable_key_type)] and 2) convert Bytes to another type to get rid of the interior mutability. I chose the second one.


data
}

#[instrument(skip_all, fields(expected_revert, status, retdata = hex::encode(&retdata)))]
pub fn handle_expect_revert(
Copy link
Member

Choose a reason for hiding this comment

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

Let's now add some docs now that this is a top lvl function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup np

retdata: Bytes,
) -> Result<(Option<Address>, Bytes)> {
trace!("handle expect revert");

ensure!(!matches!(status, return_ok!()), "Call did not revert as expected");
if let Some(expected_revert_address) = expected_revert_address {
Copy link
Member

Choose a reason for hiding this comment

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

just to make this a little more rusty let's use match here—instead of having the revert_with_data case be a fallthrough case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌🏼

Comment on lines 88 to 93
let stringify = |data: &[u8]| {
String::decode(data)
.ok()
.or_else(|| std::str::from_utf8(data).ok().map(ToOwned::to_owned))
.unwrap_or_else(|| format!("0x{}", hex::encode(data)))
};
Copy link
Member

Choose a reason for hiding this comment

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

ah right, the stringify function is this extracted to an outside func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, previously it was a closure defined inside the handle_expect_revert so I exported it into its own thing and reused it in both expect_revert_... functions.

@@ -124,6 +124,9 @@ pub struct Cheatcodes {
/// Expected revert information
pub expected_revert: Option<ExpectedRevert>,

/// Tracks the reverts that happen during evm execution if a there is an expected revert
Copy link
Member

Choose a reason for hiding this comment

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

let's add a tad more docs on how this works—to document the type properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oky 👌🏼

Comment on lines 784 to 795
if self.expected_revert.is_some() && status == InstructionResult::Revert {
self.revert_per_address
.entry(b160_to_h160(call.contract))
.or_insert_with(HashSet::new)
.insert(retdata.clone().into());
}

// Handle expected reverts
if let Some(expected_revert) = &self.expected_revert {
if data.journaled_state.depth() <= expected_revert.depth {
let expected_revert = std::mem::take(&mut self.expected_revert).unwrap();
let reverts_by_address = std::mem::take(&mut self.revert_per_address);
Copy link
Member

Choose a reason for hiding this comment

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

Right, so if I understand correctly here, we're:

  • saving the last revert seen by address
  • then, taking out the hashmap from memory and passing it to handle_expect_revert

I'm unsure if this is the correct behavior—do we really wanna do std::mem::take? as this will reset the hashmap

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 want to reset the hashmap because after entering that if statement the only way to exit is after the handle_revert func is called and that function will either be successful because the revert matched or not which will cause a revert in the evm. In both cases, we don't need the tracked reverts by address anymore so I think it is safe to reset the data.

Copy link
Member

Choose a reason for hiding this comment

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

Right, so do we need a hashmap in this case? because it then seems that this map can't hold more than 1 revert, or else we have a bug I believe because we're discarding reverts that we shouldn't. In this case we can probably just have something like an Option<ExpectedRevertWithAddress> right?

// Handle expected reverts
if let Some(expected_revert) = &self.expected_revert {
if data.journaled_state.depth() <= expected_revert.depth {
let expected_revert = std::mem::take(&mut self.expected_revert).unwrap();
let reverts_by_address = std::mem::take(&mut self.revert_per_address);

Copy link
Member

Choose a reason for hiding this comment

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

ditto

Comment on lines +193 to +247
function expectRevert(bytes calldata, address) external;

function expectRevert(bytes4, address) external;

function expectRevert(address) external;

Copy link
Member

Choose a reason for hiding this comment

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

let's add some docs here 😄

@xeno097 xeno097 requested a review from Evalir July 20, 2023 03:39
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

some more nits, looking good!

data
}

/// Verifies that a revert occurred during EVM execution and that matches the user provided data if
Copy link
Member

Choose a reason for hiding this comment

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

beautiful

Comment on lines 784 to 795
if self.expected_revert.is_some() && status == InstructionResult::Revert {
self.revert_per_address
.entry(b160_to_h160(call.contract))
.or_insert_with(HashSet::new)
.insert(retdata.clone().into());
}

// Handle expected reverts
if let Some(expected_revert) = &self.expected_revert {
if data.journaled_state.depth() <= expected_revert.depth {
let expected_revert = std::mem::take(&mut self.expected_revert).unwrap();
let reverts_by_address = std::mem::take(&mut self.revert_per_address);
Copy link
Member

Choose a reason for hiding this comment

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

Right, so do we need a hashmap in this case? because it then seems that this map can't hold more than 1 revert, or else we have a bug I believe because we're discarding reverts that we shouldn't. In this case we can probably just have something like an Option<ExpectedRevertWithAddress> right?

.unwrap_or_else(|| format!("0x{}", hex::encode(data)))
}

fn actual_error_from_bytes(data: Bytes) -> Bytes {
Copy link
Member

Choose a reason for hiding this comment

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

Right, wdyt about renaming this to something a bit more suitable like get_raw_err?

@mds1
Copy link
Collaborator

mds1 commented Jul 22, 2023

@Evalir @xeno097 One thing I'm realizing is that when using this version of expectRevert, you should be able to have multiple expectReverts for the next top-level call. Noticed this when writing https://t.me/foundry_support/41032, because you may want to assert on both reverts that happen in that call. I think the existing expectRevert doesn't allow them to be stacked which makes sense, but that can differ here.

So really I think we'll have two versions of expectRevert:

  • Existing one: Expects next call to revert at depth=1 (IIRC depth=0 is test contract). Therefore can't be used with expectEmit or expectCall, and can only have one expectRevert
  • This one: Expects next call to revert at any depth. Therefore try/catch is ok as long as the revert happened, so a revert at depth=1 is not required. Therefore this CAN be used with expectEmit or expectCall, and you CAN stack multiple of these
  • This also means you can stack this version with the existing version

@xeno097
Copy link
Contributor Author

xeno097 commented Jul 22, 2023

@Evalir @xeno097 One thing I'm realizing is that when using this version of expectRevert, you should be able to have multiple expectReverts for the next top-level call. Noticed this when writing https://t.me/foundry_support/41032, because you may want to assert on both reverts that happen in that call. I think the existing expectRevert doesn't allow them to be stacked which makes sense, but that can differ here.

So really I think we'll have two versions of expectRevert:

  • Existing one: Expects next call to revert at depth=1 (IIRC depth=0 is test contract). Therefore can't be used with expectEmit or expectCall, and can only have one expectRevert
  • This one: Expects next call to revert at any depth. Therefore try/catch is ok as long as the revert happened, so a revert at depth=1 is not required. Therefore this CAN be used with expectEmit or expectCall, and you CAN stack multiple of these
  • This also means you can stack this version with the existing version

I agree this can be extremely useful. So to define a bit ux:

  • The old expectRevert keeps working as it is now
  • This new expectRevert with the address would behave a bit more like expectEmit where you can call it multiple times and after the first function call that is not a cheat code, all the expected reverts are checked.

Should the order of the call of expectRevert with an address matter like with multiple expectEmit calls? I don't think it should as long one is able to verify that all the addresses reverted as expected by the user, but maybe it should be useful in some testing scenarios.

Please let me know what you think and if there is something else we should consider for this @Evalir @mds1

@mds1
Copy link
Collaborator

mds1 commented Jul 26, 2023

Should the order of the call of expectRevert with an address matter like with multiple expectEmit calls? I don't think it should as long one is able to verify that all the addresses reverted as expected by the user, but maybe it should be useful in some testing scenarios.

If it currently matters for expectEmit I think we should stay consistent and keep the same ordering requirement here. I also prefer it because it's more strict, and it's good to be strict when testing

@Evalir
Copy link
Member

Evalir commented Jul 28, 2023

@xeno097 should i give this another review round or should i wait? lmk, would love to get this over the line, just wanna make sure we contain the complexity of all of these variants of expectRevert because I believe it's the most confusing out of the expect* cheatcodes and ideally we'd have consistent behavior

@xeno097
Copy link
Contributor Author

xeno097 commented Jul 28, 2023

@xeno097 should i give this another review round or should i wait? lmk, would love to get this over the line, just wanna make sure we contain the complexity of all of these variants of expectRevert because I believe it's the most confusing out of the expect* cheatcodes and ideally we'd have consistent behavior

Hey @Evalir please wait. I'll be implementing the latest changes and updated logic over the weekend 🙏🏼

@xeno097
Copy link
Contributor Author

xeno097 commented Jul 31, 2023

Hey @Evalir @mds1 I have just made the latest changes. Please let me know if there is something that can be improved or something that is not clear when you can.

@xeno097 xeno097 requested a review from Evalir July 31, 2023 00:04
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

I'm still not quite sure of the implementation 😅 it's getting a bit complicated and I'm really worried about the complexity of the expectRevert cheatcode as a whole. Can't we slim this down somehow?

Comment on lines 206 to 211
// If the first expected revert has already been matched it means that all the expected
// reverts have been matched and we can safely clear the queque.
else {
state.matched_all_expected_reverts_with_address = true;
state.expected_reverts_with_address.clear();
}
Copy link
Member

Choose a reason for hiding this comment

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

I still don't really understand this—we're only really matching one event? So why not use an Option here? The vecdequeue can only hold at most one event. because we're clearing it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are matching multiple reverts because the user can call expectRevert with an address multiple times. So every time a revert happens it is compared against the first revert in the deque (because order matters).
If the revert matches with the first one then it is moved at the end of the queue, if not it is put in front once again until a matching revert is found. Following this cycle until the end of the call we could have the following scenarios:

  • The first element of the deque has been matched which means that all the other reverts after it have been matched too. This is because at some point all the reverts that have been matched have been moved at the end of the deque and the first one that initially wasn't matched is brought back to the front.
  • The first element of the dequeue has still not been matched which means that the expected revert was not thrown during EVM execution.

Comment on lines 134 to 136
/// Tracks if all the expected reverts associated to an address have been matched to alter the
/// root call return data from a revert to a simple return
pub matched_all_expected_reverts_with_address: bool,
Copy link
Member

Choose a reason for hiding this comment

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

If we need a vecdequeue here, can't we just map over it any see if all have been matched with an .any()?

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 used that boolean because I want to explicitly distinguish the scenario where the queue was empty from the start from the one where all the reverts have been matched and the queue was cleared. I can't simply check if the deque is empty because if the user did not use expectRevert with an address, modifying the return value here would be wrong.

image


// If `expected_reverts_with_address` is not empty then we did not match all the
// expected reverts with an address
if let Some(pending_expected_revert) = self.expected_reverts_with_address.pop_front() {
Copy link
Member

Choose a reason for hiding this comment

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

See: I don't really think we need a VecDequeue here if we're only checking a single event. It's b etter to use an Option 😅 it's making the whole code a bit more complicated than it needs to be

Copy link
Contributor Author

@xeno097 xeno097 Jul 31, 2023

Choose a reason for hiding this comment

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

I'm taking only the first element of the deque because it is the first one that did not match but there could be more after it.

@Evalir
Copy link
Member

Evalir commented Aug 14, 2023

@xeno097 will review this soon, sorry! could you please rebase? we recently flattened all crates into crates/ so it's causing a bit of pain on open prs

@xeno097 xeno097 force-pushed the expect-revert-from-address branch from 650c62e to 32386ea Compare August 15, 2023 16:20
@Evalir
Copy link
Member

Evalir commented Aug 18, 2023

Guys, being completely honest this PR has gotten huge and it changes a lot of very sensitive things when it comes to expect cheatcodes. @mds1 @xeno097 do you think we can just split this PR and get this in progressively? Ideally:

Sorry if this is a lot of work or derails the implementation, but this gets a tad hard to review with all the comments and we really do not wanna break this section of the code

@xeno097 xeno097 force-pushed the expect-revert-from-address branch from fde4eb3 to 72bd64d Compare August 22, 2023 23:29
@xeno097 xeno097 requested a review from Evalir August 22, 2023 23:30
@Evalir
Copy link
Member

Evalir commented Aug 30, 2023

@xeno097 just to understand—should i review this PR or are we gonna open two new ones splitting this feature?

@xeno097
Copy link
Contributor Author

xeno097 commented Aug 31, 2023

@xeno097 just to understand—should i review this PR or are we gonna open two new ones splitting this feature?

Hey @Evalir you can review this PR as it implements the cheat code as initially designed, I'll then open a new PR with the updated implementation to enable support for the new use case mentioned here #5430 (comment)

@Evalir
Copy link
Member

Evalir commented Oct 16, 2023

Hey all—will close this as stale for now, we can come back to this later. We're undergoing a huge cheatcode alloy migration now and it'll be come a pain to migrate this—it will be better to start this from scratch once we're done @xeno097

@Evalir Evalir closed this Oct 16, 2023
@xeno097
Copy link
Contributor Author

xeno097 commented Oct 17, 2023

Hey @Evalir no problem I'll pick this up once the migration is completed!

@mattsse mattsse reopened this Oct 24, 2023
@mattsse
Copy link
Member

mattsse commented Oct 24, 2023

reopening just so we don't lose it, because we still want this

@Evalir
Copy link
Member

Evalir commented Nov 7, 2023

gm @xeno097 — we can come back to this as cheatcodes have been refactored.

@zerosnacks zerosnacks self-assigned this Jul 2, 2024
@zerosnacks
Copy link
Member

Hi @xeno097, thanks for your PR! Would be great to get this merged. Would you be interested in updating the PR? If not I can pick it up from here as well

@xeno097
Copy link
Contributor Author

xeno097 commented Jul 26, 2024

Hey @zerosnacks unfortunately at the moment I'm unable to continue working on this PR, I'd be pleased if you completed it for me thanks 🔥

@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 31, 2024
@zerosnacks zerosnacks added the A-cheatcodes Area: cheatcodes label Jul 31, 2024
@yash-atreya
Copy link
Member

Superseded by #8770

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes
Projects
None yet
6 participants