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

feat(solc): add configurable Artifact type #907

Merged
merged 28 commits into from
Feb 17, 2022

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Feb 13, 2022

Motivation

Ref foundry-rs/foundry#705

Solution

  • make the Project take the ArtifactOutput as value
  • Add ConfigurableArtifacts type that can be configured to include additional values from the Contract

Open questions for follow:

  • since the new ConfigurableContractArtifact is basically a super set of the CompactContractBytecode we use by default, which are identical without additional config, should we change the default ArtifactType? I can refactor this to not break anything on foundry

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

@gakonst
Copy link
Owner

gakonst commented Feb 14, 2022

since the new ConfigurableContractArtifact is basically a super set of the CompactContractBytecode we use by default, which are identical without additional config, should we change the default ArtifactType? I can refactor this to not break anything on foundry

As discussed in DMs, let's merge the types together.

@mattsse
Copy link
Collaborator Author

mattsse commented Feb 16, 2022

Update

  • made the default project ArtifactOutput type ConfigurableArtifacts which by default behaves exactly as the minimal MinimalCombinedArtifacts which is the basic config, (basically solc --combined-json abi bin bin-runtime)

The Project now takes the ArtifactOutput handler as value, instead of PhantomData pin, which also us to dynamically configure the ConfigurableArtifacts with any variant from the ContractOutputSelection enum.

with ConfigurableArtifacts we can modify the contract's json artifact on the fly and include metadata, devdoc, ir etc... but also emit some of them as additional files, or both.

This is mirrored on foundry foundry-rs/foundry#762

@mattsse mattsse marked this pull request as draft February 16, 2022 22:14
@mattsse mattsse marked this pull request as ready for review February 16, 2022 22:26
@mattsse mattsse force-pushed the matt/make-artifactoutput-dynamic branch from 25adde4 to a03127f Compare February 16, 2022 22:32
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.

LGTM.

self.additional_files.write_extras(contract, file)
}

fn contract_to_artifact(&self, _file: &str, _name: &str, contract: Contract) -> Self::Artifact {
Copy link
Owner

Choose a reason for hiding this comment

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

how come the contract _name and _file are not needed in this case? is our abstraction maybe not ideal and should be reconsidered?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right this is not needed in this case as we only want the artifact object, but the full info could still be relevant, e.g. for an artifact that also contains the name of the contract.

@gakonst gakonst merged commit 19d2fd1 into gakonst:master Feb 17, 2022
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