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

Support loading artifact from hardhat deployments directory #567

Merged
merged 2 commits into from
Jul 8, 2021

Conversation

taminomara
Copy link
Contributor

Part of #563.

Allow loading artifacts from hardhat deployments directory by scanning its contents and parsing all allowed contracts.

@taminomara taminomara changed the base branch from main to feature/builder-refactoring July 7, 2021 09:22
@taminomara taminomara requested a review from a team July 7, 2021 09:41
@taminomara taminomara force-pushed the ethcontract-rs-563 branch from 6065424 to e572279 Compare July 7, 2021 09:43
Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

So far only reviewed hardhat dot rs. This is very long. Will have to come back later for more.

ethcontract-common/src/artifact/hardhat.rs Outdated Show resolved Hide resolved
ethcontract-common/src/artifact/hardhat.rs Show resolved Hide resolved
ethcontract-common/src/artifact/hardhat.rs Outdated Show resolved Hide resolved
ethcontract-common/src/artifact/hardhat.rs Show resolved Hide resolved
ethcontract-common/src/artifact/hardhat.rs Outdated Show resolved Hide resolved
ethcontract-common/src/artifact/hardhat.rs Outdated Show resolved Hide resolved
@taminomara
Copy link
Contributor Author

I can move example into a separate PR, it that will help with review.

@taminomara taminomara added the enhancement New feature or request label Jul 7, 2021
ethcontract-common/src/artifact/hardhat.rs Show resolved Hide resolved
ethcontract-common/src/artifact/hardhat.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

The artifact loading code (load_from_directory) was very linear and easy to review.

One meta-PR comment, I think that the example could have been done in a separate PR, so this one could focus on the relevant ethcontract-common/src/artifact/hardhat.rs changes. I see that the unit test does depend on the new example output making it hard to split the two, but maybe its worth having manually created test hardhat deployment directory somewhere (kind of like an assertion of "this is the expected format and layout for a deployment directory").

@nlordell
Copy link
Contributor

nlordell commented Jul 7, 2021

I can move example into a separate PR, it that will help with review.

TBH that would be kind of nice (if it isn't too much effort), I admittedly did not review the example carefully at all. If its too much work, don't worry about it.

@taminomara
Copy link
Contributor Author

Yeah this is difficult to review, I'll split the PR. I wish github allowed tree view the same way Intellij github plugin does.

@taminomara taminomara force-pushed the ethcontract-rs-563 branch from e572279 to 167e940 Compare July 7, 2021 11:48
@taminomara
Copy link
Contributor Author

I've moved example into a separate PR which I'll publish after this one. Filtering contracts by name and unrelated changes to documentation are moved to a separate PR as well.

@taminomara
Copy link
Contributor Author

From +13,914 −10,346 to +221 −99, way better!

@taminomara taminomara merged commit a37745d into feature/builder-refactoring Jul 8, 2021
@taminomara taminomara deleted the ethcontract-rs-563 branch July 8, 2021 09:10
taminomara pushed a commit that referenced this pull request Jul 9, 2021
Part of #563.

Allow loading artifacts from hardhat deployments directory by scanning its contents and parsing all allowed contracts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants