Skip to content
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.

Sign up for GitHub

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 verifiable Standard JSON for projects with external files #36

Merged
merged 5 commits into from
Dec 28, 2023
Merged

fix: create verifiable Standard JSON for projects with external files #36

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1d48253
test: add failing test for Standard JSON Input creation
tash-2s Dec 25, 2023
e0e1bfb
fix: create valid Standard JSON to verify for projects w/ external files
tash-2s Dec 25, 2023
5c781d0
refactor: extract path rebasing process from standard json fuction
tash-2s Dec 25, 2023
38eff70
refactor: more efficient way without VecDeque
tash-2s Dec 28, 2023
bf564fb
docs: improve `rebase_path` comment
tash-2s Dec 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 77 additions & 10 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,8 +490,6 @@ impl<T: ArtifactOutput> Project<T> {
&self,
target: impl AsRef<Path>,
) -> Result<StandardJsonCompilerInput> {
use path_slash::PathExt;

let target = target.as_ref();
tracing::trace!("Building standard-json-input for {:?}", target);
let graph = Graph::resolve(&self.paths)?;
Expand All @@ -514,14 +512,7 @@ impl<T: ArtifactOutput> Project<T> {
let root = self.root();
let sources = sources
.into_iter()
.map(|(path, source)| {
let path: PathBuf = if let Ok(stripped) = path.strip_prefix(root) {
stripped.to_slash_lossy().into_owned().into()
} else {
path.to_slash_lossy().into_owned().into()
};
(path, source.clone())
})
.map(|(path, source)| (rebase_path(root, path), source.clone()))
.collect();

let mut settings = self.solc_config.settings.clone();
Expand Down Expand Up @@ -954,6 +945,43 @@ impl<T: ArtifactOutput> ArtifactOutput for Project<T> {
}
}

// Rebases the given path to the base directory lexically.
//
// The returned path from this function usually starts either with a normal component (e.g., `src`)
// or a parent directory component (i.e., `..`), which is based on the base directory. Additionally,
// this function converts the path into a UTF-8 string and replaces all separators with forward
// slashes (`/`).
//
// The rebasing process is as follows:
//
// 1. Remove the leading components from the path that match the base components.
// 2. Prepend `..` components to the path, equal in number to the remaining base components.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add an example here that illustrates this, otherwise this is a bit hard to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the comment, adding examples to make it easier to understand.

fn rebase_path(base: impl AsRef<Path>, path: impl AsRef<Path>) -> PathBuf {
use path_slash::PathExt;

let base = base.as_ref();
let path = path.as_ref();

let mut new_path = path.components().collect::<std::collections::VecDeque<_>>();

for (i, (base_component, path_component)) in
base.components().zip(path.components()).enumerate()
{
if base_component == path_component {
new_path.pop_front();
} else {
let mut parent_dirs =
vec![std::path::Component::ParentDir; base.components().count() - i];
parent_dirs.extend(new_path);
new_path = parent_dirs.into();

break;
}
}
Copy link
Member

@mattsse mattsse Dec 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can solve this a bit differently
don't zip the two iterators but create the two iters separately and then iterate over the path
and continue as long as base and path components match, once they diverge collect the remaining into a PathBuf

this way we don't need the vecdeque

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure I understand your suggestion.

If we compare the components of base and path at the same positions, it seems similar to using zip.

Could you please elaborate on the type of implementation you have in mind?

Copy link
Member

@mattsse mattsse Dec 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like

let mut components = Vec::new(); // perhaps we can even directly push components to a PathBuf?
let mut base_iter = base.components();
let mut path_iter = path.components();

while let Some(path) = path_iter.next() {
  if Some(path) != base_iter.next() {
     components.push(path);
     components.extend(path_iter);
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your explanation. I now understand your point. I've made the change and pushed it.


new_path.iter().collect::<PathBuf>().to_slash_lossy().into_owned().into()
}

#[cfg(test)]
#[cfg(all(feature = "svm-solc", not(target_arch = "wasm32")))]
mod tests {
Expand Down Expand Up @@ -1014,4 +1042,43 @@ mod tests {
let contracts = project.compile().unwrap().succeeded().output().contracts;
assert_eq!(contracts.contracts().count(), 2);
}

#[test]
fn can_rebase_path() {
assert_eq!(rebase_path("a/b", "a/b/c"), PathBuf::from("c"));
assert_eq!(rebase_path("a/b", "a/c"), PathBuf::from("../c"));
assert_eq!(rebase_path("a/b", "c"), PathBuf::from("../../c"));

assert_eq!(
rebase_path("/home/user/project", "/home/user/project/A.sol"),
PathBuf::from("A.sol")
);
assert_eq!(
rebase_path("/home/user/project", "/home/user/project/src/A.sol"),
PathBuf::from("src/A.sol")
);
assert_eq!(
rebase_path("/home/user/project", "/home/user/project/lib/forge-std/src/Test.sol"),
PathBuf::from("lib/forge-std/src/Test.sol")
);
assert_eq!(
rebase_path("/home/user/project", "/home/user/A.sol"),
PathBuf::from("../A.sol")
);
assert_eq!(rebase_path("/home/user/project", "/home/A.sol"), PathBuf::from("../../A.sol"));
assert_eq!(rebase_path("/home/user/project", "/A.sol"), PathBuf::from("../../../A.sol"));
assert_eq!(
rebase_path("/home/user/project", "/tmp/A.sol"),
PathBuf::from("../../../tmp/A.sol")
);

assert_eq!(
rebase_path("/Users/ah/temp/verif", "/Users/ah/temp/remapped/Child.sol"),
PathBuf::from("../remapped/Child.sol")
);
assert_eq!(
rebase_path("/Users/ah/temp/verif", "/Users/ah/temp/verif/../remapped/Parent.sol"),
PathBuf::from("../remapped/Parent.sol")
);
}
}
67 changes: 66 additions & 1 deletion tests/project.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use foundry_compilers::{
info::ContractInfo,
project_util::*,
remappings::Remapping,
Artifact, CompilerInput, ConfigurableArtifacts, ExtraOutputValues, Graph, Project,
utils, Artifact, CompilerInput, ConfigurableArtifacts, ExtraOutputValues, Graph, Project,
ProjectCompileOutput, ProjectPathsConfig, Solc, TestFileFilter,
};
use pretty_assertions::assert_eq;
Expand Down Expand Up @@ -1600,6 +1600,71 @@ fn can_sanitize_bytecode_hash() {
assert!(compiled.find_first("A").is_some());
}

// https://github.com/foundry-rs/foundry/issues/5307
#[test]
fn can_create_standard_json_input_with_external_file() {
// File structure:
// .
// ├── verif
// │   └── src
// │   └── Counter.sol
// └── remapped
// ├── Child.sol
// └── Parent.sol

let dir = tempfile::tempdir().unwrap();
let verif_dir = utils::canonicalize(dir.path()).unwrap().join("verif");
let remapped_dir = utils::canonicalize(dir.path()).unwrap().join("remapped");
Comment on lines +1616 to +1617
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After my previous PR is merged, this canonicalization can be removed. It is necessary on this branch because the symlink-resolving canonicalization is still in effect, and the temporary directory on macOS is a symlink.

fs::create_dir_all(verif_dir.join("src")).unwrap();
fs::create_dir(&remapped_dir).unwrap();

let mut verif_project = Project::builder()
.paths(ProjectPathsConfig::dapptools(&verif_dir).unwrap())
.build()
.unwrap();

verif_project.paths.remappings.push(Remapping {
context: None,
name: "@remapped/".into(),
path: "../remapped/".into(),
});
verif_project.allowed_paths.insert(remapped_dir.clone());

fs::write(remapped_dir.join("Parent.sol"), "pragma solidity >=0.8.0; import './Child.sol';")
.unwrap();
fs::write(remapped_dir.join("Child.sol"), "pragma solidity >=0.8.0;").unwrap();
fs::write(
verif_dir.join("src/Counter.sol"),
"pragma solidity >=0.8.0; import '@remapped/Parent.sol'; contract Counter {}",
)
.unwrap();

// solc compiles using the host file system; therefore, this setup is considered valid
let compiled = verif_project.compile().unwrap();
compiled.assert_success();

// can create project root based paths
let std_json = verif_project.standard_json_input(verif_dir.join("src/Counter.sol")).unwrap();
assert_eq!(
std_json.sources.iter().map(|(path, _)| path.clone()).collect::<Vec<_>>(),
vec![
PathBuf::from("src/Counter.sol"),
PathBuf::from("../remapped/Parent.sol"),
PathBuf::from("../remapped/Child.sol")
]
);

// can compile using the created json
let compiler_errors = Solc::default()
.compile(&std_json)
.unwrap()
.errors
.into_iter()
.filter_map(|e| if e.severity.is_error() { Some(e.message) } else { None })
.collect::<Vec<_>>();
assert!(compiler_errors.is_empty(), "{:?}", compiler_errors);
}

#[test]
fn can_compile_std_json_input() {
let tmp = TempProject::dapptools_init().unwrap();
Expand Down
Loading