Fix single broadcast/prank nonce setting

[klkvr]

Motivation

Closes #5635

Solution

Remove single call broadcast/prank from cheatcodes inspector only when we have came back to the broadcast/prank initialization depth

I think this should get a careful review from someone with deep knowledge of evm crate architecture

[nhtyy]

Nice! This makes sense to me wrt the delegatecall issue.

but i don't think this will fix the issue of the broadcast field on the cheatcode inspector being set to None if there is two broadcast calls in the same script

maybe @Evalir can clear up semantics here

[klkvr]

The issue actualy happens not only with delegatecalls, but with any broadcasted calls which result in subsequent calls/delegatecalls to other contracts because we are dropping broadcast field at the end of the first such call

For me it solved the issue with two broadcasts in the same script which I explained in the issue

[nhtyy]

oh yeah @klkvr just tested and you're totally right my bad

while it is true that broadcast is set to None, on the next vm.broadcast, which is a call to the HEVM address, theres a call to cheatcodes::env::apply in the cheatcode inspector call that leads to this function being executed which resets the broadcast field!

foundry/crates/evm/src/executor/inspector/cheatcodes/env.rs

Lines 110 to 128 in 95a93cd

	fn broadcast(
	state: &mut Cheatcodes,
	new_origin: Address,
	original_caller: Address,
	original_origin: Address,
	depth: u64,
	single_call: bool,
	) -> Result {
	ensure!(
	state.prank.is_none(),
	"You have an active prank. Broadcasting and pranks are not compatible. \
	Disable one or the other"
	);
	ensure!(state.broadcast.is_none(), "You have an active broadcast already.");
	let broadcast = Broadcast { new_origin, original_origin, original_caller, depth, single_call };
	state.broadcast = Some(broadcast);
	Ok(Bytes::new())
	}

So ya great find and great fix!

[Evalir]

lgtm! just gonna be really careful with this and test

[Evalir]

Let's add docs on the rationale as to why this is applied iff the outer if runs—it's a change on the previous behavior and I'm a bit apprehensive it might "fix" (or break) behavior that has come to be expected. Will test this locally

[Evalir]

ditto here and the others

[klkvr]

Hey @Evalir, I have added some comments. However, IMO, the previous behavior was slightly less intuitive. I'm not entirely sure if we actually need an explanation here. The broadcast cleaning outside of the if data.journaled_state.depth() == broadcast.depth condition resulted in the logic inside that if statement not being executed in certain cases, which seems to be unexpected. So, the current implementation looks more suspicious to me. But yes, it really needs review and should be documented even if we decide to keep the current behavior