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

refactor: make Contract generic for Compiler and add metadata to CompilerOutput #1

Closed
wants to merge 4 commits into from

Conversation

elfedy
Copy link
Owner

@elfedy elfedy commented Nov 8, 2024

Right now the compiler abstraction has two couplings that force foundry-zksync to implement it's own fork of compilers:

  1. Contract is specific to solc/EVM contracts. Era VM contracts, while requiring different fields, still could use most of the functionality of the compilers pipeline.
  2. CompilerOutput has solc specific fields. zksolc compilation has relevant information that is useful to have later on, for example when storing BuildInfo

This PR implements changes to address this. If implemented, it would allow foundry-zksync to get rid of all overrides and only maintain ZKsync specific data structures/trait implementations. See sample PR. Changes include:

  1. Make Compiler generic over Contract.
  2. Add metadata field to CompilerOutput in order to add arbitrary data to compilation output.

@elfedy elfedy changed the title refactor: make Contract generic for Compiler refactor: make Contract generic for Compiler and add metadata to CompilerOutput Nov 16, 2024
Copy link

@hedgar2017 hedgar2017 left a comment

Choose a reason for hiding this comment

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

LGTM!

crates/compilers/src/compile/output/contracts.rs Outdated Show resolved Hide resolved
impl<T: ArtifactOutput + Default> TempProject<MultiCompiler, T> {
impl<
T: ArtifactOutput<CompilerContract = <MultiCompiler as Compiler>::CompilerContract> + Default,
> TempProject<MultiCompiler, T>
Copy link

Choose a reason for hiding this comment

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

Perhaps we could think about a type alias for this.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes it might make sense, want to see what foundry team says about the abstraction first because they probably have their own convention on how to handle types and names

Co-authored-by: Nisheeth Barthwal <nisheeth.barthwal@gmail.com>
@elfedy elfedy closed this Nov 19, 2024
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