Merged
Conversation
2 tasks
gakonst
approved these changes
Apr 7, 2022
Member
gakonst
left a comment
There was a problem hiding this comment.
Clean - also great example of how a proper bugfix PR should be (clear description of the issue and regression test)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reasoning about depth
callis called before the depth is increased. This effectively means thatdepthincallis the depth of the current call, anddepth + 1is the depth of the call we are about to execute.call_endis called after the depth is decreased. This effectively means thatdepth + 1is the depth of the call that just finished executingImplications:
depth == 0, then the root call (depth 1 during execution, but depth 0 incall/call_end) is terminatingdepth + 1represents the depth of the call that is about to execute incall, and the depth of the call that just finished executing incall_enddepth - 1represents the depth at which the current call will terminate incallandcall_enddepthdepthin calls to cheatcodes represent the depth of the current call, not the depth of the call to the cheatcode.Illustrated, if you have:
Then:
callmethods are invoked withdepth == 0when we first calltestSomethingcallmethods are invoked withdepth == 1when we call the cheatcodecall_endmethods are invoked withdepth == 1when the call to the cheatcode has finishedcallmethods are invoked withdepth == 1when we callsomewhere.something()call_endmethods are invoked withdepth == 1when thesomewhere.something()call has finishedcall_endmethods are invoked withdepth == 0when thetestSomethingcall has finishedexpectEmitbug (https://github.com/gakonst/foundry/issues/1214)This means that if we do:
Then:
callmethods are invoked withdepth == 0when we first calltestSomeEmitcallmethods are invoked withdepth == 1when we call the cheatcodedepth == 1call_endmethods are invoked withdepth == 1when the call to the cheatcode has finishedcall_endhandler since we just exited a cheatcode callcallmethods are invoked withdepth == 1when we callsomewhere.someValue()call_endmethods are invoked withdepth == 1when thesomewhere.someValue()call has finishedIn fact we should test for the expected emit at
depth - 1. In other words, when the call from which the cheatcode was invoked ends.tx.originbug (https://github.com/gakonst/foundry/issues/1210)Whenever
callis invoked, we check if the depth is greater than or equal to the depth at which the cheatcode was invoked. If it is, then we check:depthis exactly the depth at which the cheatcode was invoked, then we setmsg.sender. This is sound because we only want to prank the firstmsg.sender, not the nested ones in case of nesting. If the depth is greater than or equal to the depth at which the cheatcode was invoked, we settx.origin, which is sound, because we want this to persist even in the case of nesting.Whenever
call_endis invoked, we check if we have an ongoing prank. If we do, then we check if the prank is for one call only. If it is, then we erase the prank from memory, so we don't setmsg.senderin subsequent calls. This is sound. However, we always resettx.originto the original value, regardless of the depth of the call that is terminating. This is sound when we usestartPrank, since it will just continually reapplytx.origin, but forprankthis is unsound. We should only do that when we are back at the depth at which the prank was started.Closes #1214
Closes #1210