Repository: gakonst/ethers-rs
Pull request #1853 (2022-11-10 10:13:12)

fix: improve overloaded param diff matching
<!--
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
Fixes a bug discovered in foundry CI where params of overloaded functions are not matched correctly.

<!--
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
Keep track of all params already identified as diff when comparing two overloaded functions.
This prevents false positive where (`string`, `string`, `string`) compared to (`string`, `string[]`) resulted in no diffs because we used `iter::all()` as a comparator.
<!--
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/Cargo.lock b/Cargo.lock
index 494df0f8..df7334c4 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1685,7 +1685,6 @@ checksum = "3eb42d4fb72227be5778429f9ef5240a38a358925a49f05b5cf702ce7c7e558a"
 dependencies = [
  "futures-channel",
  "futures-task",
- "tokio",
 ]
 
 [[package]]
diff --git a/ethers-contract/ethers-contract-abigen/src/contract/methods.rs b/ethers-contract/ethers-contract-abigen/src/contract/methods.rs
index cfda0b00..57e16d19 100644
--- a/ethers-contract/ethers-contract-abigen/src/contract/methods.rs
+++ b/ethers-contract/ethers-contract-abigen/src/contract/methods.rs
@@ -1,4 +1,4 @@
-use std::collections::{btree_map::Entry, BTreeMap, HashMap};
+use std::collections::{btree_map::Entry, BTreeMap, HashMap, HashSet};
 
 use super::{types, util, Context};
 use crate::{
@@ -534,11 +534,21 @@ impl Context {
             }
             // compare each overloaded function with the `first_fun`
             for (idx, overloaded_fun) in functions.into_iter().skip(1) {
+                // keep track of matched params
+                let mut already_matched_param_diff = HashSet::new();
                 // attempt to find diff in the input arguments
                 let mut diff = Vec::new();
                 let mut same_params = true;
                 for (idx, i1) in overloaded_fun.inputs.iter().enumerate() {
-                    if first_fun.inputs.iter().all(|i2| i1 != i2) {
+                    // Find the first param that differs and hasn't already been matched as diff
+                    if let Some((pos, _)) = first_fun
+                        .inputs
+                        .iter()
+                        .enumerate()
+                        .filter(|(pos, _)| !already_matched_param_diff.contains(pos))
+                        .find(|(_, i2)| i1 != *i2)
+                    {
+                        already_matched_param_diff.insert(pos);
                         diff.push(i1);
                         same_params = false;
                     } else {
diff --git a/ethers-core/src/types/transaction/response.rs b/ethers-core/src/types/transaction/response.rs
index 5ee768b4..314caf0b 100644
--- a/ethers-core/src/types/transaction/response.rs
+++ b/ethers-core/src/types/transaction/response.rs
@@ -279,8 +279,6 @@ impl Transaction {
         #[cfg(feature = "celo")]
         self.decode_celo_metadata(rlp, offset)?;
 
-        self.gas = rlp.val_at(*offset)?;
-        *offset += 1;
         self.to = Some(rlp.val_at(*offset)?);
         *offset += 1;
         self.value = rlp.val_at(*offset)?;
@@ -1069,6 +1067,30 @@ mod tests {
         .expect("unable to decode raw tx");
         decoded_tx.recover_from_mut().unwrap();
         decoded_tx.hash = decoded_tx.hash();
-        dbg!(&decoded_tx);
+        assert_eq!(
+            H256::from_str("0xeae304417079580c334ccc07e3933a906699461802a17b722034a8191c4a38ea")
+                .unwrap(),
+            decoded_tx.hash
+        );
+    }
+
+    #[test]
+    fn test_rlp_decoding_issue_1848_first() {
+        // slot 5097934, tx index 40, hash
+        // 0xf98c9f1a2f30ee316ea1db18c132ccab6383b8e4933ccf6259ca9d1f27d4a364
+        let s = "01f9012e01826c6f850737be7600830493ef940c3de458b51a11da7d4616f42f66c861e3859d3e80b8c4f5b22c2a000000000000000000000000e67b950f4b84c5b06ee36ded6727a17443fe749300000000000000000000000000000000000000000000005f344f4a335cc50000000000000000000000000000000000000000000005c2f00b834b7f0000000000000000000000000000000000000000000000000005aa64a95b4a40400000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000c3de458b51a11da7d4616f42f66c861e3859d3ec080a0c4023f0b8f7daecd7e143ef7aaa9b67bd059e643a6f2ae509a0e8483a3966e28a065a20662274cb5f7fe60a2af7dbd466244154440e73243f00b6a69bd08eacda4";
+        let b = hex::decode(s).unwrap();
+        let r = rlp::Rlp::new(b.as_slice());
+        Transaction::decode(&r).unwrap();
+    }
+
+    #[test]
+    fn test_rlp_decoding_issue_1848_second() {
+        // slot 5097936, tx index 0, hash
+        // 6d38fc8aee934858815ed41273cece3b676c368e9c6e39f172313a0685e1f175
+        let s = "01f8ee0182034c853d9f1b88158307a120940087bb802d9c0e343f00510000729031ce00bf2780b8841e1326a300000000000000000000000088e6a0c2ddd26feeb64f039a2c41296fcb3f56400000000000000000000000000000000000000000000000000000001d3b3e730000000000000000000000000000000000000000000000000596b93e53696740000000000000000000000000000000000000000000000000000000000000000001c001a0bbfd754ed51b34d0a8577f69b4c42ce6b47fee6ecf49114bb135e7e8eadbb336a0433692134eb7e7686e9aefafa9f69c601aa977c00cc85c827782f5fb1f1cff0f";
+        let b = hex::decode(s).unwrap();
+        let r = rlp::Rlp::new(b.as_slice());
+        Transaction::decode(&r).unwrap();
     }
 }
diff --git a/ethers-middleware/Cargo.toml b/ethers-middleware/Cargo.toml
index 0eb9990b..7a64a1ed 100644
--- a/ethers-middleware/Cargo.toml
+++ b/ethers-middleware/Cargo.toml
@@ -26,7 +26,7 @@ auto_impl = { version = "0.5.0", default-features = false }
 serde = { version = "1.0.124", default-features = false, features = ["derive"] }
 thiserror = { version = "1.0", default-features = false }
 futures-util = { version = "^0.3" }
-futures-locks = { version = "0.7" }
+futures-locks = { version = "0.7", default-features = false }
 tracing = { version = "0.1.37", default-features = false }
 tracing-futures = { version = "0.2.5", default-features = false }
 
diff --git a/ethers-solc/src/artifacts/mod.rs b/ethers-solc/src/artifacts/mod.rs
index 4e5b7fe4..e3dd5961 100644
--- a/ethers-solc/src/artifacts/mod.rs
+++ b/ethers-solc/src/artifacts/mod.rs
@@ -1733,8 +1733,9 @@ impl fmt::Display for Error {
     }
 }
 
-#[derive(Clone, Debug, Eq, PartialEq, Hash)]
+#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd, Default)]
 pub enum Severity {
+    #[default]
     Error,
     Warning,
     Info,
diff --git a/ethers-solc/src/compile/output/mod.rs b/ethers-solc/src/compile/output/mod.rs
index 294c4e48..1a73c794 100644
--- a/ethers-solc/src/compile/output/mod.rs
+++ b/ethers-solc/src/compile/output/mod.rs
@@ -3,7 +3,7 @@
 use crate::{
     artifacts::{
         contract::{CompactContractBytecode, CompactContractRef, Contract},
-        Error,
+        Error, Severity,
     },
     buildinfo::RawBuildInfo,
     info::ContractInfoRef,
@@ -31,6 +31,8 @@ pub struct ProjectCompileOutput<T: ArtifactOutput = ConfigurableArtifacts> {
     pub(crate) cached_artifacts: Artifacts<T::Artifact>,
     /// errors that should be omitted
     pub(crate) ignored_error_codes: Vec<u64>,
+    /// set minimum level of severity that is treated as an error
+    pub(crate) compiler_severity_filter: Severity,
 }
 
 impl<T: ArtifactOutput> ProjectCompileOutput<T> {
@@ -197,7 +199,7 @@ impl<T: ArtifactOutput> ProjectCompileOutput<T> {
 
     /// Whether there were errors
     pub fn has_compiler_errors(&self) -> bool {
-        self.compiler_output.has_error()
+        self.compiler_output.has_error(&self.ignored_error_codes, &self.compiler_severity_filter)
     }
 
     /// Whether there were warnings
@@ -398,7 +400,7 @@ impl<T: ArtifactOutput> fmt::Display for ProjectCompileOutput<T> {
         if self.compiler_output.is_unchanged() {
             f.write_str("Nothing to compile")
         } else {
-            self.compiler_output.diagnostics(&self.ignored_error_codes).fmt(f)
+            self.compiler_output.diagnostics(&self.ignored_error_codes, self.compiler_severity_filter.clone()).fmt(f)
         }
     }
 }
@@ -426,8 +428,16 @@ impl AggregatedCompilerOutput {
     }
 
     /// Whether the output contains a compiler error
-    pub fn has_error(&self) -> bool {
-        self.errors.iter().any(|err| err.severity.is_error())
+    pub fn has_error(&self, ignored_error_codes: &[u64], compiler_severity_filter: &Severity) -> bool {
+        self.errors.iter().any(|err| {
+            if compiler_severity_filter.ge(&err.severity) {
+                if compiler_severity_filter.is_warning()  {
+                    return self.has_warning(ignored_error_codes)
+                }
+                return true
+            }
+            return false
+        })
     }
 
     /// Whether the output contains a compiler warning
@@ -441,8 +451,8 @@ impl AggregatedCompilerOutput {
         })
     }
 
-    pub fn diagnostics<'a>(&'a self, ignored_error_codes: &'a [u64]) -> OutputDiagnostics {
-        OutputDiagnostics { compiler_output: self, ignored_error_codes }
+    pub fn diagnostics<'a>(&'a self, ignored_error_codes: &'a [u64], compiler_severity_filter: Severity) -> OutputDiagnostics {
+        OutputDiagnostics { compiler_output: self, ignored_error_codes, compiler_severity_filter }
     }
 
     pub fn is_empty(&self) -> bool {
@@ -702,12 +712,14 @@ pub struct OutputDiagnostics<'a> {
     compiler_output: &'a AggregatedCompilerOutput,
     /// the error codes to ignore
     ignored_error_codes: &'a [u64],
+    /// set minimum level of severity that is treated as an error
+    compiler_severity_filter: Severity,
 }
 
 impl<'a> OutputDiagnostics<'a> {
     /// Returns true if there is at least one error of high severity
     pub fn has_error(&self) -> bool {
-        self.compiler_output.has_error()
+        self.compiler_output.has_error(&self.ignored_error_codes, &self.compiler_severity_filter)
     }
 
     /// Returns true if there is at least one warning
diff --git a/ethers-solc/src/compile/project.rs b/ethers-solc/src/compile/project.rs
index 5bcf376b..3994da3b 100644
--- a/ethers-solc/src/compile/project.rs
+++ b/ethers-solc/src/compile/project.rs
@@ -311,7 +311,7 @@ impl<'a, T: ArtifactOutput> CompiledState<'a, T> {
                 ctx,
                 &project.paths,
             )
-        } else if output.has_error() {
+        } else if output.has_error(&project.ignored_error_codes, &project.compiler_severity_filter) {
             trace!("skip writing cache file due to solc errors: {:?}", output.errors);
             project.artifacts_handler().output_to_artifacts(
                 &output.contracts,
@@ -358,14 +358,17 @@ impl<'a, T: ArtifactOutput> ArtifactsState<'a, T> {
     fn write_cache(self) -> Result<ProjectCompileOutput<T>> {
         trace!("write cache");
         let ArtifactsState { output, cache, compiled_artifacts } = self;
-        let ignored_error_codes = cache.project().ignored_error_codes.clone();
-        let skip_write_to_disk = cache.project().no_artifacts || output.has_error();
+        let project = cache.project();
+        let ignored_error_codes = project.ignored_error_codes.clone();
+        let compiler_severity_filter = project.compiler_severity_filter.clone();
+        let skip_write_to_disk = project.no_artifacts || output.has_error(&ignored_error_codes, &compiler_severity_filter);
         let cached_artifacts = cache.consume(&compiled_artifacts, !skip_write_to_disk)?;
         Ok(ProjectCompileOutput {
             compiler_output: output,
             compiled_artifacts,
             cached_artifacts,
             ignored_error_codes,
+            compiler_severity_filter
         })
     }
 }
diff --git a/ethers-solc/src/lib.rs b/ethers-solc/src/lib.rs
index 94805ca6..9d2947f7 100644
--- a/ethers-solc/src/lib.rs
+++ b/ethers-solc/src/lib.rs
@@ -42,7 +42,7 @@ use crate::{
     error::{SolcError, SolcIoError},
     sources::{VersionedSourceFile, VersionedSourceFiles},
 };
-use artifacts::contract::Contract;
+use artifacts::{contract::Contract, Severity};
 use compile::output::contracts::VersionedContracts;
 use error::Result;
 use semver::Version;
@@ -73,6 +73,8 @@ pub struct Project<T: ArtifactOutput = ConfigurableArtifacts> {
     pub artifacts: T,
     /// Errors/Warnings which match these error codes are not going to be logged
     pub ignored_error_codes: Vec<u64>,
+    /// The minimum severity level that is treated as a compiler error
+    pub compiler_severity_filter: Severity,
     /// The paths which will be allowed for library inclusion
     pub allowed_paths: AllowedLibPaths,
     /// The paths which will be used with solc's `--include-path` attribute
@@ -569,6 +571,8 @@ pub struct ProjectBuilder<T: ArtifactOutput = ConfigurableArtifacts> {
     artifacts: T,
     /// Which error codes to ignore
     pub ignored_error_codes: Vec<u64>,
+    /// The minimum severity level that is treated as a compiler error
+    compiler_severity_filter: Severity,
     /// All allowed paths for solc's `--allowed-paths`
     allowed_paths: AllowedLibPaths,
     /// Paths to use for solc's `--include-path`
@@ -591,6 +595,7 @@ impl<T: ArtifactOutput> ProjectBuilder<T> {
             slash_paths: true,
             artifacts,
             ignored_error_codes: Vec::new(),
+            compiler_severity_filter: Severity::Error,
             allowed_paths: Default::default(),
             include_paths: Default::default(),
             solc_jobs: None,
@@ -629,6 +634,12 @@ impl<T: ArtifactOutput> ProjectBuilder<T> {
         self
     }
 
+    #[must_use]
+    pub fn set_compiler_severity_filter(mut self, compiler_severity_filter: Severity) -> Self {
+        self.compiler_severity_filter = compiler_severity_filter;
+        self
+    }
+
     /// Disables cached builds
     #[must_use]
     pub fn ephemeral(self) -> Self {
@@ -727,6 +738,7 @@ impl<T: ArtifactOutput> ProjectBuilder<T> {
             no_artifacts,
             auto_detect,
             ignored_error_codes,
+            compiler_severity_filter,
             allowed_paths,
             include_paths,
             solc_jobs,
@@ -746,6 +758,7 @@ impl<T: ArtifactOutput> ProjectBuilder<T> {
             slash_paths,
             artifacts,
             ignored_error_codes,
+            compiler_severity_filter,
             allowed_paths,
             include_paths,
             solc_jobs,
@@ -803,6 +816,7 @@ impl<T: ArtifactOutput> ProjectBuilder<T> {
             auto_detect,
             artifacts,
             ignored_error_codes,
+            compiler_severity_filter,
             mut allowed_paths,
             include_paths,
             solc_jobs,
@@ -834,6 +848,7 @@ impl<T: ArtifactOutput> ProjectBuilder<T> {
             auto_detect,
             artifacts,
             ignored_error_codes,
+            compiler_severity_filter,
             allowed_paths,
             include_paths,
             solc_jobs: solc_jobs.unwrap_or_else(num_cpus::get),
diff --git a/ethers-solc/src/utils.rs b/ethers-solc/src/utils.rs
index b39053ce..71c10c37 100644
--- a/ethers-solc/src/utils.rs
+++ b/ethers-solc/src/utils.rs
@@ -43,7 +43,7 @@ pub static RE_THREE_OR_MORE_NEWLINES: Lazy<Regex> = Lazy::new(|| Regex::new("\n{
 
 /// Create a regex that matches any library or contract name inside a file
 pub fn create_contract_or_lib_name_regex(name: &str) -> Regex {
-    Regex::new(&format!(r#"(?:using\s+(?P<n1>{name})\s+|is\s+(?:\w+\s*,\s*)*(?P<n2>{name})(?:\s*,\s*\w+)*|(?:(?P<ignore>(?:function|error|as)\s+|\n[^\n]*(?:"([^"\n]|\\")*|'([^'\n]|\\')*))|\W+)(?P<n3>{name})name.|\(| ))"#)).unwrap()
+    Regex::new(&format!(r#"(?:using\s+(?P<n1>{name})\s+|is\s+(?:\w+\s*,\s*)*(?P<n2>{name})(?:\s*,\s*\w+)*|(?:(?P<ignore>(?:function|error|as)\s+|\n[^\n]*(?:"([^"\n]|\\")*|'([^'\n]|\\')*))|\W+)(?P<n3>{name})(?:\.|\(| ))"#)).unwrap()
 }
 
 /// Move a range by a specified offset


Test patch:
diff --git a/ethers-contract/tests/it/abigen.rs b/ethers-contract/tests/it/abigen.rs
index f38f9538..41706d01 100644
--- a/ethers-contract/tests/it/abigen.rs
+++ b/ethers-contract/tests/it/abigen.rs
@@ -737,3 +737,16 @@ fn can_generate_event_with_structs() {
     assert_eq!("MyEvent((uint256,uint256),uint256)", MyEventFilter::abi_signature());
     assert_event::<MyEventFilter>();
 }
+
+#[test]
+fn can_handle_overloaded_function_with_array() {
+    abigen!(
+        Test,
+        r#"[
+         serializeString(string calldata, string calldata, string calldata) external returns (string memory)
+         serializeString(string calldata, string calldata, string[] calldata) external returns (string memory)
+         serializeBool(string calldata, string calldata, bool) external returns (string memory)
+         serializeBool(string calldata, string calldata, bool[] calldata) external returns (string memory)
+    ]"#,
+    );
+}
diff --git a/ethers-solc/test-data/test-contract-warnings/LicenseWarning.sol b/ethers-solc/test-data/test-contract-warnings/LicenseWarning.sol
new file mode 100644
index 00000000..aeaa7a51
--- /dev/null
+++ b/ethers-solc/test-data/test-contract-warnings/LicenseWarning.sol
@@ -0,0 +1,3 @@
+pragma solidity 0.8.6;
+
+contract LicenseWarning {}
diff --git a/ethers-solc/tests/project.rs b/ethers-solc/tests/project.rs
index 1814610f..6f99e982 100644
--- a/ethers-solc/tests/project.rs
+++ b/ethers-solc/tests/project.rs
@@ -1618,6 +1618,81 @@ fn can_compile_model_checker_sample() {
     assert!(compiled.has_compiler_warnings());
 }
 
+#[test]
+fn test_compiler_severity_filter() {
+    fn gen_test_data_warning_path() -> ProjectPathsConfig {
+        let root =
+            PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("test-data/test-contract-warnings");
+        let paths = ProjectPathsConfig::builder().sources(root).build().unwrap();
+        paths
+    }
+
+    let project = Project::builder()
+        .no_artifacts()
+        .paths(gen_test_data_warning_path())
+        .ephemeral()
+        .build()
+        .unwrap();
+    let compiled = project.compile().unwrap();
+    assert!(compiled.has_compiler_warnings());
+    assert!(!compiled.has_compiler_errors());
+
+    let project = Project::builder()
+        .no_artifacts()
+        .paths(gen_test_data_warning_path())
+        .ephemeral()
+        .set_compiler_severity_filter(ethers_solc::artifacts::Severity::Warning)
+        .build()
+        .unwrap();
+    let compiled = project.compile().unwrap();
+    assert!(compiled.has_compiler_warnings());
+    assert!(compiled.has_compiler_errors());
+}
+
+#[test]
+fn test_compiler_severity_filter_and_ignored_error_codes() {
+    fn gen_test_data_licensing_warning() -> ProjectPathsConfig {
+        let root =
+            PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("test-data/test-contract-warnings/LicenseWarning.sol");
+        let paths = ProjectPathsConfig::builder().sources(root).build().unwrap();
+        paths
+    }
+
+    let missing_license_error_code = 1878;
+
+    let project = Project::builder()
+    .no_artifacts()
+    .paths(gen_test_data_licensing_warning())
+    .ephemeral()
+    .build()
+    .unwrap();
+    let compiled = project.compile().unwrap();
+    assert!(compiled.has_compiler_warnings());
+
+    let project = Project::builder()
+        .no_artifacts()
+        .paths(gen_test_data_licensing_warning())
+        .ephemeral()
+        .ignore_error_code(missing_license_error_code)
+        .build()
+        .unwrap();
+    let compiled = project.compile().unwrap();
+    assert!(!compiled.has_compiler_warnings());
+    assert!(!compiled.has_compiler_errors());
+
+    let project = Project::builder()
+    .no_artifacts()
+    .paths(gen_test_data_licensing_warning())
+    .ephemeral()
+    .ignore_error_code(missing_license_error_code)
+    .set_compiler_severity_filter(ethers_solc::artifacts::Severity::Warning)
+    .build()
+    .unwrap();
+    let compiled = project.compile().unwrap();
+    assert!(!compiled.has_compiler_warnings());
+    assert!(!compiled.has_compiler_errors());
+}
+
 fn remove_solc_if_exists(version: &Version) {
     if Solc::find_svm_installed_version(version.to_string()).unwrap().is_some() {
         svm::remove_version(version).expect("failed to remove version")