Skip to content

feat: use cached artifacts if solc config is almost the same#87

Merged
mattsse merged 2 commits intofoundry-rs:mainfrom
klkvr:klkvr/solc-config-cache
Mar 7, 2024
Merged

feat: use cached artifacts if solc config is almost the same#87
mattsse merged 2 commits intofoundry-rs:mainfrom
klkvr:klkvr/solc-config-cache

Conversation

@klkvr
Copy link
Member

@klkvr klkvr commented Mar 7, 2024

If the only difference between cached vs current solc config is outputSelection which is a subset of the cached output selection, then it's safe to use cached artifact.

I am currently adding logic to foundry to request only abi output and it would be nice if we could use cached abi where possible.

Impl is pretty ugly, but not sure how to make it better without cloning

@klkvr klkvr requested review from DaniPopes and Evalir as code owners March 7, 2024 17:07
@klkvr klkvr requested a review from mattsse March 7, 2024 17:08
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this makes a lot of sense,

smol sanity test then this is gtg

Comment on lines +123 to +133
/// Returns true if this output selection is a subset of the other output selection.
pub fn is_subset_of(&self, other: &Self) -> bool {
self.0.iter().all(|(file, selection)| {
other.0.get(file).map_or(false, |other_selection| {
selection.iter().all(|(contract, outputs)| {
other_selection.get(contract).map_or(false, |other_outputs| {
outputs.iter().all(|output| other_outputs.contains(output))
})
})
})
})
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 a sanity test for this?

looks correct though

@klkvr klkvr requested a review from mattsse March 7, 2024 17:30
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm!!

@mattsse mattsse merged commit 938e417 into foundry-rs:main Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants