-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: create valid Standard JSON to verify for projects with symlinks #35
Conversation
fs::metadata(&normalized).map(|_| normalized).map_err(|err| SolcIoError::new(err, original)) | ||
} | ||
|
||
fn clean_solidity_path(original_path: impl AsRef<Path>) -> PathBuf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust doesn't have this normalization function (resolves .
and ..
but doesn't resolve symlinks), so I wrote the function myself. The function is simple, but I found several crates that perform similar tasks, so I can switch to one of them if preferred.
e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha—I'll defer to @mattsse here, but the function is so small I don't mind keeping it here instead of pulling in an additional dep, as we also have tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is awesome, thank you so much!
I haven't reviewed this deeply yet, just some nits:
fs::metadata(&normalized).map(|_| normalized).map_err(|err| SolcIoError::new(err, original)) | ||
} | ||
|
||
fn clean_solidity_path(original_path: impl AsRef<Path>) -> PathBuf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add some docs here? I know it's an internal function, but having docs is nice and useful to revisit this later whenever needed
fs::metadata(&normalized).map(|_| normalized).map_err(|err| SolcIoError::new(err, original)) | ||
} | ||
|
||
fn clean_solidity_path(original_path: impl AsRef<Path>) -> PathBuf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha—I'll defer to @mattsse here, but the function is so small I don't mind keeping it here instead of pulling in an additional dep, as we also have tests.
Thank you for your review! I've added the doc to the function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is awesome, ty!
symlinks man...
before we merge this, could you please open a foundry PR with the foundry-compilers crate patched to this PR to see if there are any sideffects?
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few nits
src/utils.rs
Outdated
use path_slash::PathExt; | ||
let normalized = PathBuf::from(dunce::simplified(&cleaned).to_slash_lossy().as_ref()); | ||
|
||
fs::metadata(&normalized).map(|_| normalized).map_err(|err| SolcIoError::new(err, original)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this call for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function normalize_solidity_import_path
checks if the returned path exists in the file system. The use of fs::metadata(&normalized)
is to obtain an io::Error
for non-existing files without reading their entire contents. To return the same type of error as utils::canonicalize
, an io::Error
is necessary for SolcIoError
.
While there is a specific function, Path::is_file()
, that confirms if a path exists in the file system, it only returns a bool.
https://doc.rust-lang.org/std/path/struct.Path.html#method.is_file
This is a convenience function that coerces errors to false. If you want to check errors, call
fs::metadata
and handle itsResult
.
However, I have now realized that there's an alias Path::metadata()
for paths. Therefore, I will switch to using this function and add a comment.
https://doc.rust-lang.org/std/path/struct.Path.html#method.metadata
src/utils.rs
Outdated
Component::ParentDir => match new_path.last() { | ||
Some(Component::Normal(..)) => { | ||
new_path.pop(); | ||
} | ||
_ => { | ||
new_path.push(component); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use if let Some(Component::Normal) = new_path.last() {pop} else {...}
here
which is a bit nicer imo
I've addressed these comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, tysm for these fixes! 🙏
Appreciate your review on the PR. 🙏 |
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. Whileforge 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 theforge verify-contract
command and steps below:Environment: macOS (Apple silicon), forge 0.2.0 (
c312c0d
2023-12-22T00:20:29.297186000Z)Manually editing the host filesystem's absolute path as shown below allows the
solc
command to succeed.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 insolc
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.)
compilers/src/lib.rs
Lines 514 to 525 in b1561d8
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.