Open
Conversation
Co-authored-by: Eric Lau <ericglau@outlook.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request implements a comprehensive Hardhat 3 migration for Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Script as Hardhat Script
participant Plugin as `@openzeppelin/hardhat-upgrades`
participant Hooks as Solidity Hook
participant Core as `@openzeppelin/upgrades-core`
participant Network as Hardhat Network
User->>Script: Run deployment script
Script->>Network: await hre.network.connect()
Network-->>Script: connection
Script->>Plugin: upgrades(hre, connection)
Plugin->>Plugin: createUpgradesAPI(hre, connection)
Plugin-->>Script: upgradesApi
Script->>Script: deployProxy(Box, [42], {kind: 'transparent'})
Script->>Core: Generate validation
Hooks->>Hooks: Intercept solc invocation
Hooks->>Core: Generate validations
Hooks-->>Hooks: Recompile if namespaced layout supported
Core-->>Hooks: Return validations
Hooks->>Hooks: Write validations.json
Script->>Network: Deploy via connection.ethers
Network-->>Script: proxy address
Script->>Network: Close connection
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
This was referenced Apr 23, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/plugin-hardhat/test/solidity-overrides-storage.js (1)
28-31:⚠️ Potential issue | 🟡 MinorAVA
throwsAsyncthird argument is the assertion message, not the error expectation.In AVA,
t.throwsAsync(fn, expectation?, message?)passes the third argument as the assertion message (shown on test failure), not as the expected error message. The current code passes'Set __gap array to size 47'as the assertion message while leaving the expectation argument asundefined, so it does not verify the actual error message thrown.To match the error message, pass it within an expectations object:
Fix
- await t.throwsAsync(() => upgrades.upgradeProxy(proxy, GapV2_Bad), undefined, 'Set __gap array to size 47'); + await t.throwsAsync(() => upgrades.upgradeProxy(proxy, GapV2_Bad), { + message: /Set __gap array to size 47/, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/test/solidity-overrides-storage.js` around lines 28 - 31, The test uses AVA's t.throwsAsync incorrectly by passing the expected error message as the third (message) argument; update the t.throwsAsync call in the test function that calls upgrades.upgradeProxy(proxy, GapV2_Bad) so the expectation is provided as the second argument (e.g., an object or regex matching "Set __gap array to size 47") and the optional assertion message as the third argument if needed; locate the t.throwsAsync invocation in the 'use gap incorrectly' test that references GapV1, GapV2_Bad and replace the undefined second argument with the correct expectation to assert the thrown error text.packages/plugin-hardhat/test/import-with-deploy-v4.js (1)
91-91:⚠️ Potential issue | 🟡 MinorIncorrect assertion logic.
The assertion
t.not(implAddrUpgraded2 !== implAddrUpgraded, implAddrUpgraded2)compares a boolean against a string message. This always passes when the addresses differ (sincetrueis truthy,t.not(true, ...)fails, but when they're equal,t.not(false, ...)passes).Based on the comment on line 88 ("upgrade imported proxy to different impl"), the intent appears to be asserting that the addresses are different after upgrading to
GreeterV2Proxiable.Proposed fix
- t.not(implAddrUpgraded2 !== implAddrUpgraded, implAddrUpgraded2); + t.not(implAddrUpgraded2, implAddrUpgraded);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/test/import-with-deploy-v4.js` at line 91, The test assertion is comparing a boolean to a string message; replace the wrong call that currently passes the boolean expression as the first argument to t.not. Locate the assertion referencing implAddrUpgraded2 and implAddrUpgraded (currently written as t.not(implAddrUpgraded2 !== implAddrUpgraded, implAddrUpgraded2)) and change it to assert the two addresses are different by calling t.not(implAddrUpgraded2, implAddrUpgraded) (or the equivalent AVA assertion that the actual implAddrUpgraded2 !== implAddrUpgraded).packages/plugin-hardhat/src/defender/deploy.ts (1)
249-266:⚠️ Potential issue | 🟡 MinorGuard the new source-name lookup before indexing metadata.
buildInfoSourceNameis now used as a direct key into bothoutput.contractsandmetadata.sources. If the HH2/HH3 normalization ever disagrees, this will throw a rawTypeErrorinstead of the existingUpgradesError, which makes source-verification failures much harder to diagnose. Please add an explicit existence check and raise a targetedUpgradesErrorhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/src/defender/deploy.ts` around lines 249 - 266, In getSpdxLicenseIdentifier, guard the new buildInfoSourceName lookups: verify that contractInfo.buildInfo.output.contracts[buildInfoSourceName] and the specific contract entry (contractInfo.contractName) exist before accessing compilerOutput, and likewise check that metadata.sources and metadata.sources[buildInfoSourceName] exist before returning license; if any of these are missing, throw a descriptive UpgradesError (same type already used) explaining that the source name normalization mismatched and suggesting to verify compiler metadata/inputSourceName settings so failures surface as a targeted UpgradesError rather than a TypeError.
🟡 Minor comments (10)
.eslintrc.js-18-24 (1)
18-24:⚠️ Potential issue | 🟡 MinorConfiguration inconsistency: files are both ignored and overridden.
packages/plugin-hardhat/test/*.jsappears in bothignorePatterns(line 23) and in theoverrides[1].filespattern (line 38). SinceignorePatternscauses ESLint to completely skip these files, the override configuration will never be applied to them.If the intent is to ignore these files due to ES2025 import attributes syntax, the override entry for
packages/plugin-hardhat/test/*.jsshould be removed. If you want to lint these files with special rules, remove them fromignorePatterns.🔧 Suggested fix (if intent is to ignore)
{ - files: ['ava.config.js', 'packages/plugin-hardhat/test/*.js', 'packages/plugin-hardhat/src/*.js'], + files: ['ava.config.js', 'packages/plugin-hardhat/src/*.js'], parserOptions: { ecmaVersion: 'latest', sourceType: 'module', },Also applies to: 37-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.eslintrc.js around lines 18 - 24, The ESLint config currently lists 'packages/plugin-hardhat/test/*.js' in ignorePatterns and also in overrides[1].files, so the override will never run; decide which behavior you want and remove the conflicting entry: either delete that glob from ignorePatterns so the override (overrides[1].files) can apply, or remove the pattern from overrides[1].files (or delete that entire override) if you truly want those files ignored; update the ignorePatterns/overrides array accordingly and keep the remaining intent-consistent entry.packages/plugin-hardhat/src/utils/validate-impl.ts-6-6 (1)
6-6:⚠️ Potential issue | 🟡 MinorClarify or address the "not working" comment.
The inline comment
// substitute this ( not working )on thegetImplementationAddressFromBeaconimport suggests a known issue. This should either be:
- Fixed before merging if it's a blocking issue
- Converted to a proper TODO with an issue reference if it's a known limitation
- Removed if it's a debug artifact
Leaving ambiguous "not working" comments can cause confusion for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/src/utils/validate-impl.ts` at line 6, The inline comment next to getImplementationAddressFromBeacon is ambiguous — either fix the underlying issue, convert it to a clear TODO with an issue/PR reference, or remove it; locate the import of getImplementationAddressFromBeacon in validate-impl.ts and either (A) remove the “( not working )” note and implement the correct behavior so the function works as intended, (B) replace the inline text with a proper TODO comment that includes an issue/PR number and a short description of the limitation, or (C) delete the comment if it was only a temporary debug artifact.packages/plugin-hardhat/src/deploy-beacon-proxy.ts-79-80 (1)
79-80:⚠️ Potential issue | 🟡 MinorRemove unnecessary
as anycast by importing the proper provider type.The
as anycast suppresses type checking at a critical validation point. Other files in this package (validate-upgrade.ts, verify-proxy.ts, prepare-upgrade.ts, force-import.ts) callisBeacon(provider, ...)without casting by importingEthereumProviderfromhardhat/types/providers. Apply the same approach here.Suggested fix
+import type { EthereumProvider } from 'hardhat/types/providers'; import type { HardhatRuntimeEnvironment } from 'hardhat/types/hre';- const isBeaconResult = await isBeacon(provider as any, beaconAddress); + const isBeaconResult = await isBeacon(provider, beaconAddress);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/src/deploy-beacon-proxy.ts` around lines 79 - 80, The code casts provider to any when calling isBeacon(provider as any, beaconAddress); remove that cast and import the correct provider type used elsewhere (EthereumProvider from hardhat/types/providers) so you can type provider properly; update the function/method signature or local variable declaration for provider to use EthereumProvider and call isBeacon(provider, beaconAddress) without any as any cast, mirroring the pattern in validate-upgrade.ts / verify-proxy.ts where isBeacon is invoked directly.packages/plugin-hardhat/test/constructor.js-55-56 (1)
55-56:⚠️ Potential issue | 🟡 MinorRemove
initializer: falsefrom theupgradeProxycalls.
initializeris not part of theUpgradeProxyOptionstype definition (it's only supported inDeployProxyOptions). Passing it toupgradeProxyis silently ignored, making these no-ops that create confusion about the API's capabilities.Proposed change
- const proxy2 = await upgrades.upgradeProxy(proxy1, WithConstructor, { constructorArgs: [17], initializer: false }); + const proxy2 = await upgrades.upgradeProxy(proxy1, WithConstructor, { constructorArgs: [17] }); ... - const proxy2 = await upgrades.upgradeProxy(proxy1, WithConstructor, { constructorArgs: [42], initializer: false }); + const proxy2 = await upgrades.upgradeProxy(proxy1, WithConstructor, { constructorArgs: [42] });Also applies to: 69-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/test/constructor.js` around lines 55 - 56, The review notes that passing initializer: false to upgrades.upgradeProxy is invalid and silently ignored; locate the upgradeProxy calls that pass the options object (e.g., upgrades.upgradeProxy(proxy1, WithConstructor, { constructorArgs: [17], initializer: false }) and similar at the other occurrence) and remove the initializer: false property from those options objects so only supported options (like constructorArgs if needed) are passed to upgradeProxy.packages/plugin-hardhat/src/defender/client.ts-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorUse a type-only import for
HardhatRuntimeEnvironment.At Line 3, this symbol is used only in type annotations (function parameters at lines 7 and 11). Using
import typeclarifies intent and aligns with TypeScript best practices for type-only imports.Suggested diff
-import { HardhatRuntimeEnvironment } from 'hardhat/types/hre'; +import type { HardhatRuntimeEnvironment } from 'hardhat/types/hre';Note: This pattern is used inconsistently across the codebase (13 similar imports found). Consider applying this change systematically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/src/defender/client.ts` at line 3, Replace the value import with a type-only import for HardhatRuntimeEnvironment: change the import to use "import type { HardhatRuntimeEnvironment } from 'hardhat/types/hre';" and update any usages that are only type annotations (the functions that accept the hre parameter) to rely on that type-only import; ensure no runtime references to HardhatRuntimeEnvironment remain so the import stays purely type-only.packages/plugin-hardhat/examples/BoxTransparent/package.json-14-14 (1)
14-14:⚠️ Potential issue | 🟡 MinorUnusual dependency specifier syntax.
The value
@openzeppelin/hardhat-upgrades@4.0.0-alpha.0repeats the package name. Typically, this would be just^4.0.0-alpha.0or4.0.0-alpha.0. While npm may resolve this correctly, it's unconventional and could cause confusion.🔧 Suggested fix
- "@openzeppelin/hardhat-upgrades": "@openzeppelin/hardhat-upgrades@4.0.0-alpha.0", + "@openzeppelin/hardhat-upgrades": "^4.0.0-alpha.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/examples/BoxTransparent/package.json` at line 14, The dependency specifier for "@openzeppelin/hardhat-upgrades" is written as "@openzeppelin/hardhat-upgrades@4.0.0-alpha.0", which repeats the package name; update the package.json dependency value for the "@openzeppelin/hardhat-upgrades" key to a conventional semver string such as "4.0.0-alpha.0" or a caret range like "^4.0.0-alpha.0" so the entry reads "@openzeppelin/hardhat-upgrades": "4.0.0-alpha.0" (or use the caret range) to avoid the unusual/name-included specifier.packages/plugin-hardhat/examples/BoxUUPS/package.json-14-14 (1)
14-14:⚠️ Potential issue | 🟡 MinorUnusual dependency version format.
The value
@openzeppelin/hardhat-upgrades@4.0.0-alpha.0includes the package name redundantly. Standard npm format is just the version string.Suggested fix
- "@openzeppelin/hardhat-upgrades": "@openzeppelin/hardhat-upgrades@4.0.0-alpha.0", + "@openzeppelin/hardhat-upgrades": "^4.0.0-alpha.0",Note: Using the full specifier works (npm interprets it as an alias), but the caret range format is more conventional and consistent with other dependencies in this file. Based on learnings, example dependencies should demonstrate minimum required versions for baseline compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/examples/BoxUUPS/package.json` at line 14, The dependency entry for "@openzeppelin/hardhat-upgrades" uses a full specifier "@openzeppelin/hardhat-upgrades@4.0.0-alpha.0" which is redundant and inconsistent with other deps; update the value to a conventional version string (e.g. "^4.0.0-alpha.0" or the minimum required version) for the "@openzeppelin/hardhat-upgrades" key so npm treats it as a normal version range rather than an alias.packages/plugin-hardhat/examples/BoxSolidityTests/hardhat.config.ts-4-6 (1)
4-6:⚠️ Potential issue | 🟡 MinorKeep test fs permissions aligned with
FOUNDRY_OUT.When
FOUNDRY_OUTis set externally to a non-default directory,fsPermissions.readDirectorystill points toartifacts/contracts, which can cause Solidity tests to fail on file access.Suggested fix
-if (!process.env.FOUNDRY_OUT) { - process.env.FOUNDRY_OUT = 'artifacts/contracts'; -} +const foundryOut = process.env.FOUNDRY_OUT ?? 'artifacts/contracts'; +process.env.FOUNDRY_OUT = foundryOut; @@ ffi: true, fsPermissions: { - readDirectory: ['artifacts/contracts'], + readDirectory: [foundryOut], },Also applies to: 24-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/examples/BoxSolidityTests/hardhat.config.ts` around lines 4 - 6, The fsPermissions.readDirectory default is hardcoded to 'artifacts/contracts' and doesn't respect an externally set FOUNDRY_OUT; update the initialization to derive the directory from process.env.FOUNDRY_OUT (fallback to 'artifacts/contracts') and use that variable wherever fsPermissions.readDirectory is assigned (refer to process.env.FOUNDRY_OUT and fsPermissions.readDirectory in this file, including the second occurrence around lines mentioned). Ensure both the initial assignment and the later usage read the same derived value so test file permissions align with an externally provided FOUNDRY_OUT.packages/plugin-hardhat/scripts/test.sh-15-31 (1)
15-31:⚠️ Potential issue | 🟡 MinorDon’t misclassify extensionless test paths as flags.
Current routing fails for valid path-style inputs (e.g., directories or path globs without explicit
.js/.tssuffix), and exits with a misleading “flags” error.Suggested fix
for arg in "$@"; do if [[ "$arg" == *.sol ]]; then sol_tests+=("$arg") elif [[ "$arg" == *.js ]] || [[ "$arg" == *.ts ]]; then js_tests+=("$arg") + elif [[ "$arg" != -* ]] && ([[ -e "$arg" ]] || [[ "$arg" == */* ]]); then + # Allow path-style test selectors (directories/globs-like paths) + js_tests+=("$arg") else # Collect flags and other arguments other_args+=("$arg") fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/scripts/test.sh` around lines 15 - 31, The script currently treats any arg without .sol/.js/.ts as a flag; change the parsing so only args that begin with '-' are classified as flags (other_args), and treat all non-flag args as test paths (push into sol_tests/js_tests or a unified tests array); update the loop that fills sol_tests/js_tests/other_args to first check if the arg starts with '-' (flag) then check file extensions (*.sol, *.js, *.ts) and otherwise treat it as a test path (add to js_tests or a new tests array), and update the final error condition to detect "flags provided but no test files" by checking other_args contains entries that start with '-' and that there are zero test paths in the test arrays.packages/plugin-hardhat/README.md-208-208 (1)
208-208:⚠️ Potential issue | 🟡 MinorMinor: Heading anchor mismatch with MIGRATION.md reference.
The heading here is
### Solidity tests, butMIGRATION.mdline 203 references./README.md#solidity-tests-hardhat-3. The anchor generated from### Solidity testswould be#solidity-tests, not#solidity-tests-hardhat-3.📝 Suggested fix
Either update the heading to match:
-### Solidity tests +### Solidity tests (Hardhat 3)Or update the reference in MIGRATION.md to
./README.md#solidity-tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-hardhat/README.md` at line 208, The README heading "### Solidity tests" doesn't match the anchor used in MIGRATION.md ("./README.md#solidity-tests-hardhat-3"); please make them consistent by either renaming the README heading to include the suffix (e.g., "### Solidity tests - Hardhat 3" or "### Solidity tests - hardhat-3") so it produces the "#solidity-tests-hardhat-3" anchor, or update the MIGRATION.md reference to "./README.md#solidity-tests" to match the current heading; adjust only the heading text in README.md or the single reference in MIGRATION.md accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/cli/validate/build-info-file.ts`:
- Around line 295-307: The code currently only checks selection['*'] for
storageLayout and therefore rejects files that requested storageLayout
per-contract; update the check to treat storageLayout as present if either
wildcard includes it or any explicit contract key in selection (keys other than
'' and '*') is an array that includes 'storageLayout' (e.g. compute
hasStorageLayout = wildcard.includes('storageLayout') ||
Object.keys(selection).some(k => k !== '' && k !== '*' &&
Array.isArray(selection[k]) && selection[k].includes('storageLayout'))), then
use hasStorageLayout in place of wildcard.includes('storageLayout') when
deciding to throw the ValidateCommandError (symbols: outputSelection, selection,
unnamed, wildcard, ValidateCommandError, PARTIAL_COMPILE_HELP,
buildInfoFilePath).
In `@packages/plugin-hardhat/contracts/foundry/GreeterV2.sol`:
- Around line 21-23: The resetGreeting function is currently exposed to any
caller through reinitializer(2); either restrict it with access control or make
it explicitly safe: add the onlyOwner modifier to resetGreeting (and ensure the
contract imports/uses Ownable or OwnableUpgradeable) so only the admin can call
it during migration via upgrade-and-call, or alternatively remove the modifier
and harden the function body so changing greeting by arbitrary users is
harmless; locate resetGreeting/reinitializer(2) in GreeterV2.sol and apply the
appropriate change (add onlyOwner and invoke via upgrade-and-call if this is a
migration step).
In `@packages/plugin-hardhat/src/hooks/solidity.ts`:
- Around line 174-182: The catch currently swallows any non-UpgradesError thrown
during makeNamespacedInput, trySanitizeNatSpec, or the second next() call and
silently sets namespacedOutput = undefined; change it so unexpected errors are
not silently ignored: import/identify UpgradesError from
'@openzeppelin/upgrades-core' as you already do, but for non-UpgradesError cases
either rethrow the error (or at minimum log it and rethrow) instead of silently
clearing namespacedOutput; update the catch block surrounding
makeNamespacedInput / trySanitizeNatSpec / next() so only UpgradesError is
rethrown as-is and all other errors are surfaced (e.g., via throw err after
logging) to avoid hiding compiler/runtime bugs while preserving existing
UpgradesError handling.
- Around line 227-253: The code hardcodes the subfolder 'contracts' when calling
findArtifactFiles which skips non-default layouts; update injectAstIntoArtifacts
to scan the provided artifactsDir root (or use the artifactPaths provided by
onCleanUpArtifacts if available) instead of path.join(artifactsDir,
'contracts'), so findArtifactFiles starts from artifactsDir (or iterate the
supplied artifactPaths) and then collect JSON artifact files into artifactFiles
for AST injection; adjust references to artifactFiles and findArtifactFiles in
injectAstIntoArtifacts accordingly.
In `@packages/plugin-hardhat/src/utils/artifacts.ts`:
- Around line 113-118: The code currently calls readJsonFile(buildInfoPath) and
readJsonFile(buildInfoOutputPath) which can throw if the paired split build-info
file was removed; wrap the reads in a try/catch (around the two reads or
individually) and treat any read failure for either buildInfoPath or
buildInfoOutputPath as a recoverable "no build info" by returning undefined;
also keep the existing guard that checks buildInfoOutput.output.contracts (the
buildInfoOutput variable) and return undefined if missing so callers can run
their fallback path.
In `@packages/plugin-hardhat/src/utils/factory.ts`:
- Around line 164-176: The warnOnHardhatDefender function currently uses await
import('@openzeppelin/hardhat-defender') which executes the module; change it to
use require.resolve('@openzeppelin/hardhat-defender') to detect presence without
running the package, then keep the dynamic import only for
'@openzeppelin/upgrades-core' to call logWarning; also tighten the catch in
warnOnHardhatDefender to only swallow module-not-found resolution errors (check
e.code for 'MODULE_NOT_FOUND' or 'ERR_MODULE_NOT_FOUND') and rethrow any other
errors so real runtime failures are not hidden.
In `@packages/plugin-hardhat/src/verify-proxy.ts`:
- Line 514: The call to callEtherscanApi is incorrectly using POST with a body
for the verifyproxycontract endpoint; update the call in verify-proxy.ts to use
the GET/query form by removing the options object (remove { method: 'POST',
body: params }) so the call becomes callEtherscanApi(etherscan, params), keeping
the existing etherscan and params variables unchanged.
In `@packages/plugin-hardhat/test/manifest-hh2-migration.js`:
- Around line 69-71: The test currently picks an implementation entry by
insertion order (const v1Key = Object.keys(data.impls)[0]; const v1 =
data.impls[v1Key]; and the similar block at lines ~110-114), which is
non-deterministic when prior impls exist; change the selection to locate the
manifest entry that matches the actual deployed implementation (find the key in
data.impls whose entry.address or other unique identifier equals the deployed
implementation's address/hash) and use that key (e.g., via
Object.entries(data.impls).find(...) to match by address) for all mutations so
the test binds to the correct impl instead of the first map entry.
In `@packages/plugin-hardhat/test/solidity/Upgrades.t.sol`:
- Around line 147-218: The negative tests (testValidateImplementation,
testValidateLayout, testValidateLayoutUpgradesFrom, testValidateNamespaced,
testValidateNamespacedUpgradesFrom, testValidateNamespacedNoReference) currently
use bare catch {} blocks which swallow all errors and can hide regressions;
update each try/catch that calls Invoker.validateImplementation or
Invoker.validateUpgrade (and the places calling Upgrades.validateUpgrade) to use
a two-branch catch: catch Error(string memory reason) {
assertTrue(bytes(reason).length > 0); } catch { fail(); } so the test asserts a
non-empty revert reason and fails for non-Error reverts.
In `@packages/plugin-hardhat/test/transparent-initializers.js`:
- Around line 4-6: The file uses a top-level await (connection = await
hre.network.connect()) before an ESM import, which is invalid; move the import
of upgradesFactory (import { upgrades as upgradesFactory } from
'@openzeppelin/hardhat-upgrades';) and any other import statements (e.g.,
ethers-related imports) to the top of the module before calling await
hre.network.connect(), ensuring all imports appear before any executable code
like the top-level await or usage of connection and { ethers }.
In `@packages/plugin-hardhat/test/transparent-transfer-admin-ownership-signer.js`:
- Around line 4-6: The import statement for upgradesFactory is placed after a
top-level await (the call to hre.network.connect), which violates ESM ordering;
move the import of "upgrades as upgradesFactory" from
'@openzeppelin/hardhat-upgrades' above the top-level await (i.e., before the
call to hre.network.connect/const connection and const { ethers } = connection)
so all static imports occur before any top-level await, or alternatively replace
the static import with a dynamic import where upgradesFactory is loaded after
connection if you intentionally need runtime loading.
In `@packages/plugin-hardhat/test/uups-initial-owner.js`:
- Around line 4-6: Move the ESM import so no top-level await precedes it: place
the import of "upgrades as upgradesFactory" from
'@openzeppelin/hardhat-upgrades' before the top-level await that calls
hre.network.connect() (the lines using connection and destructuring const {
ethers } = connection); ensure any other imports in this test file are similarly
ordered before the await to satisfy ESM import rules.
In `@packages/plugin-hardhat/test/uups-initializers.js`:
- Around line 4-6: Move the import of upgradesFactory so all import declarations
appear before any executable code: place the line importing "upgrades as
upgradesFactory" from '@openzeppelin/hardhat-upgrades' at the top of the module
before the calls to hre.network.connect(), and ensure the code that defines
connection, const { ethers } = connection, and any other runtime statements
follow the imports; update references to upgradesFactory, connection, and ethers
accordingly.
In `@packages/plugin-hardhat/test/uups-multi-compiler.js`:
- Around line 4-8: The file uses top-level await when calling
hre.network.connect() (connection, ethers) before static imports, causing ESM
import ordering errors; move the static imports (upgradesFactory from
'@openzeppelin/hardhat-upgrades', getVersion and getContractNameAndRunValidation
from '@openzeppelin/upgrades-core', and readValidations from
'../dist/utils/validations.js') to the very top of the module before any
await/usage (or alternatively change those imports to dynamic imports where
used), then keep the call to hre.network.connect() and const { ethers } =
connection after the imports so all static imports occur before top-level await.
---
Outside diff comments:
In `@packages/plugin-hardhat/src/defender/deploy.ts`:
- Around line 249-266: In getSpdxLicenseIdentifier, guard the new
buildInfoSourceName lookups: verify that
contractInfo.buildInfo.output.contracts[buildInfoSourceName] and the specific
contract entry (contractInfo.contractName) exist before accessing
compilerOutput, and likewise check that metadata.sources and
metadata.sources[buildInfoSourceName] exist before returning license; if any of
these are missing, throw a descriptive UpgradesError (same type already used)
explaining that the source name normalization mismatched and suggesting to
verify compiler metadata/inputSourceName settings so failures surface as a
targeted UpgradesError rather than a TypeError.
In `@packages/plugin-hardhat/test/import-with-deploy-v4.js`:
- Line 91: The test assertion is comparing a boolean to a string message;
replace the wrong call that currently passes the boolean expression as the first
argument to t.not. Locate the assertion referencing implAddrUpgraded2 and
implAddrUpgraded (currently written as t.not(implAddrUpgraded2 !==
implAddrUpgraded, implAddrUpgraded2)) and change it to assert the two addresses
are different by calling t.not(implAddrUpgraded2, implAddrUpgraded) (or the
equivalent AVA assertion that the actual implAddrUpgraded2 !==
implAddrUpgraded).
In `@packages/plugin-hardhat/test/solidity-overrides-storage.js`:
- Around line 28-31: The test uses AVA's t.throwsAsync incorrectly by passing
the expected error message as the third (message) argument; update the
t.throwsAsync call in the test function that calls upgrades.upgradeProxy(proxy,
GapV2_Bad) so the expectation is provided as the second argument (e.g., an
object or regex matching "Set __gap array to size 47") and the optional
assertion message as the third argument if needed; locate the t.throwsAsync
invocation in the 'use gap incorrectly' test that references GapV1, GapV2_Bad
and replace the undefined second argument with the correct expectation to assert
the thrown error text.
---
Minor comments:
In @.eslintrc.js:
- Around line 18-24: The ESLint config currently lists
'packages/plugin-hardhat/test/*.js' in ignorePatterns and also in
overrides[1].files, so the override will never run; decide which behavior you
want and remove the conflicting entry: either delete that glob from
ignorePatterns so the override (overrides[1].files) can apply, or remove the
pattern from overrides[1].files (or delete that entire override) if you truly
want those files ignored; update the ignorePatterns/overrides array accordingly
and keep the remaining intent-consistent entry.
In `@packages/plugin-hardhat/examples/BoxSolidityTests/hardhat.config.ts`:
- Around line 4-6: The fsPermissions.readDirectory default is hardcoded to
'artifacts/contracts' and doesn't respect an externally set FOUNDRY_OUT; update
the initialization to derive the directory from process.env.FOUNDRY_OUT
(fallback to 'artifacts/contracts') and use that variable wherever
fsPermissions.readDirectory is assigned (refer to process.env.FOUNDRY_OUT and
fsPermissions.readDirectory in this file, including the second occurrence around
lines mentioned). Ensure both the initial assignment and the later usage read
the same derived value so test file permissions align with an externally
provided FOUNDRY_OUT.
In `@packages/plugin-hardhat/examples/BoxTransparent/package.json`:
- Line 14: The dependency specifier for "@openzeppelin/hardhat-upgrades" is
written as "@openzeppelin/hardhat-upgrades@4.0.0-alpha.0", which repeats the
package name; update the package.json dependency value for the
"@openzeppelin/hardhat-upgrades" key to a conventional semver string such as
"4.0.0-alpha.0" or a caret range like "^4.0.0-alpha.0" so the entry reads
"@openzeppelin/hardhat-upgrades": "4.0.0-alpha.0" (or use the caret range) to
avoid the unusual/name-included specifier.
In `@packages/plugin-hardhat/examples/BoxUUPS/package.json`:
- Line 14: The dependency entry for "@openzeppelin/hardhat-upgrades" uses a full
specifier "@openzeppelin/hardhat-upgrades@4.0.0-alpha.0" which is redundant and
inconsistent with other deps; update the value to a conventional version string
(e.g. "^4.0.0-alpha.0" or the minimum required version) for the
"@openzeppelin/hardhat-upgrades" key so npm treats it as a normal version range
rather than an alias.
In `@packages/plugin-hardhat/README.md`:
- Line 208: The README heading "### Solidity tests" doesn't match the anchor
used in MIGRATION.md ("./README.md#solidity-tests-hardhat-3"); please make them
consistent by either renaming the README heading to include the suffix (e.g.,
"### Solidity tests - Hardhat 3" or "### Solidity tests - hardhat-3") so it
produces the "#solidity-tests-hardhat-3" anchor, or update the MIGRATION.md
reference to "./README.md#solidity-tests" to match the current heading; adjust
only the heading text in README.md or the single reference in MIGRATION.md
accordingly.
In `@packages/plugin-hardhat/scripts/test.sh`:
- Around line 15-31: The script currently treats any arg without .sol/.js/.ts as
a flag; change the parsing so only args that begin with '-' are classified as
flags (other_args), and treat all non-flag args as test paths (push into
sol_tests/js_tests or a unified tests array); update the loop that fills
sol_tests/js_tests/other_args to first check if the arg starts with '-' (flag)
then check file extensions (*.sol, *.js, *.ts) and otherwise treat it as a test
path (add to js_tests or a new tests array), and update the final error
condition to detect "flags provided but no test files" by checking other_args
contains entries that start with '-' and that there are zero test paths in the
test arrays.
In `@packages/plugin-hardhat/src/defender/client.ts`:
- Line 3: Replace the value import with a type-only import for
HardhatRuntimeEnvironment: change the import to use "import type {
HardhatRuntimeEnvironment } from 'hardhat/types/hre';" and update any usages
that are only type annotations (the functions that accept the hre parameter) to
rely on that type-only import; ensure no runtime references to
HardhatRuntimeEnvironment remain so the import stays purely type-only.
In `@packages/plugin-hardhat/src/deploy-beacon-proxy.ts`:
- Around line 79-80: The code casts provider to any when calling
isBeacon(provider as any, beaconAddress); remove that cast and import the
correct provider type used elsewhere (EthereumProvider from
hardhat/types/providers) so you can type provider properly; update the
function/method signature or local variable declaration for provider to use
EthereumProvider and call isBeacon(provider, beaconAddress) without any as any
cast, mirroring the pattern in validate-upgrade.ts / verify-proxy.ts where
isBeacon is invoked directly.
In `@packages/plugin-hardhat/src/utils/validate-impl.ts`:
- Line 6: The inline comment next to getImplementationAddressFromBeacon is
ambiguous — either fix the underlying issue, convert it to a clear TODO with an
issue/PR reference, or remove it; locate the import of
getImplementationAddressFromBeacon in validate-impl.ts and either (A) remove the
“( not working )” note and implement the correct behavior so the function works
as intended, (B) replace the inline text with a proper TODO comment that
includes an issue/PR number and a short description of the limitation, or (C)
delete the comment if it was only a temporary debug artifact.
In `@packages/plugin-hardhat/test/constructor.js`:
- Around line 55-56: The review notes that passing initializer: false to
upgrades.upgradeProxy is invalid and silently ignored; locate the upgradeProxy
calls that pass the options object (e.g., upgrades.upgradeProxy(proxy1,
WithConstructor, { constructorArgs: [17], initializer: false }) and similar at
the other occurrence) and remove the initializer: false property from those
options objects so only supported options (like constructorArgs if needed) are
passed to upgradeProxy.
---
Nitpick comments:
In `@packages/core/src/impl-address.ts`:
- Around line 22-24: The inline comment beside the provider parameter in
getImplementationAddressFromBeacon is vague ("// v2 may differ from v3");
replace it with a concise, specific note describing what differs (for example:
whether the provider type is from ethers v5 vs v6, or Hardhat's provider
shape/ethers provider compatibility, and which methods/return types the function
relies on). Update the comment to state exactly which provider/version
differences matter (e.g., RPC method names, returned object shape, or
presence/absence of send/request) so future maintainers know what to change if
switching provider versions.
In `@packages/plugin-hardhat/examples/BoxTransparent/scripts/1-deploy.ts`:
- Around line 4-16: The example opens a network connection with
hre.network.connect() but never closes it; update the async main() to close the
connection (call connection.disconnect() or connection.close() depending on the
connection API) after deployment, and wrap the deployment flow (including
ethers.getContractFactory, upgrades(hre, connection), deployProxy and
box.waitForDeployment) in a try/finally so that connection.disconnect() is
always called even on error.
In `@packages/plugin-hardhat/examples/BoxTransparent/scripts/2-upgrade.ts`:
- Around line 4-10: The script currently hardcodes PROXY_ADDRESS in 2-upgrade.ts
which requires manual file edits; change PROXY_ADDRESS to read from
process.env.PROXY_ADDRESS (e.g., const PROXY_ADDRESS = process.env.PROXY_ADDRESS
|| '0x...') in the module scope, then in main() validate that PROXY_ADDRESS is
set and not the placeholder (trim/normalize the value) and exit with the same
error message if it's missing; update any references in main() and related
upgrade logic to use the new PROXY_ADDRESS variable so the script can be driven
by environment variables for repeatable runs.
In `@packages/plugin-hardhat/examples/BoxTransparent/test/upgrade.test.ts`:
- Around line 5-9: The cleanup callback passed to test.after.always should be
async and explicitly await connection.close() to make the returned Promise clear
(change test.after.always(() => connection.close()) to an async callback that
awaits connection.close()); also remove the loose cast const { ethers } =
connection as any and replace it with the appropriate Hardhat 3 connection type
for `connection` (or import the proper Connection/Network types) so you retain
type safety when destructuring ethers from the connection.
In `@packages/plugin-hardhat/examples/BoxUUPS/contracts/BoxV2.sol`:
- Around line 8-20: BoxV2 defines a fresh initializer
initialize(address,uint256) which can be misleading since the proxy was already
initialized by V1; instead reuse the V1 initializer or, if V2 truly needs extra
setup, mark the new initializer as a reinitializer with a new version. Update
BoxV2 to inherit the original Box (v1) and remove or replace
initialize(address,uint256) so you either call the inherited initializer
(reusing __Ownable_init or whatever v1 exposes) or change the function to
reinitializer(N) and only perform the additional V2-specific setup (refer to the
BoxV2 contract, its initialize function, and the v1 initializer/constructor
symbols to guide the change).
In `@packages/plugin-hardhat/examples/BoxUUPS/scripts/1-deploy.ts`:
- Around line 4-17: The script opens a network connection via
hre.network.connect() in main but never closes it; wrap the deployment logic in
a try/finally (or ensure a finally block) and call await connection.disconnect()
in the finally so the connection is explicitly closed after use (reference the
main function, the connection variable returned by hre.network.connect(), and
use connection.disconnect()).
In `@packages/plugin-hardhat/examples/BoxUUPS/test/upgrade.test.ts`:
- Around line 5-7: Replace the unsafe cast "connection as any" by giving the
connection a proper type or documenting it: annotate the variable returned by
hre.network.connect() with the expected connection shape (including the ethers
property and any methods used by upgrades) or add a one-line comment above the
line explaining why a cast is needed; update the destructuring to use that typed
variable (e.g., const { ethers } = connectionTyped) and ensure usages of
upgrades(hre, connection) reference the typed connection so IDEs and readers can
see the expected API surface (reference symbols: connection,
hre.network.connect(), ethers, upgrades, upgradesApi).
In `@packages/plugin-hardhat/scripts/assert-upgrades-core-resolution.sh`:
- Around line 35-37: The inline Node.js one-liner used to set REQ_VERSION
(currently passed to node with -e and reading VERSIONS_SOL) should be extracted
for readability: create a separate script (e.g., extract-upgrades-core.js) that
accepts the VERSIONS_SOL path, contains the fs.readFileSync + regex logic and
prints the captured group, or convert the inline code to a heredoc that pipes
into node; then replace the current node -e invocation in the REQ_VERSION
assignment to call that script (or heredoc) with "$VERSIONS_SOL" so the behavior
of extracting UPGRADES_CORE remains identical while keeping the code
maintainable.
In `@packages/plugin-hardhat/src/defender/utils.ts`:
- Around line 159-173: The function getRemoteDeployment declares an unused
parameter _connection, which is confusing; either remove the _connection
parameter from getRemoteDeployment's signature (and any upstream callers) to
keep the API clean, or if you plan to use it later, add a short explanatory
comment above the function (e.g., "// _connection reserved for future
network-scoped lookups") and keep the leading underscore to indicate
intentionally unused variable; update the getRemoteDeployment declaration and
any referenced call sites accordingly so signatures remain consistent.
In `@packages/plugin-hardhat/src/deploy-beacon.ts`:
- Around line 5-13: Deployment, DeployBeaconOptions, and DeployTransaction are
only used as types and must be imported as type-only to avoid runtime ESM import
issues; change the import of Deployment from '@openzeppelin/upgrades-core' to
use "import type { Deployment } ..." and split the second import so
DeployBeaconOptions and DeployTransaction are imported with "import type { ...
}" while keeping deploy, getUpgradeableBeaconFactory, deployBeaconImpl, and
deploy as regular value imports; update the import statements around the symbols
Deployment, DeployBeaconOptions, DeployTransaction, deploy,
getUpgradeableBeaconFactory, and deployBeaconImpl accordingly.
In `@packages/plugin-hardhat/src/deploy-implementation.ts`:
- Around line 1-2: The import for HardhatRuntimeEnvironment should be type-only
because it's only used in a parameter type annotation; update the top import to
use the TypeScript type-only form (change the import to "import type {
HardhatRuntimeEnvironment } from 'hardhat/types/hre';") so it matches the
existing type-only import for NetworkConnection and avoids tree-shaking/runtime
issues where HardhatRuntimeEnvironment is not needed at runtime.
In `@packages/plugin-hardhat/src/upgrade-proxy.ts`:
- Around line 1-23: The listed imports that are used only as types
(HardhatRuntimeEnvironment, UpgradeProxyOptions, ContractAddressOrInstance,
ContractTypeOfFactory) should be changed to use TypeScript's type-only import
syntax (import type ...) so ESM/strict TS treats them as types; update the
import statements that currently import these symbols (from 'hardhat/types/hre',
'./utils/index.js', and './type-extensions.js') to use "import type" while
leaving runtime imports (ethers, debug, getAdminAddress, deployProxyImpl,
attach, attachITransparentUpgradeableProxyV4/V5, attachProxyAdminV4/V5, etc.)
unchanged.
In `@packages/plugin-hardhat/src/utils/deploy.ts`:
- Around line 23-25: The comment "we'll handle connection internally for now"
next to the opts?.useDefenderDeploy branch is ambiguous; update the comment near
the defenderDeploy call to either remove "for now" if this is intended behavior
long-term or replace it with a TODO that explains expected future changes and
rationale (e.g., "TODO: move connection handling out of defenderDeploy when X is
implemented") so it's clear whether defenderDeploy(hre, factory, opts, ...args)
is temporary or permanent.
In `@packages/plugin-hardhat/src/utils/index.ts`:
- Line 9: Remove the transient inline review artifact comment from the export
line so the source remains clean: in the export statement that re-exports the
UpgradeOptions type (export type { UpgradeOptions } from './options.js'), delete
the trailing comment "// ✓ Correct" leaving only the export statement.
In `@packages/plugin-hardhat/src/validate-upgrade.ts`:
- Around line 1-19: Convert the three type-only symbols to type-only imports:
change the import of HardhatRuntimeEnvironment to an "import type" from
'hardhat/types/hre', change ContractAddressOrInstance to an "import type" from
'./utils/index.js', and change ValidationOptions to an "import type" from
'@openzeppelin/upgrades-core'; leave all runtime imports (e.g., ContractFactory,
getBeaconAddress, validateImpl, getDeployData) as regular imports so only the
purely type symbols (HardhatRuntimeEnvironment, ContractAddressOrInstance,
ValidationOptions) use type-only import syntax.
In `@packages/plugin-hardhat/test/beacon-000-externally-deployed.js`:
- Around line 33-47: Remove the debug console.log calls that print addresses and
the upgrade error message in the test around the Greeter/Beacon flow: delete the
console.log('Deployed Greeter at:', await greeter.getAddress()),
console.log('Deployed Beacon at:', await beacon.getAddress()), and
console.log('Upgrade error message:', e.message) so the test uses
Beacon.deploy(await greeter.getAddress()), awaits beacon.waitForDeployment(),
and still runs upgrades.upgradeBeacon(await beacon.getAddress(), GreeterV2) and
the existing catch assertion that e.message.includes(IS_NOT_REGISTERED) without
extra logging.
In `@packages/plugin-hardhat/test/beacon-initializers.js`:
- Around line 1-9: Move the OpenZeppelin upgrades import to be grouped with the
other top-level imports: relocate "import { upgrades as upgradesFactory } from
'@openzeppelin/hardhat-upgrades';" up with "import test from 'ava';" and "import
hre from 'hardhat';" so all imports are declared together; ensure references to
upgradesFactory, connection, ethers and the declared "upgrades" variable remain
unchanged after reordering.
In `@packages/plugin-hardhat/test/beacon-linked-libraries.js`:
- Around line 114-119: The loop that applies fixups uses the deprecated substr()
on bytecode (inside the for...of over fixups) — replace bytecode.substr(...) and
bytecode.substr(...) with a non-deprecated alternative (e.g.,
bytecode.substring(...) or bytecode.slice(...)) and adjust indices accordingly
so the concatenation using 2 + fixup.start * 2 and 2 + (fixup.start +
fixup.length) * 2 still produces the correct slices; update both occurrences
near the use of variables bytecode, addr, and fixup in that block.
In `@packages/plugin-hardhat/test/defender-contract-instance.js`:
- Around line 78-90: The test currently stubs hre.network.connect to create a
second connection; instead reuse the existing file-level connection and stub its
provider directly: remove the sinon.stub(hre.network, 'connect') block and
replace it by stubbing connection.ethers.provider.getTransaction (capture
originalGetTransaction via
connection.ethers.provider.getTransaction.bind(connection.ethers.provider)) and
.callsFake to return second.deploymentTransaction() when hash ===
second.deploymentTransaction().hash and otherwise call
originalGetTransaction(hash); keep references to hre.network.connect,
connection, originalGetTransaction, second.deploymentTransaction, and sinon.stub
to locate where to change.
In `@packages/plugin-hardhat/test/defender-deploy.js`:
- Around line 129-145: The test currently regenerates the artifactPayload by
calling getCombinedBuildInfo(...) and using JSON.stringify(buildInfo) inside the
sinon.assert.calledWithExactly assertion, which masks merge regressions; update
the test in packages/plugin-hardhat/test/defender-deploy.js to stop using
getCombinedBuildInfo for the expected value and instead use a fixed fixture or
hard-coded raw JSON for artifactPayload (or explicitly assert only the critical
merged fields such as metadata, bytecode, and libraries), and apply the same
change to the other repeated assertions in this file so the expected payload is
decoupled from defenderDeploy's merge logic.
In `@packages/plugin-hardhat/test/defender-utils.js`:
- Around line 39-42: Duplicate test fixtures for fakeConnection should be
replaced with a small builder to avoid repetition: add a helper function (e.g.,
buildFakeConnection or makeFakeConnection) in
packages/plugin-hardhat/test/defender-utils.js that returns { networkConfig: {
chainId: 1 }, ethers: { provider: {} } } and accepts an optional overrides param
to merge custom fields; replace every inline fakeConnection object with calls to
this builder (and export it if other test modules import it) so future signature
changes only require updating the builder.
In `@packages/plugin-hardhat/test/infer-proxy-kind.js`:
- Line 24: Remove the unused local variable by deleting the line that assigns
signer from ethers.provider.getSigner(); specifically remove the declaration
"const signer = await ethers.provider.getSigner();" (or replace it with a
meaningful use of signer if intended) so there are no unused variables left in
the test.
In `@packages/plugin-hardhat/test/propose-upgrade-with-approval-beacon.js`:
- Line 6: Remove the commented-out development artifact imports and lines
referencing defenderFactory in the test file (the commented import "// import {
defender as defenderFactory } from '@openzeppelin/hardhat-upgrades';" and any
other commented lines around the propose-upgrade-with-approval-beacon test such
as the blocks at lines referenced 13-14 and 21), leaving only active test code
(e.g., keep functions like the test case and any real imports like
upgrades/beacon usage) so the file contains no dead commented code related to
defenderFactory.
In `@packages/plugin-hardhat/test/propose-upgrade-with-approval-transparent.js`:
- Around line 45-61: The test duplicates the same esmock mock objects for
'../dist/defender/client.js' (getDeployClient/getNetworkClient ->
t.context.fakeDefenderClient) and '../dist/utils/deploy.js' (deploy ->
mockDeploy); extract these into a single sharedMocks constant (or variable) and
reference that variable in both esmock call sites instead of repeating the
object literal so the mocks for getDeployClient/getNetworkClient and deploy are
declared once and reused.
In `@packages/plugin-hardhat/test/propose-upgrade-with-approval-use-deployed.js`:
- Around line 1-9: Move the top-level await call after the static imports:
currently const connection = await hre.network.connect() (and const { ethers } =
connection) appears before other static imports like import test, import hre,
import { upgrades as upgradesFactory }, import sinon, import esmock, and import
{ mockDeploy }; reorder so all static import statements are grouped at the very
top, then call await hre.network.connect() (or perform the network connect
inside a test/setup hook) to obtain connection and ethers—update references to
connection/ethers accordingly (symbols: hre, connection, ethers,
upgradesFactory, sinon, esmock, mockDeploy).
In `@packages/plugin-hardhat/test/solidity-overrides-storage.js`:
- Around line 1-6: The static import "import { upgrades as upgradesFactory }
from '@openzeppelin/hardhat-upgrades'" must be moved above any module-level
await to avoid top-level await before static imports; reorder so all imports
(including the upgradesFactory import) appear before the call to "await
hre.network.connect()" and before creating "connection" and "ethers" so the file
imports are all static imports first and only then perform the module-level
await and subsequent "const connection = await hre.network.connect()" and
destructure "ethers".
In `@packages/plugin-hardhat/test/solidity/Upgrades.t.sol`:
- Around line 251-273: The test testWithConstructor contains debug-only
filesystem probes and logging (vm.exists calls and multiple console.log
statements) which should be removed; edit the function to delete the
vm.exists("hardhat.config.js") and vm.exists("hardhat.config.ts") calls and the
local booleans configJsExists/configTsExists, and remove all console.log(...)
lines (including the "before/after Upgrades.deployTransparentProxy" logs),
leaving the Options opts setup (opts.constructorData = abi.encode(123)), the
Upgrades.deployTransparentProxy call, and the final asserts
(WithConstructor(proxy).a() and .b()) intact so the test only asserts contract
state.
In `@packages/plugin-hardhat/test/timeout.js`:
- Around line 42-44: Capture the boolean result returned by
provider.send('evm_revert', [t.context.snapshotId]) and assert it is truthy to
explicitly confirm the snapshot revert succeeded; update the teardown in
timeout.js to store the response from provider.send and add an assertion (e.g.,
using the existing test assertion utility like t.ok / t.true or
assert.strictEqual(..., true)) referencing provider.send, 'evm_revert', and
t.context.snapshotId so the test fails clearly if revert did not succeed.
In `@packages/plugin-hardhat/test/transparent-deploy-overload.js`:
- Around line 1-9: The file mixes a runtime extraction "const { ethers } =
connection;" into the top of the file instead of grouping module-level imports;
move the ethers extraction up with the other imports by obtaining ethers from
the Hardhat import (e.g., use const { ethers } = hre or import ethers alongside
hre before any await usage) and remove the standalone "const { ethers } =
connection;" statement so imports (test, hre, ethers, upgradesFactory) follow
the same grouped pattern; locate symbols connection, ethers, hre, and
upgradesFactory to update accordingly.
In `@packages/plugin-hardhat/test/transparent-happy-path-with-call.js`:
- Around line 8-10: Remove the duplicate JSDoc type annotation above the
upgrades variable: keep a single line with /** `@type`
{import('@openzeppelin/hardhat-upgrades').HardhatUpgrades} */ and delete the
repeated one so the variable declaration let upgrades; remains annotated once;
locate the two identical comments adjacent to the upgrades declaration and
remove one to improve readability.
In `@packages/plugin-hardhat/test/transparent-happy-path-with-enums.js`:
- Around line 1-9: Move the import for "upgradesFactory" to the top with the
other imports so all module imports are grouped before any top-level awaits;
specifically, place the "import { upgrades as upgradesFactory } from
'@openzeppelin/hardhat-upgrades';" alongside "import test from 'ava';" and
"import hre from 'hardhat';" so the top-level await that calls "await
hre.network.connect()" and the destructuring of "{ ethers }" remain after
imports, keeping the declaration "let upgrades;" unchanged.
In `@packages/plugin-hardhat/test/transparent-happy-path.js`:
- Around line 1-9: Move the out-of-place import so all imports are grouped at
the top: bring "import { upgrades as upgradesFactory } from
'@openzeppelin/hardhat-upgrades';" up with the other imports (near "import test
from 'ava';" and "import hre from 'hardhat';"), keep the await call to
"hre.network.connect()" and the use of "connection" and "{ ethers }" after
imports, and ensure the existing "let upgrades;" declaration remains below so
runtime code executes only after module imports.
In `@packages/plugin-hardhat/test/transparent-linked-libraries.js`:
- Around line 1-10: The file mixes an import with a top-level await: move the
import "const { upgrades as upgradesFactory } from
'@openzeppelin/hardhat-upgrades';" (and any other imports like ethers/artifacts
declarations) to the top of the module so all imports are grouped before the
top-level await that creates "connection" (referencing hre.network.connect and
const { ethers } = connection), ensuring imports (upgradesFactory, artifacts)
are declared above the await and the "upgrades" declaration stays as-is.
In
`@packages/plugin-hardhat/test/transparent-transfer-admin-ownership-happy-path.js`:
- Around line 1-6: Move the static import "import { upgrades as upgradesFactory
} from '@openzeppelin/hardhat-upgrades';" above the module-level await so all
static imports occur before any top-level await; specifically, reorder so the
import of upgradesFactory comes before the line "const connection = await
hre.network.connect();" (which uses "connection" and "hre.network.connect") and
keep the runtime destructuring "const { ethers } = connection;" after the await.
In `@packages/plugin-hardhat/test/transparent-transfer-admin-ownership-signer.js`:
- Line 31: The inline comment at the end of the line that calls
ethers.getContractAt (const admin = await ethers.getContractAt(OWNABLE_ABI,
adminAddress);) is in Portuguese; remove it or replace it with an English
comment that explains the change (e.g., "changed from hre.ethers to ethers") so
the codebase uses English-only comments for consistency and maintainability;
update the comment near the admin/OWNABLE_ABI/adminAddress usage accordingly.
In
`@packages/plugin-hardhat/test/transparent-transfer-admin-ownership-wrong-signer.js`:
- Around line 1-9: The import ordering is inconsistent: move the named import
"import { upgrades as upgradesFactory } from '@openzeppelin/hardhat-upgrades';"
so it appears with the other top-level imports (alongside "import test from
'ava';" and "import hre from 'hardhat';") before any top-level await or runtime
code such as "const connection = await hre.network.connect();" and the
destructuring "const { ethers } = connection;"; ensure the declaration "let
upgrades;" remains after imports if needed but before use.
In `@packages/plugin-hardhat/test/transparent-upgrade-storage.js`:
- Around line 1-9: Move the lone import into the grouped import block at the
top: place "import { upgrades as upgradesFactory } from
'@openzeppelin/hardhat-upgrades';" alongside the other imports before the
top-level "const connection = await hre.network.connect();" so all imports are
contiguous; keep the top-level await and "const { ethers } = connection;" as-is
and retain the "let upgrades" declaration below the imports.
In `@packages/plugin-hardhat/test/uups-custom-proxy.js`:
- Around line 18-20: Inconsistent contract factory naming: update the
getContractFactory call that currently uses 'GreeterV3Proxiable' to the
fully-qualified artifact path like the others; change the call using
getContractFactory for GreeterV3 to use
"contracts/GreeterV3.sol:GreeterV3Proxiable" so it matches the pattern used for
t.context.Greeter and t.context.GreeterV2.
In `@packages/plugin-hardhat/test/uups-happy-path-with-library.js`:
- Around line 1-6: Move the static import of upgradesFactory before any
module-level await to follow conventional import ordering: place "import {
upgrades as upgradesFactory } from '@openzeppelin/hardhat-upgrades';" above the
line that does "const connection = await hre.network.connect();" so that all
top-level static imports (including the upgradesFactory import and other imports
like test and hre) appear before the module-level await and any runtime code;
update the file so "connection" and "ethers" are obtained after those imports.
In `@packages/plugin-hardhat/test/uups-initial-owner.js`:
- Line 24: The test declares an unused variable signer via "const signer = await
ethers.provider.getSigner();"—remove that declaration or use it where intended;
update the test in the same block (remove the signer line) if the test relies on
default signers elsewhere, or replace uses of ethers.provider.getSigner() with
signer where you meant to use an explicit signer (e.g., in calls to
deployer/contract interactions) and ensure any references use the declared
variable name (signer) so it is no longer unused.
In `@packages/plugin-hardhat/test/uups-multi-compiler.js`:
- Around line 10-11: The _upgrades variable declared at top-level (let
_upgrades) is never used after being initialized in the test.before hook; either
remove the unused declaration and the initialization in the test.before block,
or keep it but add an explicit comment above the declaration and in test.before
stating that the import/initialization is performed only for its side effects
(e.g., to register plugins) so reviewers know it is intentional; reference the
top-level symbol _upgrades and the test.before hook where it is set to locate
the code to change.
In `@packages/plugin-hardhat/test/uups-unknown-upgrade-interface.js`:
- Around line 11-21: Update the existing TODO comment "TODO: Debug logging
verification removed during Hardhat 3 migration" to include a tracking issue
reference (issue URL or GH-issue number) that documents reintroducing
injectable/testable debug logging and log-level assertions; create the tracking
issue in your repo describing the missing test seam and desired follow-up, then
append the issue link/ID and a short one-line note to that TODO so future
maintainers can find the follow-up work easily.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Member
Author
|
@SocketSecurity ignore-all |
This was referenced Apr 24, 2026
Member
Author
Hardhat 3 validation-safety invariantThe key invariant for this PR is:
At a high level, the validation path remains:
Hardhat 3 changes how artifacts and build-info are packaged, but not the underlying solc AST/storage-layout semantics. The storage-layout comparator continues to make compatibility decisions from solc layout fields such as labels, types, slots, and namespaces, not from Hardhat-specific artifact paths. HH3-specific failure modesHardhat 3 split build-info Hardhat 3 writes build-info as a main Storage layout output Core rejects build-info that does not request HH3 source name remapping Hardhat 3 may use canonical paths like Validation cache freshness The plugin overrides both Namespaced storage validation The plugin modifies the solc input to include detailed storage layouts for namespaced variables, then compiles that modified input for namespaced storage validation. Unexpected errors in that path propagate. Solidity compile errors from the modified namespaced input are handled according to the existing HH2 -> HH3 manifest transition Manifests from HH2 deployments can contain persisted storage-layout Solidity tests / Foundry path Hardhat 3 Solidity tests run through Assumptions to sanity-check
High-value code paths for context:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds Hardhat 3 support to
@openzeppelin/hardhat-upgrades(major version) and@openzeppelin/upgrades-core(backwards-compatible so that it supports Hardhat 3, Hardhat 2, and Foundry).Changes
@openzeppelin/hardhat-upgrades— Hardhat 3 migration. ESM-only, new hooks-based plugin architecture, async factory functions (upgrades(hre, connection)/defender(hre, connection)),ethersfrom the network connection. Migration guide:packages/plugin-hardhat/MIGRATION.md. Full release notes:.changeset/migrate-to-hardhat-3.md.@openzeppelin/upgrades-core— Hardhat 3 build-info format support. Backwards compatible across HH2, HH3, and Foundry. Full release notes:.changeset/support-hardhat-3-build-info.md.packages/plugin-hardhat/examples/(Transparent, UUPS, Solidity tests).Related PRs
@openzeppelin/foundry-upgradesto be delivered in Foundry Upgrades PR Support use in Hardhat 3 solidity tests openzeppelin-foundry-upgrades#121Notes
masterwill be for Hardhat 3. If we need to do maintenance releases for Hardhat 2 in the future, we should branch from a previous version tag for Hardhat 2.docs/will not be updated. That folder is no longer needed since the move to the new docs site. We should plan to delete thedocs/folder from this project later after we confirm the intended workflow for propagating Foundry Upgrades API docs.@openzeppelin/foundry-upgradesdevDependency pin is^0.4.1-alpha.0(caret). Picks up the foundry-upgrades stable release automatically when it becomes available; release order between this repo and foundry-upgrades doesn't matter.Fixes #1191
Summary by CodeRabbit
Release Notes
Breaking Changes
hre.upgrades.New Features
Documentation