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

feat(era-cheatcodes): expectRevert cheatcode #200

Merged
merged 21 commits into from
Jan 5, 2024

Conversation

Karrq
Copy link
Contributor

@Karrq Karrq commented Dec 15, 2023

What 💻

  • expectRevert implementations and test
  • expectRevert works with general messages
  • expectRevert works with CustomErrors
  • expectRevert works with low level calls

How

Once the cheatcode expectRevert set, we have to check the next call, is it success or revert.

To do it, after cheatcode installation, we check returns on a certain depth for reverting or correct execution. Unfortunately, after setting this cheatcode we have at least one return on this depth, it's a return from cheatcode contract, and after we probably have one another return 0x..8002 address (ACCOUNT_CODE_STORAGE_ADDRESS), it's a system contract, that returns correct bytecode, based on address. So we have to skip either 1 or 2 returns before the necessary return.

Once we found the necessary return, we have to check it and probably change the execution path. E.g. convert revert to ok.

And it's the place where magic starts. We have pc and it represents the next command to execute.
So, we have 2 possible paths:

  1. Handle exception
  2. Continue execution

The exception handler is placed in the current depth of the callstack, because it's a property of return, but the continue pc is placed in the desired depth, depth - 1.
So for proper jump to the necessary path we have to grab two pc. and later on jump to one of them.

Evidence 📷

@Karrq Karrq self-assigned this Dec 15, 2023
@Karrq Karrq force-pushed the karrq/feat/cheatcode-expectrevert branch from d93045e to 2ff4e5a Compare December 15, 2023 08:34
Karrq and others added 4 commits December 18, 2023 11:30
fix(era:cc:expectRevert): offset expected depth
fix(era:cc:expectRevert): check only shallower than expected
feat(era:cheatcodes): delay action by n statements
refactor(era:cc): recurring actions
@Karrq Karrq force-pushed the karrq/feat/cheatcode-expectrevert branch from 2a2b999 to 3af1a58 Compare December 20, 2023 22:00
@Karrq Karrq force-pushed the karrq/feat/cheatcode-expectrevert branch from 6c54bb5 to 5606e38 Compare December 22, 2023 12:47
@Deniallugo Deniallugo force-pushed the karrq/feat/cheatcode-expectrevert branch from 9323302 to c6da906 Compare January 4, 2024 20:29
@Karrq Karrq changed the title [WIP] expectRevert cheatcode [WIP] feat(era-cheatcodes): expectRevert cheatcode Jan 4, 2024
@Deniallugo Deniallugo changed the title [WIP] feat(era-cheatcodes): expectRevert cheatcode feat(era-cheatcodes): expectRevert cheatcode Jan 5, 2024
@Deniallugo Deniallugo force-pushed the karrq/feat/cheatcode-expectrevert branch 4 times, most recently from 05ed383 to 39b750a Compare January 5, 2024 12:00
Signed-off-by: Danil <[email protected]>
@Deniallugo Deniallugo force-pushed the karrq/feat/cheatcode-expectrevert branch from 39b750a to 7a282c1 Compare January 5, 2024 12:01
@Deniallugo Deniallugo marked this pull request as ready for review January 5, 2024 12:10
Signed-off-by: Danil <[email protected]>
@Deniallugo Deniallugo force-pushed the karrq/feat/cheatcode-expectrevert branch from 0449a36 to 243d072 Compare January 5, 2024 12:25
Comment on lines +148 to +153
// function testExpectRevertCallsThenReverts() public {
// Reverter reverter = new Reverter();
// Dummy dummy = new Dummy();
// cheatcodes.expectRevert("called a function and then reverted");
// reverter.callThenRevert(dummy, "called a function and then reverted");
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these cases not yet supported?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume @Karrq copypasted them from the cheatcodes and we will activate it, once we are supporting cheat codes calls directly

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Karrq Can these be enabled (using the the long form call syntax)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I did copy paste, and I simply had these removed for now not for the direct calls but to simplify a bit the tests honestly. These should be enabled (and fixed)

Copy link
Collaborator

@nbaztec nbaztec left a comment

Choose a reason for hiding this comment

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

Some minor comments, and in general it makes sense to add documentation to most items in the PR, given its complex nature

Signed-off-by: Danil <[email protected]>
@nbaztec nbaztec merged commit d5de05e into main Jan 5, 2024
10 checks passed
@nbaztec nbaztec deleted the karrq/feat/cheatcode-expectrevert branch January 5, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants