Skip to content

Use artifact_id instead of file_id#7536

Closed
wtdcode wants to merge 1 commit into
foundry-rs:masterfrom
wtdcode:use-artifact-id
Closed

Use artifact_id instead of file_id#7536
wtdcode wants to merge 1 commit into
foundry-rs:masterfrom
wtdcode:use-artifact-id

Conversation

@wtdcode

@wtdcode wtdcode commented Apr 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

There is a design flaw with ContractSources, that always assumes 1 file_id only has 1 artifact. However, It can often happen like:

interface A {}
contract T {
    A a ...
}

In this case, 1 file_id would have two artifacts. As a result, the sourcemap of T would be overwritten by A if T is iterated earlier in this loop:

foundry/crates/common/src/compile.rs

Lines 296 to 319 in bd56eef

pub fn from_project_output(
output: &ProjectCompileOutput,
root: &Path,
) -> Result<ContractSources> {
let mut sources = ContractSources::default();
for (id, artifact) in output.artifact_ids() {
if let Some(file_id) = artifact.id {
let abs_path = root.join(&id.source);
let source_code = std::fs::read_to_string(abs_path).wrap_err_with(|| {
format!("failed to read artifact source file for `{}`", id.identifier())
})?;
let compact = CompactContractBytecode {
abi: artifact.abi.clone(),
bytecode: artifact.bytecode.clone(),
deployed_bytecode: artifact.deployed_bytecode.clone(),
};
let contract = compact_to_contract(compact)?;
sources.insert(&id, file_id, source_code, contract);
} else {
warn!(id = id.identifier(), "source not found");
}
}
Ok(sources)
}

This currently breaks my forge test --debug by showing no source map from contract xxxx.

Solution

This PR fixes it by using artifact_id instead of file_id. I carefully checked the API exposed by ContractSources, and I think currently no other component seems to be using it so it should be safe to make this change. I'm not very sure if this is the best solution but I'm open to discussion.

@mattsse mattsse requested a review from klkvr April 1, 2024 16:38
@onbjerg onbjerg added the T-debt Type: code debt label Apr 1, 2024
@klkvr

klkvr commented Apr 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Nice catch! This logic def should be revisited, I'm also not sure about the contract names values we are operating on here

I believe the current impl is incorrect as we are dependent on file_id matching source_element.index from source maps:

foundry/crates/debugger/src/tui/draw.rs

Lines 361 to 365 in 0578aae

source_element
.index
.and_then(|index|
// if index matches current file_id, return current source code
(index == file_id).then(|| (source_element.clone(), source_code)))

and this would not be true for internal indexes this PR introduces

@klkvr

klkvr commented Apr 1, 2024

Copy link
Copy Markdown
Member

This should probably be solvable by having

pub struct ContractSources {
    /// Map over artifacts' contract names -> vector of file IDs
    pub ids_by_name: HashMap<String, Vec<u32>>,
    /// Map over file_id -> source code
    pub sources_by_id: FxHashMap<u32, String>,
    /// Map over file_id -> contract name -> bytecode
    pub artifacts_by_id: FxHashMap<u32, HashMap<String, ContractBytecodeSome>>,
}

@klkvr klkvr mentioned this pull request Apr 2, 2024
Merged
@klkvr

klkvr commented Apr 2, 2024

Copy link
Copy Markdown
Member

Closing in favor of #7550

@klkvr klkvr closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@Evalir Evalir Awaiting requested review from Evalir

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

@klkvr klkvr Awaiting requested review from klkvr

Assignees

@klkvr klkvr

Labels

T-debt Type: code debt

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wtdcode @klkvr @onbjerg