fix(forge): selectively enable Etherscan trace resolving when a test in ran in a forked environment and return block number in trace on a failed test #7606
Conversation
…est basis for whether we need to resolve via Etherscan or not
crates/forge/src/result.rs
Outdated
| /// Various types of test environments | ||
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub enum TestEnvironment { |
There was a problem hiding this comment.
I like this, this makes it easier to add more modes
…electFork inside of a function) we need to capture the environment
…h successful and failing tests
Closes: foundry-rs/foundry#6792 Due to a small logical error and a missing smoke test the Etherscan cache was constantly being invalidated. On a large codebase with 197 tests, without this optimization: foundry-rs/foundry#7606 Comparison: 1. Ran 17 test suites in 53.41s (64.87s CPU time): 197 tests passed, 0 failed, 0 skipped (197 total tests) (no cache) 2. Ran 17 test suites in 6.29s (49.10s CPU time): 197 tests passed, 0 failed, 0 skipped (197 total tests) (with cache)
crates/forge/tests/fixtures/can_emit_block_number_in_trace.stderr
Outdated
Show resolved
Hide resolved
… block contexts to avoid unnecessary tracking overhead
| pub struct ContextCollector { | ||
| /// The collected execution contexts. | ||
| pub contexts: Vec<Context>, | ||
| } |
There was a problem hiding this comment.
why do we need an inspector for this?
There was a problem hiding this comment.
This was added as an inspector so that it would run on every call occurring inside the function body, that way we were able to track the modifications made to the environment (such as the updating of the block number).
It isn't strictly necessary as we only need the most recent change to the block env, excluding a reset to genesis
| let block_number = ecx.inner.env.block.number; | ||
|
|
||
| // Skip if the block number is at genesis | ||
| if block_number == U256::from(1) { | ||
| return None; | ||
| } | ||
|
|
||
| // Skip if the previous context is the same | ||
| if let Some(Context { block_number: prev_block_number }) = self.contexts.last() { | ||
| if *prev_block_number == block_number { | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| // Push the new context | ||
| self.contexts.push(Context { block_number }); |
There was a problem hiding this comment.
hmm, not sure this will work, because there are also cheatcodes that modify the block number.
the fork context is essentially already tracked in the Backend internally.
can we instead extract this after executing a test in the testresult?
There was a problem hiding this comment.
The cheatcodes overwrite the env of the backend referenced here
foundry/crates/evm/core/src/backend/mod.rs
Lines 782 to 783 in ba399ae
By extracting it after the test we can only capture the last context as the backend overwrites the env, sufficient for the current usecase
…orming block env that gets overwritten in the backend, if not implemented as an inspector we would not be able to capture the modifications made by cheatcodes
zerosnacks
changed the title
zerosnacks
changed the title
|
Closing as stale as the PR as it is up now is insufficient #7574 is still relevant and can be picked up separately Conditional Etherscan trace resolving not worth spending more time on given the cache busting fix in foundry-rs/block-explorers#40 |
Motivation
Closes #7555 (bug)
Closes #7312 (bug)
Closes #7574 (feature)
In short, when an
ETHERSCAN_API_KEYis defined in a users'.envit would trigger Etherscan trace resolving for local tests which would result in a massive slowdown after running into rate limiting.Solution
Implements a way to mark test results as whether having ran in a forked or non-forked environment. This is then used to selectively enable or disable Etherscan tracing (without re-initializing).
As requested in #7574, upon a encountering a test failure in a forked test it will now display the fork block number. On succesful runs it is not displayed.
Known limitations:
vm.roll(n)local forks are createdGiven that trace caching is now resolved these limitations are not that big of a deal, at worst the experience is as bad as it is now but only for the initial run.
The chosen approach was to add high-level context tracking which can easily be expanded to track additional call context related information.