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.

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

forge(fix): dont give out error if artifact has no source on script #1880

Merged
merged 3 commits into from Jun 8, 2022

Conversation

joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Jun 8, 2022

Motivation

If the artifact for whatever reason has no source (it should), we don't want to exit, because they're only necessary for the debugger.

Solution

@joshieDo joshieDo added the T-bug Type: bug label Jun 8, 2022
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, but can we please add a smol note there?

also does this indicate that something's wrong the the artifacts or is this script related?

@joshieDo
Copy link
Collaborator Author

joshieDo commented Jun 8, 2022

also does this indicate that something's wrong the the artifacts or is this script related?

Given that source_file() returns an Option, I am assuming that there is a situation where artifacts have no source... But I don't know which tbh

We can add a warn! if it doesn't find one?

@gakonst
Copy link
Member

gakonst commented Jun 8, 2022

Is this related to the guy on Telegram that was getting "source not found"?

This makes sense - but worth us understanding why they got that error in the first place?

@mattsse
Copy link
Member

mattsse commented Jun 8, 2022

just a hunch but None is only returned if there's no id present in the Configurable artifacts, we only added this ~1-2 weeks ago so perhaps they updated forge build ran with old artifacts

https://github.com/gakonst/ethers-rs/blob/master/ethers-solc/src/artifact_output/configurable.rs#L87

I think should be ok to merge, but let's add a warn! trace to the else branch

@sendra
Copy link

sendra commented Jun 8, 2022

Is this related to the guy on Telegram that was getting "source not found"?

This makes sense - but worth us understanding why they got that error in the first place?

The error I got was because the path I was using to get the artifacts was not correct. When changed to use path from project root, it worked fine.

@mattsse mattsse merged commit e7a5185 into foundry-rs:master Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants