fix(solc): duplicate contracts segments

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
+    /// <root>/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 `<remapping
+    /// name>/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<PathBuf> {
         // 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"
+                // (`<name>/=.../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)

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[..]);

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<Path>) -> Result<PathBuf, SolcIoError> {
     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<PathBuf> {
-    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.