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

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

Closed
wants to merge 52 commits into from
Closed
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
68bfdb8
add has_forks to backend
zerosnacks Apr 8, 2024
7251ef4
Merge branch 'master' into zerosnacks/fix-tracing-regression
zerosnacks Apr 8, 2024
5eca07c
pass down environment, add dynamic enabled / disabled flag on a per t…
zerosnacks Apr 9, 2024
004dc11
add clarifying comment
zerosnacks Apr 9, 2024
46ae039
check has_forks at test runtime
zerosnacks Apr 9, 2024
acba4c4
pass down block number
zerosnacks Apr 9, 2024
f3fc621
fix build issues, cast block number as u64
zerosnacks Apr 9, 2024
d63e4fc
flip order
zerosnacks Apr 9, 2024
0ff4a3f
clarify
zerosnacks Apr 9, 2024
c42c3a6
remove unused code
zerosnacks Apr 9, 2024
f9ad7c0
Merge branch 'master' into zerosnacks/fix-tracing-regression
zerosnacks Apr 10, 2024
c5a4690
simplify by making enable_etherscan flip based on parameter `yes`
zerosnacks Apr 10, 2024
15a9bfd
add environment report formatter, add block on failure
zerosnacks Apr 10, 2024
7e4bb99
add block format to test, looks incorrect, especially in the USDTCall…
zerosnacks Apr 10, 2024
c473f01
fix, has_forks was inverted
zerosnacks Apr 10, 2024
7f1f3c2
Merge branch 'master' into zerosnacks/fix-tracing-regression
zerosnacks Apr 15, 2024
efc05d3
to be able to capture forking state inside of calls (as in vm.createS…
zerosnacks Apr 15, 2024
d2ba46b
filter out block number at genesis, assume non-forked environment in …
zerosnacks Apr 15, 2024
5ae23e5
add basic tests cases
zerosnacks Apr 15, 2024
4edb749
simplify has_forks check, we can simply check `issued_local_forks_ids…
zerosnacks Apr 15, 2024
8fb7445
add specific test for verbose tracing and non verbose tracing for bot…
zerosnacks Apr 15, 2024
676b249
revert simplification
zerosnacks Apr 15, 2024
d48c3b3
add note
zerosnacks Apr 15, 2024
b48a587
pull in latest master
zerosnacks Apr 16, 2024
5404c5c
revert repro_6531, clean up comments
zerosnacks Apr 16, 2024
059decc
make sure to output block number in both test results as well as the …
zerosnacks Apr 16, 2024
5799cc2
Merge branch 'master' into zerosnacks/fix-tracing-regression
zerosnacks Apr 22, 2024
07dde75
merge in master
zerosnacks Apr 23, 2024
07fceb5
remove env passing, to implement an alternative, move etherscan toggl…
zerosnacks Apr 23, 2024
bfca63c
add context capturing, collecting all block changes during execution
zerosnacks Apr 23, 2024
85bb139
fix overflow
zerosnacks Apr 23, 2024
39807c9
fix clippy
zerosnacks Apr 23, 2024
56e1006
add test trace for multiblock fork
zerosnacks Apr 23, 2024
52e2ca5
fix title
zerosnacks Apr 23, 2024
28986ac
add context tracking to setup
zerosnacks Apr 23, 2024
ad74f98
add context tracking support for fuzz and invariant counter example c…
zerosnacks Apr 23, 2024
4fcbdce
enhance with context, move definition to core
zerosnacks Apr 23, 2024
a2ba880
re-add environment with context
zerosnacks Apr 23, 2024
dd2592b
fix clippy
zerosnacks Apr 23, 2024
715d8b2
Merge branch 'master' into zerosnacks/fix-tracing-regression
zerosnacks Apr 25, 2024
f6310ff
appears formatting issue could relate to diff error throw
zerosnacks Apr 25, 2024
00897fa
update trace
zerosnacks Apr 25, 2024
163e337
add fuzz test case, also logs block correctly - avoid pushing genesis…
zerosnacks Apr 25, 2024
e5037d1
remove debug trace
zerosnacks Apr 25, 2024
2b6865f
improve documentation
zerosnacks Apr 25, 2024
b311550
pull in master
zerosnacks Apr 30, 2024
1536799
only rely on context tracing to determine fork status
zerosnacks Apr 30, 2024
af34ff8
remove context as inspector, simply inline
zerosnacks Apr 30, 2024
d42eb2e
remove inspector, only rely on executor env block number at the end o…
zerosnacks Apr 30, 2024
2e5918b
re-add inspector - cleaner than inlining and allows capture of transf…
zerosnacks Apr 30, 2024
c0775aa
add comment on block number modification due to cheat codes
zerosnacks Apr 30, 2024
747e17c
remove wrongly merged funcs file, avoid formatting diff
zerosnacks Apr 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion crates/evm/core/src/backend/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,11 @@ impl Backend {
}
}

/// Returns whether the backend has any forks
pub fn has_forks(&self) -> bool {
self.inner.has_forks()
}

/// Returns all snapshots created in this backend
pub fn snapshots(&self) -> &Snapshots<BackendSnapshot<BackendDatabaseSnapshot>> {
&self.inner.snapshots
Expand Down Expand Up @@ -1539,7 +1544,7 @@ pub struct BackendInner {
/// issued _local_ numeric identifier, that remains constant, even if the underlying fork
/// backend changes.
pub issued_local_fork_ids: HashMap<LocalForkId, ForkId>,
/// tracks all the created forks
/// Tracks all the created forks
/// Contains the index of the corresponding `ForkDB` in the `forks` vec
pub created_forks: HashMap<ForkId, ForkLookupIndex>,
/// Holds all created fork databases
Expand Down Expand Up @@ -1598,6 +1603,19 @@ impl BackendInner {
self.ensure_fork_index(self.ensure_fork_id(id)?)
}

/// Returns whether there are any forks
pub fn has_forks(&self) -> bool {
if self.launched_with_fork.is_some() {
return true;
}

if !self.created_forks.is_empty() {
return true;
}

self.forks.iter().any(|fork| fork.is_some())
}

/// Returns the underlying fork mapped to the index
#[track_caller]
fn get_fork(&self, idx: ForkLookupIndex) -> &Fork {
Expand Down
12 changes: 12 additions & 0 deletions crates/evm/traces/src/identifier/etherscan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ pub struct EtherscanIdentifier {
invalid_api_key: Arc<AtomicBool>,
pub contracts: BTreeMap<Address, Metadata>,
pub sources: BTreeMap<u32, String>,
// Tracks whether the Etherscan identifier is enabled
// Enabled for forking tests
// Disabled for local tests
pub enabled: bool,
}

impl EtherscanIdentifier {
Expand All @@ -53,9 +57,17 @@ impl EtherscanIdentifier {
invalid_api_key: Arc::new(AtomicBool::new(false)),
contracts: BTreeMap::new(),
sources: BTreeMap::new(),
// By default, the Etherscan identifier is enabled to cover edge cases.
// It is disabled for local tests and enabled for forked tests on a per test basis.
enabled: true,
}))
}

/// Enables or disables the Etherscan identifier.
pub fn enable(&mut self, yes: bool) {
self.enabled = yes;
}

/// Goes over the list of contracts we have pulled from the traces, clones their source from
/// Etherscan and compiles them locally, for usage in the debugger.
pub async fn get_compiled_contracts(&self) -> eyre::Result<ContractSources> {
Expand Down
11 changes: 10 additions & 1 deletion crates/evm/traces/src/identifier/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ impl TraceIdentifier for TraceIdentifiers<'_> {
identities.extend(local.identify_addresses(addresses.clone()));
}
if let Some(etherscan) = &mut self.etherscan {
identities.extend(etherscan.identify_addresses(addresses));
if etherscan.enabled {
identities.extend(etherscan.identify_addresses(addresses));
}
}
identities
}
Expand All @@ -86,6 +88,13 @@ impl<'a> TraceIdentifiers<'a> {
Ok(self)
}

/// Enables or disables the Etherscan identifier.
pub fn enable_etherscan(&mut self, yes: bool) {
if let Some(etherscan) = &mut self.etherscan {
etherscan.enable(yes);
}
}

/// Returns `true` if there are no set identifiers.
pub fn is_empty(&self) -> bool {
self.local.is_none() && self.etherscan.is_none()
Expand Down
9 changes: 8 additions & 1 deletion crates/forge/bin/cmd/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,8 @@ impl TestArgs {
let mut identifier = TraceIdentifiers::new().with_local(&known_contracts);

// Avoid using etherscan for gas report as we decode more traces and this will be expensive.
// Because users can enable forking through cheat codes, we cannot simply disable etherscan
// for non-forking tests here.
if !self.gas_report {
identifier = identifier.with_etherscan(&config, remote_chain_id)?;
}
Expand Down Expand Up @@ -423,7 +425,7 @@ impl TestArgs {

// Process individual test results, printing logs and traces when necessary.
for (name, result) in tests {
shell::println(result.short_result(name))?;
shell::println(result.short_success_result(name))?;

// We only display logs at level 2 and above
if verbosity >= 2 {
Expand Down Expand Up @@ -456,6 +458,11 @@ impl TestArgs {
let mut decoded_traces = Vec::with_capacity(result.traces.len());
for (kind, arena) in &result.traces {
if identify_addresses {
// Enable Etherscan decoding for forking tests.
// Disable Etherscan decoding for local tests to avoid unnecessary API calls
// that can slow down execution significantly if rate-limited.
identifier.enable_etherscan(result.is_fork());
zerosnacks marked this conversation as resolved.
Show resolved Hide resolved

decoder.identify(arena, &mut identifier);
}

Expand Down
124 changes: 95 additions & 29 deletions crates/forge/src/result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ impl TestOutcome {
let term = if failed > 1 { "tests" } else { "test" };
shell::println(format!("Encountered {failed} failing {term} in {suite_name}"))?;
for (name, result) in suite.failures() {
shell::println(result.short_result(name))?;
shell::println(result.short_failure_result(name))?;
}
shell::println("")?;
}
Expand Down Expand Up @@ -248,6 +248,11 @@ impl SuiteResult {
self.test_results.is_empty()
}

/// Whether this test suite has any forked tests.
pub fn has_fork(&self) -> bool {
self.test_results.values().any(|result| result.is_fork())
}

/// The number of tests in this test suite.
pub fn len(&self) -> usize {
self.test_results.len()
Expand Down Expand Up @@ -359,9 +364,12 @@ pub struct TestResult {
/// The decoded DSTest logging events and Hardhat's `console.log` from [logs](Self::logs).
pub decoded_logs: Vec<String>,

/// What kind of test this was
/// What kind of test this was.
pub kind: TestKind,

/// What kind of environment this test was run in.
pub environment: TestEnvironment,

/// Traces
#[serde(skip)]
pub traces: Traces,
Expand Down Expand Up @@ -424,15 +432,65 @@ impl TestResult {
Self { status: TestStatus::Failure, reason: Some(reason), ..Default::default() }
}

/// Returns `true` if this is the result of a fork test
pub fn is_fork(&self) -> bool {
matches!(self.environment, TestEnvironment::Fork { .. })
}

/// Returns `true` if this is the result of a fuzz test
pub fn is_fuzz(&self) -> bool {
matches!(self.kind, TestKind::Fuzz { .. })
}

/// Formats the test result into a string (for printing).
pub fn short_result(&self, name: &str) -> String {
/// Formats a successful test result into a string (for printing).
pub fn short_success_result(&self, name: &str) -> String {
format!("{self} {name} {}", self.kind.report())
}

/// Formats a failed test result into a string (for printing).
pub fn short_failure_result(&self, name: &str) -> String {
format!("{self} {name} {}{}", self.environment.report(), self.kind.report())
}
}

/// Various types of tests
#[derive(Clone, Debug, Serialize, Deserialize)]
pub enum TestKind {
/// A standard test that consists of calling the defined solidity function
///
/// Holds the consumed gas
Standard(u64),
/// A solidity fuzz test, that stores all test cases
Fuzz {
/// we keep this for the debugger
first_case: FuzzCase,
runs: usize,
mean_gas: u64,
median_gas: u64,
},
/// A solidity invariant test, that stores all test cases
Invariant { runs: usize, calls: usize, reverts: usize },
}

impl Default for TestKind {
fn default() -> Self {
Self::Standard(0)
}
}

impl TestKind {
/// The gas consumed by this test
pub fn report(&self) -> TestKindReport {
match self {
TestKind::Standard(gas) => TestKindReport::Standard { gas: *gas },
TestKind::Fuzz { runs, mean_gas, median_gas, .. } => {
TestKindReport::Fuzz { runs: *runs, mean_gas: *mean_gas, median_gas: *median_gas }
}
TestKind::Invariant { runs, calls, reverts } => {
TestKindReport::Invariant { runs: *runs, calls: *calls, reverts: *reverts }
}
}
}
}

/// Data report by a test.
Expand Down Expand Up @@ -472,42 +530,50 @@ impl TestKindReport {
}
}

/// Various types of tests
/// Various types of test environments
#[derive(Clone, Debug, Serialize, Deserialize)]
pub enum TestKind {
/// A standard test that consists of calling the defined solidity function
///
/// Holds the consumed gas
Standard(u64),
/// A solidity fuzz test, that stores all test cases
Fuzz {
/// we keep this for the debugger
first_case: FuzzCase,
runs: usize,
mean_gas: u64,
median_gas: u64,
pub enum TestEnvironment {
/// A standard test environment
Standard,
/// A forked test environment
Fork {
/// The block number at which the test was executed
block_number: u64,
Copy link
Member

Choose a reason for hiding this comment

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

what if the test uses multiple forks at different heights?

what are we using the block number for?

Copy link
Member Author

Choose a reason for hiding this comment

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

the block number is used for #7574

Copy link
Member Author

Choose a reason for hiding this comment

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

Since been updated, I've added a test case for tracking multiple forks. For now we collect the block heights of the multiple forks but only refer to the last one (the failure case) when we decorate the failure case with the block height

},
/// A solidity invariant test, that stores all test cases
Invariant { runs: usize, calls: usize, reverts: usize },
}

impl Default for TestKind {
impl Default for TestEnvironment {
fn default() -> Self {
Self::Standard(0)
Self::Standard
}
}

impl TestKind {
/// The gas consumed by this test
pub fn report(&self) -> TestKindReport {
impl TestEnvironment {
// The environment in which the test was run
pub fn report(&self) -> TestEnvironmentReport {
match self {
TestKind::Standard(gas) => TestKindReport::Standard { gas: *gas },
TestKind::Fuzz { runs, mean_gas, median_gas, .. } => {
TestKindReport::Fuzz { runs: *runs, mean_gas: *mean_gas, median_gas: *median_gas }
TestEnvironment::Standard => TestEnvironmentReport::Standard,
TestEnvironment::Fork { block_number } => {
TestEnvironmentReport::Fork { block_number: *block_number }
}
TestKind::Invariant { runs, calls, reverts } => {
TestKindReport::Invariant { runs: *runs, calls: *calls, reverts: *reverts }
}
}
}

/// Environment report by a test.
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum TestEnvironmentReport {
Standard,
Fork { block_number: u64 },
}

impl fmt::Display for TestEnvironmentReport {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
TestEnvironmentReport::Fork { block_number } => {
write!(f, "(block: {block_number}) ")
}
_ => Ok(()),
}
}
}
Expand Down
Loading
Loading