feat(solc): yul compilation #994
Conversation
| Source::read_all_from(path.as_ref()).map(Self::with_sources) | ||
| } | ||
|
|
||
| /// Creates a new Compiler input with default settings and the given sources | ||
| pub fn with_sources(sources: Sources) -> Self { | ||
| Self { language: "Solidity".to_string(), sources, settings: Default::default() } | ||
| pub fn with_sources(sources: Sources) -> Vec<Self> { |
There was a problem hiding this comment.
Since this returns at most vec![sol, yul]
can we add something like this perhaps
enum ProjectCompilerInput {
Solidity(CompilerInput),
Yul(..),
Mixed(..,..)
}?
There was a problem hiding this comment.
I think the CompilerInput abstraction is correct right now - i.e. it's the unit that get's fed into solc compiler. The with_sources method though doesn't work because if sources have different types that means that you'll have 2 compiler inputs the needs to be fed into solc sequentially. So to me the signature looks good. Plus the vec is extensible for other sources (if any :))
To be 100% clean we could update the new method so it's just a plain constructor taking language as an input. For that method it's really uncommon to return a Vec. I just left it as it is for now because it could be used in many places.
Wdyt?
There was a problem hiding this comment.
makes sense, however working with a Vec<CompilerInput> can be a bit annoying.
Should be fine for now, but I think we can make this more ergonomic with ProjectCompilerInput which requires some refactoring, which we should leave for a follow up.
There was a problem hiding this comment.
I am OK with this as-is, seems good enough for now!
|
While this implementation is definitely more elegant than mine (#993) , given this approach of tying in Yul sources with Solidity sources for when I add Yul to foundry (which I am beginning after this, so lmk if you want to collab on it), but the one issue I can think of is with ABI, as Yul doesn't compile with an ABI by default, so adding some way to override the ABI of a Yul source would definitely be very helpful |
Yeah I was definitely thinking about adding abi for yul. My thinking was to load file with the same name as abi. E.g. if you have In any case I'm ok to merge this one or yours or some combined version. The key is to have a mix of Solidity / Yul sources in one project which will be kinda cool since hardhat and other guys don't have it :) |
I can move over some of my changes here (mostly with the test) as suggestions and you can move them in as needed. The mix is definitely really cool as only Truffle can do it. What would you think of an approach where we have a way to cast a Solidity interface over the Yul file (so yul_file_name.abi.sol) and compile the interface, and put the ABI on top of the Yul artifact, then those who want to skip it with call() have the option to, while others can use it like nothing is different Edit : If so I can get that done relatively quickly as I've done this for my Yul+ toolchains before |
Supportive of this :) I think let's coalesce the efforts around this PR for simplicity. @ControlCplusControlV can you make a PR against this one with any changes you have in mind? |
|
@gakonst yeah will make that PR later today as I am going to add in that change to handle Yul ABI alongside with it. |
|
My PR is up to @alexeuler's branch, still needs some work with one last type resolution issue which I can hopefully get resolved tomorrow as ABI should be fully functional from there! |
|
So type issues is fixed, but need help with one last thing. In this part of my fork of artifact_output mod.rs I have both the abi I need to replace and the artifact of which I need to insert the ABI into. But it doesn't appear there is a clean way to do that as Artifact won't let me modify the ABI directly and if I convert it into a CompactContract I am running into issues converting it back, so was curious as to the best way to do this or any other approaches. Will continue testing things but wanted to ask here in case there is something I am missing which could speed things up |
|
I see, what modifications are necessary? |
|
The ABI needs to be accessible so that it can be replaced for a given |
|
@mattsse is there a specific way you envision extending |
you mean the I don't have the full picture yet, and would need some more context on what needs to be done here https://github.com/ControlCplusControlV/ethers-rs-1/blob/feature/yul_compilation/ethers-solc/src/artifact_output/mod.rs#L580 |
|
@mattsse I was actually able to resolve it by avoiding type conversions and just saving the Contract Object for each Yul contract, so that I could then just modify and replace it later. Just have one bug where it is compiling correctly but it seems the caching is overwriting the file contents instead of a cache, if you want to help I have pushed all my code to my fork and the test is fairly reliable producing it (It overwrites SimpleStore.yul with the cache of the artifact) |
|
@gakonst (and @alexeuler if you have availability to provide input) could you review my changes for ABI to start moving it upstream? PR is here and my fork is here. So sorry for how long this took, did realize how bad my rust type skills are (although they've improved much more now!). I've added tests but as I was iterating so much code style is a bit sloppy so if there are any code style choices I can gladly implement them. It is a bit polluted with commits so squashing and merging will be much cleaner. |
|
great progress @ControlCplusControlV ! What'd be helpful is, if you could outline the motivation behind this with some additional details, especially how yul artifacts differ and why this is necessary? |
|
My candid recommendation would be that we merge this as-is, and @ControlCplusControlV can post a separate PR on top of master, given that it's not a blocker for this PR |
Motivation
Currently Foundry and ethers-rs is not capable of compiling Yul sources (foundry-rs/foundry#759)
Solution
Add yul compilation