[Investigation] Config overrides should be based on merging the user config, not the resolved #6490
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
v-next/hardhat/src/internal/builtin-plugins/network-manager/hook-handlers/hre.ts
Outdated
Show resolved
Hide resolved
v-next/hardhat/src/internal/builtin-plugins/network-manager/config-resolution.ts
Outdated
Show resolved
Hide resolved
v-next/hardhat/src/internal/builtin-plugins/network-manager/network-manager.ts
Outdated
Show resolved
Hide resolved
ChristopherDedominici
added
no changeset needed
| * @param resolveConfigurationVariable A function to resolve configuration variables. | ||
| * @returns A Partial<NetworkConfig> containing the resolved values from the override. | ||
| */ | ||
| export function resolveNetworkConfigOverride( |
There was a problem hiding this comment.
This method was removed as the validation logic has been simplified.
The user config is now merged with the Hardhat config and fully re-validated (so pickResolvedFromOverrides can be deleted too). Instead of selectively resolving overrides, it's sufficient to use resolveHttpNetwork or resolveEdrNetwork.
|
|
||
| export default async (): Promise<Partial<HardhatRuntimeEnvironmentHooks>> => ({ | ||
| created: async (context, hre) => { | ||
| let networkManager: NetworkManager | undefined; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- although `userConfig` is present on the hre, it isn’t typed in the official definition | ||
| const userConfigNetworks = (hre as HardhatRuntimeEnvironmentImplementation) |
There was a problem hiding this comment.
Is it expected to have userConfig in hre, or is this an error? Is it expected but intentionally left untyped to discourage users from accessing it?
There was a problem hiding this comment.
@alcuadrado can you clarify this?
There was a problem hiding this comment.
oh, this was clarified on slack. This should be properly declared in the HRE type.
…lify-user-config-override
| networkConfigOverride, | ||
| this.#networkConfigs[resolvedNetworkName], | ||
| ); | ||
| const newConfig = mergeConfigOverride( |
There was a problem hiding this comment.
I think this logic, mergeConfigOverride is probably now outdated. What do you think, @schaable ?
There was a problem hiding this comment.
I think how we could do with a deep merge, without anything fancy.
There was a problem hiding this comment.
It doesn't seem like we have a deep merge in hh-utils. Should I add one?
There was a problem hiding this comment.
I would say go for it, but keep it simple. You can probably reuse the mergeConfigOverride code. I'd leave functions, symbols, and classes out, since we probably don't want to merge those.
…lify-user-config-override
No description provided.