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 missing hardhat network variables #643

Closed
wants to merge 1 commit into from

Conversation

fredlacs
Copy link

@fredlacs fredlacs commented Jul 12, 2021

Fixes #624, fixes #640, and fixes wighawag/hardhat-deploy#132

Proposed fix inspired from discussions in hardhat-deploy issue 132

@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2021

Codecov Report

Merging #643 (eaa47c3) into master (f0aa876) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #643   +/-   ##
=======================================
  Coverage   98.68%   98.68%           
=======================================
  Files          22       22           
  Lines         986      986           
=======================================
  Hits          973      973           
  Misses         13       13           
Impacted Files Coverage Δ
plugins/resources/nomiclabs.utils.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0aa876...eaa47c3. Read the comment docs.

@@ -134,6 +134,7 @@ function configureNetworkEnv(env, networkName, networkConfig, provider, isHardha
env.config.defaultNetwork = networkName;

env.network = {

Choose a reason for hiding this comment

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

The fact that it still create a new object will cause issue with plugin that modify the network object before this is called.
It is better to simply modify the network object

Copy link
Author

@fredlacs fredlacs Jul 21, 2021

Choose a reason for hiding this comment

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

I'm using the fork with this fork and seems to be working well with hardhat deploy :)

If they modify it before, it shouldn't be an issue since we first set all previous object values then only override the needed ones ...env.network.
If you meant using Object.assign I believe the main difference would only be that setters are called on the object. According to a random SO answer: "spread operator will not copy the the source object’s prototype to the target object"

How would you suggest this be implemented? Glad to iterate on it or take a PR heh

Choose a reason for hiding this comment

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

Plugins could have copied the reference to hre.network before and will later modify an unrelated object. Modifying the network object would be safer. I proposed this in that comment: wighawag/hardhat-deploy#132 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Object.assign retains the reference to the original object too. I think that is more conventional. But simplicity is always good so maybe @wighawag solution of just doing inplace modifications (env.network.name = networkName etc.) is better.

Copy link
Member

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

Thanks for this. Went with #647. Happy to revisit if that's causing problems down the line.

@cgewecke cgewecke closed this Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants