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

Load local networks configuration from local path and default home #3

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

nkuba
Copy link
Contributor

@nkuba nkuba commented Jul 21, 2021

In this PR we update the possibility to load networks configuration from three places at the same time:

  • specified localNetworksConfig file,
  • networks configuration in hardhat project config file,
  • default user home networks configuration file ~/.hardhat/networks.json.

Conflicts resolution

Conflicts will be resolved in the following order:

  1. Local networks specific config file (based on localNetworksConfig property).
  2. Project networks specific configuration.
  3. Default local network configuration (~/.hardhat/networks.json).

Relative path support

Added support of relative paths that will be resolved based on Hardhat's project root directory hardhatConfig.paths.root.

File does not exist

If the localNetworksConfig path is configured and the file doesn't exist it will throw an error.
If the ~/.hardhat/networks.json file does not exist nothing will happen.

LocalNetworksConfig interface

LocalNetworksConfig interface was updated to reflect what was already described in README.md: make properties optional. Also, the types for the properties were changed to the ones used by hardhat for user config NetworksUserConfig and NetworkUserConfig as these types contain optional properties.

With this change, a user will be able to define their configuration file with type import, like in the example:

import { LocalNetworksConfig } from "hardhat-local-networks-config-plugin"

const config: LocalNetworksConfig = {
  networks: {
    ropsten: {
      url: "http://localhost:8567",
      accounts: ["0x1234567890"],
    },
  },
}

module.exports = config

Tests

We added more tests and made the test fixtures more consistent to reduce confusion.

It is expected that configuration for networks can be available under
`~/.hardhat/networks.json` which is a default path. Additionally to this
path user can also configure `localNetworksConfig` path in the project
configuration.

The code was refactored in preparation to change the order of how
configs are resolved and overwritten.

With this change the order remains the same:
1. Project network specific configuration
2. Local network specific configuration
3. Local default network configuration (`~/.hardhat/networks.json`)
This commit contains same updates to test fixtures to make them more
consistent and easier to follow.

1. `test/helpers/fixtures/local/hardhat.config` were renamed as the
previous name was confusing, the files contain network configuration
specific for the plugin, not the whole hardhat config.

2. Created separate configuration for a default `test/helpers/fixtures/homedir/.hardhat/networks.json` configuration file
that reflects the one placed in user home directory and used for testing.

3. Extracted common network configuration for hardhat project config
   file.

4. Made the three configurations (home, project and local) more
   consistent and aligned.
Modfied useEnvironment function to expect the name and resolve the path
as it's common.
Order in which networks configuration are read can be updated thanks to
the change in 7c26b2b.
As the default config is read separately from local config we assume
that user want to use specific external configuration file over the
configuration specified in project configuration.

The new order is:
1. Local network specific configuration (`localNetworksConfig`).
2. Project network specific configuration.
3. Default local network configuration (`~/.hardhat/networks.json`).
The path that is provided in localNetworksConfig can be a relative path
that is resolved based on the project root dir.
The LocalNetworksConfig interface was changed in two ways:

1. `networks` and `defaultConfig` properties are optional, this was
   mentioned already in README.md.

2. Types were changed to `NetworksUserConfig` and `NetworkUserConfig`,
   as this are the correct types that are configurable by an user. The
   types got optional properties.

Added `LocalNetworksConfigInternal` that extends `LocalNetworksConfig`.
The difference is that both properties are required. This type is used
internally when we read the config.
Added two cases:
1. The config file that the localNetworksConfig path points to can be
empty.

2. The project config may not contain `networks` property.
A pacakge @types/mock-os is not avialble so we can't iinstall it to use
import.
@pumpurum2
Copy link

@facuspagnuolo @nkuba hello, will this PR merged?
At least relative path support is very desirable for me

@nkuba
Copy link
Contributor Author

nkuba commented Nov 6, 2021

@facuspagnuolo @nkuba hello, will this PR merged?

At least relative path support is very desirable for me

I hope it will get merged 🤞

@facuspagnuolo
Copy link
Owner

@facuspagnuolo @nkuba hello, will this PR merged?

At least relative path support is very desirable for me

I hope it will get merged 🤞

I promise to merge it this week, sorry for the late reply guys!

Copy link
Owner

@facuspagnuolo facuspagnuolo 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 suggesting this change, left some comments we should fix before merging.

}

export function getDefaultLocalNetworksConfigPath() {
return path.join(getLocalConfigDir(), HARDHAT_NETWORK_CONFIG_FILE)
export function getDefaultHomeLocalNetworksConfigPath() {
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about?

Suggested change
export function getDefaultHomeLocalNetworksConfigPath() {
export function getHomeNetworksConfigPath() {

if (typeof configuredLocalNetworksConfigPath === 'string' && configuredLocalNetworksConfigPath) {
const resolvedLocalNetworksConfigPath = untildify(configuredLocalNetworksConfigPath)

const localNetworksConfigPath = path.isAbsolute(resolvedLocalNetworksConfigPath)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const localNetworksConfigPath = path.isAbsolute(resolvedLocalNetworksConfigPath)
const localNetworksConfigPath = path.isAbsolute(resolvedLocalNetworksConfigPath)

}

extendConfig((hardhatConfig: HardhatConfig, userConfig: HardhatUserConfig): void => {
const localNetworksConfig = readLocalNetworksConfig(userConfig)
const homeLocalNetworksConfig = readHomeNetworksConfig()
Copy link
Owner

Choose a reason for hiding this comment

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

I find homeLocal a bit redundant, what do you think about distinguishing between the home config (default) and the local (custom) one?

Suggested change
const homeLocalNetworksConfig = readHomeNetworksConfig()
const homeNetworksConfig = readHomeNetworksConfig()


if (localNetworksConfigPath && !fs.existsSync(localNetworksConfigPath)) {
throw new HardhatPluginError(
`hardhat-local-networks-config-plugin`,
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
`hardhat-local-networks-config-plugin`,
'hardhat-local-networks-config-plugin',

Comment on lines 33 to 35
localNetworksConfig.defaultConfig,
userNetworkConfig as object,
localNetworksConfig.networks[networkName] || {},
Copy link
Owner

Choose a reason for hiding this comment

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

This is a bit weird, cause it seems you're prioritizing specific network configs of the local config file over the project's config, but not the default config in the local config file over the project's config.


return localNetworksConfigPath
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'd keep this in the index.ts file

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

Successfully merging this pull request may close these issues.

3 participants