foundry-rs / compilers

Utilities for working with native solc and compiling projects.
Apache License 2.0
59 stars 36 forks source link

fix: create valid Standard JSON to verify for projects with symlinks #35

Closed tash-2s closed 6 months ago

tash-2s commented 6 months ago

Currently, when a project includes symbolic links to Solidity files, Project::standard_json_input returns a Standard JSON Input struct that is unusable for contract verification on block explorers. solc cannot compile these contracts based on the json, leading to verification failures. This pull request addresses this issue.

I encountered this problem when using the forge verify-contract command, which internally uses this function. While forge build completes successfully, the verification command fails, indicating that the block explorer cannot find a source file.

This problem occurs in projects that contain symlinked Solidity files and when these files are imported from other files. My project, structured as a monorepo, uses external libraries installed via pnpm. These libraries, accessible from the ./node_modules/ directory, are set up with remappings in my Foundry project. However, directories under ./node_modules/ are symlinks pointing to other locations, leading to this issue.

Reproduction

I added a test named can_create_standard_json_input_with_symlink to demonstrate the issue within this repository. Also, the error can be reproduced using the forge verify-contract command and steps below:

Environment: macOS (Apple silicon), forge 0.2.0 (c312c0d 2023-12-22T00:20:29.297186000Z)

$ mkdir repro && cd repro

$ mkdir -p project/src project/node_modules dependency

$ cat << EOF > project/src/A.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.23;
import "@dependency/B.sol";
contract A is B {}
EOF

$ cat << EOF > dependency/B.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.23;
import "./C.sol";
contract B is C {}
EOF

$ cat << EOF > dependency/C.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.23;
contract C {}
EOF

$ cat << EOF > project/foundry.toml
[profile.default]
remappings = ["@dependency/=node_modules/dependency/"]
allow_paths = ["../dependency/"]
EOF

# Create a symbolic link
$ cd project/node_modules
$ ln -s ../../dependency dependency
$ cd ../..

# Display the file structure
$ tree
.
├── dependency
│   ├── B.sol
│   └── C.sol
└── project
    ├── foundry.toml
    ├── node_modules
    │   └── dependency -> ../../dependency
    └── src
        └── A.sol

# `build` succeeds
$ cd project
$ forge build

# `verify-contract` generates an unintended json (C.sol has a host absolute path name)
$ forge verify-contract --show-standard-json-input 0x0000000000000000000000000000000000000000 A | jq . > A.json
$ cat A.json
{
  "language": "Solidity",
  "sources": {
    "src/A.sol": {
      "content": "// SPDX-License-Identifier: UNLICENSED\npragma solidity ^0.8.23;\nimport \"@dependency/B.sol\";\ncontract A is B {}\n"
    },
    "node_modules/dependency/B.sol": {
      "content": "// SPDX-License-Identifier: UNLICENSED\npragma solidity ^0.8.23;\nimport \"./C.sol\";\ncontract B is C {}\n"
    },
    "/Users/t/bench/repro/dependency/C.sol": {
      "content": "// SPDX-License-Identifier: UNLICENSED\npragma solidity ^0.8.23;\ncontract C {}\n"
    }
  },
  "settings": {
    "remappings": [
      "@dependency/=node_modules/dependency/",
      "dependency/=node_modules/dependency/"
    ],
    "optimizer": {
      "enabled": true,
      "runs": 200
    },
    "metadata": {
      "useLiteralContent": false,
      "bytecodeHash": "ipfs",
      "appendCBOR": true
    },
    "outputSelection": {
      "*": {
        "": [
          "ast"
        ],
        "*": [
          "abi",
          "evm.bytecode",
          "evm.deployedBytecode",
          "evm.methodIdentifiers",
          "metadata"
        ]
      }
    },
    "evmVersion": "paris",
    "libraries": {}
  }
}

# `solc` cannot compile using the json
$ solc --standard-json --no-import-callback A.json | jq .errors
[
  {
    "component": "general",
    "errorCode": "6275",
    "formattedMessage": "ParserError: Source \"node_modules/dependency/C.sol\" not found: No import callback.\n --> node_modules/dependency/B.sol:3:1:\n  |\n3 | import \"./C.sol\";\n  | ^^^^^^^^^^^^^^^^^\n\n",
    "message": "Source \"node_modules/dependency/C.sol\" not found: No import callback.",
    "severity": "error",
    "sourceLocation": {
      "end": 81,
      "file": "node_modules/dependency/B.sol",
      "start": 64
    },
    "type": "ParserError"
  }
]

Manually editing the host filesystem's absolute path as shown below allows the solc command to succeed.

   "node_modules/dependency/B.sol": {
     "content": "// SPDX-License-Identifier: UNLICENSED\npragma solidity ^0.8.23;\nimport \"./C.sol\";\ncontract B is C {}\n"
   },
-  "/Users/t/bench/repro/dependency/C.sol": {
+  "node_modules/dependency/C.sol": {
     "content": "// SPDX-License-Identifier: UNLICENSED\npragma solidity ^0.8.23;\ncontract C {}\n"
   }
 },

Cause

The issue arises because import paths in Solidity files are processed by the std::fs::canonicalize function. This function resolves symlinks and normalizes paths. When symlinks are resolved, it results in solc being unable to locate the corresponding source paths in the json, as it relies on Solidity import statements. Therefore, symlinks should not be resolved here. The paths should be maintained as specified in the Solidity files, except for basic normalization.

Solution

To address this, I implemented an import path normalization function and replaced the canonicalization function where necessary. Based on the Solidity documentation page, this function resolves . and .. segments/components without resolving symlinks.

The Standard JSON's source paths, for verification purposes, should be based on the project root. This allows the compiler to find sources within the json. The conversion of normalized paths to project root-based paths occurs after all sources are processed. That conversion is already implemented, so this PR doesn't need to address it. (It seems like the path conversion needs improvement, but it is a separate issue and should be handled in another PR.)

https://github.com/foundry-rs/compilers/blob/b1561d807c246c066c7c4b0c72c8eb64c696a43d/src/lib.rs#L514-L525

With the changes proposed in this PR, I confirmed the fix of the issue by building forge with this patch and testing both the reproduction case and my project. The resulting json matches the manually edited json diff mentioned earlier.

A potential downside of this approach is that the same file could be represented differently in the json if accessed via both symlinked and real paths. However, I see no issues with this, and it aligns with the behavior of the compiler's virtual filesystem.

tash-2s commented 6 months ago

Thank you for your review! I've added the doc to the function.

tash-2s commented 6 months ago

@mattsse Thank you for taking the time to review this pull request.

I've created a companion PR in foundry with this patch. Also, I have confirmed that the tests in foundry passed successfully.

tash-2s commented 6 months ago

I've addressed these comments.

tash-2s commented 6 months ago

Appreciate your review on the PR. 🙏