From db963934e6a7c8029abccb172991b8dd8969d42a Mon Sep 17 00:00:00 2001 From: evalir Date: Wed, 10 May 2023 06:55:58 -0400 Subject: [PATCH 01/12] feat(cheatcodes): restrict cheatcode usage on precompiles (#4905) * feat(cheatcodes): restrict cheatcodes on precompiles * chore: exclude address(0) from precompiles check * chore: fix test * chore: add revert tests --- evm/src/executor/inspector/cheatcodes/env.rs | 8 +++++++- evm/src/executor/inspector/cheatcodes/util.rs | 5 +++++ testdata/cheats/Deal.t.sol | 2 +- testdata/cheats/Etch.t.sol | 9 +++++++++ testdata/cheats/Load.t.sol | 7 +++++++ testdata/cheats/Store.t.sol | 10 ++++++++++ 6 files changed, 39 insertions(+), 2 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/env.rs b/evm/src/executor/inspector/cheatcodes/env.rs index d83bf56ac80a5..da688b8a6edf0 100644 --- a/evm/src/executor/inspector/cheatcodes/env.rs +++ b/evm/src/executor/inspector/cheatcodes/env.rs @@ -3,7 +3,10 @@ use crate::{ abi::HEVMCalls, executor::{ backend::DatabaseExt, - inspector::cheatcodes::{util::with_journaled_account, DealRecord}, + inspector::cheatcodes::{ + util::{is_potential_precompile, with_journaled_account}, + DealRecord, + }, }, utils::{b160_to_h160, h160_to_b160, ru256_to_u256, u256_to_ru256}, }; @@ -232,6 +235,7 @@ pub fn apply( Bytes::new() } HEVMCalls::Store(inner) => { + ensure!(!is_potential_precompile(inner.0), "Store cannot be used on precompile addresses (N < 10). Please use an address bigger than 10 instead"); data.journaled_state.load_account(h160_to_b160(inner.0), data.db)?; // ensure the account is touched data.journaled_state.touch(&h160_to_b160(inner.0)); @@ -245,6 +249,7 @@ pub fn apply( Bytes::new() } HEVMCalls::Load(inner) => { + ensure!(!is_potential_precompile(inner.0), "Load cannot be used on precompile addresses (N < 10). Please use an address bigger than 10 instead"); // TODO: Does this increase gas usage? data.journaled_state.load_account(h160_to_b160(inner.0), data.db)?; let (val, _) = data.journaled_state.sload( @@ -257,6 +262,7 @@ pub fn apply( HEVMCalls::Breakpoint0(inner) => add_breakpoint(state, caller, &inner.0, true)?, HEVMCalls::Breakpoint1(inner) => add_breakpoint(state, caller, &inner.0, inner.1)?, HEVMCalls::Etch(inner) => { + ensure!(!is_potential_precompile(inner.0), "Etch cannot be used on precompile addresses (N < 10). Please use an address bigger than 10 instead"); let code = inner.1.clone(); trace!(address=?inner.0, code=?hex::encode(&code), "etch cheatcode"); // TODO: Does this increase gas usage? diff --git a/evm/src/executor/inspector/cheatcodes/util.rs b/evm/src/executor/inspector/cheatcodes/util.rs index d86b71a72346e..fe4827dfba209 100644 --- a/evm/src/executor/inspector/cheatcodes/util.rs +++ b/evm/src/executor/inspector/cheatcodes/util.rs @@ -350,6 +350,11 @@ pub fn check_if_fixed_gas_limit( && call_gas_limit > 2300 } +/// Small utility function that checks if an address is a potential precompile. +pub fn is_potential_precompile(address: H160) -> bool { + address < H160::from_low_u64_be(10) && address != H160::zero() +} + #[cfg(test)] mod tests { use super::*; diff --git a/testdata/cheats/Deal.t.sol b/testdata/cheats/Deal.t.sol index eabe5c72d7373..efd37d585776d 100644 --- a/testdata/cheats/Deal.t.sol +++ b/testdata/cheats/Deal.t.sol @@ -8,7 +8,7 @@ contract DealTest is DSTest { Cheats constant cheats = Cheats(HEVM_ADDRESS); function testDeal(uint256 amount) public { - address target = address(1); + address target = address(10); assertEq(target.balance, 0, "initial balance incorrect"); // Give half the amount diff --git a/testdata/cheats/Etch.t.sol b/testdata/cheats/Etch.t.sol index 1c1caf771de98..e92904dfc2171 100644 --- a/testdata/cheats/Etch.t.sol +++ b/testdata/cheats/Etch.t.sol @@ -13,4 +13,13 @@ contract EtchTest is DSTest { cheats.etch(target, code); assertEq(string(code), string(target.code)); } + + function testEtchNotAvailableOnPrecompiles() public { + address target = address(1); + bytes memory code = hex"1010"; + cheats.expectRevert( + bytes("Etch cannot be used on precompile addresses (N < 10). Please use an address bigger than 10 instead") + ); + cheats.etch(target, code); + } } diff --git a/testdata/cheats/Load.t.sol b/testdata/cheats/Load.t.sol index 2fc2e12de576c..b2ab39761b506 100644 --- a/testdata/cheats/Load.t.sol +++ b/testdata/cheats/Load.t.sol @@ -26,6 +26,13 @@ contract LoadTest is DSTest { assertEq(val, 20, "load failed"); } + function testLoadNotAvailableOnPrecompiles() public { + cheats.expectRevert( + bytes("Load cannot be used on precompile addresses (N < 10). Please use an address bigger than 10 instead") + ); + uint256 val = uint256(cheats.load(address(1), bytes32(0))); + } + function testLoadOtherStorage() public { uint256 val = uint256(cheats.load(address(store), bytes32(0))); assertEq(val, 10, "load failed"); diff --git a/testdata/cheats/Store.t.sol b/testdata/cheats/Store.t.sol index c5ef0c9261dc0..ea9529877bd1f 100644 --- a/testdata/cheats/Store.t.sol +++ b/testdata/cheats/Store.t.sol @@ -26,6 +26,16 @@ contract StoreTest is DSTest { assertEq(store.slot1(), 20, "store failed"); } + function testStoreNotAvailableOnPrecompiles() public { + assertEq(store.slot0(), 10, "initial value for slot 0 is incorrect"); + assertEq(store.slot1(), 20, "initial value for slot 1 is incorrect"); + + cheats.expectRevert( + bytes("Store cannot be used on precompile addresses (N < 10). Please use an address bigger than 10 instead") + ); + cheats.store(address(1), bytes32(0), bytes32(uint256(1))); + } + function testStoreFuzzed(uint256 slot0, uint256 slot1) public { assertEq(store.slot0(), 10, "initial value for slot 0 is incorrect"); assertEq(store.slot1(), 20, "initial value for slot 1 is incorrect"); From 89f430c4534cee4aac18d4619fd1c0091d08cf08 Mon Sep 17 00:00:00 2001 From: evalir Date: Thu, 11 May 2023 04:52:20 -0400 Subject: [PATCH 02/12] feat(invariant): add `statefulFuzz` as an alias to `invariant` (#4922) * feat(tests): add statefulFuzz alias to invariant * chore: tests --- common/src/traits.rs | 2 +- forge/tests/it/invariant.rs | 11 ++++++++++- testdata/fuzz/invariant/common/InvariantTest1.t.sol | 4 ++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/common/src/traits.rs b/common/src/traits.rs index 1669635a5f4bc..b660206278853 100644 --- a/common/src/traits.rs +++ b/common/src/traits.rs @@ -51,7 +51,7 @@ impl TestFunctionExt for Function { impl<'a> TestFunctionExt for &'a str { fn is_invariant_test(&self) -> bool { - self.starts_with("invariant") + self.starts_with("invariant") || self.starts_with("statefulFuzz") } fn is_fuzz_test(&self) -> bool { diff --git a/forge/tests/it/invariant.rs b/forge/tests/it/invariant.rs index 211a88de1b70c..a31d78c3e3bb1 100644 --- a/forge/tests/it/invariant.rs +++ b/forge/tests/it/invariant.rs @@ -30,7 +30,16 @@ fn test_invariant() { ), ( "fuzz/invariant/common/InvariantTest1.t.sol:InvariantTest", - vec![("invariant_neverFalse()", false, Some("false.".into()), None, None)], + vec![ + ("invariant_neverFalse()", false, Some("false.".into()), None, None), + ( + "statefulFuzz_neverFalseWithInvariantAlias()", + false, + Some("false.".into()), + None, + None, + ), + ], ), ( "fuzz/invariant/target/ExcludeContracts.t.sol:ExcludeContracts", diff --git a/testdata/fuzz/invariant/common/InvariantTest1.t.sol b/testdata/fuzz/invariant/common/InvariantTest1.t.sol index 0668a46ba03ae..b011fa518b4ae 100644 --- a/testdata/fuzz/invariant/common/InvariantTest1.t.sol +++ b/testdata/fuzz/invariant/common/InvariantTest1.t.sol @@ -32,4 +32,8 @@ contract InvariantTest is DSTest { function invariant_neverFalse() public { require(inv.flag1(), "false."); } + + function statefulFuzz_neverFalseWithInvariantAlias() public { + require(inv.flag1(), "false."); + } } From c1dbafd6faed950c8da31139a9aa786aa6cd3bf0 Mon Sep 17 00:00:00 2001 From: Amar Singh Date: Thu, 11 May 2023 04:56:59 -0400 Subject: [PATCH 03/12] feat(forge): change startPrank to overwrite existing prank instead of erroring (#4826) * init start change prank * testChangePrank * revert startChangePrank and change startPrank to overwrite existing prank instead of erroring as per review suggestion * add tests for prank0 after prank1 and prank1 after prank0 * fmt * add error if prank is not used at least once before overwritten as per suggestion * fmt * unit test for startPrank0 - and startPrank1 - * fix * remove clones by only updating prank after first time applied * fmt * more readable names * chore: fix/add tests, use ensure util * chore: add missing edge case test --------- Co-authored-by: Matthias Seitz Co-authored-by: evalir --- evm/src/executor/inspector/cheatcodes/env.rs | 43 +++++- evm/src/executor/inspector/cheatcodes/mod.rs | 10 ++ testdata/cheats/Prank.t.sol | 153 +++++++++++++++++++ 3 files changed, 203 insertions(+), 3 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/env.rs b/evm/src/executor/inspector/cheatcodes/env.rs index da688b8a6edf0..9c9609f724def 100644 --- a/evm/src/executor/inspector/cheatcodes/env.rs +++ b/evm/src/executor/inspector/cheatcodes/env.rs @@ -51,6 +51,39 @@ pub struct Prank { pub depth: u64, /// Whether the prank stops by itself after the next call pub single_call: bool, + /// Whether the prank has been used yet (false if unused) + pub used: bool, +} + +impl Prank { + pub fn new( + prank_caller: Address, + prank_origin: Address, + new_caller: Address, + new_origin: Option
, + depth: u64, + single_call: bool, + ) -> Prank { + Prank { + prank_caller, + prank_origin, + new_caller, + new_origin, + depth, + single_call, + used: false, + } + } + + /// Apply the prank by setting `used` to true iff it is false + /// Only returns self in the case it is updated (first application) + pub fn first_time_applied(&self) -> Option { + if self.used { + None + } else { + Some(Prank { used: true, ..self.clone() }) + } + } } /// Sets up broadcasting from a script using `origin` as the sender @@ -102,14 +135,18 @@ fn prank( depth: u64, single_call: bool, ) -> Result { + let prank = Prank::new(prank_caller, prank_origin, new_caller, new_origin, depth, single_call); + + if let Some(Prank { used, .. }) = state.prank { + ensure!(used, "You cannot overwrite `prank` until it is applied at least once"); + } + ensure!( state.broadcast.is_none(), - "You cannot `prank` for a broadcasted transaction. \ + "You cannot `prank` for a broadcasted transaction.\ Pass the desired tx.origin into the broadcast cheatcode call" ); - ensure!(state.prank.is_none(), "You have an active prank already."); - let prank = Prank { prank_caller, prank_origin, new_caller, new_origin, depth, single_call }; state.prank = Some(prank); Ok(Bytes::new()) } diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index ce824d935a09a..345e320f1dedc 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -607,15 +607,25 @@ where if data.journaled_state.depth() >= prank.depth && call.context.caller == h160_to_b160(prank.prank_caller) { + let mut prank_applied = false; // At the target depth we set `msg.sender` if data.journaled_state.depth() == prank.depth { call.context.caller = h160_to_b160(prank.new_caller); call.transfer.source = h160_to_b160(prank.new_caller); + prank_applied = true; } // At the target depth, or deeper, we set `tx.origin` if let Some(new_origin) = prank.new_origin { data.env.tx.caller = h160_to_b160(new_origin); + prank_applied = true; + } + + // If prank applied for first time, then update + if prank_applied { + if let Some(applied_prank) = prank.first_time_applied() { + self.prank = Some(applied_prank); + } } } } diff --git a/testdata/cheats/Prank.t.sol b/testdata/cheats/Prank.t.sol index fccccff7c9697..0dddb2ffe702d 100644 --- a/testdata/cheats/Prank.t.sol +++ b/testdata/cheats/Prank.t.sol @@ -118,6 +118,159 @@ contract PrankTest is DSTest { ); } + function testPrank1AfterPrank0(address sender, address origin) public { + // Perform the prank + address oldOrigin = tx.origin; + Victim victim = new Victim(); + cheats.prank(sender); + victim.assertCallerAndOrigin( + sender, "msg.sender was not set during prank", oldOrigin, "tx.origin was not set during prank" + ); + + // Ensure we cleaned up correctly + victim.assertCallerAndOrigin( + address(this), "msg.sender was not cleaned up", oldOrigin, "tx.origin invariant failed" + ); + + // Overwrite the prank + cheats.prank(sender, origin); + victim.assertCallerAndOrigin( + sender, "msg.sender was not set during prank", origin, "tx.origin invariant failed" + ); + + // Ensure we cleaned up correctly + victim.assertCallerAndOrigin( + address(this), "msg.sender was not cleaned up", oldOrigin, "tx.origin invariant failed" + ); + } + + function testPrank0AfterPrank1(address sender, address origin) public { + // Perform the prank + address oldOrigin = tx.origin; + Victim victim = new Victim(); + cheats.prank(sender, origin); + victim.assertCallerAndOrigin( + sender, "msg.sender was not set during prank", origin, "tx.origin was not set during prank" + ); + + // Ensure we cleaned up correctly + victim.assertCallerAndOrigin( + address(this), "msg.sender was not cleaned up", oldOrigin, "tx.origin invariant failed" + ); + + // Overwrite the prank + cheats.prank(sender); + victim.assertCallerAndOrigin( + sender, "msg.sender was not set during prank", oldOrigin, "tx.origin invariant failed" + ); + + // Ensure we cleaned up correctly + victim.assertCallerAndOrigin( + address(this), "msg.sender was not cleaned up", oldOrigin, "tx.origin invariant failed" + ); + } + + function testStartPrank0AfterPrank1(address sender, address origin) public { + // Perform the prank + address oldOrigin = tx.origin; + Victim victim = new Victim(); + cheats.startPrank(sender, origin); + victim.assertCallerAndOrigin( + sender, "msg.sender was not set during prank", origin, "tx.origin was not set during prank" + ); + + // Overwrite the prank + cheats.startPrank(sender); + victim.assertCallerAndOrigin( + sender, "msg.sender was not set during prank", oldOrigin, "tx.origin invariant failed" + ); + + cheats.stopPrank(); + // Ensure we cleaned up correctly after stopping the prank + victim.assertCallerAndOrigin( + address(this), "msg.sender was not cleaned up", oldOrigin, "tx.origin invariant failed" + ); + } + + function testStartPrank1AfterStartPrank0(address sender, address origin) public { + // Perform the prank + address oldOrigin = tx.origin; + Victim victim = new Victim(); + cheats.startPrank(sender); + victim.assertCallerAndOrigin( + sender, "msg.sender was not set during prank", oldOrigin, "tx.origin was set during prank incorrectly" + ); + + // Ensure prank is still up as startPrank covers multiple calls + victim.assertCallerAndOrigin( + sender, "msg.sender was cleaned up incorrectly", oldOrigin, "tx.origin invariant failed" + ); + + // Overwrite the prank + cheats.startPrank(sender, origin); + victim.assertCallerAndOrigin(sender, "msg.sender was not set during prank", origin, "tx.origin was not set"); + + // Ensure prank is still up as startPrank covers multiple calls + victim.assertCallerAndOrigin( + sender, "msg.sender was cleaned up incorrectly", origin, "tx.origin invariant failed" + ); + + cheats.stopPrank(); + // Ensure everything is back to normal after stopPrank + victim.assertCallerAndOrigin( + address(this), "msg.sender was not cleaned up", oldOrigin, "tx.origin invariant failed" + ); + } + + function testFailOverwriteUnusedPrank(address sender, address origin) public { + // Set the prank, but not use it + address oldOrigin = tx.origin; + Victim victim = new Victim(); + cheats.startPrank(sender, origin); + // try to overwrite the prank. This should fail. + cheats.startPrank(address(this), origin); + } + + function testFailOverwriteUnusedPrankAfterSuccessfulPrank(address sender, address origin) public { + // Set the prank, but not use it + address oldOrigin = tx.origin; + Victim victim = new Victim(); + cheats.startPrank(sender, origin); + victim.assertCallerAndOrigin( + sender, "msg.sender was not set during prank", origin, "tx.origin was set during prank incorrectly" + ); + cheats.startPrank(address(this), origin); + // try to overwrite the prank. This should fail. + cheats.startPrank(sender, origin); + } + + function testStartPrank0AfterStartPrank1(address sender, address origin) public { + // Perform the prank + address oldOrigin = tx.origin; + Victim victim = new Victim(); + cheats.startPrank(sender, origin); + victim.assertCallerAndOrigin( + sender, "msg.sender was not set during prank", origin, "tx.origin was not set during prank" + ); + + // Ensure prank is still ongoing as we haven't called stopPrank + victim.assertCallerAndOrigin( + sender, "msg.sender was cleaned up incorrectly", origin, "tx.origin was cleaned up incorrectly" + ); + + // Overwrite the prank + cheats.startPrank(sender); + victim.assertCallerAndOrigin( + sender, "msg.sender was not set during prank", oldOrigin, "tx.origin was not reset correctly" + ); + + cheats.stopPrank(); + // Ensure we cleaned up correctly + victim.assertCallerAndOrigin( + address(this), "msg.sender was not cleaned up", oldOrigin, "tx.origin invariant failed" + ); + } + function testPrankConstructorSender(address sender) public { cheats.prank(sender); ConstructorVictim victim = new ConstructorVictim( From f3c20d5664c8773d4ec3b2b67148cc1032f48f58 Mon Sep 17 00:00:00 2001 From: Andrea Simeoni Date: Fri, 12 May 2023 00:13:57 +0200 Subject: [PATCH 04/12] feat: ux fuzz invariant (#4744) * Parse FuzzConfig from string (brief impl) Unit tests * ConfParser trait is able to extract configurations out of a structured text Unit tests * cargo +nightly fmt * FuzzConfig implements ConfParser trait Unit tests * InvariantConfig implements ConfParser trait Unit tests * Parsing logic optimized Meaningful e2e test * Configurations can be parsed from project compilation output * E2E tests for inline configuration load * - ConfParser: parse fn is now try_merge - TestOptions struct extended to track test specific configs - Tests * Since TestOptions is no more Copy => TEST_OPTS constant is now a function * Inline config matcher uses stripped file prefixes to identify contracts * TestOptionsBuilder docs * Inline fuzz configs are applied during fuzz test execution + E2E tests * Inline invariant configs are applied during fuzz test execution + E2E tests + Docs * typos * Docs typo * cargo +nightly fmt * Added test for block comments * Renamed ConfParser to InlineConfigParser * Use NodeType enum to match condition * Use helper type to describe the HashMap key * Misconfigured line number added to the error - Need UNIT TESTS * Added very descriptive context to the parse error + unit test * Emphasis on the "Invalid" keyword * Big refactoring. Design is cleaner and more appropriate. It allows better validation flexibility. Need to fix tests * natspec unit tests * Refactor Unit tests InvariantConfig + FuzzConfig * Noisy comment test * Use meaningful names * Profile validation implemented + Unit tests * Given a natspec, extract current profile configs + Unit tests * TestOptions instantiated with new validation rules - NEED TESTS * Integration tests working * Integration tests docs and typos * Utility function to get all available profiles in config - unit tests * try update PR * Punctuation in config/src/inline/conf_parser.rs Co-authored-by: evalir * Punctuation in config/src/inline/conf_parser.rs Co-authored-by: evalir * review: docs in cli/src/cmd/forge/test/mod.rs * review: naming convention in InlineConfigParser * review: test renaming suggestion Co-authored-by: evalir * review: test renaming suggestion Co-authored-by: evalir * review: test renaming suggestion Co-authored-by: evalir * review: docs punctuation Co-authored-by: evalir * review: docs Co-authored-by: evalir * review: function internal utils function renaming + docs * review: get_fn_docs unit tests * review: test renaming suggestion Co-authored-by: evalir * review: clarify intent * review: document functions * review: applied case typos * FIX CI: Available profiles fallback to vec![current_profile] in case the foundry.toml path cannot be resolved * cargo +nightly fmt * review: case typo * review: remove double quotes from src line * review: removed duplicated error msg; removed row:col:len detail (it was not accurate) * fix CI --------- Co-authored-by: evalir --- Cargo.lock | 1 + cli/src/cmd/forge/test/mod.rs | 23 ++- config/Cargo.toml | 1 + config/src/fuzz.rs | 86 ++++++++ config/src/inline/conf_parser.rs | 206 +++++++++++++++++++ config/src/inline/mod.rs | 81 ++++++++ config/src/inline/natspec.rs | 238 ++++++++++++++++++++++ config/src/invariant.rs | 89 +++++++- config/src/lib.rs | 3 + config/src/utils.rs | 78 +++++++ forge/src/lib.rs | 208 ++++++++++++++++++- forge/src/multi_runner.rs | 8 +- forge/src/runner.rs | 47 +++-- forge/tests/it/config.rs | 60 +++--- forge/tests/it/core.rs | 10 +- forge/tests/it/fork.rs | 2 +- forge/tests/it/fuzz.rs | 6 +- forge/tests/it/inline.rs | 114 +++++++++++ forge/tests/it/invariant.rs | 14 +- forge/tests/it/main.rs | 1 + forge/tests/it/test_helpers.rs | 2 +- testdata/inline/FuzzInlineConf.t.sol | 14 ++ testdata/inline/InvariantInlineConf.t.sol | 52 +++++ 23 files changed, 1265 insertions(+), 79 deletions(-) create mode 100644 config/src/inline/conf_parser.rs create mode 100644 config/src/inline/mod.rs create mode 100644 config/src/inline/natspec.rs create mode 100644 forge/tests/it/inline.rs create mode 100644 testdata/inline/FuzzInlineConf.t.sol create mode 100644 testdata/inline/InvariantInlineConf.t.sol diff --git a/Cargo.lock b/Cargo.lock index 098f4361be145..7603c83aaba7c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2526,6 +2526,7 @@ dependencies = [ "reqwest", "semver", "serde", + "serde_json", "serde_regex", "tempfile", "thiserror", diff --git a/cli/src/cmd/forge/test/mod.rs b/cli/src/cmd/forge/test/mod.rs index bc9ca5b03a017..c5501a0ee54bc 100644 --- a/cli/src/cmd/forge/test/mod.rs +++ b/cli/src/cmd/forge/test/mod.rs @@ -18,14 +18,14 @@ use forge::{ identifier::{EtherscanIdentifier, LocalTraceIdentifier, SignaturesIdentifier}, CallTraceDecoderBuilder, TraceKind, }, - MultiContractRunner, MultiContractRunnerBuilder, TestOptions, + MultiContractRunner, MultiContractRunnerBuilder, TestOptions, TestOptionsBuilder, }; use foundry_common::{ compile::{self, ProjectCompiler}, evm::EvmArgs, get_contract_name, get_file_name, }; -use foundry_config::{figment, Config}; +use foundry_config::{figment, get_available_profiles, Config}; use regex::Regex; use std::{collections::BTreeMap, path::PathBuf, sync::mpsc::channel, thread, time::Duration}; use tracing::trace; @@ -123,8 +123,6 @@ impl TestArgs { // Merge all configs let (mut config, mut evm_opts) = self.load_config_and_evm_opts_emit_warnings()?; - let test_options = TestOptions { fuzz: config.fuzz, invariant: config.invariant }; - let mut filter = self.filter(&config); trace!(target: "forge::test", ?filter, "using filter"); @@ -150,6 +148,19 @@ impl TestArgs { compiler.compile(&project) }?; + // Create test options from general project settings + // and compiler output + let project_root = &project.paths.root; + let toml = config.get_config_path(); + let profiles = get_available_profiles(toml)?; + + let test_options: TestOptions = TestOptionsBuilder::default() + .fuzz(config.fuzz) + .invariant(config.invariant) + .compile_output(&output) + .profiles(profiles) + .build(project_root)?; + // Determine print verbosity and executor verbosity let verbosity = evm_opts.verbosity; if self.gas_report && evm_opts.verbosity < 3 { @@ -167,8 +178,8 @@ impl TestArgs { .sender(evm_opts.sender) .with_fork(evm_opts.get_fork(&config, env.clone())) .with_cheats_config(CheatsConfig::new(&config, &evm_opts)) - .with_test_options(test_options) - .build(project.paths.root, output, env, evm_opts)?; + .with_test_options(test_options.clone()) + .build(project_root, output, env, evm_opts)?; if self.debug.is_some() { filter.args_mut().test_pattern = self.debug; diff --git a/config/Cargo.toml b/config/Cargo.toml index f71f98b6f698a..1bc6b324d93f2 100644 --- a/config/Cargo.toml +++ b/config/Cargo.toml @@ -19,6 +19,7 @@ figment = { version = "0.10", features = ["toml", "env"] } number_prefix = "0.4.0" serde = { version = "1.0", features = ["derive"] } serde_regex = "1.1.0" +serde_json = "1.0.95" toml = { version = "0.7", features = ["preserve_order"] } toml_edit = "0.19" diff --git a/config/src/fuzz.rs b/config/src/fuzz.rs index 02937b64f4a00..b43ae10e156cd 100644 --- a/config/src/fuzz.rs +++ b/config/src/fuzz.rs @@ -3,6 +3,10 @@ use ethers_core::types::U256; use serde::{Deserialize, Serialize}; +use crate::inline::{ + parse_config_u32, InlineConfigParser, InlineConfigParserError, INLINE_CONFIG_FUZZ_KEY, +}; + /// Contains for fuzz testing #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub struct FuzzConfig { @@ -35,6 +39,34 @@ impl Default for FuzzConfig { } } +impl InlineConfigParser for FuzzConfig { + fn config_key() -> String { + INLINE_CONFIG_FUZZ_KEY.into() + } + + fn try_merge(&self, configs: &[String]) -> Result, InlineConfigParserError> { + let overrides: Vec<(String, String)> = Self::get_config_overrides(configs); + + if overrides.is_empty() { + return Ok(None) + } + + // self is Copy. We clone it with dereference. + let mut conf_clone = *self; + + for pair in overrides { + let key = pair.0; + let value = pair.1; + match key.as_str() { + "runs" => conf_clone.runs = parse_config_u32(key, value)?, + "max-test-rejects" => conf_clone.max_test_rejects = parse_config_u32(key, value)?, + _ => Err(InlineConfigParserError::InvalidConfigProperty(key))?, + } + } + Ok(Some(conf_clone)) + } +} + /// Contains for fuzz testing #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub struct FuzzDictionaryConfig { @@ -70,3 +102,57 @@ impl Default for FuzzDictionaryConfig { } } } + +#[cfg(test)] +mod tests { + use crate::{inline::InlineConfigParser, FuzzConfig}; + + #[test] + fn unrecognized_property() { + let configs = &["forge-config: default.fuzz.unknownprop = 200".to_string()]; + let base_config = FuzzConfig::default(); + if let Err(e) = base_config.try_merge(configs) { + assert_eq!(e.to_string(), "'unknownprop' is an invalid config property"); + } else { + assert!(false) + } + } + + #[test] + fn successful_merge() { + let configs = &["forge-config: default.fuzz.runs = 42424242".to_string()]; + let base_config = FuzzConfig::default(); + let merged: FuzzConfig = base_config.try_merge(configs).expect("No errors").unwrap(); + assert_eq!(merged.runs, 42424242); + } + + #[test] + fn merge_is_none() { + let empty_config = &[]; + let base_config = FuzzConfig::default(); + let merged = base_config.try_merge(empty_config).expect("No errors"); + assert!(merged.is_none()); + } + + #[test] + fn merge_is_none_unrelated_property() { + let unrelated_configs = &["forge-config: default.invariant.runs = 2".to_string()]; + let base_config = FuzzConfig::default(); + let merged = base_config.try_merge(unrelated_configs).expect("No errors"); + assert!(merged.is_none()); + } + + #[test] + fn override_detection() { + let configs = &[ + "forge-config: default.fuzz.runs = 42424242".to_string(), + "forge-config: ci.fuzz.runs = 666666".to_string(), + "forge-config: default.invariant.runs = 2".to_string(), + ]; + let variables = FuzzConfig::get_config_overrides(configs); + assert_eq!( + variables, + vec![("runs".into(), "42424242".into()), ("runs".into(), "666666".into())] + ); + } +} diff --git a/config/src/inline/conf_parser.rs b/config/src/inline/conf_parser.rs new file mode 100644 index 0000000000000..a64cd67299184 --- /dev/null +++ b/config/src/inline/conf_parser.rs @@ -0,0 +1,206 @@ +use regex::Regex; + +use crate::{InlineConfigError, NatSpec}; + +use super::{remove_whitespaces, INLINE_CONFIG_PREFIX}; + +/// Errors returned by the [`InlineConfigParser`] trait. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +pub enum InlineConfigParserError { + /// An invalid configuration property has been provided. + /// The property cannot be mapped to the configuration object + #[error("'{0}' is an invalid config property")] + InvalidConfigProperty(String), + /// An invalid profile has been provided + #[error("'{0}' specifies an invalid profile. Available profiles are: {1}")] + InvalidProfile(String, String), + /// An error occurred while trying to parse an integer configuration value + #[error("Invalid config value for key '{0}'. Unable to parse '{1}' into an integer value")] + ParseInt(String, String), + /// An error occurred while trying to parse a boolean configuration value + #[error("Invalid config value for key '{0}'. Unable to parse '{1}' into a boolean value")] + ParseBool(String, String), +} + +/// This trait is intended to parse configurations from +/// structured text. Foundry users can annotate Solidity test functions, +/// providing special configs just for the execution of a specific test. +/// +/// An example: +/// +/// ```solidity +/// contract MyTest is Test { +/// /// forge-config: default.fuzz.runs = 100 +/// /// forge-config: ci.fuzz.runs = 500 +/// function test_SimpleFuzzTest(uint256 x) public {...} +/// +/// /// forge-config: default.fuzz.runs = 500 +/// /// forge-config: ci.fuzz.runs = 10000 +/// function test_ImportantFuzzTest(uint256 x) public {...} +/// } +/// ``` +pub trait InlineConfigParser +where + Self: Clone + Default + Sized + 'static, +{ + /// Returns a config key that is common to all valid configuration lines + /// for the current impl. This helps to extract correct values out of a text. + /// + /// An example key would be `fuzz` of `invariant`. + fn config_key() -> String; + + /// Tries to override `self` properties with values specified in the `configs` parameter. + /// + /// Returns + /// - `Some(Self)` in case some configurations are merged into self. + /// - `None` in case there are no configurations that can be applied to self. + /// - `Err(InlineConfigParserError)` in case of wrong configuration. + fn try_merge(&self, configs: &[String]) -> Result, InlineConfigParserError>; + + /// Validates all configurations contained in a natspec that apply + /// to the current configuration key. + /// + /// i.e. Given the `invariant` config key and a natspec comment of the form, + /// ```solidity + /// /// forge-config: default.invariant.runs = 500 + /// /// forge-config: default.invariant.depth = 500 + /// /// forge-config: ci.invariant.depth = 500 + /// /// forge-config: ci.fuzz.runs = 10 + /// ``` + /// would validate the whole `invariant` configuration. + fn validate_configs(natspec: &NatSpec) -> Result<(), InlineConfigError> { + let config_key = Self::config_key(); + + let configs = natspec + .config_lines() + .into_iter() + .filter(|l| l.contains(&config_key)) + .collect::>(); + + Self::default().try_merge(&configs).map_err(|e| { + let line = natspec.debug_context(); + InlineConfigError { line, source: e } + })?; + + Ok(()) + } + + /// Given a list of `config_lines, returns all available pairs (key, value) + /// matching the current config key + /// + /// i.e. Given the `invariant` config key and a vector of config lines + /// ```rust + /// let _config_lines = vec![ + /// "forge-config: default.invariant.runs = 500", + /// "forge-config: default.invariant.depth = 500", + /// "forge-config: ci.invariant.depth = 500", + /// "forge-config: ci.fuzz.runs = 10" + /// ]; + /// ``` + /// would return the whole set of `invariant` configs. + /// ```rust + /// let _result = vec![ + /// ("runs", "500"), + /// ("depth", "500"), + /// ("depth", "500"), + /// ]; + /// ``` + fn get_config_overrides(config_lines: &[String]) -> Vec<(String, String)> { + let mut result: Vec<(String, String)> = vec![]; + let config_key = Self::config_key(); + let profile = ".*"; + let prefix = format!("^{INLINE_CONFIG_PREFIX}:{profile}{config_key}\\."); + let re = Regex::new(&prefix).unwrap(); + + config_lines + .iter() + .map(|l| remove_whitespaces(l)) + .filter(|l| re.is_match(l)) + .map(|l| re.replace(&l, "").to_string()) + .for_each(|line| { + let key_value = line.split('=').collect::>(); // i.e. "['runs', '500']" + if let Some(key) = key_value.first() { + if let Some(value) = key_value.last() { + result.push((key.to_string(), value.to_string())); + } + } + }); + + result + } +} + +/// Checks if all configuration lines specified in `natspec` use a valid profile. +/// +/// i.e. Given available profiles +/// ```rust +/// let _profiles = vec!["ci", "default"]; +/// ``` +/// A configuration like `forge-config: ciii.invariant.depth = 1` would result +/// in an error. +pub fn validate_profiles(natspec: &NatSpec, profiles: &[String]) -> Result<(), InlineConfigError> { + for config in natspec.config_lines() { + if !profiles.iter().any(|p| config.starts_with(&format!("{INLINE_CONFIG_PREFIX}:{p}."))) { + let err_line: String = natspec.debug_context(); + let profiles = format!("{profiles:?}"); + Err(InlineConfigError { + source: InlineConfigParserError::InvalidProfile(config, profiles), + line: err_line, + })? + } + } + Ok(()) +} + +/// Tries to parse a `u32` from `value`. The `key` argument is used to give details +/// in the case of an error. +pub fn parse_config_u32(key: String, value: String) -> Result { + value.parse().map_err(|_| InlineConfigParserError::ParseInt(key, value)) +} + +/// Tries to parse a `bool` from `value`. The `key` argument is used to give details +/// in the case of an error. +pub fn parse_config_bool(key: String, value: String) -> Result { + value.parse().map_err(|_| InlineConfigParserError::ParseBool(key, value)) +} + +#[cfg(test)] +mod tests { + use crate::{inline::conf_parser::validate_profiles, NatSpec}; + + #[test] + fn can_reject_invalid_profiles() { + let profiles = ["ci".to_string(), "default".to_string()]; + let natspec = NatSpec { + contract: Default::default(), + function: Default::default(), + line: Default::default(), + docs: r#" + forge-config: ciii.invariant.depth = 1 + forge-config: default.invariant.depth = 1 + "# + .into(), + }; + + let result = validate_profiles(&natspec, &profiles); + assert!(result.is_err()); + } + + #[test] + fn can_accept_valid_profiles() { + let profiles = ["ci".to_string(), "default".to_string()]; + let natspec = NatSpec { + contract: Default::default(), + function: Default::default(), + line: Default::default(), + docs: r#" + forge-config: ci.invariant.depth = 1 + forge-config: default.invariant.depth = 1 + "# + .into(), + }; + + let result = validate_profiles(&natspec, &profiles); + assert!(result.is_ok()); + } +} diff --git a/config/src/inline/mod.rs b/config/src/inline/mod.rs new file mode 100644 index 0000000000000..a96f816ade6fd --- /dev/null +++ b/config/src/inline/mod.rs @@ -0,0 +1,81 @@ +mod conf_parser; +pub use conf_parser::{ + parse_config_bool, parse_config_u32, validate_profiles, InlineConfigParser, + InlineConfigParserError, +}; +use once_cell::sync::Lazy; +use std::collections::HashMap; + +mod natspec; +pub use natspec::NatSpec; + +use crate::Config; + +pub const INLINE_CONFIG_FUZZ_KEY: &str = "fuzz"; +pub const INLINE_CONFIG_INVARIANT_KEY: &str = "invariant"; +const INLINE_CONFIG_PREFIX: &str = "forge-config"; + +static INLINE_CONFIG_PREFIX_SELECTED_PROFILE: Lazy = Lazy::new(|| { + let selected_profile = Config::selected_profile().to_string(); + format!("{INLINE_CONFIG_PREFIX}:{selected_profile}.") +}); + +/// Wrapper error struct that catches config parsing +/// errors [`InlineConfigParserError`], enriching them with context information +/// reporting the misconfigured line. +#[derive(thiserror::Error, Debug)] +#[error("Inline config error detected at {line}")] +pub struct InlineConfigError { + /// Specifies the misconfigured line. This is something of the form + /// `dir/TestContract.t.sol:FuzzContract:10:12:111` + pub line: String, + /// The inner error + pub source: InlineConfigParserError, +} + +/// Represents a (test-contract, test-function) pair +type InlineConfigKey = (String, String); + +/// Represents per-test configurations, declared inline +/// as structured comments in Solidity test files. This allows +/// to create configs directly bound to a solidity test. +#[derive(Default, Debug, Clone)] +pub struct InlineConfig { + /// Maps a (test-contract, test-function) pair + /// to a specific configuration provided by the user. + configs: HashMap, +} + +impl InlineConfig { + /// Returns an inline configuration, if any, for a test function. + /// Configuration is identified by the pair "contract", "function". + pub fn get>(&self, contract_id: S, fn_name: S) -> Option<&T> { + self.configs.get(&(contract_id.into(), fn_name.into())) + } + + /// Inserts an inline configuration, for a test function. + /// Configuration is identified by the pair "contract", "function". + pub fn insert>(&mut self, contract_id: S, fn_name: S, config: T) { + self.configs.insert((contract_id.into(), fn_name.into()), config); + } +} + +fn remove_whitespaces(s: &str) -> String { + s.chars().filter(|c| !c.is_whitespace()).collect() +} + +#[cfg(test)] +mod tests { + use super::InlineConfigParserError; + use crate::InlineConfigError; + + #[test] + fn can_format_inline_config_errors() { + let source = InlineConfigParserError::ParseBool("key".into(), "invalid-bool-value".into()); + let line = "dir/TestContract.t.sol:FuzzContract".to_string(); + let error = InlineConfigError { line: line.clone(), source: source.clone() }; + + let expected = format!("Inline config error detected at {line}"); + assert_eq!(error.to_string(), expected); + } +} diff --git a/config/src/inline/natspec.rs b/config/src/inline/natspec.rs new file mode 100644 index 0000000000000..ae85089c1aa97 --- /dev/null +++ b/config/src/inline/natspec.rs @@ -0,0 +1,238 @@ +use std::{collections::BTreeMap, path::Path}; + +use ethers_solc::{ + artifacts::{ast::NodeType, Node}, + ProjectCompileOutput, +}; +use serde_json::Value; + +use super::{remove_whitespaces, INLINE_CONFIG_PREFIX, INLINE_CONFIG_PREFIX_SELECTED_PROFILE}; + +/// Convenient struct to hold in-line per-test configurations +pub struct NatSpec { + /// The parent contract of the natspec + pub contract: String, + /// The function annotated with the natspec + pub function: String, + /// The line the natspec appears, in the form + /// `row:col:length` i.e. `10:21:122` + pub line: String, + /// The actual natspec comment, without slashes or block + /// punctuation + pub docs: String, +} + +impl NatSpec { + /// Factory function that extracts a vector of [`NatSpec`] instances from + /// a solc compiler output. The root path is to express contract base dirs. + /// That is essential to match per-test configs at runtime. + pub fn parse

(output: &ProjectCompileOutput, root: &P) -> Vec + where + P: AsRef, + { + let mut natspecs: Vec = vec![]; + + let output = output.clone(); + for artifact in output.with_stripped_file_prefixes(root).into_artifacts() { + if let Some(ast) = artifact.1.ast.as_ref() { + let contract: String = artifact.0.identifier(); + if let Some(node) = contract_root_node(&ast.nodes, &contract) { + apply(&mut natspecs, &contract, node) + } + } + } + natspecs + } + + /// Returns a string describing the natspec + /// context, for debugging purposes 🐞 + /// i.e. `test/Counter.t.sol:CounterTest:testSetNumber` + pub fn debug_context(&self) -> String { + format!("{}:{}", self.contract, self.function) + } + + /// Returns a list of configuration lines that match the current profile + pub fn current_profile_configs(&self) -> Vec { + let prefix: &str = INLINE_CONFIG_PREFIX_SELECTED_PROFILE.as_ref(); + self.config_lines_with_prefix(prefix) + } + + /// Returns a list of configuration lines that match a specific string prefix + pub fn config_lines_with_prefix>(&self, prefix: S) -> Vec { + let prefix: String = prefix.into(); + self.config_lines().into_iter().filter(|l| l.starts_with(&prefix)).collect() + } + + /// Returns a list of all the configuration lines available in the natspec + pub fn config_lines(&self) -> Vec { + self.docs + .split('\n') + .map(remove_whitespaces) + .filter(|line| line.contains(INLINE_CONFIG_PREFIX)) + .collect::>() + } +} + +/// Given a list of nodes, find a "ContractDefinition" node that matches +/// the provided contract_id. +fn contract_root_node<'a>(nodes: &'a [Node], contract_id: &'a str) -> Option<&'a Node> { + for n in nodes.iter() { + if let NodeType::ContractDefinition = n.node_type { + let contract_data = &n.other; + if let Value::String(contract_name) = contract_data.get("name")? { + if contract_id.ends_with(contract_name) { + return Some(n) + } + } + } + } + None +} + +/// Implements a DFS over a compiler output node and its children. +/// If a natspec is found it is added to `natspecs` +fn apply(natspecs: &mut Vec, contract: &str, node: &Node) { + for n in node.nodes.iter() { + if let Some((function, docs, line)) = get_fn_data(n) { + natspecs.push(NatSpec { contract: contract.into(), function, line, docs }) + } + apply(natspecs, contract, n); + } +} + +/// Given a compilation output node, if it is a function definition +/// that also contains a natspec then return a tuple of: +/// - Function name +/// - Natspec text +/// - Natspec position with format "row:col:length" +/// +/// Return None otherwise. +fn get_fn_data(node: &Node) -> Option<(String, String, String)> { + if let NodeType::FunctionDefinition = node.node_type { + let fn_data = &node.other; + let fn_name: String = get_fn_name(fn_data)?; + let (fn_docs, docs_src_line): (String, String) = get_fn_docs(fn_data)?; + return Some((fn_name, fn_docs, docs_src_line)) + } + + None +} + +/// Given a dictionary of function data returns the name of the function. +fn get_fn_name(fn_data: &BTreeMap) -> Option { + match fn_data.get("name")? { + Value::String(fn_name) => Some(fn_name.into()), + _ => None, + } +} + +/// Inspects Solc compiler output for documentation comments. Returns: +/// - `Some((String, String))` in case the function has natspec comments. First item is a textual +/// natspec representation, the second item is the natspec src line, in the form "raw:col:length". +/// - `None` in case the function has not natspec comments. +fn get_fn_docs(fn_data: &BTreeMap) -> Option<(String, String)> { + if let Value::Object(fn_docs) = fn_data.get("documentation")? { + if let Value::String(comment) = fn_docs.get("text")? { + if comment.contains(INLINE_CONFIG_PREFIX) { + let mut src_line = fn_docs + .get("src") + .map(|src| src.to_string()) + .unwrap_or(String::from("")); + + src_line.retain(|c| c != '"'); + return Some((comment.into(), src_line)) + } + } + } + None +} + +#[cfg(test)] +mod tests { + use crate::{inline::natspec::get_fn_docs, NatSpec}; + use serde_json::{json, Value}; + use std::collections::BTreeMap; + + #[test] + fn config_lines() { + let natspec = natspec(); + let config_lines = natspec.config_lines(); + assert_eq!( + config_lines, + vec![ + "forge-config:default.fuzz.runs=600".to_string(), + "forge-config:ci.fuzz.runs=500".to_string(), + "forge-config:default.invariant.runs=1".to_string() + ] + ) + } + + #[test] + fn current_profile_configs() { + let natspec = natspec(); + let config_lines = natspec.current_profile_configs(); + + assert_eq!( + config_lines, + vec![ + "forge-config:default.fuzz.runs=600".to_string(), + "forge-config:default.invariant.runs=1".to_string() + ] + ); + } + + #[test] + fn config_lines_with_prefix() { + use super::INLINE_CONFIG_PREFIX; + let natspec = natspec(); + let prefix = format!("{INLINE_CONFIG_PREFIX}:default"); + let config_lines = natspec.config_lines_with_prefix(prefix); + assert_eq!( + config_lines, + vec![ + "forge-config:default.fuzz.runs=600".to_string(), + "forge-config:default.invariant.runs=1".to_string() + ] + ) + } + + #[test] + fn can_handle_unavailable_src_line_with_fallback() { + let mut fn_data: BTreeMap = BTreeMap::new(); + let doc_withouth_src_field = json!({ "text": "forge-config:default.fuzz.runs=600" }); + fn_data.insert("documentation".into(), doc_withouth_src_field); + let (_, src_line) = get_fn_docs(&fn_data).expect("Some docs"); + assert_eq!(src_line, "".to_string()); + } + + #[test] + fn can_handle_available_src_line() { + let mut fn_data: BTreeMap = BTreeMap::new(); + let doc_withouth_src_field = + json!({ "text": "forge-config:default.fuzz.runs=600", "src": "73:21:12" }); + fn_data.insert("documentation".into(), doc_withouth_src_field); + let (_, src_line) = get_fn_docs(&fn_data).expect("Some docs"); + assert_eq!(src_line, "73:21:12".to_string()); + } + + fn natspec() -> NatSpec { + let conf = r#" + forge-config: default.fuzz.runs = 600 + forge-config: ci.fuzz.runs = 500 + ========= SOME NOISY TEXT ============= + 䩹𧀫Jx닧Ʀ̳盅K擷􅟽Ɂw첊}ꏻk86ᖪk-檻ܴ렝[Dz𐤬oᘓƤ + ꣖ۻ%Ƅ㪕ς:(饁΍av/烲ڻ̛߉橞㗡𥺃̹M봓䀖ؿ̄󵼁)𯖛d􂽰񮍃 + ϊ&»ϿЏ񊈞2򕄬񠪁鞷砕eߥH󶑶J粊񁼯머?槿ᴴጅ𙏑ϖ뀓򨙺򷃅Ӽ츙4󍔹 + 醤㭊r􎜕󷾸𶚏 ܖ̹灱녗V*竅􋹲⒪苏贗񾦼=숽ؓ򗋲бݧ󫥛𛲍ʹ園Ьi + ======================================= + forge-config: default.invariant.runs = 1 + "#; + + NatSpec { + contract: "dir/TestContract.t.sol:FuzzContract".to_string(), + function: "test_myFunction".to_string(), + line: "10:12:111".to_string(), + docs: conf.to_string(), + } + } +} diff --git a/config/src/invariant.rs b/config/src/invariant.rs index b71b88d525e62..ad49e40fffb84 100644 --- a/config/src/invariant.rs +++ b/config/src/invariant.rs @@ -1,6 +1,12 @@ //! Configuration for invariant testing -use crate::fuzz::FuzzDictionaryConfig; +use crate::{ + fuzz::FuzzDictionaryConfig, + inline::{ + parse_config_bool, parse_config_u32, InlineConfigParser, InlineConfigParserError, + INLINE_CONFIG_INVARIANT_KEY, + }, +}; use serde::{Deserialize, Serialize}; /// Contains for invariant testing @@ -31,3 +37,84 @@ impl Default for InvariantConfig { } } } + +impl InlineConfigParser for InvariantConfig { + fn config_key() -> String { + INLINE_CONFIG_INVARIANT_KEY.into() + } + + fn try_merge(&self, configs: &[String]) -> Result, InlineConfigParserError> { + let overrides: Vec<(String, String)> = Self::get_config_overrides(configs); + + if overrides.is_empty() { + return Ok(None) + } + + // self is Copy. We clone it with dereference. + let mut conf_clone = *self; + + for pair in overrides { + let key = pair.0; + let value = pair.1; + match key.as_str() { + "runs" => conf_clone.runs = parse_config_u32(key, value)?, + "depth" => conf_clone.depth = parse_config_u32(key, value)?, + "fail-on-revert" => conf_clone.fail_on_revert = parse_config_bool(key, value)?, + "call-override" => conf_clone.call_override = parse_config_bool(key, value)?, + _ => Err(InlineConfigParserError::InvalidConfigProperty(key.to_string()))?, + } + } + Ok(Some(conf_clone)) + } +} + +#[cfg(test)] +mod tests { + use crate::{inline::InlineConfigParser, InvariantConfig}; + + #[test] + fn unrecognized_property() { + let configs = &["forge-config: default.invariant.unknownprop = 200".to_string()]; + let base_config = InvariantConfig::default(); + if let Err(e) = base_config.try_merge(configs) { + assert_eq!(e.to_string(), "'unknownprop' is an invalid config property"); + } else { + assert!(false) + } + } + + #[test] + fn successful_merge() { + let configs = &["forge-config: default.invariant.runs = 42424242".to_string()]; + let base_config = InvariantConfig::default(); + let merged: InvariantConfig = base_config.try_merge(configs).expect("No errors").unwrap(); + assert_eq!(merged.runs, 42424242); + } + + #[test] + fn merge_is_none() { + let empty_config = &[]; + let base_config = InvariantConfig::default(); + let merged = base_config.try_merge(empty_config).expect("No errors"); + assert!(merged.is_none()); + } + + #[test] + fn can_merge_unrelated_properties_into_config() { + let unrelated_configs = &["forge-config: default.fuzz.runs = 2".to_string()]; + let base_config = InvariantConfig::default(); + let merged = base_config.try_merge(unrelated_configs).expect("No errors"); + assert!(merged.is_none()); + } + + #[test] + fn override_detection() { + let configs = &[ + "forge-config: default.fuzz.runs = 42424242".to_string(), + "forge-config: ci.fuzz.runs = 666666".to_string(), + "forge-config: default.invariant.runs = 2".to_string(), + ]; + let variables = InvariantConfig::get_config_overrides(configs); + assert_eq!(variables, vec![("runs".into(), "2".into())]); + } +} diff --git a/config/src/lib.rs b/config/src/lib.rs index b6dac70e17cf1..328869408c78e 100644 --- a/config/src/lib.rs +++ b/config/src/lib.rs @@ -92,6 +92,9 @@ use crate::fs_permissions::PathPermission; pub use invariant::InvariantConfig; use providers::remappings::RemappingsProvider; +mod inline; +pub use inline::{validate_profiles, InlineConfig, InlineConfigError, InlineConfigParser, NatSpec}; + /// Foundry configuration /// /// # Defaults diff --git a/config/src/utils.rs b/config/src/utils.rs index d51a4d20dc15a..abbf3f727cca2 100644 --- a/config/src/utils.rs +++ b/config/src/utils.rs @@ -9,6 +9,7 @@ use std::{ path::{Path, PathBuf}, str::FromStr, }; +use toml_edit::{Document, Item}; /// Loads the config for the current project workspace pub fn load_config() -> Config { @@ -168,6 +169,45 @@ pub(crate) fn get_dir_remapping(dir: impl AsRef) -> Option { } } +/// Returns all available `profile` keys in a given `.toml` file +/// +/// i.e. The toml below would return would return `["default", "ci", "local"]` +/// ```toml +/// [profile.default] +/// ... +/// [profile.ci] +/// ... +/// [profile.local] +/// ``` +pub fn get_available_profiles(toml_path: impl AsRef) -> eyre::Result> { + let mut result = vec![Config::DEFAULT_PROFILE.to_string()]; + + if !toml_path.as_ref().exists() { + return Ok(result) + } + + let doc = read_toml(toml_path)?; + + if let Some(Item::Table(profiles)) = doc.as_table().get(Config::PROFILE_SECTION) { + for (_, (profile, _)) in profiles.iter().enumerate() { + let p = profile.to_string(); + if !result.contains(&p) { + result.push(p); + } + } + } + + Ok(result) +} + +/// Returns a [`toml_edit::Document`] loaded from the provided `path`. +/// Can raise an error in case of I/O or parsing errors. +fn read_toml(path: impl AsRef) -> eyre::Result { + let path = path.as_ref().to_owned(); + let doc: Document = std::fs::read_to_string(path)?.parse()?; + Ok(doc) +} + /// Deserialize stringified percent. The value must be between 0 and 100 inclusive. pub(crate) fn deserialize_stringified_percent<'de, D>(deserializer: D) -> Result where @@ -209,3 +249,41 @@ where }; Ok(num) } + +#[cfg(test)] +mod tests { + use crate::get_available_profiles; + use std::path::Path; + + #[test] + fn get_profiles_from_toml() { + figment::Jail::expect_with(|jail| { + jail.create_file( + "foundry.toml", + r#" + [foo.baz] + libs = ['node_modules', 'lib'] + + [profile.default] + libs = ['node_modules', 'lib'] + + [profile.ci] + libs = ['node_modules', 'lib'] + + [profile.local] + libs = ['node_modules', 'lib'] + "#, + )?; + + let path = Path::new("./foundry.toml"); + let profiles = get_available_profiles(path).unwrap(); + + assert_eq!( + profiles, + vec!["default".to_string(), "ci".to_string(), "local".to_string()] + ); + + Ok(()) + }); + } +} diff --git a/forge/src/lib.rs b/forge/src/lib.rs index 591752ac80c42..c6b86d8d1d575 100644 --- a/forge/src/lib.rs +++ b/forge/src/lib.rs @@ -1,3 +1,10 @@ +use std::path::Path; + +use ethers::solc::ProjectCompileOutput; +use foundry_config::{ + validate_profiles, Config, FuzzConfig, InlineConfig, InlineConfigError, InlineConfigParser, + InvariantConfig, NatSpec, +}; use proptest::test_runner::{RngAlgorithm, TestRng, TestRunner}; use tracing::trace; @@ -24,21 +31,77 @@ pub mod result; pub use foundry_evm::*; /// Metadata on how to run fuzz/invariant tests -#[derive(Debug, Clone, Copy, Default)] +#[derive(Debug, Clone, Default)] pub struct TestOptions { - /// The fuzz test configuration - pub fuzz: foundry_config::FuzzConfig, - /// The invariant test configuration - pub invariant: foundry_config::InvariantConfig, + /// The base "fuzz" test configuration. To be used as a fallback in case + /// no more specific configs are found for a given run. + pub fuzz: FuzzConfig, + /// The base "invariant" test configuration. To be used as a fallback in case + /// no more specific configs are found for a given run. + pub invariant: InvariantConfig, + /// Contains per-test specific "fuzz" configurations. + pub inline_fuzz: InlineConfig, + /// Contains per-test specific "invariant" configurations. + pub inline_invariant: InlineConfig, } impl TestOptions { - pub fn invariant_fuzzer(&self) -> TestRunner { - self.fuzzer_with_cases(self.invariant.runs) + /// Returns a "fuzz" test runner instance. Parameters are used to select tight scoped fuzz + /// configs that apply for a contract-function pair. A fallback configuration is applied + /// if no specific setup is found for a given input. + /// + /// - `contract_id` is the id of the test contract, expressed as a relative path from the + /// project root. + /// - `test_fn` is the name of the test function declared inside the test contract. + pub fn fuzz_runner(&self, contract_id: S, test_fn: S) -> TestRunner + where + S: Into, + { + let fuzz = self.fuzz_config(contract_id, test_fn); + self.fuzzer_with_cases(fuzz.runs) + } + + /// Returns an "invariant" test runner instance. Parameters are used to select tight scoped fuzz + /// configs that apply for a contract-function pair. A fallback configuration is applied + /// if no specific setup is found for a given input. + /// + /// - `contract_id` is the id of the test contract, expressed as a relative path from the + /// project root. + /// - `test_fn` is the name of the test function declared inside the test contract. + pub fn invariant_runner(&self, contract_id: S, test_fn: S) -> TestRunner + where + S: Into, + { + let invariant = self.invariant_config(contract_id, test_fn); + self.fuzzer_with_cases(invariant.runs) + } + + /// Returns a "fuzz" configuration setup. Parameters are used to select tight scoped fuzz + /// configs that apply for a contract-function pair. A fallback configuration is applied + /// if no specific setup is found for a given input. + /// + /// - `contract_id` is the id of the test contract, expressed as a relative path from the + /// project root. + /// - `test_fn` is the name of the test function declared inside the test contract. + pub fn fuzz_config(&self, contract_id: S, test_fn: S) -> &FuzzConfig + where + S: Into, + { + self.inline_fuzz.get(contract_id, test_fn).unwrap_or(&self.fuzz) } - pub fn fuzzer(&self) -> TestRunner { - self.fuzzer_with_cases(self.fuzz.runs) + /// Returns an "invariant" configuration setup. Parameters are used to select tight scoped + /// invariant configs that apply for a contract-function pair. A fallback configuration is + /// applied if no specific setup is found for a given input. + /// + /// - `contract_id` is the id of the test contract, expressed as a relative path from the + /// project root. + /// - `test_fn` is the name of the test function declared inside the test contract. + pub fn invariant_config(&self, contract_id: S, test_fn: S) -> &InvariantConfig + where + S: Into, + { + self.inline_invariant.get(contract_id, test_fn).unwrap_or(&self.invariant) } pub fn fuzzer_with_cases(&self, cases: u32) -> TestRunner { @@ -62,3 +125,130 @@ impl TestOptions { } } } + +impl<'a, P> TryFrom<(&'a ProjectCompileOutput, &'a P, Vec, FuzzConfig, InvariantConfig)> + for TestOptions +where + P: AsRef, +{ + type Error = InlineConfigError; + + /// Tries to create an instance of `Self`, detecting inline configurations from the project + /// compile output. + /// + /// Param is a tuple, whose elements are: + /// 1. Solidity compiler output, essential to extract natspec test configs. + /// 2. Root path to express contract base dirs. This is essential to match inline configs at + /// runtime. 3. List of available configuration profiles + /// 4. Reference to a fuzz base configuration. + /// 5. Reference to an invariant base configuration. + fn try_from( + value: (&'a ProjectCompileOutput, &'a P, Vec, FuzzConfig, InvariantConfig), + ) -> Result { + let output = value.0; + let root = value.1; + let profiles = &value.2; + let base_fuzz: FuzzConfig = value.3; + let base_invariant: InvariantConfig = value.4; + + let natspecs: Vec = NatSpec::parse(output, root); + let mut inline_invariant = InlineConfig::::default(); + let mut inline_fuzz = InlineConfig::::default(); + + for natspec in natspecs { + // Perform general validation + validate_profiles(&natspec, profiles)?; + FuzzConfig::validate_configs(&natspec)?; + InvariantConfig::validate_configs(&natspec)?; + + // Apply in-line configurations for the current profile + let configs: Vec = natspec.current_profile_configs(); + let c: &str = &natspec.contract; + let f: &str = &natspec.function; + let line: String = natspec.debug_context(); + + match base_fuzz.try_merge(&configs) { + Ok(Some(conf)) => inline_fuzz.insert(c, f, conf), + Err(e) => Err(InlineConfigError { line: line.clone(), source: e })?, + _ => { /* No inline config found, do nothing */ } + } + + match base_invariant.try_merge(&configs) { + Ok(Some(conf)) => inline_invariant.insert(c, f, conf), + Err(e) => Err(InlineConfigError { line: line.clone(), source: e })?, + _ => { /* No inline config found, do nothing */ } + } + } + + Ok(Self { fuzz: base_fuzz, invariant: base_invariant, inline_fuzz, inline_invariant }) + } +} + +/// Builder utility to create a [`TestOptions`] instance. +#[derive(Default)] +pub struct TestOptionsBuilder { + fuzz: Option, + invariant: Option, + profiles: Option>, + output: Option, +} + +impl TestOptionsBuilder { + /// Sets a [`FuzzConfig`] to be used as base "fuzz" configuration. + #[must_use = "A base 'fuzz' config must be provided"] + pub fn fuzz(mut self, conf: FuzzConfig) -> Self { + self.fuzz = Some(conf); + self + } + + /// Sets a [`InvariantConfig`] to be used as base "invariant" configuration. + #[must_use = "A base 'invariant' config must be provided"] + pub fn invariant(mut self, conf: InvariantConfig) -> Self { + self.invariant = Some(conf); + self + } + + /// Sets available configuration profiles. Profiles are useful to validate existing in-line + /// configurations. This argument is necessary in case a `compile_output`is provided. + pub fn profiles(mut self, p: Vec) -> Self { + self.profiles = Some(p); + self + } + + /// Sets a project compiler output instance. This is used to extract + /// inline test configurations that override `self.fuzz` and `self.invariant` + /// specs when necessary. + pub fn compile_output(mut self, output: &ProjectCompileOutput) -> Self { + self.output = Some(output.clone()); + self + } + + /// Creates an instance of [`TestOptions`]. This takes care of creating "fuzz" and + /// "invariant" fallbacks, and extracting all inline test configs, if available. + /// + /// `root` is a reference to the user's project root dir. This is essential + /// to determine the base path of generated contract identifiers. This is to provide correct + /// matchers for inline test configs. + pub fn build(self, root: impl AsRef) -> Result { + let default_profiles = vec![Config::selected_profile().into()]; + let profiles: Vec = self.profiles.unwrap_or(default_profiles); + let base_fuzz = self.fuzz.unwrap_or_default(); + let base_invariant = self.invariant.unwrap_or_default(); + + match self.output { + Some(compile_output) => Ok(TestOptions::try_from(( + &compile_output, + &root, + profiles, + base_fuzz, + base_invariant, + ))?), + None => Ok(TestOptions { + fuzz: base_fuzz, + invariant: base_invariant, + inline_fuzz: InlineConfig::default(), + inline_invariant: InlineConfig::default(), + }), + } + } +} diff --git a/forge/src/multi_runner.rs b/forge/src/multi_runner.rs index a4402033d233f..c323c4bd88c45 100644 --- a/forge/src/multi_runner.rs +++ b/forge/src/multi_runner.rs @@ -152,7 +152,7 @@ impl MultiContractRunner { executor, deploy_code.clone(), libs, - (filter, test_options), + (filter, test_options.clone()), )?; tracing::trace!(contract= ?identifier, "executed all tests in contract"); @@ -171,16 +171,15 @@ impl MultiContractRunner { Ok(results) } - // The _name field is unused because we only want it for tracing #[tracing::instrument( name = "contract", skip_all, err, - fields(name = %_name) + fields(name = %name) )] fn run_tests( &self, - _name: &str, + name: &str, contract: &Abi, executor: Executor, deploy_code: Bytes, @@ -188,6 +187,7 @@ impl MultiContractRunner { (filter, test_options): (&impl TestFilter, TestOptions), ) -> Result { let runner = ContractRunner::new( + name, executor, contract, deploy_code, diff --git a/forge/src/runner.rs b/forge/src/runner.rs index f27b1983cc1d1..228ac5979c294 100644 --- a/forge/src/runner.rs +++ b/forge/src/runner.rs @@ -11,7 +11,7 @@ use foundry_common::{ contracts::{ContractsByAddress, ContractsByArtifact}, TestFunctionExt, }; -use foundry_config::FuzzConfig; +use foundry_config::{FuzzConfig, InvariantConfig}; use foundry_evm::{ decode::decode_console_logs, executor::{CallResult, DeployResult, EvmError, ExecutionErr, Executor}, @@ -35,9 +35,9 @@ use tracing::{error, trace}; /// A type that executes all tests of a contract #[derive(Debug, Clone)] pub struct ContractRunner<'a> { + pub name: &'a str, /// The executor used by the runner. pub executor: Executor, - /// Library contracts to be deployed before the test contract pub predeploy_libs: &'a [Bytes], /// The deployed contract's code @@ -56,6 +56,7 @@ pub struct ContractRunner<'a> { impl<'a> ContractRunner<'a> { #[allow(clippy::too_many_arguments)] pub fn new( + name: &'a str, executor: Executor, contract: &'a Abi, code: Bytes, @@ -65,6 +66,7 @@ impl<'a> ContractRunner<'a> { predeploy_libs: &'a [Bytes], ) -> Self { Self { + name, executor, contract, code, @@ -288,12 +290,16 @@ impl<'a> ContractRunner<'a> { .par_iter() .flat_map(|(func, should_fail)| { if func.is_fuzz_test() { + let fn_name = &func.name; + let runner = test_options.fuzz_runner(self.name, fn_name); + let fuzz_config = test_options.fuzz_config(self.name, fn_name); + self.run_fuzz_test( func, *should_fail, - test_options.fuzzer(), + runner, setup.clone(), - test_options.fuzz, + *fuzz_config, ) } else { self.clone().run_test(func, *should_fail, setup.clone()) @@ -306,6 +312,7 @@ impl<'a> ContractRunner<'a> { if has_invariants { let identified_contracts = load_contracts(setup.traces.clone(), known_contracts); + let functions: Vec<&Function> = self .contract .functions() @@ -314,14 +321,26 @@ impl<'a> ContractRunner<'a> { }) .collect(); - let results = self.run_invariant_test( - test_options.invariant_fuzzer(), - setup, - test_options, - functions.clone(), - known_contracts, - identified_contracts, - )?; + let mut results: Vec = vec![]; + + for func in functions.iter() { + let fn_name = &func.name; + let runner = test_options.invariant_runner(self.name, fn_name); + let invariant_config = test_options.invariant_config(self.name, fn_name); + + let invariant_results = self.run_invariant_test( + runner, + setup.clone(), + *invariant_config, + vec![func], + known_contracts, + identified_contracts.clone(), + )?; + + for test_result in invariant_results { + results.push(test_result); + } + } results.into_iter().zip(functions.iter()).for_each(|(result, function)| { match result.kind { @@ -471,7 +490,7 @@ impl<'a> ContractRunner<'a> { &mut self, runner: TestRunner, setup: TestSetup, - test_options: TestOptions, + invariant_config: InvariantConfig, functions: Vec<&Function>, known_contracts: Option<&ContractsByArtifact>, identified_contracts: ContractsByAddress, @@ -484,7 +503,7 @@ impl<'a> ContractRunner<'a> { let mut evm = InvariantExecutor::new( &mut self.executor, runner, - test_options.invariant, + invariant_config, &identified_contracts, project_contracts, ); diff --git a/forge/tests/it/config.rs b/forge/tests/it/config.rs index 53deca42e0e46..8a0034a7364f3 100644 --- a/forge/tests/it/config.rs +++ b/forge/tests/it/config.rs @@ -32,7 +32,7 @@ impl TestConfig { } pub fn with_filter(runner: MultiContractRunner, filter: Filter) -> Self { - Self { runner, should_fail: false, filter, opts: TEST_OPTS } + Self { runner, should_fail: false, filter, opts: test_opts() } } pub fn filter(filter: Filter) -> Self { @@ -55,7 +55,7 @@ impl TestConfig { /// Executes the test runner pub fn test(&mut self) -> BTreeMap { - self.runner.test(&self.filter, None, self.opts).unwrap() + self.runner.test(&self.filter, None, self.opts.clone()).unwrap() } #[track_caller] @@ -69,7 +69,7 @@ impl TestConfig { /// * filter matched 0 test cases /// * a test results deviates from the configured `should_fail` setting pub fn try_run(&mut self) -> eyre::Result<()> { - let suite_result = self.runner.test(&self.filter, None, self.opts).unwrap(); + let suite_result = self.runner.test(&self.filter, None, self.opts.clone()).unwrap(); if suite_result.is_empty() { eyre::bail!("empty test result"); } @@ -100,33 +100,37 @@ impl Default for TestConfig { } } -pub static TEST_OPTS: TestOptions = TestOptions { - fuzz: FuzzConfig { - runs: 256, - max_test_rejects: 65536, - seed: None, - dictionary: FuzzDictionaryConfig { - include_storage: true, - include_push_bytes: true, - dictionary_weight: 40, - max_fuzz_dictionary_addresses: 10_000, - max_fuzz_dictionary_values: 10_000, +pub fn test_opts() -> TestOptions { + TestOptions { + fuzz: FuzzConfig { + runs: 256, + max_test_rejects: 65536, + seed: None, + dictionary: FuzzDictionaryConfig { + include_storage: true, + include_push_bytes: true, + dictionary_weight: 40, + max_fuzz_dictionary_addresses: 10_000, + max_fuzz_dictionary_values: 10_000, + }, }, - }, - invariant: InvariantConfig { - runs: 256, - depth: 15, - fail_on_revert: false, - call_override: false, - dictionary: FuzzDictionaryConfig { - dictionary_weight: 80, - include_storage: true, - include_push_bytes: true, - max_fuzz_dictionary_addresses: 10_000, - max_fuzz_dictionary_values: 10_000, + invariant: InvariantConfig { + runs: 256, + depth: 15, + fail_on_revert: false, + call_override: false, + dictionary: FuzzDictionaryConfig { + dictionary_weight: 80, + include_storage: true, + include_push_bytes: true, + max_fuzz_dictionary_addresses: 10_000, + max_fuzz_dictionary_values: 10_000, + }, }, - }, -}; + inline_fuzz: Default::default(), + inline_invariant: Default::default(), + } +} pub fn manifest_root() -> PathBuf { let mut root = Path::new(env!("CARGO_MANIFEST_DIR")); diff --git a/forge/tests/it/core.rs b/forge/tests/it/core.rs index ddb042e416067..1d88cc4d8b234 100644 --- a/forge/tests/it/core.rs +++ b/forge/tests/it/core.rs @@ -10,7 +10,7 @@ use std::{collections::BTreeMap, env}; #[test] fn test_core() { let mut runner = runner(); - let results = runner.test(&Filter::new(".*", ".*", ".*core"), None, TEST_OPTS).unwrap(); + let results = runner.test(&Filter::new(".*", ".*", ".*core"), None, test_opts()).unwrap(); assert_multiple( &results, @@ -86,7 +86,7 @@ fn test_core() { #[test] fn test_logs() { let mut runner = runner(); - let results = runner.test(&Filter::new(".*", ".*", ".*logs"), None, TEST_OPTS).unwrap(); + let results = runner.test(&Filter::new(".*", ".*", ".*logs"), None, test_opts()).unwrap(); assert_multiple( &results, @@ -649,7 +649,7 @@ fn test_env_vars() { // test `setEnv` first, and confirm that it can correctly set environment variables, // so that we can use it in subsequent `env*` tests - runner.test(&Filter::new("testSetEnv", ".*", ".*"), None, TEST_OPTS).unwrap(); + runner.test(&Filter::new("testSetEnv", ".*", ".*"), None, test_opts()).unwrap(); let env_var_key = "_foundryCheatcodeSetEnvTestKey"; let env_var_val = "_foundryCheatcodeSetEnvTestVal"; let res = env::var(env_var_key); @@ -664,7 +664,7 @@ Reason: `setEnv` failed to set an environment variable `{env_var_key}={env_var_v fn test_doesnt_run_abstract_contract() { let mut runner = runner(); let results = runner - .test(&Filter::new(".*", ".*", ".*Abstract.t.sol".to_string().as_str()), None, TEST_OPTS) + .test(&Filter::new(".*", ".*", ".*Abstract.t.sol".to_string().as_str()), None, test_opts()) .unwrap(); assert!(results.get("core/Abstract.t.sol:AbstractTestBase").is_none()); assert!(results.get("core/Abstract.t.sol:AbstractTest").is_some()); @@ -673,7 +673,7 @@ fn test_doesnt_run_abstract_contract() { #[test] fn test_trace() { let mut runner = tracing_runner(); - let suite_result = runner.test(&Filter::new(".*", ".*", ".*trace"), None, TEST_OPTS).unwrap(); + let suite_result = runner.test(&Filter::new(".*", ".*", ".*trace"), None, test_opts()).unwrap(); // TODO: This trace test is very basic - it is probably a good candidate for snapshot // testing. diff --git a/forge/tests/it/fork.rs b/forge/tests/it/fork.rs index 340854920fced..337e0530585c2 100644 --- a/forge/tests/it/fork.rs +++ b/forge/tests/it/fork.rs @@ -18,7 +18,7 @@ fn test_cheats_fork_revert() { &format!(".*cheats{RE_PATH_SEPARATOR}Fork"), ), None, - TEST_OPTS, + test_opts(), ) .unwrap(); assert_eq!(suite_result.len(), 1); diff --git a/forge/tests/it/fuzz.rs b/forge/tests/it/fuzz.rs index acc4ae027e9c7..54439aaad2066 100644 --- a/forge/tests/it/fuzz.rs +++ b/forge/tests/it/fuzz.rs @@ -15,7 +15,7 @@ fn test_fuzz() { .exclude_tests(r#"invariantCounter|testIncrement\(address\)|testNeedle\(uint256\)"#) .exclude_paths("invariant"), None, - TEST_OPTS, + test_opts(), ) .unwrap(); @@ -53,12 +53,12 @@ fn test_fuzz() { fn test_fuzz_collection() { let mut runner = runner(); - let mut opts = TEST_OPTS; + let mut opts = test_opts(); opts.invariant.depth = 100; opts.invariant.runs = 1000; opts.fuzz.runs = 1000; opts.fuzz.seed = Some(U256::from(6u32)); - runner.test_options = opts; + runner.test_options = opts.clone(); let results = runner.test(&Filter::new(".*", ".*", ".*fuzz/FuzzCollection.t.sol"), None, opts).unwrap(); diff --git a/forge/tests/it/inline.rs b/forge/tests/it/inline.rs new file mode 100644 index 0000000000000..c6c1437a0e695 --- /dev/null +++ b/forge/tests/it/inline.rs @@ -0,0 +1,114 @@ +#[cfg(test)] +mod tests { + use crate::{ + config::runner, + test_helpers::{filter::Filter, COMPILED, PROJECT}, + }; + use forge::{ + result::{SuiteResult, TestKind, TestResult}, + TestOptions, TestOptionsBuilder, + }; + use foundry_config::{FuzzConfig, InvariantConfig}; + + #[test] + fn inline_config_run_fuzz() { + let opts = test_options(); + + let filter = Filter::new(".*", ".*", ".*inline/FuzzInlineConf.t.sol"); + + let mut runner = runner(); + runner.test_options = opts.clone(); + + let result = runner.test(&filter, None, opts).expect("Test ran"); + let suite_result: &SuiteResult = + result.get("inline/FuzzInlineConf.t.sol:FuzzInlineConf").unwrap(); + let test_result: &TestResult = + suite_result.test_results.get("testInlineConfFuzz(uint8)").unwrap(); + match &test_result.kind { + TestKind::Fuzz { runs, .. } => { + assert_eq!(runs, &1024); + } + _ => { + assert!(false); // Force test to fail + } + } + } + + #[test] + fn inline_config_run_invariant() { + const ROOT: &str = "inline/InvariantInlineConf.t.sol"; + + let opts = test_options(); + let filter = Filter::new(".*", ".*", ".*inline/InvariantInlineConf.t.sol"); + let mut runner = runner(); + runner.test_options = opts.clone(); + + let result = runner.test(&filter, None, opts).expect("Test ran"); + + let suite_result_1 = + result.get(&format!("{ROOT}:InvariantInlineConf")).expect("Result exists"); + let suite_result_2 = + result.get(&format!("{ROOT}:InvariantInlineConf2")).expect("Result exists"); + + let test_result_1 = suite_result_1.test_results.get("invariant_neverFalse()").unwrap(); + let test_result_2 = suite_result_2.test_results.get("invariant_neverFalse()").unwrap(); + + match &test_result_1.kind { + TestKind::Invariant { runs, .. } => { + assert_eq!(runs, &333); + } + _ => { + assert!(false); // Force test to fail + } + } + + match &test_result_2.kind { + TestKind::Invariant { runs, .. } => { + assert_eq!(runs, &42); + } + _ => { + assert!(false); // Force test to fail + } + } + } + + #[test] + fn build_test_options() { + let root = &PROJECT.paths.root; + let profiles = vec!["default".to_string(), "ci".to_string()]; + let build_result = TestOptionsBuilder::default() + .fuzz(FuzzConfig::default()) + .invariant(InvariantConfig::default()) + .compile_output(&COMPILED) + .profiles(profiles) + .build(root); + + assert!(build_result.is_ok()); + } + + #[test] + fn build_test_options_just_one_valid_profile() { + let root = &PROJECT.paths.root; + let valid_profiles = vec!["profile-sheldon-cooper".to_string()]; + let build_result = TestOptionsBuilder::default() + .fuzz(FuzzConfig::default()) + .invariant(InvariantConfig::default()) + .compile_output(&COMPILED) + .profiles(valid_profiles) + .build(root); + + // We expect an error, since COMPILED contains in-line + // per-test configs for "default" and "ci" profiles + assert!(build_result.is_err()); + } + + fn test_options() -> TestOptions { + let root = &PROJECT.paths.root; + TestOptionsBuilder::default() + .fuzz(FuzzConfig::default()) + .invariant(InvariantConfig::default()) + .compile_output(&COMPILED) + .build(root) + .expect("Config loaded") + } +} diff --git a/forge/tests/it/invariant.rs b/forge/tests/it/invariant.rs index a31d78c3e3bb1..8c88f37e801c3 100644 --- a/forge/tests/it/invariant.rs +++ b/forge/tests/it/invariant.rs @@ -13,7 +13,7 @@ fn test_invariant() { .test( &Filter::new(".*", ".*", ".*fuzz/invariant/(target|targetAbi|common)"), None, - TEST_OPTS, + test_opts(), ) .unwrap(); @@ -88,9 +88,9 @@ fn test_invariant() { fn test_invariant_override() { let mut runner = runner(); - let mut opts = TEST_OPTS; + let mut opts = test_opts(); opts.invariant.call_override = true; - runner.test_options = opts; + runner.test_options = opts.clone(); let results = runner .test( @@ -113,10 +113,10 @@ fn test_invariant_override() { fn test_invariant_storage() { let mut runner = runner(); - let mut opts = TEST_OPTS; + let mut opts = test_opts(); opts.invariant.depth = 100; opts.fuzz.seed = Some(U256::from(6u32)); - runner.test_options = opts; + runner.test_options = opts.clone(); let results = runner .test( @@ -146,9 +146,9 @@ fn test_invariant_storage() { fn test_invariant_shrink() { let mut runner = runner(); - let mut opts = TEST_OPTS; + let mut opts = test_opts(); opts.fuzz.seed = Some(U256::from(102u32)); - runner.test_options = opts; + runner.test_options = opts.clone(); let results = runner .test( diff --git a/forge/tests/it/main.rs b/forge/tests/it/main.rs index 31c0cf748672e..0ed7348c0bef2 100644 --- a/forge/tests/it/main.rs +++ b/forge/tests/it/main.rs @@ -4,6 +4,7 @@ mod core; mod fork; mod fs; mod fuzz; +mod inline; mod invariant; mod repros; mod spec; diff --git a/forge/tests/it/test_helpers.rs b/forge/tests/it/test_helpers.rs index 1b9465f6e8f2d..a423b01d223d0 100644 --- a/forge/tests/it/test_helpers.rs +++ b/forge/tests/it/test_helpers.rs @@ -83,7 +83,7 @@ pub fn fuzz_executor(executor: &Executor) -> FuzzedExecutor { executor, proptest::test_runner::TestRunner::new(cfg), CALLER, - config::TEST_OPTS.fuzz, + config::test_opts().fuzz, ) } diff --git a/testdata/inline/FuzzInlineConf.t.sol b/testdata/inline/FuzzInlineConf.t.sol new file mode 100644 index 0000000000000..8ab3a16d310bd --- /dev/null +++ b/testdata/inline/FuzzInlineConf.t.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: Unlicense +pragma solidity >=0.8.0; + +import "ds-test/test.sol"; + +contract FuzzInlineConf is DSTest { + /** + * forge-config: default.fuzz.runs = 1024 + * forge-config: default.fuzz.max-test-rejects = 500 + */ + function testInlineConfFuzz(uint8 x) public { + require(true, "this is not going to revert"); + } +} diff --git a/testdata/inline/InvariantInlineConf.t.sol b/testdata/inline/InvariantInlineConf.t.sol new file mode 100644 index 0000000000000..41e534f44a90a --- /dev/null +++ b/testdata/inline/InvariantInlineConf.t.sol @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: Unlicense +pragma solidity >=0.8.0; + +import "ds-test/test.sol"; + +contract InvariantBreaker { + bool public flag0 = true; + bool public flag1 = true; + + function set0(int256 val) public returns (bool) { + if (val % 100 == 0) { + flag0 = false; + } + return flag0; + } + + function set1(int256 val) public returns (bool) { + if (val % 10 == 0 && !flag0) { + flag1 = false; + } + return flag1; + } +} + +contract InvariantInlineConf is DSTest { + InvariantBreaker inv; + + function setUp() public { + inv = new InvariantBreaker(); + } + + /// forge-config: default.invariant.runs = 333 + /// forge-config: default.invariant.depth = 32 + /// forge-config: default.invariant.fail-on-revert = false + /// forge-config: default.invariant.call-override = true + function invariant_neverFalse() public { + require(true, "this is not going to revert"); + } +} + +contract InvariantInlineConf2 is DSTest { + InvariantBreaker inv; + + function setUp() public { + inv = new InvariantBreaker(); + } + + /// forge-config: default.invariant.runs = 42 + function invariant_neverFalse() public { + require(true, "this is not going to revert"); + } +} From cbad9c9c53b54d16921b5b8ccc44c945e62ef9b8 Mon Sep 17 00:00:00 2001 From: Ethereumdegen Date: Fri, 12 May 2023 07:18:38 -0400 Subject: [PATCH 05/12] improving docs generation by adding homepage to config (#4702) * improving docs generation by adding homepage to config * comments * remove comments * chore: rizz up as per comments * chore: skip serializing if empty * chore: fix if --------- Co-authored-by: Enrique Ortiz --- config/src/doc.rs | 6 ++++++ doc/src/builder.rs | 22 +++++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/config/src/doc.rs b/config/src/doc.rs index 4e1952f35a631..2dfac01b4df44 100644 --- a/config/src/doc.rs +++ b/config/src/doc.rs @@ -12,6 +12,11 @@ pub struct DocConfig { pub title: String, /// Path to user provided `book.toml`. pub book: PathBuf, + /// Path to user provided welcome markdown. + /// + /// If none is provided, it defaults to `README.md`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub homepage: Option, /// The repository url. #[serde(default, skip_serializing_if = "Option::is_none")] pub repository: Option, @@ -24,6 +29,7 @@ impl Default for DocConfig { Self { out: PathBuf::from("docs"), book: PathBuf::from("book.toml"), + homepage: Some(PathBuf::from("README.md")), title: String::default(), repository: None, ignore: Vec::default(), diff --git a/doc/src/builder.rs b/doc/src/builder.rs index e629caa053fc5..50c628f366254 100644 --- a/doc/src/builder.rs +++ b/doc/src/builder.rs @@ -231,19 +231,31 @@ impl DocBuilder { fs::create_dir_all(&out_dir_src)?; // Write readme content if any - let readme_content = { - let src_readme = self.sources.join(Self::README); + let homepage_content = { + // Default to the homepage README if it's available. + // If not, use the src README as a fallback. + let homepage_or_src_readme = self + .config + .homepage + .as_ref() + .map(|homepage| self.root.join(homepage)) + .unwrap_or_else(|| self.sources.join(Self::README)); + // Grab the root readme. let root_readme = self.root.join(Self::README); - if src_readme.exists() { - fs::read_to_string(src_readme)? + + //Check to see if there is a 'homepage' option specified in config. + //If not, fall back to src and root readme files, in that order. + if homepage_or_src_readme.exists() { + fs::read_to_string(homepage_or_src_readme)? } else if root_readme.exists() { fs::read_to_string(root_readme)? } else { String::new() } }; + let readme_path = out_dir_src.join(Self::README); - fs::write(&readme_path, readme_content)?; + fs::write(&readme_path, homepage_content)?; // Write summary and section readmes let mut summary = BufWriter::default(); From bd4b2907eb7522371375657f1d2efabb88bd6de6 Mon Sep 17 00:00:00 2001 From: evalir Date: Fri, 12 May 2023 09:13:31 -0400 Subject: [PATCH 06/12] fix(cheatcodes): Fix `expectCall` behavior (#4912) * chore: add tests to test proper behavior * fix(cheatcodes): properly handle all cases for expectCall * chore: allow too many arguments * chore: store calldata as a vec instead of bytes to avoid interior mutability lint * chore: more clippy * chore: add more docs and abstract signature --- .../executor/inspector/cheatcodes/expect.rs | 267 +++++++++++------- evm/src/executor/inspector/cheatcodes/mod.rs | 133 ++++++--- testdata/cheats/ExpectCall.t.sol | 78 +++++ 3 files changed, 333 insertions(+), 145 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/expect.rs b/evm/src/executor/inspector/cheatcodes/expect.rs index a657dfd869604..9371f6b6f8317 100644 --- a/evm/src/executor/inspector/cheatcodes/expect.rs +++ b/evm/src/executor/inspector/cheatcodes/expect.rs @@ -168,16 +168,26 @@ pub fn handle_expect_emit(state: &mut Cheatcodes, log: RawLog, address: &Address #[derive(Clone, Debug, Default)] pub struct ExpectedCallData { - /// The expected calldata - pub calldata: Bytes, /// The expected value sent in the call pub value: Option, /// The expected gas supplied to the call pub gas: Option, /// The expected *minimum* gas supplied to the call pub min_gas: Option, - /// The number of times the call is expected to be made - pub count: Option, + /// The number of times the call is expected to be made. + /// If the type of call is `NonCount`, this is the lower bound for the number of calls + /// that must be seen. + /// If the type of call is `Count`, this is the exact number of calls that must be seen. + pub count: u64, + /// The type of call + pub call_type: ExpectedCallType, +} + +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub enum ExpectedCallType { + #[default] + Count, + NonCount, } #[derive(Clone, Debug, Default, PartialEq, Eq)] @@ -220,6 +230,71 @@ fn expect_safe_memory(state: &mut Cheatcodes, start: u64, end: u64, depth: u64) Ok(Bytes::new()) } +/// Handles expected calls specified by the `vm.expectCall` cheatcode. +/// +/// It can handle calls in two ways: +/// - If the cheatcode was used with a `count` argument, it will expect the call to be made exactly +/// `count` times. +/// e.g. `vm.expectCall(address(0xc4f3), abi.encodeWithSelector(0xd34db33f), 4)` will expect the +/// call to address(0xc4f3) with selector `0xd34db33f` to be made exactly 4 times. If the amount of +/// calls is less or more than 4, the test will fail. Note that the `count` argument cannot be +/// overwritten with another `vm.expectCall`. If this is attempted, `expectCall` will revert. +/// - If the cheatcode was used without a `count` argument, it will expect the call to be made at +/// least the amount of times the cheatcode +/// was called. This means that `vm.expectCall` without a count argument can be called many times, +/// but cannot be called with a `count` argument after it was called without one. If the latter +/// happens, `expectCall` will revert. e.g `vm.expectCall(address(0xc4f3), +/// abi.encodeWithSelector(0xd34db33f))` will expect the call to address(0xc4f3) and selector +/// `0xd34db33f` to be made at least once. If the amount of calls is 0, the test will fail. If the +/// call is made more than once, the test will pass. +#[allow(clippy::too_many_arguments)] +fn expect_call( + state: &mut Cheatcodes, + target: H160, + calldata: Vec, + value: Option, + gas: Option, + min_gas: Option, + count: u64, + call_type: ExpectedCallType, +) -> Result { + match call_type { + ExpectedCallType::Count => { + // Get the expected calls for this target. + let expecteds = state.expected_calls.entry(target).or_default(); + // In this case, as we're using counted expectCalls, we should not be able to set them + // more than once. + ensure!( + !expecteds.contains_key(&calldata), + "Counted expected calls can only bet set once." + ); + expecteds + .insert(calldata, (ExpectedCallData { value, gas, min_gas, count, call_type }, 0)); + Ok(Bytes::new()) + } + ExpectedCallType::NonCount => { + let expecteds = state.expected_calls.entry(target).or_default(); + // Check if the expected calldata exists. + // If it does, increment the count by one as we expect to see it one more time. + if let Some(expected) = expecteds.get_mut(&calldata) { + // Ensure we're not overwriting a counted expectCall. + ensure!( + expected.0.call_type == ExpectedCallType::NonCount, + "Cannot overwrite a counted expectCall with a non-counted expectCall." + ); + expected.0.count += 1; + } else { + // If it does not exist, then create it. + expecteds.insert( + calldata, + (ExpectedCallData { value, gas, min_gas, count, call_type }, 0), + ); + } + Ok(Bytes::new()) + } + } +} + pub fn apply( state: &mut Cheatcodes, data: &mut EVMData<'_, DB>, @@ -267,125 +342,113 @@ pub fn apply( }); Ok(Bytes::new()) } - HEVMCalls::ExpectCall0(inner) => { - state.expected_calls.entry(inner.0).or_default().push(( - ExpectedCallData { - calldata: inner.1.to_vec().into(), - value: None, - gas: None, - min_gas: None, - count: None, - }, - 0, - )); - Ok(Bytes::new()) - } - HEVMCalls::ExpectCall1(inner) => { - state.expected_calls.entry(inner.0).or_default().push(( - ExpectedCallData { - calldata: inner.1.to_vec().into(), - value: None, - gas: None, - min_gas: None, - count: Some(inner.2), - }, - 0, - )); - Ok(Bytes::new()) - } - HEVMCalls::ExpectCall2(inner) => { - state.expected_calls.entry(inner.0).or_default().push(( - ExpectedCallData { - calldata: inner.2.to_vec().into(), - value: Some(inner.1), - gas: None, - min_gas: None, - count: None, - }, - 0, - )); - Ok(Bytes::new()) - } - HEVMCalls::ExpectCall3(inner) => { - state.expected_calls.entry(inner.0).or_default().push(( - ExpectedCallData { - calldata: inner.2.to_vec().into(), - value: Some(inner.1), - gas: None, - min_gas: None, - count: Some(inner.3), - }, - 0, - )); - Ok(Bytes::new()) - } + HEVMCalls::ExpectCall0(inner) => expect_call( + state, + inner.0, + inner.1.to_vec(), + None, + None, + None, + 1, + ExpectedCallType::NonCount, + ), + HEVMCalls::ExpectCall1(inner) => expect_call( + state, + inner.0, + inner.1.to_vec(), + None, + None, + None, + inner.2, + ExpectedCallType::Count, + ), + HEVMCalls::ExpectCall2(inner) => expect_call( + state, + inner.0, + inner.2.to_vec(), + Some(inner.1), + None, + None, + 1, + ExpectedCallType::NonCount, + ), + HEVMCalls::ExpectCall3(inner) => expect_call( + state, + inner.0, + inner.2.to_vec(), + Some(inner.1), + None, + None, + inner.3, + ExpectedCallType::Count, + ), HEVMCalls::ExpectCall4(inner) => { let value = inner.1; - // If the value of the transaction is non-zero, the EVM adds a call stipend of 2300 gas // to ensure that the basic fallback function can be called. let positive_value_cost_stipend = if value > U256::zero() { 2300 } else { 0 }; - state.expected_calls.entry(inner.0).or_default().push(( - ExpectedCallData { - calldata: inner.3.to_vec().into(), - value: Some(value), - gas: Some(inner.2 + positive_value_cost_stipend), - min_gas: None, - count: None, - }, - 0, - )); - Ok(Bytes::new()) + expect_call( + state, + inner.0, + inner.3.to_vec(), + Some(value), + Some(inner.2 + positive_value_cost_stipend), + None, + 1, + ExpectedCallType::NonCount, + ) } HEVMCalls::ExpectCall5(inner) => { let value = inner.1; + // If the value of the transaction is non-zero, the EVM adds a call stipend of 2300 gas + // to ensure that the basic fallback function can be called. let positive_value_cost_stipend = if value > U256::zero() { 2300 } else { 0 }; - state.expected_calls.entry(inner.0).or_default().push(( - ExpectedCallData { - calldata: inner.3.to_vec().into(), - value: Some(value), - gas: Some(inner.2 + positive_value_cost_stipend), - min_gas: None, - count: Some(inner.4), - }, - 0, - )); - Ok(Bytes::new()) + + expect_call( + state, + inner.0, + inner.3.to_vec(), + Some(value), + Some(inner.2 + positive_value_cost_stipend), + None, + inner.4, + ExpectedCallType::Count, + ) } HEVMCalls::ExpectCallMinGas0(inner) => { let value = inner.1; - // If the value of the transaction is non-zero, the EVM adds a call stipend of 2300 gas // to ensure that the basic fallback function can be called. let positive_value_cost_stipend = if value > U256::zero() { 2300 } else { 0 }; - state.expected_calls.entry(inner.0).or_default().push(( - ExpectedCallData { - calldata: inner.3.to_vec().into(), - value: Some(value), - gas: None, - min_gas: Some(inner.2 + positive_value_cost_stipend), - count: None, - }, - 0, - )); - Ok(Bytes::new()) + expect_call( + state, + inner.0, + inner.3.to_vec(), + Some(value), + None, + Some(inner.2 + positive_value_cost_stipend), + 1, + ExpectedCallType::NonCount, + ) } HEVMCalls::ExpectCallMinGas1(inner) => { let value = inner.1; + // If the value of the transaction is non-zero, the EVM adds a call stipend of 2300 gas + // to ensure that the basic fallback function can be called. let positive_value_cost_stipend = if value > U256::zero() { 2300 } else { 0 }; - state.expected_calls.entry(inner.0).or_default().push(( - ExpectedCallData { - calldata: inner.3.to_vec().into(), - value: Some(value), - gas: None, - min_gas: Some(inner.2 + positive_value_cost_stipend), - count: Some(inner.4), - }, - 0, - )); - Ok(Bytes::new()) + + expect_call( + state, + inner.0, + inner.3.to_vec(), + Some(value), + None, + Some(inner.2 + positive_value_cost_stipend), + inner.4, + ExpectedCallType::Count, + ) } HEVMCalls::MockCall0(inner) => { // TODO: Does this increase gas usage? diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index 345e320f1dedc..99bc0baa12780 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -1,6 +1,6 @@ use self::{ env::Broadcast, - expect::{handle_expect_emit, handle_expect_revert}, + expect::{handle_expect_emit, handle_expect_revert, ExpectedCallType}, util::{check_if_fixed_gas_limit, process_create, BroadcastableTransactions}, }; use crate::{ @@ -69,6 +69,15 @@ mod error; pub(crate) use error::{bail, ensure, err}; pub use error::{Error, Result}; +/// Tracks the expected calls per address. +/// For each address, we track the expected calls per call data. We track it in such manner +/// so that we don't mix together calldatas that only contain selectors and calldatas that contain +/// selector and arguments (partial and full matches). +/// This then allows us to customize the matching behavior for each call data on the +/// `ExpectedCallData` struct and track how many times we've actually seen the call on the second +/// element of the tuple. +pub type ExpectedCallTracker = BTreeMap, (ExpectedCallData, u64)>>; + /// An inspector that handles calls to various cheatcodes, each with their own behavior. /// /// Cheatcodes can be called by contracts during execution to modify the VM environment, such as @@ -126,7 +135,7 @@ pub struct Cheatcodes { pub mocked_calls: BTreeMap>, /// Expected calls - pub expected_calls: BTreeMap>, + pub expected_calls: ExpectedCallTracker, /// Expected emits pub expected_emits: Vec, @@ -565,15 +574,29 @@ where } } else if call.contract != h160_to_b160(HARDHAT_CONSOLE_ADDRESS) { // Handle expected calls - if let Some(expecteds) = self.expected_calls.get_mut(&(b160_to_h160(call.contract))) { - if let Some((_, count)) = expecteds.iter_mut().find(|(expected, _)| { - expected.calldata.len() <= call.input.len() && - expected.calldata == call.input[..expected.calldata.len()] && - expected.value.map_or(true, |value| value == call.transfer.value.into()) && + + // Grab the different calldatas expected. + if let Some(expected_calls_for_target) = + self.expected_calls.get_mut(&(b160_to_h160(call.contract))) + { + // Match every partial/full calldata + for (calldata, (expected, actual_count)) in expected_calls_for_target.iter_mut() { + // Increment actual times seen if... + // The calldata is at most, as big as this call's input, and + if calldata.len() <= call.input.len() && + // Both calldata match, taking the length of the assumed smaller one (which will have at least the selector), and + *calldata == call.input[..calldata.len()] && + // The value matches, if provided + expected + .value + .map_or(true, |value| value == call.transfer.value.into()) && + // The gas matches, if provided expected.gas.map_or(true, |gas| gas == call.gas_limit) && + // The minimum gas matches, if provided expected.min_gas.map_or(true, |min_gas| min_gas <= call.gas_limit) - }) { - *count += 1; + { + *actual_count += 1; + } } } @@ -782,43 +805,67 @@ where // If the depth is 0, then this is the root call terminating if data.journaled_state.depth() == 0 { - for (address, expecteds) in &self.expected_calls { - for (expected, actual_count) in expecteds { - let ExpectedCallData { calldata, gas, min_gas, value, count } = expected; - let calldata = calldata.clone(); - let expected_values = [ - Some(format!("data {calldata}")), - value.map(|v| format!("value {v}")), - gas.map(|g| format!("gas {g}")), - min_gas.map(|g| format!("minimum gas {g}")), - ] - .into_iter() - .flatten() - .join(" and "); - if count.is_none() { - if *actual_count == 0 { - return ( - InstructionResult::Revert, - remaining_gas, - format!("Expected at least one call to {address:?} with {expected_values}, but got none") + // Match expected calls + for (address, calldatas) in &self.expected_calls { + // Loop over each address, and for each address, loop over each calldata it expects. + for (calldata, (expected, actual_count)) in calldatas { + // Grab the values we expect to see + let ExpectedCallData { gas, min_gas, value, count, call_type } = expected; + let calldata = Bytes::from(calldata.clone()); + + // We must match differently depending on the type of call we expect. + match call_type { + // If the cheatcode was called with a `count` argument, + // we must check that the EVM performed a CALL with this calldata exactly + // `count` times. + ExpectedCallType::Count => { + if *count != *actual_count { + let expected_values = [ + Some(format!("data {calldata}")), + value.map(|v| format!("value {v}")), + gas.map(|g| format!("gas {g}")), + min_gas.map(|g| format!("minimum gas {g}")), + ] + .into_iter() + .flatten() + .join(" and "); + return ( + InstructionResult::Revert, + remaining_gas, + format!( + "Expected call to {address:?} with {expected_values} to be called {count} time(s), but was called {actual_count} time(s)" + ) .encode() .into(), - ) + ) + } + } + // If the cheatcode was called without a `count` argument, + // we must check that the EVM performed a CALL with this calldata at least + // `count` times. The amount of times to check was + // the amount of time the cheatcode was called. + ExpectedCallType::NonCount => { + if *count > *actual_count { + let expected_values = [ + Some(format!("data {calldata}")), + value.map(|v| format!("value {v}")), + gas.map(|g| format!("gas {g}")), + min_gas.map(|g| format!("minimum gas {g}")), + ] + .into_iter() + .flatten() + .join(" and "); + return ( + InstructionResult::Revert, + remaining_gas, + format!( + "Expected call to {address:?} with {expected_values} to be called at least {count} time(s), but was called {actual_count} time(s)" + ) + .encode() + .into(), + ) + } } - } else if *count != Some(*actual_count) { - return ( - InstructionResult::Revert, - remaining_gas, - format!( - "Expected call to {:?} with {} to be made {} time(s), but was called {} time(s)", - address, - expected_values, - count.unwrap(), - actual_count, - ) - .encode() - .into(), - ) } } } diff --git a/testdata/cheats/ExpectCall.t.sol b/testdata/cheats/ExpectCall.t.sol index 6d088155a7e17..783ce296e350e 100644 --- a/testdata/cheats/ExpectCall.t.sol +++ b/testdata/cheats/ExpectCall.t.sol @@ -62,6 +62,33 @@ contract ExpectCallTest is DSTest { target.add(1, 2); } + function testExpectMultipleCallsWithDataAdditive() public { + Contract target = new Contract(); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); + target.add(1, 2); + target.add(1, 2); + } + + function testExpectMultipleCallsWithDataAdditiveLowerBound() public { + Contract target = new Contract(); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); + target.add(1, 2); + target.add(1, 2); + target.add(1, 2); + } + + function testFailExpectMultipleCallsWithDataAdditive() public { + Contract target = new Contract(); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); + // Not enough calls to satisfy the additive expectCall, which expects 3 calls. + target.add(1, 2); + target.add(1, 2); + } + function testFailExpectCallWithData() public { Contract target = new Contract(); cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); @@ -312,3 +339,54 @@ contract ExpectCallCountTest is DSTest { target.addHardGasLimit(); } } + +contract ExpectCallMixedTest is DSTest { + Cheats constant cheats = Cheats(HEVM_ADDRESS); + + function testFailOverrideNoCountWithCount() public { + Contract target = new Contract(); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); + // You should not be able to overwrite a expectCall that had no count with some count. + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2), 2); + target.add(1, 2); + target.add(1, 2); + } + + function testFailOverrideCountWithCount() public { + Contract target = new Contract(); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2), 2); + // You should not be able to overwrite a expectCall that had a count with some count. + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2), 1); + target.add(1, 2); + target.add(1, 2); + } + + function testFailOverrideCountWithNoCount() public { + Contract target = new Contract(); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2), 2); + // You should not be able to overwrite a expectCall that had a count with no count. + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); + target.add(1, 2); + target.add(1, 2); + } + + function testExpectMatchPartialAndFull() public { + Contract target = new Contract(); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector), 2); + // Even if a partial match is speciifed, you should still be able to look for full matches + // as one does not override the other. + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); + target.add(1, 2); + target.add(1, 2); + } + + function testExpectMatchPartialAndFullFlipped() public { + Contract target = new Contract(); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector)); + // Even if a partial match is speciifed, you should still be able to look for full matches + // as one does not override the other. + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2), 2); + target.add(1, 2); + target.add(1, 2); + } +} From 387c5eb045beccf8f76d5d2d3d5be6e4fc46faae Mon Sep 17 00:00:00 2001 From: evalir Date: Fri, 12 May 2023 10:44:01 -0400 Subject: [PATCH 07/12] feat(cheatcodes): Make `expectEmit` only work for the next call (#4920) * chore: add new expect emit logic * feat: handle expect emits on the next immediate call and error appropiately * chore: tests * chore: simplify errors * chore: remove unused actual count * chore: clippy * chore: remove unneeded test artifacts * chore: ignore STATICCALLs * chore: fix additive behavior * chore: add more tests * chore: lint * chore: be able to match in between events rather than strictly full-sequences * chore: clippy * chore: lint expect emit * chore: simplify if --- .../executor/inspector/cheatcodes/expect.rs | 121 ++++++++++------ evm/src/executor/inspector/cheatcodes/mod.rs | 53 ++++--- testdata/cheats/ExpectEmit.t.sol | 130 ++++++++++++++++-- testdata/cheats/GetDeployedCode.t.sol | 2 +- 4 files changed, 234 insertions(+), 72 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/expect.rs b/evm/src/executor/inspector/cheatcodes/expect.rs index 9371f6b6f8317..046955fbba9de 100644 --- a/evm/src/executor/inspector/cheatcodes/expect.rs +++ b/evm/src/executor/inspector/cheatcodes/expect.rs @@ -123,46 +123,79 @@ pub struct ExpectedEmit { } pub fn handle_expect_emit(state: &mut Cheatcodes, log: RawLog, address: &Address) { - // Fill or check the expected emits - if let Some(next_expect_to_fill) = - state.expected_emits.iter_mut().find(|expect| expect.log.is_none()) - { - // We have unfilled expects, so we fill the first one - next_expect_to_fill.log = Some(log); - } else if let Some(next_expect) = state.expected_emits.iter_mut().find(|expect| !expect.found) { - // We do not have unfilled expects, so we try to match this log with the first unfound - // log that we expect - let expected = - next_expect.log.as_ref().expect("we should have a log to compare against here"); - - let expected_topic_0 = expected.topics.get(0); - let log_topic_0 = log.topics.get(0); - - // same topic0 and equal number of topics should be verified further, others are a no - // match - if expected_topic_0 - .zip(log_topic_0) - .map_or(false, |(a, b)| a == b && expected.topics.len() == log.topics.len()) - { - // Match topics - next_expect.found = log - .topics - .iter() - .skip(1) - .enumerate() - .filter(|(i, _)| next_expect.checks[*i]) - .all(|(i, topic)| topic == &expected.topics[i + 1]); - - // Maybe match source address - if let Some(addr) = next_expect.address { - next_expect.found &= addr == *address; + // Fill or check the expected emits. + // We expect for emit checks to be filled as they're declared (from oldest to newest), + // so we fill them and push them to the back of the queue. + // If the user has properly filled all the emits, they'll end up in their original order. + // If not, the queue will not be in the order the events will be intended to be filled, + // and we'll be able to later detect this and bail. + + // First, we can return early if all events have been matched. + // This allows a contract to arbitrarily emit more events than expected (additive behavior), + // as long as all the previous events were matched in the order they were expected to be. + if state.expected_emits.iter().all(|expected| expected.found) { + return + } + + // if there's anything to fill, we need to pop back. + let event_to_fill_or_check = + if state.expected_emits.iter().any(|expected| expected.log.is_none()) { + state.expected_emits.pop_back() + // Else, if there are any events that are unmatched, we try to match to match them + // in the order declared, so we start popping from the front (like a queue). + } else { + state.expected_emits.pop_front() + }; + + let mut event_to_fill_or_check = + event_to_fill_or_check.expect("We should have an emit to fill or check. This is a bug"); + + match event_to_fill_or_check.log { + Some(ref expected) => { + let expected_topic_0 = expected.topics.get(0); + let log_topic_0 = log.topics.get(0); + + // same topic0 and equal number of topics should be verified further, others are a no + // match + if expected_topic_0 + .zip(log_topic_0) + .map_or(false, |(a, b)| a == b && expected.topics.len() == log.topics.len()) + { + // Match topics + event_to_fill_or_check.found = log + .topics + .iter() + .skip(1) + .enumerate() + .filter(|(i, _)| event_to_fill_or_check.checks[*i]) + .all(|(i, topic)| topic == &expected.topics[i + 1]); + + // Maybe match source address + if let Some(addr) = event_to_fill_or_check.address { + event_to_fill_or_check.found &= addr == *address; + } + + // Maybe match data + if event_to_fill_or_check.checks[3] { + event_to_fill_or_check.found &= expected.data == log.data; + } } - // Maybe match data - if next_expect.checks[3] { - next_expect.found &= expected.data == log.data; + // If we found the event, we can push it to the back of the queue + // and begin expecting the next event. + if event_to_fill_or_check.found { + state.expected_emits.push_back(event_to_fill_or_check); + } else { + // We did not match this event, so we need to keep waiting for the right one to + // appear. + state.expected_emits.push_front(event_to_fill_or_check); } } + // Fill the event. + None => { + event_to_fill_or_check.log = Some(log); + state.expected_emits.push_back(event_to_fill_or_check); + } } } @@ -309,16 +342,16 @@ pub fn apply( expect_revert(state, Some(inner.0.into()), data.journaled_state.depth()) } HEVMCalls::ExpectEmit0(_) => { - state.expected_emits.push(ExpectedEmit { - depth: data.journaled_state.depth() - 1, + state.expected_emits.push_back(ExpectedEmit { + depth: data.journaled_state.depth(), checks: [true, true, true, true], ..Default::default() }); Ok(Bytes::new()) } HEVMCalls::ExpectEmit1(inner) => { - state.expected_emits.push(ExpectedEmit { - depth: data.journaled_state.depth() - 1, + state.expected_emits.push_back(ExpectedEmit { + depth: data.journaled_state.depth(), checks: [true, true, true, true], address: Some(inner.0), ..Default::default() @@ -326,16 +359,16 @@ pub fn apply( Ok(Bytes::new()) } HEVMCalls::ExpectEmit2(inner) => { - state.expected_emits.push(ExpectedEmit { - depth: data.journaled_state.depth() - 1, + state.expected_emits.push_back(ExpectedEmit { + depth: data.journaled_state.depth(), checks: [inner.0, inner.1, inner.2, inner.3], ..Default::default() }); Ok(Bytes::new()) } HEVMCalls::ExpectEmit3(inner) => { - state.expected_emits.push(ExpectedEmit { - depth: data.journaled_state.depth() - 1, + state.expected_emits.push_back(ExpectedEmit { + depth: data.journaled_state.depth(), checks: [inner.0, inner.1, inner.2, inner.3], address: Some(inner.4), ..Default::default() diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index 99bc0baa12780..700f6311ee02d 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -29,7 +29,7 @@ use revm::{ }; use serde_json::Value; use std::{ - collections::{BTreeMap, HashMap}, + collections::{BTreeMap, HashMap, VecDeque}, fs::File, io::BufReader, ops::Range, @@ -138,7 +138,7 @@ pub struct Cheatcodes { pub expected_calls: ExpectedCallTracker, /// Expected emits - pub expected_emits: Vec, + pub expected_emits: VecDeque, /// Map of context depths to memory offset ranges that may be written to within the call depth. pub allowed_mem_writes: BTreeMap>>, @@ -536,7 +536,6 @@ where topics: &[B256], data: &bytes::Bytes, ) { - // Match logs if `expectEmit` has been called if !self.expected_emits.is_empty() { handle_expect_emit( self, @@ -786,21 +785,38 @@ where } } - // Handle expected emits at current depth - if !self + // At the end of the call, + // we need to check if we've found all the emits. + // We know we've found all the expected emits in the right order + // if the queue is fully matched. + // If it's not fully matched, then either: + // 1. Not enough events were emitted (we'll know this because the amount of times we + // inspected events will be less than the size of the queue) 2. The wrong events + // were emitted (The inspected events should match the size of the queue, but still some + // events will not be matched) + + // First, check that we're at the call depth where the emits were declared from. + let should_check_emits = self .expected_emits .iter() - .filter(|expected| expected.depth == data.journaled_state.depth()) - .all(|expected| expected.found) - { - return ( - InstructionResult::Revert, - remaining_gas, - "Log != expected log".to_string().encode().into(), - ) - } else { - // Clear the emits we expected at this depth that have been found - self.expected_emits.retain(|expected| !expected.found) + .any(|expected| expected.depth == data.journaled_state.depth()) && + // Ignore staticcalls + !call.is_static; + // If so, check the emits + if should_check_emits { + // Not all emits were matched. + if self.expected_emits.iter().any(|expected| !expected.found) { + return ( + InstructionResult::Revert, + remaining_gas, + "Log != expected log".to_string().encode().into(), + ) + } else { + // All emits were found, we're good. + // Clear the queue, as we expect the user to declare more events for the next call + // if they wanna match further events. + self.expected_emits.clear() + } } // If the depth is 0, then this is the root call terminating @@ -871,11 +887,14 @@ where } // Check if we have any leftover expected emits + // First, if any emits were found at the root call, then we its ok and we remove them. + self.expected_emits.retain(|expected| !expected.found); + // If not empty, we got mismatched emits if !self.expected_emits.is_empty() { return ( InstructionResult::Revert, remaining_gas, - "Expected an emit, but no logs were emitted afterward" + "Expected an emit, but no logs were emitted afterward. You might have mismatched events or not enough events were emitted." .to_string() .encode() .into(), diff --git a/testdata/cheats/ExpectEmit.t.sol b/testdata/cheats/ExpectEmit.t.sol index a5e22576a9149..717f11db07bb4 100644 --- a/testdata/cheats/ExpectEmit.t.sol +++ b/testdata/cheats/ExpectEmit.t.sol @@ -5,6 +5,8 @@ import "ds-test/test.sol"; import "./Cheats.sol"; contract Emitter { + uint256 public thing; + event Something(uint256 indexed topic1, uint256 indexed topic2, uint256 indexed topic3, uint256 data); /// This event has 0 indexed topics, but the one in our tests @@ -15,6 +17,8 @@ contract Emitter { /// Ref: issue #760 event SomethingElse(uint256 data); + event SomethingNonIndexed(uint256 data); + function emitEvent(uint256 topic1, uint256 topic2, uint256 topic3, uint256 data) public { emit Something(topic1, topic2, topic3, data); } @@ -29,13 +33,33 @@ contract Emitter { emit Something(topic1[1], topic2[1], topic3[1], data[1]); } + function emitAndNest() public { + emit Something(1, 2, 3, 4); + emitNested(Emitter(address(this)), 1, 2, 3, 4); + } + + function emitOutOfExactOrder() public { + emit SomethingNonIndexed(1); + emit Something(1, 2, 3, 4); + emit Something(1, 2, 3, 4); + emit Something(1, 2, 3, 4); + } + function emitNested(Emitter inner, uint256 topic1, uint256 topic2, uint256 topic3, uint256 data) public { inner.emitEvent(topic1, topic2, topic3, data); } + function getVar() public view returns (uint256) { + return 1; + } + /// Ref: issue #1214 function doesNothing() public pure {} + function changeThing(uint256 num) public { + thing = num; + } + /// Ref: issue #760 function emitSomethingElse(uint256 data) public { emit SomethingElse(data); @@ -59,6 +83,8 @@ contract ExpectEmitTest is DSTest { event SomethingElse(uint256 indexed topic1); + event SomethingNonIndexed(uint256 data); + function setUp() public { emitter = new Emitter(); } @@ -182,6 +208,15 @@ contract ExpectEmitTest is DSTest { ); } + function testExpectedEmitMultipleNested() public { + cheats.expectEmit(); + emit Something(1, 2, 3, 4); + cheats.expectEmit(); + emit Something(1, 2, 3, 4); + + emitter.emitAndNest(); + } + function testExpectEmitMultipleWithArgs() public { cheats.expectEmit(true, true, true, true); emit Something(1, 2, 3, 4); @@ -193,6 +228,37 @@ contract ExpectEmitTest is DSTest { ); } + function testExpectEmitCanMatchWithoutExactOrder() public { + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + + emitter.emitOutOfExactOrder(); + } + + function testFailExpectEmitCanMatchWithoutExactOrder() public { + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + // This should fail, as this event is never emitted + // in between the other two Something events. + cheats.expectEmit(true, true, true, true); + emit SomethingElse(1); + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + + emitter.emitOutOfExactOrder(); + } + + function testExpectEmitCanMatchWithoutExactOrder2() public { + cheats.expectEmit(true, true, true, true); + emit SomethingNonIndexed(1); + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + + emitter.emitOutOfExactOrder(); + } + function testExpectEmitAddress() public { cheats.expectEmit(address(emitter)); emit Something(1, 2, 3, 4); @@ -232,6 +298,18 @@ contract ExpectEmitTest is DSTest { caller.f(); } + function testFailNoEmitDirectlyOnNextCall() public { + LowLevelCaller caller = new LowLevelCaller(); + + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + + // This call does not emit. As emit expects the next call to emit, this should fail. + caller.f(); + // This call does emit, but it is a call later than expected. + emitter.emitEvent(1, 2, 3, 4); + } + /// Ref: issue #760 function testFailDifferentIndexedParameters() public { cheats.expectEmit(true, false, false, false); @@ -243,6 +321,37 @@ contract ExpectEmitTest is DSTest { emitter.emitSomethingElse(1); } + function testCanDoStaticCall() public { + cheats.expectEmit(true, true, true, true); + emit Something(emitter.getVar(), 2, 3, 4); + + emitter.emitEvent(1, 2, 3, 4); + } + + /// Tests for additive behavior. + // As long as we match the event we want in order, it doesn't matter which events are emitted afterwards. + function testAdditiveBehavior() public { + cheats.expectEmit(true, true, true, true, address(emitter)); + emit Something(1, 2, 3, 4); + + emitter.emitMultiple( + [uint256(1), uint256(5)], [uint256(2), uint256(6)], [uint256(3), uint256(7)], [uint256(4), uint256(8)] + ); + } + + /// This test should fail, as the call to `changeThing` is not a static call. + /// While we can ignore static calls, we cannot ignore normal calls. + function testFailEmitOnlyAppliesToNextCall() public { + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + // This works because it's a staticcall. + emitter.doesNothing(); + // This should make the test fail as it's a normal call. + emitter.changeThing(block.timestamp); + + emitter.emitEvent(1, 2, 3, 4); + } + /// This test will fail if we check that all expected logs were emitted /// after every call from the same depth as the call that invoked the cheatcode. /// @@ -250,14 +359,15 @@ contract ExpectEmitTest is DSTest { /// was invoked ends. /// /// Ref: issue #1214 - function testExpectEmitIsCheckedWhenCurrentCallTerminates() public { - cheats.expectEmit(true, true, true, true); - emitter.doesNothing(); - emit Something(1, 2, 3, 4); - - // This should fail since `SomethingElse` in the test - // and in the `Emitter` contract have differing - // amounts of indexed topics. - emitter.emitEvent(1, 2, 3, 4); - } + /// NOTE: This is now invalid behavior. + // function testExpectEmitIsCheckedWhenCurrentCallTerminates() public { + // cheats.expectEmit(true, true, true, true); + // emitter.doesNothing(); + // emit Something(1, 2, 3, 4); + + // // This should fail since `SomethingElse` in the test + // // and in the `Emitter` contract have differing + // // amounts of indexed topics. + // emitter.emitEvent(1, 2, 3, 4); + // } } diff --git a/testdata/cheats/GetDeployedCode.t.sol b/testdata/cheats/GetDeployedCode.t.sol index 4e9753cd992dc..9021e4253e5df 100644 --- a/testdata/cheats/GetDeployedCode.t.sol +++ b/testdata/cheats/GetDeployedCode.t.sol @@ -33,8 +33,8 @@ contract GetDeployedCodeTest is DSTest { Override over = Override(overrideAddress); vm.expectEmit(true, false, false, true); - over.emitPayload(address(0), "hello"); emit Payload(address(this), address(0), "hello"); + over.emitPayload(address(0), "hello"); } } From 9eb89746042f605b5f9e8d957a51b9a9d9bea126 Mon Sep 17 00:00:00 2001 From: grandizzy <38490174+grandizzy@users.noreply.github.com> Date: Fri, 12 May 2023 18:03:16 +0300 Subject: [PATCH 08/12] Invariants feat: add config option to turn off shrinking (#4868) * - add try_shrinking config in [invariant], default true, tries to reduce number of calls in scenario to min - when set to false test will just error with the original number of calls in scenario * Add comment * Improve option name: shrink_sequence * Changes after request: proper way to fix warnings * add shrink-sequence to valid config keys for the parser --------- Co-authored-by: Matthias Seitz Co-authored-by: Enrique Ortiz --- config/README.md | 1 + config/src/invariant.rs | 4 ++++ config/src/lib.rs | 1 + evm/src/fuzz/invariant/error.rs | 10 +++++++++- evm/src/fuzz/invariant/executor.rs | 12 ++++++++++-- evm/src/fuzz/invariant/mod.rs | 1 + forge/tests/it/config.rs | 1 + testdata/foundry.toml | 1 + 8 files changed, 28 insertions(+), 3 deletions(-) diff --git a/config/README.md b/config/README.md index 4d68412755a01..f1699c531668e 100644 --- a/config/README.md +++ b/config/README.md @@ -187,6 +187,7 @@ call_override = false dictionary_weight = 80 include_storage = true include_push_bytes = true +shrink_sequence = true [fmt] line_length = 100 diff --git a/config/src/invariant.rs b/config/src/invariant.rs index ad49e40fffb84..2e4ebe43bc1a1 100644 --- a/config/src/invariant.rs +++ b/config/src/invariant.rs @@ -24,6 +24,8 @@ pub struct InvariantConfig { /// The fuzz dictionary configuration #[serde(flatten)] pub dictionary: FuzzDictionaryConfig, + /// Attempt to shrink the failure case to its smallest sequence of calls + pub shrink_sequence: bool, } impl Default for InvariantConfig { @@ -34,6 +36,7 @@ impl Default for InvariantConfig { fail_on_revert: false, call_override: false, dictionary: FuzzDictionaryConfig { dictionary_weight: 80, ..Default::default() }, + shrink_sequence: true, } } } @@ -61,6 +64,7 @@ impl InlineConfigParser for InvariantConfig { "depth" => conf_clone.depth = parse_config_u32(key, value)?, "fail-on-revert" => conf_clone.fail_on_revert = parse_config_bool(key, value)?, "call-override" => conf_clone.call_override = parse_config_bool(key, value)?, + "shrink-sequence" => conf_clone.shrink_sequence = parse_config_bool(key, value)?, _ => Err(InlineConfigParserError::InvalidConfigProperty(key.to_string()))?, } } diff --git a/config/src/lib.rs b/config/src/lib.rs index 328869408c78e..990b4e20bcb4c 100644 --- a/config/src/lib.rs +++ b/config/src/lib.rs @@ -3377,6 +3377,7 @@ mod tests { depth = 15 fail_on_revert = false call_override = false + shrink_sequence = true "#, )?; diff --git a/evm/src/fuzz/invariant/error.rs b/evm/src/fuzz/invariant/error.rs index 31064331aca2a..619f5a40a4f7c 100644 --- a/evm/src/fuzz/invariant/error.rs +++ b/evm/src/fuzz/invariant/error.rs @@ -31,6 +31,8 @@ pub struct InvariantFuzzError { pub func: Option, /// Inner fuzzing Sequence coming from overriding calls. pub inner_sequence: Vec>, + /// Shrink the failed test case to the smallest sequence. + pub shrink: bool, } impl InvariantFuzzError { @@ -40,6 +42,7 @@ impl InvariantFuzzError { calldata: &[BasicTxDetails], call_result: RawCallResult, inner_sequence: &[Option], + shrink_sequence: bool, ) -> Self { let mut func = None; let origin: String; @@ -80,6 +83,7 @@ impl InvariantFuzzError { addr: invariant_contract.address, func, inner_sequence: inner_sequence.to_vec(), + shrink: shrink_sequence, } } @@ -99,7 +103,11 @@ impl InvariantFuzzError { TestError::Fail(_, ref calls) => calls, }; - let calls = self.try_shrinking(calls, &executor); + if self.shrink { + let _ = self.try_shrinking(calls, &executor); + } else { + trace!(target: "forge::test", "Shrinking disabled."); + } // We want traces for a failed case. executor.set_tracing(true); diff --git a/evm/src/fuzz/invariant/executor.rs b/evm/src/fuzz/invariant/executor.rs index f0658a862e4ff..c982e904398a3 100644 --- a/evm/src/fuzz/invariant/executor.rs +++ b/evm/src/fuzz/invariant/executor.rs @@ -187,6 +187,7 @@ impl<'a> InvariantExecutor<'a> { &inputs, &mut failures.borrow_mut(), self.config.fail_on_revert, + self.config.shrink_sequence, ); if !can_continue { @@ -558,6 +559,7 @@ fn can_continue( calldata: &[BasicTxDetails], failures: &mut InvariantFailures, fail_on_revert: bool, + shrink_sequence: bool, ) -> (bool, Option>) { let mut call_results = None; if !call_result.reverted { @@ -571,8 +573,14 @@ fn can_continue( // The user might want to stop all execution if a revert happens to // better bound their testing space. if fail_on_revert { - let error = - InvariantFuzzError::new(invariant_contract, None, calldata, call_result, &[]); + let error = InvariantFuzzError::new( + invariant_contract, + None, + calldata, + call_result, + &[], + shrink_sequence, + ); failures.revert_reason = Some(error.revert_reason.clone()); diff --git a/evm/src/fuzz/invariant/mod.rs b/evm/src/fuzz/invariant/mod.rs index b956f80742d56..79e428f87b2bf 100644 --- a/evm/src/fuzz/invariant/mod.rs +++ b/evm/src/fuzz/invariant/mod.rs @@ -95,6 +95,7 @@ pub fn assert_invariants( calldata, call_result, &inner_sequence, + true, )), ); found_case = true; diff --git a/forge/tests/it/config.rs b/forge/tests/it/config.rs index 8a0034a7364f3..d703738a2b0c7 100644 --- a/forge/tests/it/config.rs +++ b/forge/tests/it/config.rs @@ -126,6 +126,7 @@ pub fn test_opts() -> TestOptions { max_fuzz_dictionary_addresses: 10_000, max_fuzz_dictionary_values: 10_000, }, + shrink_sequence: true, }, inline_fuzz: Default::default(), inline_invariant: Default::default(), diff --git a/testdata/foundry.toml b/testdata/foundry.toml index 29dc2bf427822..da4c21696c2dd 100644 --- a/testdata/foundry.toml +++ b/testdata/foundry.toml @@ -16,6 +16,7 @@ ffi = false force = false invariant_fail_on_revert = false invariant_call_override = false +invariant_shrink_sequence = true gas_limit = 9223372036854775807 gas_price = 0 gas_reports = ['*'] From 7be2425f72dbe6d2a16496db4ab2f33ff9a7c50f Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Fri, 12 May 2023 17:16:13 +0200 Subject: [PATCH 09/12] fix: ensure prevrandao is set (#4929) * Script: ensure prevrandao is set, even if no mixHash in response * Update evm/src/executor/fork/init.rs Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com> * Update evm/src/executor/fork/init.rs Co-authored-by: evalir * Update evm/src/executor/fork/init.rs * chore: ensure prevrandao is set --------- Co-authored-by: 0xCalibur <0xCalibur@protonmail.com> Co-authored-by: evalir Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com> --- evm/src/executor/fork/init.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evm/src/executor/fork/init.rs b/evm/src/executor/fork/init.rs index 193a5fd172929..9bbadc9776788 100644 --- a/evm/src/executor/fork/init.rs +++ b/evm/src/executor/fork/init.rs @@ -67,7 +67,7 @@ where timestamp: block.timestamp.into(), coinbase: h160_to_b160(block.author.unwrap_or_default()), difficulty: block.difficulty.into(), - prevrandao: block.mix_hash.map(h256_to_b256), + prevrandao: Some(block.mix_hash.map(h256_to_b256).unwrap_or_default()), basefee: block.base_fee_per_gas.unwrap_or_default().into(), gas_limit: block.gas_limit.into(), }, From a17896182a5525684579ef0af7c68654f533ff26 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Fri, 12 May 2023 17:20:21 +0200 Subject: [PATCH 10/12] fix: add missing ethers-solc feature (#4930) --- evm/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evm/Cargo.toml b/evm/Cargo.toml index ccc155d5cba4c..0e6ccec7c61f3 100644 --- a/evm/Cargo.toml +++ b/evm/Cargo.toml @@ -16,7 +16,7 @@ foundry-macros = { path = "../macros" } serde_json = "1.0.95" serde = "1.0.159" hex = "0.4.3" -ethers = { workspace = true, features = ["solc-full", "abigen"] } +ethers = { workspace = true, features = ["solc-full", "abigen", "ethers-solc"] } jsonpath_lib = "0.3.0" # Error handling From 46823a586f91e360faf851e496b056461c8e3257 Mon Sep 17 00:00:00 2001 From: evalir Date: Fri, 12 May 2023 11:31:09 -0400 Subject: [PATCH 11/12] feat(repo): add `CHANGELOG.md` (#4925) * chore: add changelog * chore: remove brackets from changes that still have not been done * chore: evm version change --- CHANGELOG.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000000000..2a57543ab643e --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,31 @@ +## Pre 1.0 + +### Important note for users + +Multiple breaking changes will occur so Semver can be followed as soon as Foundry 1.0 is released. They will be listed here, along with the updates needed for your projects. + +If you need a stable Foundry version, we recommend using the latest pinned nightly of May 2nd, locally and on your CI. + +To use the latest pinned nightly locally, use the following command: + +``` +foundryup --version nightly-e15e33a07c0920189fc336391f538c3dad53da73 +```` + +To use the latest pinned nightly on your CI, modify your Foundry installation step to use an specific version: + +``` +- name: Install Foundry + uses: foundry-rs/foundry-toolchain@v1 + with: + version: nightly-e15e33a07c0920189fc336391f538c3dad53da73 +``` + +### Breaking changes + +- [expectEmit](https://github.com/foundry-rs/foundry/pull/4920) will now only work for the next call. +- expectCall will now only work if the call(s) are made exactly after the cheatcode is invoked. +- expectRevert will now work if the next call does revert, instead of expecting a revert during the whole test. +- [precompiles will not be compatible with all cheatcodes](https://github.com/foundry-rs/foundry/pull/4905). +- The difficulty and prevrandao cheatcodes now [fail if not used with the correct EVM version](https://github.com/foundry-rs/foundry/pull/4904). +- The default EVM version will be Shanghai. If you're using an EVM chain which is not compatible with [EIP-3855](https://eips.ethereum.org/EIPS/eip-3855) you need to change your EVM version. See [Matt Solomon's thread](https://twitter.com/msolomon44) for more information. \ No newline at end of file From 82df0e92486fa2359dd1d149ba1352e7951760a1 Mon Sep 17 00:00:00 2001 From: Devan Non <89424366+devanoneth@users.noreply.github.com> Date: Fri, 12 May 2023 19:15:48 +0200 Subject: [PATCH 12/12] Fix link in CHANGELOG.md (#4932) * Fix link in CHANGELOG.md * Really fix it this time --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a57543ab643e..eba276c44f576 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,4 +28,4 @@ To use the latest pinned nightly on your CI, modify your Foundry installation st - expectRevert will now work if the next call does revert, instead of expecting a revert during the whole test. - [precompiles will not be compatible with all cheatcodes](https://github.com/foundry-rs/foundry/pull/4905). - The difficulty and prevrandao cheatcodes now [fail if not used with the correct EVM version](https://github.com/foundry-rs/foundry/pull/4904). -- The default EVM version will be Shanghai. If you're using an EVM chain which is not compatible with [EIP-3855](https://eips.ethereum.org/EIPS/eip-3855) you need to change your EVM version. See [Matt Solomon's thread](https://twitter.com/msolomon44) for more information. \ No newline at end of file +- The default EVM version will be Shanghai. If you're using an EVM chain which is not compatible with [EIP-3855](https://eips.ethereum.org/EIPS/eip-3855) you need to change your EVM version. See [Matt Solomon's thread](https://twitter.com/msolomon44/status/1656411871635972096) for more information.