Repository: gakonst/ethers-rs (rust)

Issue:
Compilation artifacts are altered by JSON deserialization - resulting in a mismatch with the bytecode's appended IPFS hash
**Version**
master branch, building from source

**Platform**
Darwin joaquims-mbp.lan 21.3.0 Darwin Kernel Version 21.3.0: Wed Jan  5 21:37:58 PST 2022; root:xnu-8019.80.24~20/RELEASE_ARM64_T6000 arm64

**Description**

When running `forge build --extra-output metadata`, with `bytecode_hash: "ipfs"`, I encountered an unexpected mismatch when comparing the metadata IPFS hash from the bytecode and the IPFS hash of the metadata outputted by forge.

I first thought it might be because of absolute vs relative paths, filed https://github.com/foundry-rs/foundry/issues/1738 which got resolved, but even with that fix the IPFS don't match.

I kept investigating and found the root of the issue: the raw solc compilation output, particularly the [metadata](https://docs.soliditylang.org/en/latest/metadata.html) is being deserialized by `ethers-rs` and then serialized again when writing the final artifacts to disk.

This causes multiple alterations to the original metadata output from solc. Here's the ones I've confirmed:

## 1. The abi in `metadata.output.abi` is getting all empty inputs stripped, and `indexed` is missing from all event properties.

The empty inputs being stripped is caused by the json deserialization policy:

```rust
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
pub struct SolcAbi {
  #[serde(default, skip_serializing_if = "Vec::is_empty")]
  pub inputs: Vec<Item>,
  ...
 ```
This can easily be verified by compiling a contract with an empty constructor with forge. The abi in the metadata will be missing the inputs property for the constructor, whereas the top level abi (the one that always get outputted by forge) will have it.

I believe that `SolcAbi` model is missing the `indexed` optional property as well, causing wrong event ABIs.

## 2. When the contract has no libraries, the `libraries: {}` entry in the metadata gets stripped.

Similarly, when deserializing the `settings` entry in the metadata, the JSON policy strips out any empty objects:

```rust
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
pub struct Settings {
  ...
   #[serde(default, skip_serializing_if = "Libraries::is_empty")]
   pub libraries: Libraries,
}
```
Because of that, the metadata output is altered from its original content.

## 3. The metadata output contains the key `outputSelection` which is not in the solidity metadata spec

This is due to the re-use of `pub struct Settings` for representing both solc inputs AND outputs. But in the spec, `outputSelection` is only an input field, and should not be in the finale metadata output.

## 4. Deserializing and serializing the compiler output shuffles the keys in the final output JSON

The compiler outputs metadata with keys in **alphabetical** order, but because of the deserialization and serialization process, those keys get shuffled, resulting in a different JSON than the one that was generated by the compiler.

---

These are the immediate issues I found while testing with a simple contract, I believe there might be more.

My recommendation would be to avoid deserializing / serializing the compiler metadata output, and instead just output the raw string that is produced by the compiler without altering it. This ensures that the produced bytecode IPFS hash matches the exact metadata contents that gets written in the final artifact files.

I believe that if end users want to do something with the content of the metadata, it is trivial for them to parse the raw string into JSON and do what they need to do. It should not be the responsibility of `ethers-rs` or `forge` to format that into JSON, especially since it breaks the guarantee that this metadata matches the bytecode.

For what it's worth, both hardhat and truffle do exactly this, in their build output there is a `metadata` entry that is just a raw, escaped JSON string, taken straight from the solc output. Without any alteration or formatting, like this:

```
{
  ... rest of the artifact output
  "metadata": "{\"compiler\":{\"version\":\"0.8.13+commit.abaa5c0e\"},\"language\":\"Solidity\",...}"
 }
```

Let me know if that makes sense, I'd be happy to try and do these changes in a PR.


Pull request:
feat(solc): emit additional raw metadata field
<!--
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
Closes #1325
<!--
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 an additional `rawMetadata:String` field to artifact output which preserves solc's metadata string
<!--
Summarize the solution and provide any necessary context needed to understand
the code change.
-->

## PR Checklist

- [ ] Added Tests
- [x] Added Documentation
- [x] Updated the changelog


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c0dab0e4..2d06ca4d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -99,6 +99,8 @@
 
 ### Unreleased
 
+- Add `rawMetadata:String` field to configurable contract output 
+  [#1365](https://github.com/gakonst/ethers-rs/pull/1365)
 - Use relative source paths and `solc --base-path`
   [#1317](https://github.com/gakonst/ethers-rs/pull/1317)
 - Save cache entry objects with relative paths
diff --git a/Cargo.lock b/Cargo.lock
index 7b1c0c03..bb3c5f10 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -3573,9 +3573,9 @@ dependencies = [
 
 [[package]]
 name = "solang-parser"
-version = "0.1.13"
+version = "0.1.14"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "395b6e1ec5af117bd08f963c7cd80f8efd4eed51c5a332aed42b13e3f9bc860b"
+checksum = "4a62cd1bd34217d4ac27aa4fad0e868983583a0c54cacc3a8590ea7029b50c2b"
 dependencies = [
  "itertools",
  "lalrpop",
diff --git a/ethers-core/src/types/block.rs b/ethers-core/src/types/block.rs
index c516318c..cffee94a 100644
--- a/ethers-core/src/types/block.rs
+++ b/ethers-core/src/types/block.rs
@@ -533,6 +533,26 @@ impl BlockNumber {
             _ => None,
         }
     }
+
+    /// Returns `true` if a numeric block number is set
+    pub fn is_number(&self) -> bool {
+        matches!(self, BlockNumber::Number(_))
+    }
+
+    /// Returns `true` if it's "latest"
+    pub fn is_latest(&self) -> bool {
+        matches!(self, BlockNumber::Latest)
+    }
+
+    /// Returns `true` if it's "pending"
+    pub fn is_pending(&self) -> bool {
+        matches!(self, BlockNumber::Pending)
+    }
+
+    /// Returns `true` if it's "earliest"
+    pub fn is_earliest(&self) -> bool {
+        matches!(self, BlockNumber::Earliest)
+    }
 }
 
 impl<T: Into<U64>> From<T> for BlockNumber {
@@ -590,6 +610,12 @@ impl fmt::Display for BlockNumber {
     }
 }
 
+impl Default for BlockNumber {
+    fn default() -> Self {
+        BlockNumber::Latest
+    }
+}
+
 #[cfg(test)]
 #[cfg(not(feature = "celo"))]
 mod tests {
diff --git a/ethers-solc/Cargo.toml b/ethers-solc/Cargo.toml
index 42455906..d4c0ff64 100644
--- a/ethers-solc/Cargo.toml
+++ b/ethers-solc/Cargo.toml
@@ -35,7 +35,7 @@ tempfile = { version = "3.3.0", optional = true }
 fs_extra = { version = "1.2.0", optional = true }
 sha2 = { version = "0.9.8", default-features = false, optional = true }
 dunce = "1.0.2"
-solang-parser = { default-features = false, version = "=0.1.13" }
+solang-parser = { default-features = false, version = "=0.1.14" }
 rayon = "1.5.3"
 rand = { version = "0.8.5", optional = true }
 path-slash = "0.1.4"
diff --git a/ethers-solc/src/artifact_output/configurable.rs b/ethers-solc/src/artifact_output/configurable.rs
index 28120d7a..a964b20e 100644
--- a/ethers-solc/src/artifact_output/configurable.rs
+++ b/ethers-solc/src/artifact_output/configurable.rs
@@ -9,7 +9,8 @@ use crate::{
             EwasmOutputSelection,
         },
         Ast, CompactContractBytecodeCow, DevDoc, Evm, Ewasm, FunctionDebugData, GasEstimates,
-        GeneratedSource, LosslessAbi, Metadata, Offsets, Settings, StorageLayout, UserDoc,
+        GeneratedSource, LosslessAbi, LosslessMetadata, Metadata, Offsets, Settings, StorageLayout,
+        UserDoc,
     },
     sources::VersionedSourceFile,
     ArtifactOutput, SolcConfig, SolcError, SourceFile,
@@ -41,6 +42,8 @@ pub struct ConfigurableContractArtifact {
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub gas_estimates: Option<GasEstimates>,
     #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub raw_metadata: Option<String>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
     pub metadata: Option<Metadata>,
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub storage_layout: Option<StorageLayout>,
@@ -232,6 +235,7 @@ impl ArtifactOutput for ConfigurableArtifacts {
     ) -> Self::Artifact {
         let mut artifact_userdoc = None;
         let mut artifact_devdoc = None;
+        let mut artifact_raw_metadata = None;
         let mut artifact_metadata = None;
         let mut artifact_ir = None;
         let mut artifact_ir_optimized = None;
@@ -258,7 +262,10 @@ impl ArtifactOutput for ConfigurableArtifacts {
         } = contract;
 
         if self.additional_values.metadata {
-            artifact_metadata = metadata;
+            if let Some(LosslessMetadata { raw_metadata, metadata }) = metadata {
+                artifact_raw_metadata = Some(raw_metadata);
+                artifact_metadata = Some(metadata);
+            }
         }
         if self.additional_values.userdoc {
             artifact_userdoc = Some(userdoc);
@@ -318,6 +325,7 @@ impl ArtifactOutput for ConfigurableArtifacts {
             function_debug_data: artifact_function_debug_data,
             method_identifiers: artifact_method_identifiers,
             gas_estimates: artifact_gas_estimates,
+            raw_metadata: artifact_raw_metadata,
             metadata: artifact_metadata,
             storage_layout: artifact_storage_layout,
             userdoc: artifact_userdoc,
@@ -564,7 +572,7 @@ impl ExtraOutputFiles {
         if self.metadata {
             if let Some(ref metadata) = contract.metadata {
                 let file = file.with_extension("metadata.json");
-                fs::write(&file, serde_json::to_string_pretty(metadata)?)
+                fs::write(&file, serde_json::to_string_pretty(&metadata.raw_json()?)?)
                     .map_err(|err| SolcError::io(err, file))?
             }
         }
diff --git a/ethers-solc/src/artifacts/contract.rs b/ethers-solc/src/artifacts/contract.rs
index 2779e63e..f3a33434 100644
--- a/ethers-solc/src/artifacts/contract.rs
+++ b/ethers-solc/src/artifacts/contract.rs
@@ -4,7 +4,8 @@ use crate::artifacts::{
     bytecode::{
         Bytecode, BytecodeObject, CompactBytecode, CompactDeployedBytecode, DeployedBytecode,
     },
-    serde_helpers, DevDoc, Evm, Ewasm, LosslessAbi, Metadata, Offsets, StorageLayout, UserDoc,
+    serde_helpers, DevDoc, Evm, Ewasm, LosslessAbi, LosslessMetadata, Offsets, StorageLayout,
+    UserDoc,
 };
 use ethers_core::{abi::Contract as Abi, types::Bytes};
 use serde::{Deserialize, Serialize};
@@ -22,7 +23,7 @@ pub struct Contract {
         skip_serializing_if = "Option::is_none",
         with = "serde_helpers::json_string_opt"
     )]
-    pub metadata: Option<Metadata>,
+    pub metadata: Option<LosslessMetadata>,
     #[serde(default)]
     pub userdoc: UserDoc,
     #[serde(default)]
diff --git a/ethers-solc/src/artifacts/mod.rs b/ethers-solc/src/artifacts/mod.rs
index acc9229e..7e436b0c 100644
--- a/ethers-solc/src/artifacts/mod.rs
+++ b/ethers-solc/src/artifacts/mod.rs
@@ -856,6 +856,61 @@ pub struct Metadata {
     pub version: i64,
 }
 
+/// A helper type that ensures lossless (de)serialisation so we can preserve the exact String
+/// metadata value that's being hashed by solc
+#[derive(Clone, Debug, PartialEq)]
+pub struct LosslessMetadata {
+    /// The complete abi as json value
+    pub raw_metadata: String,
+    /// The deserialised metadata of `raw_metadata`
+    pub metadata: Metadata,
+}
+
+// === impl LosslessMetadata ===
+
+impl LosslessMetadata {
+    /// Returns the whole string raw metadata as `serde_json::Value`
+    pub fn raw_json(&self) -> serde_json::Result<serde_json::Value> {
+        serde_json::from_str(&self.raw_metadata)
+    }
+}
+
+impl Serialize for LosslessMetadata {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        self.raw_metadata.serialize(serializer)
+    }
+}
+
+impl<'de> Deserialize<'de> for LosslessMetadata {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        struct LosslessMetadataVisitor;
+
+        impl<'de> Visitor<'de> for LosslessMetadataVisitor {
+            type Value = LosslessMetadata;
+
+            fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+                write!(formatter, "metadata string")
+            }
+
+            fn visit_str<E>(self, value: &str) -> Result<Self::Value, E>
+            where
+                E: serde::de::Error,
+            {
+                let raw_metadata = value.to_string();
+                let metadata = serde_json::from_str(value).map_err(serde::de::Error::custom)?;
+                Ok(LosslessMetadata { raw_metadata, metadata })
+            }
+        }
+        deserializer.deserialize_str(LosslessMetadataVisitor)
+    }
+}
+
 /// Compiler settings
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 pub struct MetadataSettings {
diff --git a/ethers-solc/src/artifacts/serde_helpers.rs b/ethers-solc/src/artifacts/serde_helpers.rs
index 7e664df8..d935e1a9 100644
--- a/ethers-solc/src/artifacts/serde_helpers.rs
+++ b/ethers-solc/src/artifacts/serde_helpers.rs
@@ -66,8 +66,8 @@ pub mod json_string_opt {
             if s.is_empty() {
                 return Ok(None)
             }
-
-            serde_json::from_str(&s).map_err(de::Error::custom).map(Some)
+            let value = serde_json::Value::String(s);
+            serde_json::from_value(value).map_err(de::Error::custom).map(Some)
         } else {
             Ok(None)
         }


Test patch:
diff --git a/ethers-solc/tests/project.rs b/ethers-solc/tests/project.rs
index 4600d09a..30309cc5 100644
--- a/ethers-solc/tests/project.rs
+++ b/ethers-solc/tests/project.rs
@@ -148,6 +148,7 @@ fn can_compile_configured() {
     let compiled = project.compile().unwrap();
     let artifact = compiled.find("Dapp").unwrap();
     assert!(artifact.metadata.is_some());
+    assert!(artifact.raw_metadata.is_some());
     assert!(artifact.ir.is_some());
     assert!(artifact.ir_optimized.is_some());
 }