Repository: gakonst/ethers-rs
Pull request #1625 (2022-08-20 16:01:47)

fix(solc): consider case sensitive conflicting artifact paths
<!--
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
Followup to https://github.com/gakonst/ethers-rs/pull/1621

As reported here https://github.com/foundry-rs/foundry/issues/2843#issuecomment-1221326854

there still can be conflicts for cases like `a.sol` and `inner/A.sol`, this was because we used  `PathBuf` as the key which is case sensitive.
<!--
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
* add new helper type `MappedArtifactFiles` that now uses lowercase(PathBuf.to_string()) as the key type so that we treat the output candidate `a.sol/A.json` and `A.sol/A.json` as the same file. (`a.sol/a.json`)
<!--
Summarize the solution and provide any necessary context needed to understand
the code change.
-->

## PR Checklist

- [x] Added Tests
- [x] Added Documentation
- [ ] Updated the changelog



Patch:
diff --git a/ethers-core/src/types/mod.rs b/ethers-core/src/types/mod.rs
index a5703e95..c03e9a7a 100644
--- a/ethers-core/src/types/mod.rs
+++ b/ethers-core/src/types/mod.rs
@@ -68,3 +68,6 @@ mod other;
 pub use other::OtherFields;
 
 pub mod serde_helpers;
+
+mod syncing;
+pub use syncing::{SyncProgress, SyncingStatus};
diff --git a/ethers-core/src/types/syncing.rs b/ethers-core/src/types/syncing.rs
new file mode 100644
index 00000000..58e685a6
--- /dev/null
+++ b/ethers-core/src/types/syncing.rs
@@ -0,0 +1,164 @@
+//! Types for `eth_syncing` RPC call
+
+use crate::types::U64;
+use serde::{Deserialize, Deserializer, Serialize, Serializer};
+
+/// Structure used in `eth_syncing` RPC
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub enum SyncingStatus {
+    /// When client is synced to highest block, eth_syncing with return string "false"
+    IsFalse,
+    /// When client is still syncing past blocks we get IsSyncing information.
+    IsSyncing(Box<SyncProgress>),
+}
+
+impl Serialize for SyncingStatus {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        match self {
+            SyncingStatus::IsFalse => serializer.serialize_bool(false),
+            SyncingStatus::IsSyncing(sync) => sync.serialize(serializer),
+        }
+    }
+}
+
+impl<'de> Deserialize<'de> for SyncingStatus {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        #[derive(Debug, Serialize, Deserialize)]
+        #[serde(untagged)]
+        pub enum SyncingStatusIntermediate {
+            /// When client is synced to the highest block, eth_syncing with return string "false"
+            IsFalse(bool),
+            /// When client is still syncing past blocks we get IsSyncing information.
+            IsSyncing(Box<SyncProgress>),
+        }
+
+        match SyncingStatusIntermediate::deserialize(deserializer)? {
+            SyncingStatusIntermediate::IsFalse(false) => Ok(SyncingStatus::IsFalse),
+            SyncingStatusIntermediate::IsFalse(true) => Err(serde::de::Error::custom(
+                "eth_syncing returned `true` that is undefined value.",
+            )),
+            SyncingStatusIntermediate::IsSyncing(sync) => Ok(SyncingStatus::IsSyncing(sync)),
+        }
+    }
+}
+
+/// Represents the sync status of the node
+///
+/// **Note:** while the `eth_syncing` RPC response is defined as:
+///
+/// > Returns:
+/// >
+/// > Object|Boolean, An object with sync status data or FALSE, when not syncing:
+///
+/// > startingBlock: QUANTITY - The block at which the import started (will only be reset, after the
+/// > sync reached his head)
+/// > currentBlock: QUANTITY - The current block, same as eth_blockNumber
+/// > highestBlock: QUANTITY - The estimated highest block
+///
+/// Geth returns additional fields: <https://github.com/ethereum/go-ethereum/blob/0ce494b60cd00d70f1f9f2dd0b9bfbd76204168a/ethclient/ethclient.go#L597-L617>
+#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct SyncProgress {
+    pub current_block: U64,
+    pub highest_block: U64,
+    pub starting_block: U64,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub pulled_states: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub known_states: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub healed_bytecode_bytes: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub healed_bytecodes: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub healed_trienode_bytes: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub healed_trienodes: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub healing_bytecode: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub healing_trienodes: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub synced_account_bytes: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub synced_accounts: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub synced_bytecode_bytes: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub synced_bytecodes: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub synced_storage: Option<U64>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub synced_storage_bytes: Option<U64>,
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // <https://github.com/gakonst/ethers-rs/issues/1623>
+    #[test]
+    fn deserialize_sync_geth() {
+        let s = r#"{
+        "currentBlock": "0xeaa2b4",
+        "healedBytecodeBytes": "0xaad91fe",
+        "healedBytecodes": "0x61d3",
+        "healedTrienodeBytes": "0x156ac02b1",
+        "healedTrienodes": "0x2885aa4",
+        "healingBytecode": "0x0",
+        "healingTrienodes": "0x454",
+        "highestBlock": "0xeaa329",
+        "startingBlock": "0xea97ee",
+        "syncedAccountBytes": "0xa29fec90d",
+        "syncedAccounts": "0xa7ed9ad",
+        "syncedBytecodeBytes": "0xdec39008",
+        "syncedBytecodes": "0x8d407",
+        "syncedStorage": "0x2a517da1",
+        "syncedStorageBytes": "0x23634dbedf"
+    }"#;
+
+        let sync: SyncingStatus = serde_json::from_str(s).unwrap();
+        match sync {
+            SyncingStatus::IsFalse => {
+                panic!("unexpected variant")
+            }
+            SyncingStatus::IsSyncing(_) => {}
+        }
+    }
+
+    #[test]
+    fn deserialize_sync_minimal() {
+        let s = r#"{
+        "currentBlock": "0xeaa2b4",
+        "highestBlock": "0xeaa329",
+        "startingBlock": "0xea97ee"
+    }"#;
+
+        let sync: SyncingStatus = serde_json::from_str(s).unwrap();
+        match sync {
+            SyncingStatus::IsFalse => {
+                panic!("unexpected variant")
+            }
+            SyncingStatus::IsSyncing(_) => {}
+        }
+    }
+
+    #[test]
+    fn deserialize_sync_false() {
+        let s = r#"false"#;
+
+        let sync: SyncingStatus = serde_json::from_str(s).unwrap();
+        match sync {
+            SyncingStatus::IsFalse => {}
+            SyncingStatus::IsSyncing(_) => {
+                panic!("unexpected variant")
+            }
+        }
+    }
+}
diff --git a/ethers-providers/src/lib.rs b/ethers-providers/src/lib.rs
index f5f58fa3..8a323db1 100644
--- a/ethers-providers/src/lib.rs
+++ b/ethers-providers/src/lib.rs
@@ -88,15 +88,6 @@ where
     }
 }
 
-/// Structure used in eth_syncing RPC
-#[derive(Clone, Debug)]
-pub enum SyncingStatus {
-    /// When client is synced to highest block, eth_syncing with return string "false"
-    IsFalse,
-    /// When client is still syncing past blocks we get IsSyncing information.
-    IsSyncing { starting_block: U256, current_block: U256, highest_block: U256 },
-}
-
 /// A middleware allows customizing requests send and received from an ethereum node.
 ///
 /// Writing a middleware is as simple as:
diff --git a/ethers-providers/src/provider.rs b/ethers-providers/src/provider.rs
index ed57facd..7d36041b 100644
--- a/ethers-providers/src/provider.rs
+++ b/ethers-providers/src/provider.rs
@@ -29,7 +29,7 @@ use ethers_core::{
     utils,
 };
 use hex::FromHex;
-use serde::{de::DeserializeOwned, Deserialize, Serialize};
+use serde::{de::DeserializeOwned, Serialize};
 use thiserror::Error;
 use url::{ParseError, Url};
 
@@ -539,26 +539,7 @@ impl<P: JsonRpcClient> Middleware for Provider<P> {
 
     /// Return current client syncing status. If IsFalse sync is over.
     async fn syncing(&self) -> Result<SyncingStatus, Self::Error> {
-        #[derive(Debug, Serialize, Deserialize)]
-        #[serde(untagged)]
-        pub enum SyncingStatusIntermediate {
-            /// When client is synced to highest block, eth_syncing with return string "false"
-            IsFalse(bool),
-            /// When client is still syncing past blocks we get IsSyncing information.
-            IsSyncing { starting_block: U256, current_block: U256, highest_block: U256 },
-        }
-        let intermediate: SyncingStatusIntermediate = self.request("eth_syncing", ()).await?;
-        match intermediate {
-            SyncingStatusIntermediate::IsFalse(false) => Ok(SyncingStatus::IsFalse),
-            SyncingStatusIntermediate::IsFalse(true) => Err(ProviderError::CustomError(
-                "eth_syncing returned `true` that is undefined value.".to_owned(),
-            )),
-            SyncingStatusIntermediate::IsSyncing {
-                starting_block,
-                current_block,
-                highest_block,
-            } => Ok(SyncingStatus::IsSyncing { starting_block, current_block, highest_block }),
-        }
+        self.request("eth_syncing", ()).await
     }
 
     /// Returns the network version.
diff --git a/ethers-providers/src/pubsub.rs b/ethers-providers/src/pubsub.rs
index 63339437..a31c0dcb 100644
--- a/ethers-providers/src/pubsub.rs
+++ b/ethers-providers/src/pubsub.rs
@@ -12,6 +12,7 @@ use std::{
     pin::Pin,
     task::{Context, Poll},
 };
+use tracing::error;
 
 /// A transport implementation supporting pub sub subscriptions.
 pub trait PubsubClient: JsonRpcClient {
@@ -90,7 +91,10 @@ where
         match futures_util::ready!(this.rx.poll_next(ctx)) {
             Some(item) => match serde_json::from_str(item.get()) {
                 Ok(res) => Poll::Ready(Some(res)),
-                _ => Poll::Pending,
+                Err(err) => {
+                    error!("failed to deserialize item {:?}", err);
+                    Poll::Pending
+                }
             },
             None => Poll::Ready(None),
         }
diff --git a/ethers-providers/src/transports/ws.rs b/ethers-providers/src/transports/ws.rs
index 098a2d87..e43d5ed8 100644
--- a/ethers-providers/src/transports/ws.rs
+++ b/ethers-providers/src/transports/ws.rs
@@ -22,6 +22,7 @@ use std::{
     },
 };
 use thiserror::Error;
+use tracing::trace;
 
 use super::common::{Params, Response};
 
@@ -317,6 +318,7 @@ where
     }
 
     async fn handle_text(&mut self, inner: String) -> Result<(), ClientError> {
+        trace!(msg=?inner, "received message");
         let (id, result) = match serde_json::from_str(&inner)? {
             Response::Success { id, result } => (id, Ok(result.to_owned())),
             Response::Error { id, error } => (id, Err(error)),
diff --git a/ethers-solc/src/artifact_output/files.rs b/ethers-solc/src/artifact_output/files.rs
new file mode 100644
index 00000000..12c88ad4
--- /dev/null
+++ b/ethers-solc/src/artifact_output/files.rs
@@ -0,0 +1,60 @@
+use crate::contracts::VersionedContract;
+use std::{
+    collections::HashMap,
+    hash::Hash,
+    ops::{Deref, DerefMut},
+    path::{Path, PathBuf},
+};
+
+/// Container type for all contracts mapped to their output file
+pub struct MappedArtifactFiles<'a> {
+    /// Represents the determined output artifact file and the contract(s) that target it
+    ///
+    /// This is guaranteed to be `len >= 1`
+    ///
+    /// If there is more than 1 contract in the map, this means we have a naming conflict where
+    /// different contracts target the same output file. This happens if the solidity file and
+    /// contract name match, but they are in different folders.
+    pub files: HashMap<MappedArtifactFile, Vec<MappedContract<'a>>>,
+}
+
+impl<'a> MappedArtifactFiles<'a> {
+    pub fn with_capacity(len: usize) -> Self {
+        Self { files: HashMap::with_capacity(len) }
+    }
+}
+
+impl<'a> Deref for MappedArtifactFiles<'a> {
+    type Target = HashMap<MappedArtifactFile, Vec<MappedContract<'a>>>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.files
+    }
+}
+
+impl<'a> DerefMut for MappedArtifactFiles<'a> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.files
+    }
+}
+
+/// Represents the targeted path of a contract or multiple contracts
+///
+/// To account for case-sensitivity we identify it via lowercase path
+#[derive(Debug, Hash, PartialEq, Eq)]
+pub struct MappedArtifactFile {
+    lower_case_path: String,
+}
+
+impl MappedArtifactFile {
+    pub fn new(path: &Path) -> Self {
+        Self { lower_case_path: path.to_string_lossy().to_lowercase() }
+    }
+}
+
+pub struct MappedContract<'a> {
+    pub file: &'a str,
+    pub name: &'a str,
+    pub contract: &'a VersionedContract,
+    pub artifact_path: PathBuf,
+}
diff --git a/ethers-solc/src/artifact_output/mod.rs b/ethers-solc/src/artifact_output/mod.rs
index a33c6095..37528dc1 100644
--- a/ethers-solc/src/artifact_output/mod.rs
+++ b/ethers-solc/src/artifact_output/mod.rs
@@ -17,16 +17,20 @@ use semver::Version;
 use serde::{de::DeserializeOwned, Serialize};
 use std::{
     borrow::Cow,
-    collections::{btree_map::BTreeMap, HashMap, HashSet},
+    collections::{btree_map::BTreeMap, HashSet},
     ffi::OsString,
-    fmt, fs, io,
+    fmt, fs,
+    hash::Hash,
+    io,
     ops::Deref,
     path::{Path, PathBuf},
 };
 use tracing::{error, trace};
 
 mod configurable;
-use crate::contracts::VersionedContract;
+pub(crate) mod files;
+
+use crate::files::MappedContract;
 pub use configurable::*;
 
 /// Represents unique artifact metadata for identifying artifacts on output
@@ -120,11 +124,6 @@ impl<T> ArtifactFile<T> {
     }
 }
 
-/// Internal helper type alias that maps all files for the contracts
-/// `output -> [(file, name, contract)]`
-pub(crate) type ArtifactFiles<'a> =
-    HashMap<PathBuf, Vec<(&'a str, &'a str, &'a VersionedContract)>>;
-
 /// local helper type alias `file name -> (contract name  -> Vec<..>)`
 pub(crate) type ArtifactsMap<T> = FileToContractsMap<Vec<ArtifactFile<T>>>;
 
@@ -808,8 +807,9 @@ pub trait ArtifactOutput {
         // converting stand-alone artifacts in the next step
         let mut final_artifact_paths = HashSet::new();
 
-        for (artifact_path, contracts) in artifact_files {
-            for (idx, (file, name, contract)) in contracts.iter().enumerate() {
+        for contracts in artifact_files.files.into_values() {
+            for (idx, mapped_contract) in contracts.iter().enumerate() {
+                let MappedContract { file, name, contract, artifact_path } = mapped_contract;
                 // track `SourceFile`s that can be mapped to contracts
                 let source_file = sources.find_file_and_version(file, &contract.version);
 
@@ -824,11 +824,11 @@ pub trait ArtifactOutput {
                     // contracts need to adjust the paths properly
 
                     // we keep the top most conflicting file unchanged
-                    let is_top_most = contracts.iter().enumerate().filter(|(i, _)| *i != idx).all(
-                        |(_, (f, _, _))| {
-                            Path::new(file).components().count() < Path::new(f).components().count()
-                        },
-                    );
+                    let is_top_most =
+                        contracts.iter().enumerate().filter(|(i, _)| *i != idx).all(|(_, c)| {
+                            Path::new(file).components().count() <
+                                Path::new(c.file).components().count()
+                        });
                     if !is_top_most {
                         // we resolve the conflicting by finding a new unique, alternative path
                         artifact_path = Self::conflict_free_output_file(
diff --git a/ethers-solc/src/compile/output/contracts.rs b/ethers-solc/src/compile/output/contracts.rs
index 48a8453d..58787f8a 100644
--- a/ethers-solc/src/compile/output/contracts.rs
+++ b/ethers-solc/src/compile/output/contracts.rs
@@ -3,7 +3,8 @@ use crate::{
         contract::{CompactContractRef, Contract},
         FileToContractsMap,
     },
-    ArtifactFiles, ArtifactOutput, OutputContext,
+    files::{MappedArtifactFile, MappedArtifactFiles, MappedContract},
+    ArtifactOutput, OutputContext,
 };
 use semver::Version;
 use serde::{Deserialize, Serialize};
@@ -47,8 +48,8 @@ impl VersionedContracts {
     pub(crate) fn artifact_files<T: ArtifactOutput + ?Sized>(
         &self,
         ctx: &OutputContext,
-    ) -> ArtifactFiles {
-        let mut output_files = ArtifactFiles::with_capacity(self.len());
+    ) -> MappedArtifactFiles {
+        let mut output_files = MappedArtifactFiles::with_capacity(self.len());
         for (file, contracts) in self.iter() {
             for (name, versioned_contracts) in contracts {
                 for contract in versioned_contracts {
@@ -56,7 +57,7 @@ impl VersionedContracts {
                     // we reuse the path, this will make sure that even if there are conflicting
                     // files (files for witch `T::output_file()` would return the same path) we use
                     // consistent output paths
-                    let output = if let Some(existing_artifact) =
+                    let artifact_path = if let Some(existing_artifact) =
                         ctx.existing_artifact(file, name, &contract.version).cloned()
                     {
                         trace!("use existing artifact file {:?}", existing_artifact,);
@@ -69,12 +70,18 @@ impl VersionedContracts {
 
                     trace!(
                         "use artifact file {:?} for contract file {} {}",
-                        output,
+                        artifact_path,
                         file,
                         contract.version
                     );
-                    let contract = (file.as_str(), name.as_str(), contract);
-                    output_files.entry(output).or_default().push(contract);
+                    let artifact = MappedArtifactFile::new(&artifact_path);
+                    let contract = MappedContract {
+                        file: file.as_str(),
+                        name: name.as_str(),
+                        contract,
+                        artifact_path,
+                    };
+                    output_files.entry(artifact).or_default().push(contract);
                 }
             }
         }


Test patch:
diff --git a/ethers-solc/tests/project.rs b/ethers-solc/tests/project.rs
index f5a77edb..41fcb6f6 100644
--- a/ethers-solc/tests/project.rs
+++ b/ethers-solc/tests/project.rs
@@ -2226,6 +2226,103 @@ fn can_handle_conflicting_files_recompile() {
     assert!(inner_recompiled.get_abi().unwrap().functions.contains_key("baz"));
 }
 
+// <https://github.com/foundry-rs/foundry/issues/2843>
+#[test]
+fn can_handle_conflicting_files_case_sensitive_recompile() {
+    let project = TempProject::<ConfigurableArtifacts>::dapptools().unwrap();
+
+    project
+        .add_source(
+            "a",
+            r#"
+    pragma solidity ^0.8.10;
+
+    contract A {
+            function foo() public{}
+    }
+   "#,
+        )
+        .unwrap();
+
+    project
+        .add_source(
+            "inner/A",
+            r#"
+    pragma solidity ^0.8.10;
+
+    contract A {
+            function bar() public{}
+    }
+   "#,
+        )
+        .unwrap();
+
+    let compiled = project.compile().unwrap();
+    assert!(!compiled.has_compiler_errors());
+
+    let artifacts = compiled.artifacts().count();
+    assert_eq!(artifacts, 2);
+
+    // nothing to compile
+    let compiled = project.compile().unwrap();
+    assert!(compiled.is_unchanged());
+    let artifacts = compiled.artifacts().count();
+    assert_eq!(artifacts, 2);
+
+    let cache = SolFilesCache::read(project.cache_path()).unwrap();
+
+    let mut source_files = cache.files.keys().cloned().collect::<Vec<_>>();
+    source_files.sort_unstable();
+
+    assert_eq!(source_files, vec![PathBuf::from("src/a.sol"), PathBuf::from("src/inner/A.sol"),]);
+
+    let mut artifacts =
+        project.artifacts_snapshot().unwrap().artifacts.into_stripped_file_prefixes(project.root());
+    artifacts.strip_prefix_all(&project.paths().artifacts);
+
+    assert_eq!(artifacts.len(), 2);
+    let mut artifact_files = artifacts.artifact_files().map(|f| f.file.clone()).collect::<Vec<_>>();
+    artifact_files.sort_unstable();
+
+    let expected_files = vec![PathBuf::from("a.sol/A.json"), PathBuf::from("inner/A.sol/A.json")];
+    assert_eq!(artifact_files, expected_files);
+
+    // overwrite conflicting nested file, effectively changing it
+    project
+        .add_source(
+            "inner/A",
+            r#"
+    pragma solidity ^0.8.10;
+    contract A {
+    function bar() public{}
+    function baz() public{}
+    }
+   "#,
+        )
+        .unwrap();
+
+    let compiled = project.compile().unwrap();
+    assert!(!compiled.has_compiler_errors());
+
+    let mut recompiled_artifacts =
+        project.artifacts_snapshot().unwrap().artifacts.into_stripped_file_prefixes(project.root());
+    recompiled_artifacts.strip_prefix_all(&project.paths().artifacts);
+
+    assert_eq!(recompiled_artifacts.len(), 2);
+    let mut artifact_files =
+        recompiled_artifacts.artifact_files().map(|f| f.file.clone()).collect::<Vec<_>>();
+    artifact_files.sort_unstable();
+    assert_eq!(artifact_files, expected_files);
+
+    // ensure that `a.sol/A.json` is unchanged
+    let outer = artifacts.find("src/a.sol", "A").unwrap();
+    let outer_recompiled = recompiled_artifacts.find("src/a.sol", "A").unwrap();
+    assert_eq!(outer, outer_recompiled);
+
+    let inner_recompiled = recompiled_artifacts.find("src/inner/A.sol", "A").unwrap();
+    assert!(inner_recompiled.get_abi().unwrap().functions.contains_key("baz"));
+}
+
 #[test]
 fn can_checkout_repo() {
     let project = TempProject::checkout("transmissions11/solmate").unwrap();