Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

chore: make compiled_artifacts public in ProjectCompileOutput struct #2671

Closed

Conversation

dutterbutter
Copy link

Motivation

This pull request proposes a change to the ProjectCompileOutput struct by making the compiled_artifacts field public. The modification is required for the usage of extending foundry to work with non-solc compilers like zksolc for zkSync.

Context:

Currently, the compiled_artifacts field is marked as pub(crate), making it accessible only within the same crate. This encapsulation poses a limitation when the struct is used as part of a larger build process where access to compiled_artifacts is needed.

Rationale:

In effort to extend foundry support to zkSync ecosystem, direct access is needed to the compiled_artifacts field. You can see where we make use of compiled_artifacts here.

  • Enhanced Flexibility: Making compiled_artifacts public allows for more flexible post-compilation processes, including custom artifact handling, storage, and modifications tailored to specific use cases.

Functionality Impact:

The proposed change will have no impact on existing functionality. It merely extends the accessibility of the compiled_artifacts field, enabling external crates to utilize the compiled bytecode. This change is backward-compatible and does not alter the behavior of any existing code that depends on the ProjectCompileOutput struct.

Solution

Solution:

pub compiled_artifacts: Artifacts<T::Artifact>,

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

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.

henlo @dutterbutter :)

we're about to deprecate this (#2667) and moved this over to https://github.com/foundry-rs/compilers same code, just no longer dependent on ethers, instead it uses alloy deps now for json abi etc.

foundry is also using this already

so please open the PR there, and a getter function instead of pub :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants