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

feat(solc): flatten #774

Merged
merged 24 commits into from
Jan 17, 2022
Merged

feat(solc): flatten #774

merged 24 commits into from
Jan 17, 2022

Conversation

rkrasiuk
Copy link
Contributor

@rkrasiuk rkrasiuk commented Jan 7, 2022

Motivation

Implementation of flatten feature discussed in foundry #312

Solution

Use the dependency graph to recursively resolve the file imports into a single string.

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

Copy link
Collaborator

@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.

while this approach looks like it could work, I have some suggestions how to improve this a bit.

we're already parsing the solidity file and detecting import statements:

SourceUnitPart::ImportDirective(_, import) => {
let import = match import {
Import::Plain(s) => s,
Import::GlobalSymbol(s, _) => s,
Import::Rename(s, _) => s,
};

all these import variants include a Loc value which has the start and end of the whole statement.
So ideally we want to use this data so we can later simply replace that slice [start..end] with the content of the file that's imported here.

however since solang only targets >0.7.0, it can happen that need to use the regex fall back, so ideally the utils::find_import_paths will also return them, which should be possible since the regex::Match type has start and end https://docs.rs/regex/latest/regex/struct.Match.html

If we roll a struct Import {path, start, end} for the SolData type we should be able to flatten the entire graph.

This depth-first operation where you start with the targeted file and then recursively loop through all imports and their imports... and replace every import statement with content. That's probably a bit tricky since we need to keep track of updated locations because after we replaced [start..end] with the first import the start and end locations of the next import are no longer valid

ethers-solc/src/lib.rs Outdated Show resolved Hide resolved
ethers-solc/src/config.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

great progress!
some suggestions and comments

Comment on lines 199 to 201
let result = String::from_utf8(extended).map_err(|err| {
SolcError::msg(format!("failed to convert extended bytes to string: {}", err))
})?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can definitely do to_string_lossy here?

ethers-solc/src/resolver.rs Outdated Show resolved Hide resolved
ethers-solc/src/lib.rs Show resolved Hide resolved
ethers-solc/src/config.rs Outdated Show resolved Hide resolved
ethers-solc/src/config.rs Outdated Show resolved Hide resolved
ethers-solc/src/config.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

few nits,

overall this is looking good,
I'd like to see couple more tests, for the dapp/hardhat examples
and perhaps you could try manually try this with some real repos and see if that works

ethers-solc/src/config.rs Outdated Show resolved Hide resolved
ethers-solc/src/config.rs Outdated Show resolved Hide resolved
ethers-solc/src/config.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@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!

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

test failure is unrelated, merging and we can iterate as we close the feedback loop with usage in foundry.

is there any follow up that needs to be done which we know we need? probably need to get the solidity compiler version saved in the artifacts (#755)

@gakonst gakonst merged commit afcba95 into gakonst:master Jan 17, 2022
@rkrasiuk rkrasiuk deleted the solc/feat-flatten branch January 18, 2022 08:06
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.

3 participants