Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Don't overwrite remappings if provided by Settings #2530

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wtdcode
Copy link
Contributor

@wtdcode wtdcode commented Jul 31, 2023

Motivation

remappings should not be simply overwritten if there are any, which is especially the case when downloading standard json from etherscan.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Comment on lines +162 to +166
if self.settings.remappings.is_empty() {
self.settings.remappings = remappings;
} else {
warn!("remappings in settings conflicts with path configurations");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I disagree here,
I'd expect that a function called with_remappings would always set the remappings

Copy link
Contributor Author

@wtdcode wtdcode Jul 31, 2023

Choose a reason for hiding this comment

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

That's the point where I'm open to discussion. Note as the code says, for yul it won't set remapping so I did it this way. So your expectation is already wrong.

Another way is to modify here:

.with_remappings(paths.remappings.clone())
by checking Settings there. Are you fine with that?

@DaniPopes
Copy link
Collaborator

@wtdcode The assumption is that the function always sets the remappings. Yul doesn't have remappings (I think?), and in practice it's almost always false. What would this fix?

@wtdcode
Copy link
Contributor Author

wtdcode commented Aug 18, 2023

@wtdcode The assumption is that the function always sets the remappings. Yul doesn't have remappings (I think?), and in practice it's almost always false. What would this fix?

When compiling a project, it simply overwrites all existing remappings (in many cases, empty remappings), which overwrites the remappings inherited from the settings.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants