From 04e2263ff8ffcd7bfd2b705a0f7af08209f800e4 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Mon, 8 Apr 2024 19:13:41 +0400 Subject: [PATCH] fix: smarter `getCode` validation (#7597) * fix: smarter getCode validation * clippy + doc fix * fix * multi-version profile and tests * fmt * add more parsing options * clippy + fmt --- crates/cheatcodes/src/config.rs | 7 + crates/cheatcodes/src/fs.rs | 122 +++++++++++++----- crates/chisel/src/executor.rs | 1 + crates/forge/bin/cmd/coverage.rs | 1 + crates/forge/bin/cmd/test/mod.rs | 1 + crates/forge/src/multi_runner.rs | 55 ++++---- crates/forge/tests/it/cheats.rs | 19 ++- crates/forge/tests/it/test_helpers.rs | 14 +- crates/script/src/execute.rs | 1 + crates/script/src/lib.rs | 8 +- testdata/default/cheats/GetCode.t.sol | 12 ++ testdata/multi-version/Counter.sol | 11 ++ testdata/multi-version/Importer.sol | 7 + testdata/multi-version/cheats/GetCode.t.sol | 25 ++++ testdata/multi-version/cheats/GetCode17.t.sol | 26 ++++ 15 files changed, 238 insertions(+), 72 deletions(-) create mode 100644 testdata/multi-version/Counter.sol create mode 100644 testdata/multi-version/Importer.sol create mode 100644 testdata/multi-version/cheats/GetCode.t.sol create mode 100644 testdata/multi-version/cheats/GetCode17.t.sol diff --git a/crates/cheatcodes/src/config.rs b/crates/cheatcodes/src/config.rs index 9d0ca2ea460d..f93ab4f5867c 100644 --- a/crates/cheatcodes/src/config.rs +++ b/crates/cheatcodes/src/config.rs @@ -8,6 +8,7 @@ use foundry_config::{ ResolvedRpcEndpoints, }; use foundry_evm_core::opts::EvmOpts; +use semver::Version; use std::{ collections::HashMap, path::{Path, PathBuf}, @@ -47,6 +48,8 @@ pub struct CheatsConfig { /// If Some, `vm.getDeployedCode` invocations are validated to be in scope of this list. /// If None, no validation is performed. pub available_artifacts: Option>, + /// Version of the script/test contract which is currently running. + pub running_version: Option, } impl CheatsConfig { @@ -56,6 +59,7 @@ impl CheatsConfig { evm_opts: EvmOpts, available_artifacts: Option>, script_wallets: Option, + running_version: Option, ) -> Self { let mut allowed_paths = vec![config.__root.0.clone()]; allowed_paths.extend(config.libs.clone()); @@ -82,6 +86,7 @@ impl CheatsConfig { labels: config.labels.clone(), script_wallets, available_artifacts, + running_version, } } @@ -200,6 +205,7 @@ impl Default for CheatsConfig { labels: Default::default(), script_wallets: None, available_artifacts: Default::default(), + running_version: Default::default(), } } } @@ -215,6 +221,7 @@ mod tests { Default::default(), None, None, + None, ) } diff --git a/crates/cheatcodes/src/fs.rs b/crates/cheatcodes/src/fs.rs index 9760b4f162c1..54335a5e529b 100644 --- a/crates/cheatcodes/src/fs.rs +++ b/crates/cheatcodes/src/fs.rs @@ -271,14 +271,40 @@ impl Cheatcode for getDeployedCodeCall { } /// Returns the path to the json artifact depending on the input +/// +/// Can parse following input formats: +/// - `path/to/artifact.json` +/// - `path/to/contract.sol` +/// - `path/to/contract.sol:ContractName` +/// - `path/to/contract.sol:ContractName:0.8.23` +/// - `path/to/contract.sol:0.8.23` +/// - `ContractName` +/// - `ContractName:0.8.23` fn get_artifact_path(state: &Cheatcodes, path: &str) -> Result { if path.ends_with(".json") { Ok(PathBuf::from(path)) } else { let mut parts = path.split(':'); - let file = PathBuf::from(parts.next().unwrap()); - let contract_name = parts.next(); - let version = parts.next(); + + let mut file = None; + let mut contract_name = None; + let mut version = None; + + let path_or_name = parts.next().unwrap(); + if path_or_name.ends_with(".sol") { + file = Some(PathBuf::from(path_or_name)); + if let Some(name_or_version) = parts.next() { + if name_or_version.contains('.') { + version = Some(name_or_version); + } else { + contract_name = Some(name_or_version); + version = parts.next(); + } + } + } else { + contract_name = Some(path_or_name); + version = parts.next(); + } let version = if let Some(version) = version { Some(Version::parse(version).map_err(|_| fmt_err!("Error parsing version"))?) @@ -288,44 +314,70 @@ fn get_artifact_path(state: &Cheatcodes, path: &str) -> Result { // Use available artifacts list if available if let Some(available_ids) = &state.config.available_artifacts { - let mut artifact = None; - - for id in available_ids.iter() { - // name might be in the form of "Counter.0.8.23" - let id_name = id.name.split('.').next().unwrap(); - - if !id.source.ends_with(&file) { - continue; - } - if let Some(name) = contract_name { - if id_name != name { - continue; + let filtered = available_ids + .iter() + .filter(|id| { + // name might be in the form of "Counter.0.8.23" + let id_name = id.name.split('.').next().unwrap(); + + if let Some(path) = &file { + if !id.source.ends_with(path) { + return false; + } } - } - if let Some(ref version) = version { - if id.version.minor != version.minor || - id.version.major != version.major || - id.version.patch != version.patch - { - continue; + if let Some(name) = contract_name { + if id_name != name { + return false; + } } + if let Some(ref version) = version { + if id.version.minor != version.minor || + id.version.major != version.major || + id.version.patch != version.patch + { + return false; + } + } + true + }) + .collect::>(); + + let artifact = match filtered.len() { + 0 => Err(fmt_err!("No matching artifact found")), + 1 => Ok(filtered[0]), + _ => { + // If we know the current script/test contract solc version, try to filter by it + state + .config + .running_version + .as_ref() + .and_then(|version| { + let filtered = filtered + .into_iter() + .filter(|id| id.version == *version) + .collect::>(); + + (filtered.len() == 1).then_some(filtered[0]) + }) + .ok_or_else(|| fmt_err!("Multiple matching artifacts found")) } - if artifact.is_some() { - return Err(fmt_err!("Multiple matching artifacts found")); - } - artifact = Some(id); - } + }?; - let artifact = artifact.ok_or_else(|| fmt_err!("No matching artifact found"))?; Ok(artifact.path.clone()) } else { - let file = file.to_string_lossy(); - let contract_name = if let Some(contract_name) = contract_name { - contract_name.to_owned() - } else { - file.replace(".sol", "") - }; - Ok(state.config.paths.artifacts.join(format!("{file}/{contract_name}.json"))) + let path_in_artifacts = + match (file.map(|f| f.to_string_lossy().to_string()), contract_name) { + (Some(file), Some(contract_name)) => Ok(format!("{file}/{contract_name}.json")), + (None, Some(contract_name)) => { + Ok(format!("{contract_name}.sol/{contract_name}.json")) + } + (Some(file), None) => { + let name = file.replace(".sol", ""); + Ok(format!("{file}/{name}.json")) + } + _ => Err(fmt_err!("Invalid artifact path")), + }?; + Ok(state.config.paths.artifacts.join(path_in_artifacts)) } } } diff --git a/crates/chisel/src/executor.rs b/crates/chisel/src/executor.rs index e0774a2b6878..6343013b18ba 100644 --- a/crates/chisel/src/executor.rs +++ b/crates/chisel/src/executor.rs @@ -308,6 +308,7 @@ impl SessionSource { self.config.evm_opts.clone(), None, None, + self.solc.version().ok(), ) .into(), ) diff --git a/crates/forge/bin/cmd/coverage.rs b/crates/forge/bin/cmd/coverage.rs index 8dc4629b9f7e..29538164b84a 100644 --- a/crates/forge/bin/cmd/coverage.rs +++ b/crates/forge/bin/cmd/coverage.rs @@ -317,6 +317,7 @@ impl CoverageArgs { evm_opts.clone(), Some(artifact_ids), None, + None, )) .with_test_options(TestOptions { fuzz: config.fuzz, diff --git a/crates/forge/bin/cmd/test/mod.rs b/crates/forge/bin/cmd/test/mod.rs index 104fa4c486e0..16f5ec516633 100644 --- a/crates/forge/bin/cmd/test/mod.rs +++ b/crates/forge/bin/cmd/test/mod.rs @@ -287,6 +287,7 @@ impl TestArgs { evm_opts.clone(), Some(artifact_ids), None, + None, // populated separately for each test contract )) .with_test_options(test_options) .enable_isolation(evm_opts.isolate) diff --git a/crates/forge/src/multi_runner.rs b/crates/forge/src/multi_runner.rs index 94d0b0155a8e..f19e67d22ec3 100644 --- a/crates/forge/src/multi_runner.rs +++ b/crates/forge/src/multi_runner.rs @@ -9,13 +9,8 @@ use foundry_compilers::{ artifacts::Libraries, contracts::ArtifactContracts, Artifact, ArtifactId, ProjectCompileOutput, }; use foundry_evm::{ - backend::Backend, - decode::RevertDecoder, - executors::{Executor, ExecutorBuilder}, - fork::CreateFork, - inspectors::CheatsConfig, - opts::EvmOpts, - revm, + backend::Backend, decode::RevertDecoder, executors::ExecutorBuilder, fork::CreateFork, + inspectors::CheatsConfig, opts::EvmOpts, revm, }; use foundry_linking::{LinkOutput, Linker}; use rayon::prelude::*; @@ -158,18 +153,6 @@ impl MultiContractRunner { // The DB backend that serves all the data. let db = Backend::spawn(self.fork.take()); - let executor = ExecutorBuilder::new() - .inspectors(|stack| { - stack - .cheatcodes(self.cheats_config.clone()) - .trace(self.evm_opts.verbosity >= 3 || self.debug) - .debug(self.debug) - .coverage(self.coverage) - .enable_isolation(self.isolation) - }) - .spec(self.evm_spec) - .gas_limit(self.evm_opts.gas_limit()) - .build(self.env.clone(), db); let find_timer = Instant::now(); let contracts = self.matching_contracts(filter).collect::>(); @@ -182,30 +165,46 @@ impl MultiContractRunner { ); contracts.par_iter().for_each_with(tx, |tx, &(id, contract)| { - let identifier = id.identifier(); - let executor = executor.clone(); - let result = self.run_tests(&identifier, contract, executor, filter); - let _ = tx.send((identifier, result)); + let result = self.run_tests(id, contract, db.clone(), filter); + let _ = tx.send((id.identifier(), result)); }) } fn run_tests( &self, - name: &str, + artifact_id: &ArtifactId, contract: &TestContract, - executor: Executor, + db: Backend, filter: &dyn TestFilter, ) -> SuiteResult { - let mut span_name = name; + let identifier = artifact_id.identifier(); + let mut span_name = identifier.as_str(); + + let mut cheats_config = self.cheats_config.as_ref().clone(); + cheats_config.running_version = Some(artifact_id.version.clone()); + + let executor = ExecutorBuilder::new() + .inspectors(|stack| { + stack + .cheatcodes(Arc::new(cheats_config)) + .trace(self.evm_opts.verbosity >= 3 || self.debug) + .debug(self.debug) + .coverage(self.coverage) + .enable_isolation(self.isolation) + }) + .spec(self.evm_spec) + .gas_limit(self.evm_opts.gas_limit()) + .build(self.env.clone(), db); + if !enabled!(tracing::Level::TRACE) { - span_name = get_contract_name(span_name); + span_name = get_contract_name(&identifier); } let _guard = info_span!("run_tests", name = span_name).entered(); debug!("start executing all tests in contract"); let runner = ContractRunner::new( - name, + &identifier, executor, contract, self.evm_opts.initial_balance, diff --git a/crates/forge/tests/it/cheats.rs b/crates/forge/tests/it/cheats.rs index fd6e9866e61d..920dd8f2d2c1 100644 --- a/crates/forge/tests/it/cheats.rs +++ b/crates/forge/tests/it/cheats.rs @@ -2,14 +2,13 @@ use crate::{ config::*, - test_helpers::{RE_PATH_SEPARATOR, TEST_DATA_DEFAULT}, + test_helpers::{ForgeTestData, RE_PATH_SEPARATOR, TEST_DATA_DEFAULT, TEST_DATA_MULTI_VERSION}, }; use foundry_config::{fs_permissions::PathPermission, FsPermissions}; use foundry_test_utils::Filter; /// Executes all cheat code tests but not fork cheat codes -#[tokio::test(flavor = "multi_thread")] -async fn test_cheats_local() { +async fn test_cheats_local(test_data: &ForgeTestData) { let mut filter = Filter::new(".*", ".*", &format!(".*cheats{RE_PATH_SEPARATOR}*")).exclude_paths("Fork"); @@ -18,9 +17,19 @@ async fn test_cheats_local() { filter = filter.exclude_tests("(Ffi|File|Line|Root)"); } - let mut config = TEST_DATA_DEFAULT.config.clone(); + let mut config = test_data.config.clone(); config.fs_permissions = FsPermissions::new(vec![PathPermission::read_write("./")]); - let runner = TEST_DATA_DEFAULT.runner_with_config(config); + let runner = test_data.runner_with_config(config); TestConfig::with_filter(runner, filter).run().await; } + +#[tokio::test(flavor = "multi_thread")] +async fn test_cheats_local_default() { + test_cheats_local(&TEST_DATA_DEFAULT).await +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_cheats_local_multi_version() { + test_cheats_local(&TEST_DATA_MULTI_VERSION).await +} diff --git a/crates/forge/tests/it/test_helpers.rs b/crates/forge/tests/it/test_helpers.rs index 8d54976cc786..a8ab8229c4bf 100644 --- a/crates/forge/tests/it/test_helpers.rs +++ b/crates/forge/tests/it/test_helpers.rs @@ -32,6 +32,7 @@ const TESTDATA: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/../../testdata"); pub enum ForgeTestProfile { Default, Cancun, + MultiVersion, } impl fmt::Display for ForgeTestProfile { @@ -39,6 +40,7 @@ impl fmt::Display for ForgeTestProfile { match self { ForgeTestProfile::Default => write!(f, "default"), ForgeTestProfile::Cancun => write!(f, "cancun"), + ForgeTestProfile::MultiVersion => write!(f, "multi-version"), } } } @@ -206,7 +208,13 @@ impl ForgeTestData { let output = self.output.clone(); let artifact_ids = output.artifact_ids().map(|(id, _)| id).collect(); self.base_runner() - .with_cheats_config(CheatsConfig::new(&config, opts.clone(), Some(artifact_ids), None)) + .with_cheats_config(CheatsConfig::new( + &config, + opts.clone(), + Some(artifact_ids), + None, + None, + )) .sender(config.sender) .with_test_options(self.test_opts.clone()) .build(root, output, env, opts.clone()) @@ -274,6 +282,10 @@ pub static TEST_DATA_DEFAULT: Lazy = pub static TEST_DATA_CANCUN: Lazy = Lazy::new(|| ForgeTestData::new(ForgeTestProfile::Cancun)); +/// Data for tests requiring Cancun support on Solc and EVM level. +pub static TEST_DATA_MULTI_VERSION: Lazy = + Lazy::new(|| ForgeTestData::new(ForgeTestProfile::MultiVersion)); + pub fn manifest_root() -> &'static Path { let mut root = Path::new(env!("CARGO_MANIFEST_DIR")); // need to check here where we're executing the test from, if in `forge` we need to also allow diff --git a/crates/script/src/execute.rs b/crates/script/src/execute.rs index 7a2546ec822c..7b811a67cd91 100644 --- a/crates/script/src/execute.rs +++ b/crates/script/src/execute.rs @@ -102,6 +102,7 @@ impl PreExecutionState { self.build_data.build_data.artifact_ids.clone(), self.script_wallets.clone(), self.args.debug, + self.build_data.build_data.target.clone(), ) .await?; let mut result = self.execute_with_runner(&mut runner).await?; diff --git a/crates/script/src/lib.rs b/crates/script/src/lib.rs index 5b27a5ff420e..530c84d36c7b 100644 --- a/crates/script/src/lib.rs +++ b/crates/script/src/lib.rs @@ -559,13 +559,14 @@ impl ScriptConfig { artifact_ids: Vec, script_wallets: ScriptWallets, debug: bool, + target: ArtifactId, ) -> Result { - self._get_runner(Some((artifact_ids, script_wallets)), debug).await + self._get_runner(Some((artifact_ids, script_wallets, target)), debug).await } async fn _get_runner( &mut self, - cheats_data: Option<(Vec, ScriptWallets)>, + cheats_data: Option<(Vec, ScriptWallets, ArtifactId)>, debug: bool, ) -> Result { trace!("preparing script runner"); @@ -594,7 +595,7 @@ impl ScriptConfig { .spec(self.config.evm_spec_id()) .gas_limit(self.evm_opts.gas_limit()); - if let Some((artifact_ids, script_wallets)) = cheats_data { + if let Some((artifact_ids, script_wallets, target)) = cheats_data { builder = builder.inspectors(|stack| { stack .debug(debug) @@ -604,6 +605,7 @@ impl ScriptConfig { self.evm_opts.clone(), Some(artifact_ids), Some(script_wallets), + Some(target.version), ) .into(), ) diff --git a/testdata/default/cheats/GetCode.t.sol b/testdata/default/cheats/GetCode.t.sol index d308712e9d6b..73980d7b2981 100644 --- a/testdata/default/cheats/GetCode.t.sol +++ b/testdata/default/cheats/GetCode.t.sol @@ -6,6 +6,8 @@ import "cheats/Vm.sol"; contract TestContract {} +contract TestContractGetCode {} + contract GetCodeTest is DSTest { Vm constant vm = Vm(HEVM_ADDRESS); @@ -80,4 +82,14 @@ contract GetCodeTest is DSTest { vm._expectCheatcodeRevert("No matching artifact found"); vm.getCode("cheats/GetCode.t.sol:TestContract:0.8.19"); } + + function testByName() public { + bytes memory code = vm.getCode("TestContractGetCode"); + assertEq(type(TestContractGetCode).creationCode, code); + } + + function testByNameAndVersion() public { + bytes memory code = vm.getCode("TestContractGetCode:0.8.18"); + assertEq(type(TestContractGetCode).creationCode, code); + } } diff --git a/testdata/multi-version/Counter.sol b/testdata/multi-version/Counter.sol new file mode 100644 index 000000000000..4f0c350335f8 --- /dev/null +++ b/testdata/multi-version/Counter.sol @@ -0,0 +1,11 @@ +contract Counter { + uint256 public number; + + function setNumber(uint256 newNumber) public { + number = newNumber; + } + + function increment() public { + number++; + } +} diff --git a/testdata/multi-version/Importer.sol b/testdata/multi-version/Importer.sol new file mode 100644 index 000000000000..d8e274e8f544 --- /dev/null +++ b/testdata/multi-version/Importer.sol @@ -0,0 +1,7 @@ +pragma solidity 0.8.17; + +import "./Counter.sol"; + +// Please do not remove or change version pragma for this file. +// If you need to ensure that some of the files are compiled with +// solc 0.8.17, you should add imports of them to this file. diff --git a/testdata/multi-version/cheats/GetCode.t.sol b/testdata/multi-version/cheats/GetCode.t.sol new file mode 100644 index 000000000000..e4a7bd14ae4e --- /dev/null +++ b/testdata/multi-version/cheats/GetCode.t.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +pragma solidity 0.8.18; + +import "ds-test/test.sol"; +import "cheats/Vm.sol"; +import "../Counter.sol"; + +contract GetCodeTest is DSTest { + Vm constant vm = Vm(HEVM_ADDRESS); + + function testGetCodeMultiVersion() public { + assertEq(vm.getCode("Counter.sol"), type(Counter).creationCode); + require( + keccak256(vm.getCode("Counter.sol")) != keccak256(vm.getCode("Counter.sol:Counter:0.8.17")), + "Invalid artifact" + ); + assertEq(vm.getCode("Counter.sol"), vm.getCode("Counter.sol:Counter:0.8.18")); + } + + function testGetCodeByNameMultiVersion() public { + assertEq(vm.getCode("Counter"), type(Counter).creationCode); + require(keccak256(vm.getCode("Counter")) != keccak256(vm.getCode("Counter:0.8.17")), "Invalid artifact"); + assertEq(vm.getCode("Counter"), vm.getCode("Counter:0.8.18")); + } +} diff --git a/testdata/multi-version/cheats/GetCode17.t.sol b/testdata/multi-version/cheats/GetCode17.t.sol new file mode 100644 index 000000000000..068a910cf7b6 --- /dev/null +++ b/testdata/multi-version/cheats/GetCode17.t.sol @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +pragma solidity 0.8.17; + +import "ds-test/test.sol"; +import "cheats/Vm.sol"; +import "../Counter.sol"; + +// Same as GetCode.t.sol but for 0.8.17 version +contract GetCodeTest17 is DSTest { + Vm constant vm = Vm(HEVM_ADDRESS); + + function testGetCodeMultiVersion() public { + assertEq(vm.getCode("Counter.sol"), type(Counter).creationCode); + require( + keccak256(vm.getCode("Counter.sol")) != keccak256(vm.getCode("Counter.sol:Counter:0.8.18")), + "Invalid artifact" + ); + assertEq(vm.getCode("Counter.sol"), vm.getCode("Counter.sol:Counter:0.8.17")); + } + + function testGetCodeByNameMultiVersion() public { + assertEq(vm.getCode("Counter"), type(Counter).creationCode); + require(keccak256(vm.getCode("Counter")) != keccak256(vm.getCode("Counter:0.8.18")), "Invalid artifact"); + assertEq(vm.getCode("Counter.sol"), vm.getCode("Counter:0.8.17")); + } +}