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

Add support for hardhat and hardhat-deploy artifacts and deployments. #512

Closed
10 tasks done
nlordell opened this issue May 17, 2021 · 19 comments · Fixed by #573
Closed
10 tasks done

Add support for hardhat and hardhat-deploy artifacts and deployments. #512

nlordell opened this issue May 17, 2021 · 19 comments · Fixed by #573
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@nlordell
Copy link
Contributor

nlordell commented May 17, 2021

Since we are migrating more and more of our repositories to hardhat and hardhat-deploy, it would be nice to support this natively.

@nlordell nlordell added enhancement New feature or request good first issue Good for newcomers labels May 17, 2021
@taminomara
Copy link
Contributor

I want to look into this during Ecosystem Fridays maybe?

@taminomara
Copy link
Contributor

I'll start with adding this to ethcontract-generate, then move on to macro.

@taminomara
Copy link
Contributor

So, hardhat artefacts are structured differently from truffle artefacts. In hardhat, you get a list of contracts for each network, while in truffle it's a list of networks for each contract. This difference has two major impacts. First, in hardhat it's possible that a contract will have different ABIs on different networks. Second, we have a single json file containing all info about all contracts. So, potentially, we can generate multiple contracts with a single macro invocation.
I don't want to support contracts that have different ABIs on different networks, I think it's ok to fail compilation when we see this.

As for generating multiple contracts from a single macro invocation, I think this could be useful; we won't have to load and parse the same file multiple times. However, that will require changing the interface of both builder and macro.

I'm thinking about what a good interface will look like.

@nlordell
Copy link
Contributor Author

I don't want to support contracts that have different ABIs on different networks, I think it's ok to fail compilation when we see this.

I also agree. I think having multiple contracts with different ABIs is a huge can of worms that I don't want to open.

Also, as long as we can have a way to specify a single deployment, we can techincally still support different ABIs on different networks by having multiple contract! macro invocations.

So, potentially, we can generate multiple contracts with a single macro invocation.

Just to make sure I understood correctly, would the suggestion be that we can specify multiple paths to JSON artifacts and then the network information would just be merged (erroring if non-mergable differences - such as different ABIs are encountered)?

@taminomara
Copy link
Contributor

we can specify multiple paths to JSON artifacts and then the network information would just be merged

My line of thought was the following. One artifact (that is, one JSON file) may now export multiple contracts. Thus, we should be able to generate all of them in one go.

Merging data form different JSONs sounds interesting, I'll look into it.

@taminomara
Copy link
Contributor

So, we have several formats for artifacts.

Truffle and Waffle:

  1. one named contract with networks,
  2. one unnamed contract with networks.

HardHat:

  1. contracts for single network,
  2. contracts for multiple networks.

With these in mind, I propose the following API.

In ethcontract-common:

  • struct Contract -- info about a contract: its name, abi, and other things (what's currently known as truffle::Artifact),
  • trait Artifact -- a storage for contracts. Allows looking up contracts by name and getting list of all contracts. It will have several implementations. Preliminarily, those are:
    • TruffleArtifact -- loads truffle artifacts,
    • WaffleArtifact -- loads waffle artifacts,
    • HardhatExport -- loads artifacts generated with hardhat --export,
    • HardhatMultiExport -- loads artifacts generated with hardhat --export-all,
    • HardhatDeployments -- lazily pulls contracts from deployments/ folder generated by hardhat (this will be done later).
  • perhaps instead of having an Artifact implementation for every artifact format we'll have ArtifactLoaders instead. I'll see which approach is better.

In ethcontract-generate:

  • Builder -- builds a single contract, allows changing contract's name, mod and other settings,
  • helper functions to create a builder from path to an artifact (i.e. Builder::load_truffle),
  • maybe helper functions to generate all contracts in an artifact.

Then build.rs may look like this:

let artifact = HardhatDeployments::new("deployments/")
    .unwrap();

Builder::new(artifact.contract("ContractName"))
    .generate()
    .unwrap()
    .write_to_file(Path::new(&dest).join("rust_coin.rs"))
    .unwrap();

Builder::new(artifact.contract("AnotherContractName"))
    .generate()
    .unwrap()
    .write_to_file(Path::new(&dest).join("rust_coin.rs"))
    .unwrap();

As for macros, I have a few ideas, but none that I'm satisfied with.

taminomara pushed a commit that referenced this issue Jun 21, 2021
In preparation for #512, we're separating artifacts (that is, JSON
files that contain contracts) from actual contracts data.
@nlordell
Copy link
Contributor Author

I like the proposed abstractions, as it will make it easier to extend it and add new formats in the future.

taminomara pushed a commit that referenced this issue Jun 22, 2021
In preparation for #512, we're separating artifacts (that is, JSON files that contain contracts) from actual contracts data.
@taminomara
Copy link
Contributor

I want to move code that loads contracts from etherscan and other places from ethcontract-generate to ethcontract-common. It can be optional (under cfg). IMO it makes sense to keep all code for loading and parsing artifacts in one place.

@nlordell
Copy link
Contributor Author

The issue with that is that it means that we would need to pull in curl even if it doesn't get used by ethcontract runtime. Maybe it makes sense to have a ethcontract-artifact crate that does all the Artifact loading and creating of Contract instances which is all that is needed by the runtime.

It also allows some of the JSON parsing code to move out of ethcontract-common which would be nice.

@taminomara
Copy link
Contributor

taminomara commented Jun 23, 2021

I was thinking that we may enable curl support conditionally, via a feature flag (i.e. #[cfg(feature = "curl")]).

I'm not sure about ethcontract-artifact. If parsing JSON lives in ethcontract-artifact, then ethcontract users who want to deploy their contracts will have to use it. If parsing JSON lives in ethcontract-common, then it defeats the whole purpose of ethcontract-artifact. In any way, it sounds like a separate issue.

@nlordell
Copy link
Contributor Author

nlordell commented Jun 23, 2021

The idea would be that Contract type, would continue to live in ethcontract-common, which will end up being the common type used by the underlying ethcontract::contract::Instances. This way, all the code for loading JSON artifact files, or retrieving from NPM, Etherscan, etc. as well as the Artifact implementations that take these JSON objects and generates Contract instances can live is ethcontract-artifact.

Then, hopefully, we could even make it so we can create a Contract instance in the generated contract code without requiring lazy_static and JSON parsing (which is what we currently do 😅).

@taminomara
Copy link
Contributor

I see. Then I'll continue with the current structure for now, and we can implement this proposal later.

@taminomara
Copy link
Contributor

taminomara commented Jun 24, 2021

This refactoring will also help with #209, #196 and #170.

taminomara pushed a commit that referenced this issue Jun 24, 2021
Part of #512.

We're adding the Artifact trait that will provide a uniform interface for working with different artifact sources.
taminomara pushed a commit that referenced this issue Jun 25, 2021
Part of #512.

Implements artifact loader for hardhat export (hardhat export command).
taminomara pushed a commit that referenced this issue Jul 1, 2021
Fix #170, part of #512.

Since we support multiple artifact formats now, contract generation should not rely on source json. This PR allows serializing contracts and makes generation code to use serialization.

Builder API still requires source JSON though. It will be fixed in #512 and #196.
@taminomara
Copy link
Contributor

So I still don't have any nice API for a macro that could yield multiple contracts at once. So for now, I propose that we live the current macro API, just slightly modify it:

  • add format setting,
  • if artifact contains any named contracts, contract setting will be used to determine which contract to use. Otherwise, it sets a name for an unnamed artifact.
  • add contract_rename setting that can be used to rename contracts.

Example:

contract!(
    pub "build/export.json",
    format = hardhat_multi,
    contract = WETH9,
   contract_rename = Weth,
);

@nlordell
Copy link
Contributor Author

nlordell commented Jul 1, 2021

add format setting,

Would it be possible to try and autodetect this as well? Otherwise I like the format setting.

add contract_rename setting that can be used to rename contracts

We can also use as in a multi-contract context:

contract!(
    pub "build/export.json",
    format = hardhat_multi,
    contract = WETH9 as Weth,
);

@taminomara
Copy link
Contributor

Would it be possible to try and autodetect this as well? Otherwise I like the format setting.

It's possible, but there are corner cases, and it will require parsing JSON into a Value and then exploring said value. Basically, "if source points to a JSON file, and there's abi field in said JSON, and ...". Although it could be useful. I'll add format, and we can add format = auto later.

We can also use as in a multi-contract context

Love it!

@nlordell
Copy link
Contributor Author

nlordell commented Jul 1, 2021

Although it could be useful. I'll add format, and we can add format = auto later.

For sure, just another improvement idea to make the macro as ergonomic as possible.

@taminomara
Copy link
Contributor

Maybe format = auto should take into account artifact source. If it's etherscan, we know format is raw ABI.

taminomara pushed a commit that referenced this issue Jul 5, 2021
Fix #196, part of #512, merge after #554.

Refactor builder interface to separate code that loads artifacts, parses them and generates bindings into independent modules. Now code generation takes a reference to `Contract`, loading contracts from source is no longer builder's job.

Additionally, `Builder` was renamed to `ContractBuilder` in anticipation that we will have `ArtifactBuilder` in the future. Also `ethcontract_generate::contract` was renamed to `ethcontract_generate::generate`. I can move these changes into separate pullrequests if anyone things it's necessary.

Procedural macro will be updated later in #512.
@taminomara
Copy link
Contributor

Moved some tasks to separate issues: #514, #563, #564.

taminomara pushed a commit that referenced this issue Jul 7, 2021
Fix #170, part of #512.

Since we support multiple artifact formats now, contract generation should not rely on source json. This PR allows serializing contracts and makes generation code to use serialization.

Builder API still requires source JSON though. It will be fixed in #512 and #196.
taminomara pushed a commit that referenced this issue Jul 7, 2021
Fix #196, part of #512, merge after #554.

Refactor builder interface to separate code that loads artifacts, parses them and generates bindings into independent modules. Now code generation takes a reference to `Contract`, loading contracts from source is no longer builder's job.

Additionally, `Builder` was renamed to `ContractBuilder` in anticipation that we will have `ArtifactBuilder` in the future. Also `ethcontract_generate::contract` was renamed to `ethcontract_generate::generate`. I can move these changes into separate pullrequests if anyone things it's necessary.

Procedural macro will be updated later in #512.
taminomara pushed a commit that referenced this issue Jul 7, 2021
Part of #512, merge after #555.

Refactor `contract!` macro a bit, add support for artifact formats, rework contract renaming syntax (see discussion in #512).
taminomara pushed a commit that referenced this issue Jul 9, 2021
Fix #170, part of #512.

Since we support multiple artifact formats now, contract generation should not rely on source json. This PR allows serializing contracts and makes generation code to use serialization.

Builder API still requires source JSON though. It will be fixed in #512 and #196.
taminomara pushed a commit that referenced this issue Jul 9, 2021
Fix #196, part of #512, merge after #554.

Refactor builder interface to separate code that loads artifacts, parses them and generates bindings into independent modules. Now code generation takes a reference to `Contract`, loading contracts from source is no longer builder's job.

Additionally, `Builder` was renamed to `ContractBuilder` in anticipation that we will have `ArtifactBuilder` in the future. Also `ethcontract_generate::contract` was renamed to `ethcontract_generate::generate`. I can move these changes into separate pullrequests if anyone things it's necessary.

Procedural macro will be updated later in #512.
taminomara pushed a commit that referenced this issue Jul 9, 2021
Part of #512, merge after #555.

Refactor `contract!` macro a bit, add support for artifact formats, rework contract renaming syntax (see discussion in #512).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants