Repository: gakonst/ethers-rs
Pull request #995 (2022-03-07 21:07:09)

fix(solc): flatten nodes only once
<!--
Thank you for your Pull Request. Please provide a description above and review
the requirements below.

Bug fixes and new features should include tests.

Contributors guide: https://github.com/gakonst/ethers-rs/blob/master/CONTRIBUTING.md

The contributors guide includes instructions for running rustfmt and building the
documentation.
-->

## Motivation
https://github.com/gakonst/foundry/issues/811
<!--
Explain the context and why you're making that change. What is the problem
you're trying to solve? In some cases there is not a problem and this can be
thought of as being the motivation for your change.
-->

## Solution
Moved the insert above the recursive function, which should guarantee that we include a node only once.

Which would prevent `A import B imports A` which would start recursive `flatten_node` at `B` but `A` is not yet included in the imports set so it will be included again.

CC @rkrasiuk 

<!--
Summarize the solution and provide any necessary context needed to understand
the code change.
-->

## PR Checklist

- [x] Added Tests
- [ ] Added Documentation
- [ ] Updated the changelog



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1fa750c8..c44561a0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -47,6 +47,8 @@
 - Add a getter to `ProjectCompileOutput` that returns a mapping of compiler
   versions to a vector of name + contract struct tuples
   [#908](https://github.com/gakonst/ethers-rs/pull/908)
+- Enforce commutativity of ENS reverse resolution
+  [#996](https://github.com/gakonst/ethers-rs/pull/996)
 
 ## ethers-contract-abigen
 
diff --git a/ethers-providers/src/provider.rs b/ethers-providers/src/provider.rs
index c5192ced..dee01796 100644
--- a/ethers-providers/src/provider.rs
+++ b/ethers-providers/src/provider.rs
@@ -112,6 +112,10 @@ pub enum ProviderError {
     #[error("ens name not found: {0}")]
     EnsError(String),
 
+    /// Invalid reverse ENS name
+    #[error("reverse ens name not pointing to itself: {0}")]
+    EnsNotOwned(String),
+
     #[error(transparent)]
     SerdeJson(#[from] serde_json::Error),
 
@@ -794,7 +798,14 @@ impl<P: JsonRpcClient> Middleware for Provider<P> {
     /// a string. This should theoretically never happen.
     async fn lookup_address(&self, address: Address) -> Result<String, ProviderError> {
         let ens_name = ens::reverse_address(address);
-        self.query_resolver(ParamType::String, &ens_name, ens::NAME_SELECTOR).await
+        let domain: String =
+            self.query_resolver(ParamType::String, &ens_name, ens::NAME_SELECTOR).await?;
+        let reverse_address = self.resolve_name(&domain).await?;
+        if address != reverse_address {
+            Err(ProviderError::EnsNotOwned(domain))
+        } else {
+            Ok(domain)
+        }
     }
 
     /// Returns the avatar HTTP link of the avatar that the `ens_name` resolves to (or None
diff --git a/ethers-solc/src/config.rs b/ethers-solc/src/config.rs
index bff8bdea..a2689d0e 100644
--- a/ethers-solc/src/config.rs
+++ b/ethers-solc/src/config.rs
@@ -10,6 +10,7 @@ use crate::{
 use crate::artifacts::output_selection::ContractOutputSelection;
 use serde::{Deserialize, Serialize};
 use std::{
+    collections::HashSet,
     fmt::{self, Formatter},
     fs,
     path::{Component, Path, PathBuf},
@@ -229,7 +230,7 @@ impl ProjectPathsConfig {
     pub fn flatten(&self, target: &Path) -> Result<String> {
         tracing::trace!("flattening file");
         let graph = Graph::resolve(self)?;
-        self.flatten_node(target, &graph, &mut vec![], false, false)
+        self.flatten_node(target, &graph, &mut Default::default(), false, false)
     }
 
     /// Flattens a single node from the dependency graph
@@ -237,7 +238,7 @@ impl ProjectPathsConfig {
         &self,
         target: &Path,
         graph: &Graph,
-        imported: &mut Vec<usize>,
+        imported: &mut HashSet<usize>,
         strip_version_pragma: bool,
         strip_license: bool,
     ) -> Result<String> {
@@ -248,9 +249,11 @@ impl ProjectPathsConfig {
             SolcError::msg(format!("cannot resolve file at \"{:?}\"", target.display()))
         })?;
 
-        if imported.iter().any(|&idx| idx == *target_index) {
+        if imported.contains(target_index) {
+            // short circuit nodes that were already imported, if both A.sol and B.sol import C.sol
             return Ok(String::new())
         }
+        imported.insert(*target_index);
 
         let target_node = graph.node(*target_index);
 
@@ -278,21 +281,17 @@ impl ProjectPathsConfig {
 
         for import in imports.iter() {
             let import_path = self.resolve_import(target_dir, import.data())?;
-            let import_content = self
-                .flatten_node(&import_path, graph, imported, true, true)?
-                .trim()
-                .as_bytes()
-                .to_owned();
+            let s = self.flatten_node(&import_path, graph, imported, true, true)?;
+            let import_content = s.trim().as_bytes();
             let import_content_len = import_content.len() as isize;
             let (start, end) = import.loc_by_offset(offset);
-            content.splice(start..end, import_content);
+            content.splice(start..end, import_content.iter().copied());
             offset += import_content_len - ((end - start) as isize);
         }
 
         let result = String::from_utf8(content).map_err(|err| {
             SolcError::msg(format!("failed to convert extended bytes to string: {}", err))
         })?;
-        imported.push(*target_index);
 
         Ok(result)
     }


Test patch:
diff --git a/ethers-solc/tests/project.rs b/ethers-solc/tests/project.rs
index 4777c041..2cdbe6d3 100644
--- a/ethers-solc/tests/project.rs
+++ b/ethers-solc/tests/project.rs
@@ -417,6 +417,57 @@ fn can_flatten_file_in_dapp_sample() {
     assert!(result.contains("contract DappTest"));
 }
 
+#[test]
+fn can_flatten_unique() {
+    let project = TempProject::dapptools().unwrap();
+
+    let f = project
+        .add_source(
+            "A",
+            r#"
+pragma solidity ^0.8.10;
+import "./C.sol";
+import "./B.sol";
+contract A { }
+"#,
+        )
+        .unwrap();
+
+    project
+        .add_source(
+            "B",
+            r#"
+pragma solidity ^0.8.10;
+import "./C.sol";
+contract B { }
+"#,
+        )
+        .unwrap();
+
+    project
+        .add_source(
+            "C",
+            r#"
+pragma solidity ^0.8.10;
+import "./A.sol";
+contract C { }
+"#,
+        )
+        .unwrap();
+
+    let result = project.flatten(&f).unwrap();
+
+    assert_eq!(
+        result,
+        r#"
+pragma solidity ^0.8.10;
+contract C { }
+contract B { }
+contract A { }
+"#
+    );
+}
+
 #[test]
 fn can_flatten_file_with_duplicates() {
     let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("test-data/test-flatten-duplicates");