-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat(solc): migrate new ethers-solc backend #660
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! This also addresses the double artifacts issue, which resulted in twice the tests being run? So we should expect a speedup there too?
let artifact = artifacts[0].clone(); | ||
let abs_path = dunce::canonicalize(PathBuf::from(contract_path))?; | ||
|
||
let cache = SolFilesCache::read_joined(&project.paths)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does the cache end up being the main entrpoint to reading an artifact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, because the cache now contains the actual path to the artifacts of a solidity file,
so we have file
-> (contract
-> [(solc version
, path
)])
"artifacts": {
"Dapp": {
"0.8.11+commit.d7f03943.Darwin.appleclang": "Dapp.sol/Dapp.json"
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it - in general I'd expect the cache to not be a user facing component? Maybe we can just document it as intended usage of the lib to avoid confusion? Like "Common ethers solc workflows" (not a blocker for this PR, just saying)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, sg, should be added to the docs and examples in ethers-solc
it should, the double artifacts issues resulted in us not properly handling source unit names so solc couldn't find imports in the we now always use the absolute path for source unit names, with applied remappings for imports, the same way solc would do it, so solc will find them directly in the input json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
letsgooo
DO NOT MERGE - CURRENTLY PATCHED gakonst/ethers-rs#802
All tests are passing now, but we should double-check the debugging manually, @brockelmore could you check whether debug also works?