From 5da2eb1eb922c20bf9da88780e5a368cf3641176 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Thu, 27 Jan 2022 11:04:14 +0100 Subject: [PATCH] fix(solc): duplicate contracts segments (#832) * chore: simplify touch * forge install: ds-test * fix: fix duplicate contracts segments * fix: typos --- ethers-solc/src/config.rs | 65 ++++++++++++++++++++++++++++++++--- ethers-solc/src/remappings.rs | 59 +++++++++++++++++-------------- ethers-solc/src/utils.rs | 36 +------------------ 3 files changed, 95 insertions(+), 65 deletions(-) diff --git a/ethers-solc/src/config.rs b/ethers-solc/src/config.rs index ba2e26dd..bc810501 100644 --- a/ethers-solc/src/config.rs +++ b/ethers-solc/src/config.rs @@ -128,14 +128,69 @@ impl ProjectPathsConfig { /// Attempts to find the path to the real solidity file that's imported via the given `import` /// path by applying the configured remappings and checking the library dirs + /// + /// # Example + /// + /// Following `@aave` dependency in the `lib` folder `node_modules` + /// + /// ```text + /// /node_modules/@aave + /// ├── aave-token + /// │ ├── contracts + /// │ │ ├── open-zeppelin + /// │ │ ├── token + /// ├── governance-v2 + /// ├── contracts + /// ├── interfaces + /// ``` + /// + /// has this remapping: `@aave/=@aave/` (name:path) so contracts can be imported as + /// + /// ```solidity + /// import "@aave/governance-v2/contracts/governance/Executor.sol"; + /// ``` + /// + /// So that `Executor.sol` can be found by checking each `lib` folder (`node_modules`) with + /// applied remappings. Applying remapping works by checking if the import path of an import + /// statement starts with the name of a remapping and replacing it with the remapping's `path`. + /// + /// There are some caveats though, dapptools style remappings usually include the `src` folder + /// `ds-test/=lib/ds-test/src/` so that imports look like `import "ds-test/test.sol";` (note the + /// missing `src` in the import path). + /// + /// For hardhat/npm style that's not always the case, most notably for [openzeppelin-contracts](https://github.com/OpenZeppelin/openzeppelin-contracts) if installed via npm. + /// The remapping is detected as `'@openzeppelin/=node_modules/@openzeppelin/contracts/'`, which + /// includes the source directory `contracts`, however it's common to see import paths like: + /// + /// `import "@openzeppelin/contracts/token/ERC20/IERC20.sol";` + /// + /// instead of + /// + /// `import "@openzeppelin/token/ERC20/IERC20.sol";` + /// + /// There is no strict rule behind this, but because [`Remappings::find_many`] returns + /// `'@openzeppelin/=node_modules/@openzeppelin/contracts/'` we should handle the case if the + /// remapping path ends with `contracts` and the import path starts with `/contracts`. Otherwise we can end up with a resolved path that has a duplicate + /// `contracts` segment: `@openzeppelin/contracts/contracts/token/ERC20/IERC20.sol` we check + /// for this edge case here so that both styles work out of the box. pub fn resolve_library_import(&self, import: &Path) -> Option { // if the import path starts with the name of the remapping then we get the resolved path by // removing the name and adding the remainder to the path of the remapping - if let Some(path) = self - .remappings - .iter() - .find_map(|r| import.strip_prefix(&r.name).ok().map(|p| Path::new(&r.path).join(p))) - { + if let Some(path) = self.remappings.iter().find_map(|r| { + import.strip_prefix(&r.name).ok().map(|stripped_import| { + let lib_path = Path::new(&r.path).join(stripped_import); + + // we handle the edge case where the path of a remapping ends with "contracts" + // (`/=.../contracts`) and the stripped import also starts with `contracts` + if let Ok(adjusted_import) = stripped_import.strip_prefix("contracts/") { + if r.path.ends_with("contracts/") && !lib_path.exists() { + return Path::new(&r.path).join(adjusted_import) + } + } + lib_path + }) + }) { Some(self.root.join(path)) } else { utils::resolve_library(&self.libraries, import) diff --git a/ethers-solc/src/remappings.rs b/ethers-solc/src/remappings.rs index 6492a018..681bf66b 100644 --- a/ethers-solc/src/remappings.rs +++ b/ethers-solc/src/remappings.rs @@ -491,7 +491,7 @@ fn last_nested_source_dir(root: &Path, dir: &Path) -> PathBuf { #[cfg(test)] mod tests { use super::*; - use crate::utils::tempdir; + use crate::{utils::tempdir, ProjectPathsConfig}; #[test] fn relative_remapping() { @@ -538,6 +538,9 @@ mod tests { fn mkdir_or_touch(tmp: &std::path::Path, paths: &[&str]) { for path in paths { + if let Some(parent) = Path::new(path).parent() { + std::fs::create_dir_all(tmp.join(parent)).unwrap(); + } if path.ends_with(".sol") { let path = tmp.join(path); touch(&path).unwrap(); @@ -569,35 +572,52 @@ mod tests { assert_eq!(remappings[0].path, format!("{}/src/", path)); } + #[test] + fn can_resolve_oz_remappings() { + let tmp_dir = tempdir("node_modules").unwrap(); + let tmp_dir_node_modules = tmp_dir.path().join("node_modules"); + let paths = [ + "node_modules/@openzeppelin/contracts/interfaces/IERC1155.sol", + "node_modules/@openzeppelin/contracts/finance/VestingWallet.sol", + "node_modules/@openzeppelin/contracts/proxy/Proxy.sol", + "node_modules/@openzeppelin/contracts/token/ERC20/IERC20.sol", + ]; + mkdir_or_touch(tmp_dir.path(), &paths[..]); + let remappings = Remapping::find_many(&tmp_dir_node_modules); + + let mut paths = ProjectPathsConfig::hardhat(tmp_dir.path()).unwrap(); + paths.remappings = remappings; + + let resolved = paths + .resolve_library_import(Path::new("@openzeppelin/contracts/token/ERC20/IERC20.sol")) + .unwrap(); + assert!(resolved.exists()); + + // adjust remappings + paths.remappings[0].name = "@openzeppelin/contracts/".to_string(); + + let resolved = paths + .resolve_library_import(Path::new("@openzeppelin/contracts/token/ERC20/IERC20.sol")) + .unwrap(); + assert!(resolved.exists()); + } + #[test] fn recursive_remappings() { let tmp_dir = tempdir("lib").unwrap(); let tmp_dir_path = tmp_dir.path(); let paths = [ - "repo1/src/", "repo1/src/contract.sol", - "repo1/lib/", - "repo1/lib/ds-test/src/", "repo1/lib/ds-test/src/test.sol", - "repo1/lib/ds-math/src/", "repo1/lib/ds-math/src/contract.sol", - "repo1/lib/ds-math/lib/ds-test/src/", "repo1/lib/ds-math/lib/ds-test/src/test.sol", - "repo1/lib/guni-lev/src", "repo1/lib/guni-lev/src/contract.sol", - "repo1/lib/solmate/src/auth", "repo1/lib/solmate/src/auth/contract.sol", - "repo1/lib/solmate/src/tokens", "repo1/lib/solmate/src/tokens/contract.sol", - "repo1/lib/solmate/lib/ds-test/src/", "repo1/lib/solmate/lib/ds-test/src/test.sol", - "repo1/lib/solmate/lib/ds-test/demo/", "repo1/lib/solmate/lib/ds-test/demo/demo.sol", - "repo1/lib/openzeppelin-contracts/contracts/access", "repo1/lib/openzeppelin-contracts/contracts/access/AccessControl.sol", - "repo1/lib/ds-token/lib/ds-stop/src", "repo1/lib/ds-token/lib/ds-stop/src/contract.sol", - "repo1/lib/ds-token/lib/ds-stop/lib/ds-note/src", "repo1/lib/ds-token/lib/ds-stop/lib/ds-note/src/contract.sol", ]; mkdir_or_touch(tmp_dir_path, &paths[..]); @@ -692,12 +712,8 @@ mod tests { "ds-test/demo", "ds-test/demo/demo.sol", "ds-test/src/test.sol", - "openzeppelin/src", - "openzeppelin/src/interfaces", "openzeppelin/src/interfaces/c.sol", - "openzeppelin/src/token/ERC/", "openzeppelin/src/token/ERC/c.sol", - "standards/src/interfaces", "standards/src/interfaces/iweth.sol", "uniswapv2/src", ]; @@ -730,24 +746,17 @@ mod tests { let tmp_dir = tempdir("node_modules").unwrap(); let tmp_dir_node_modules = tmp_dir.path().join("node_modules"); let paths = [ - "node_modules/@aave/aave-token/contracts/token/", "node_modules/@aave/aave-token/contracts/token/AaveToken.sol", - "node_modules/@aave/governance-v2/contracts/governance/", "node_modules/@aave/governance-v2/contracts/governance/Executor.sol", "node_modules/@aave/protocol-v2/contracts/protocol/lendingpool/", "node_modules/@aave/protocol-v2/contracts/protocol/lendingpool/LendingPool.sol", - "node_modules/@ensdomains/ens/contracts/", "node_modules/@ensdomains/ens/contracts/contract.sol", "node_modules/prettier-plugin-solidity/tests/format/ModifierDefinitions/", "node_modules/prettier-plugin-solidity/tests/format/ModifierDefinitions/ ModifierDefinitions.sol", - "node_modules/@openzeppelin/contracts/tokens", "node_modules/@openzeppelin/contracts/tokens/contract.sol", - "node_modules/@openzeppelin/contracts/access", "node_modules/@openzeppelin/contracts/access/contract.sol", - "node_modules/eth-gas-reporter/mock/contracts", "node_modules/eth-gas-reporter/mock/contracts/ConvertLib.sol", - "node_modules/eth-gas-reporter/mock/test/", "node_modules/eth-gas-reporter/mock/test/TestMetacoin.sol", ]; mkdir_or_touch(tmp_dir.path(), &paths[..]); diff --git a/ethers-solc/src/utils.rs b/ethers-solc/src/utils.rs index fc1960b4..f6672a5f 100644 --- a/ethers-solc/src/utils.rs +++ b/ethers-solc/src/utils.rs @@ -2,10 +2,7 @@ use std::path::{Component, Path, PathBuf}; -use crate::{ - error::{self, SolcError}, - ProjectPathsConfig, SolcIoError, -}; +use crate::{error::SolcError, SolcIoError}; use once_cell::sync::Lazy; use regex::{Match, Regex}; use semver::Version; @@ -85,37 +82,6 @@ pub fn canonicalize(path: impl AsRef) -> Result { dunce::canonicalize(&path).map_err(|err| SolcIoError::new(err, path)) } -/// Try to resolve import to a local file or library path -pub fn resolve_import_component( - import: &Path, - node_dir: &Path, - paths: &ProjectPathsConfig, -) -> error::Result { - let component = match import.components().next() { - Some(inner) => inner, - None => { - return Err(SolcError::msg(format!( - "failed to resolve import at \"{:?}\"", - import.display() - ))) - } - }; - - if component == Component::CurDir || component == Component::ParentDir { - // if the import is relative we assume it's already part of the processed input file set - canonicalize(node_dir.join(import)).map_err(|err| err.into()) - } else { - // resolve library file - match paths.resolve_library_import(import.as_ref()) { - Some(lib) => Ok(lib), - None => Err(SolcError::msg(format!( - "failed to resolve library import \"{:?}\"", - import.display() - ))), - } - } -} - /// Returns the path to the library if the source path is in fact determined to be a library path, /// and it exists. /// Note: this does not handle relative imports or remappings.