Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Commit

Permalink
fix(solc): duplicate contracts segments (#832)
Browse files Browse the repository at this point in the history 
* chore: simplify touch

* forge install: ds-test

* fix: fix duplicate contracts segments

* fix: typos
  • Loading branch information
@mattsse
mattsse authored Jan 27, 2022
1 parent 80e1b4f commit 5da2eb1
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 65 deletions.
65 changes: 60 additions & 5 deletions ethers-solc/src/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
59 changes: 34 additions & 25 deletions ethers-solc/src/remappings.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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[..]);
Expand Down Expand Up @@ -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",
];
Expand Down Expand Up @@ -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[..]);
Expand Down
36 changes: 1 addition & 35 deletions ethers-solc/src/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit 5da2eb1

Please sign in to comment.