Conversation
Hardhat v3 Transpiler support
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.
|
WalkthroughEl cambio migra la configuración, helpers y suite de pruebas a Hardhat 3. Se sustituye la configuración JavaScript por Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/helpers/erc7739.js (1)
42-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the connected signer from
ERC7739Signer.connect
test/helpers/erc7739.jshasconnect(provider)callingthis.#signer.connect(provider)but not returning it, so the method returnsundefinedand breaksAbstractSigner/signer chaining expectations (ethers’connectreturns a connectedSigner).Proposed fix
connect(provider) { - this.#signer.connect(provider); + return new ERC7739Signer(this.#signer.connect(provider), this.#domain); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/helpers/erc7739.js` around lines 42 - 44, The connect(provider) method on ERC7739Signer currently calls this.#signer.connect(provider) but does not return the connected signer; update ERC7739Signer.connect to return the connected signer (i.e., return the result of this.#signer.connect(provider)) so it matches ethers' Signer.connect chaining expectations and preserves AbstractSigner behavior. Ensure you reference the private field `#signer` and the connect method on ERC7739Signer when making the change.test/helpers/signers.js (1)
194-196:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix deterministic signer ordering comparator in
MultiERC7913SigningKey.
test/helpers/signers.js:195subtracts twoethers.keccak256(...)hex strings, forcing JS to coerce them toNumber(53-bit precision). This can produce incorrect/unstable ordering versus Solidity-stylebytes32ordering. Compare hashes asbigintinstead.Proposed fix
- this.#signers = signers.sort( - (s1, s2) => ethers.keccak256(s1.bytes ?? s1.address) - ethers.keccak256(s2.bytes ?? s2.address), - ); + this.#signers = signers.sort((s1, s2) => { + const h1 = ethers.toBigInt(ethers.keccak256(s1.bytes ?? s1.address)); + const h2 = ethers.toBigInt(ethers.keccak256(s2.bytes ?? s2.address)); + return h1 < h2 ? -1 : h1 > h2 ? 1 : 0; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/helpers/signers.js` around lines 194 - 196, The comparator used when assigning this.#signers in MultiERC7913SigningKey converts ethers.keccak256(...) hex strings to Numbers by subtraction, causing unstable ordering; change the comparator to parse each keccak256 hex string into a BigInt (e.g. BigInt(hash)) and compare those BigInt values (return -1/0/1 or the difference as a Number derived from the BigInt comparison) to produce a deterministic bytes32 ordering for the signers.sort call.
🧹 Nitpick comments (8)
test/helpers/access-manager.js (1)
50-55: ⚡ Quick winUpdate
prepareOperationcontext contract in docs.The
@requirescontract is stale: this function now relies onthis.helpers.time(Line 54-Line 55), but that dependency is undocumented.Proposed doc fix
/** - * `@requires` this.{manager, caller, target, calldata} + * `@requires` this.helpers.time */ export async function prepareOperation(manager, { caller, target, calldata, delay }) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/helpers/access-manager.js` around lines 50 - 55, Update the JSDoc `@requires` for prepareOperation to mention the time helper dependency: the function prepareOperation(manager, { caller, target, calldata, delay }) now uses this.helpers.time (calls timestamp() and increaseTo.timestamp()), so add this.helpers.time to the `@requires` contract alongside this.manager, this.caller, this.target, and this.calldata to document the time helper dependency.hardhat/hardhat-oz-contracts-helpers/hook-handlers/network.ts (1)
17-33: ⚡ Quick winMerge existing
connection.helpersinstead of overwriting it.
network.tssetshelpersviaObject.assign(connection, { helpers: { ... } }), which replaces anyconnection.helpersalready present. This repo’shardhat-oz-contracts-helperspackage only writeshelpersfrom this handler (no other in-packagenewConnection/helpersmutations found), but preserving existing entries is safer for hook/plugin composition.♻️ Proposed change
export default async (): Promise<Partial<NetworkHooks>> => ({ newConnection: async <ChainTypeT extends ChainType | string>( context: HookContext, next: (nextContext: HookContext) => Promise<NetworkConnection<ChainTypeT>>, ): Promise<NetworkConnection<ChainTypeT>> => - next(context).then(async connection => - Object.assign(connection, { + next(context).then(async connection => { + const existingHelpers = (connection as NetworkConnection<ChainTypeT> & { helpers?: object }).helpers ?? {}; + return Object.assign(connection, { helpers: { + ...existingHelpers, chain: await getLocalChain(connection.provider), impersonate: impersonate(connection), storage: { getSlot: getSlot(connection), getAddressInSlot: getAddressInSlot(connection), setSlot: setSlot(connection), }, time: { clock: clock(connection), clockFromReceipt: clockFromReceipt(connection), increaseBy: increaseBy(connection), increaseTo: increaseTo(connection), duration: duration(connection), }, }, - }), - ), + }); + }), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hardhat/hardhat-oz-contracts-helpers/hook-handlers/network.ts` around lines 17 - 33, The current Object.assign call overwrites any existing connection.helpers; change it to merge with existing helpers instead: read the existing connection.helpers and merge its entries with the new ones you construct (including chain via getLocalChain(connection.provider), impersonate(connection), storage helpers getSlot/getAddressInSlot/setSlot, and time helpers clock/clockFromReceipt/increaseBy/increaseTo/duration) so you preserve prior keys; update the Object.assign usage around connection.helpers (or compute helpers = { ...connection.helpers, ...newHelpers } before assigning) to ensure additive behavior without removing existing helper entries.test/metatx/ERC2771Forwarder.test.js (1)
148-163: ⚡ Quick winAdd explicit no-balance-change assertions on revert paths.
These OOG/revert-path tests should also assert that no unintended ether transfer happened (e.g.,
to.not.changeEtherBalances(...)) in addition to revert and gas checks.Based on learnings: In tests that exercise revert semantics, use negative balance assertions to verify no ether balance changes occurred.
Also applies to: 165-193, 347-367, 369-398
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/metatx/ERC2771Forwarder.test.js` around lines 148 - 163, Add explicit assertions that no ether balances changed when the calls revert: in the 'bubbles out of gas' test (the it block invoking this.forgeRequest with this.receiver.interface.encodeFunctionData('mockFunctionOutOfGas') and executing via this.forwarder.execute) wrap the failing call in a balance-check assertion to assert no balances changed (e.g., use the test helper expect(...).to.not.changeEtherBalances or equivalent) for the relayer/sender/receiver addresses; apply the same pattern to the other OOG/revert-path tests mentioned (the blocks at 165-193, 347-367, 369-398) to ensure each revert path includes a no-balance-change assertion alongside the revert and gas checks.test/helpers/storage.js (1)
36-40: 💤 Low valueSilent error swallowing may hide unexpected failures.
The
.then(..., () => offset)pattern catches all errors fromreadArtifact, not just "artifact not found". IfreadArtifactfails for another reason (e.g., malformed artifact JSON, filesystem permission error), it will silently returnoffsetinstead of surfacing the actual problem.Consider narrowing the catch to artifact-not-found scenarios if the API provides a specific error type.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/helpers/storage.js` around lines 36 - 40, The current upgradeableSlot helper swallows any readArtifact error by using the second .then handler; change this to explicitly handle only the "artifact not found" case and rethrow other errors. Replace the dual-argument .then with a single .then + .catch on artifacts.readArtifact in the upgradeableSlot function, and in the .catch inspect the thrown error (e.g., check a specific error.code or error.message pattern like /artifact not found/i) to return offset only for that case and rethrow the error for all other failures so unexpected issues (malformed JSON, permission errors) surface.hardhat/hardhat-transpiler/tasks/transpile.ts (2)
20-23: 💤 Low valueMinor typo in comment.
"key the keys intact" should be "keep the keys intact".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hardhat/hardhat-transpiler/tasks/transpile.ts` around lines 20 - 23, The comment above function transformKeys contains a typo ("key the keys intact"); update the comment to read "keep the keys intact" so it accurately describes the function behavior; locate the comment immediately above transformKeys and replace the erroneous phrase while leaving the rest of the comment and the function signature unchanged.
48-55: ⚡ Quick winNon-null assertions may cause runtime crashes if build info paths are missing.
The
!non-null assertion operator is used onfilereturned bygetBuildInfoPathandgetBuildInfoOutputPath. If a build ID exists incacheHitsbut its build info file is missing or the path returnsundefined, this will throw an unhelpful error when trying to readundefined.Consider adding explicit null checks with descriptive error messages:
🛡️ Suggested defensive handling
- const { input, solcVersion } = await hre.artifacts - .getBuildInfoPath(buildId) - .then(file => fs.readFile(file!, 'utf-8')) - .then(JSON.parse); - const { output } = await hre.artifacts - .getBuildInfoOutputPath(buildId) - .then(file => fs.readFile(file!, 'utf-8')) - .then(JSON.parse); + const buildInfoPath = await hre.artifacts.getBuildInfoPath(buildId); + assert(buildInfoPath, `Build info path not found for buildId: ${buildId}`); + const { input, solcVersion } = JSON.parse(await fs.readFile(buildInfoPath, 'utf-8')); + + const buildInfoOutputPath = await hre.artifacts.getBuildInfoOutputPath(buildId); + assert(buildInfoOutputPath, `Build info output path not found for buildId: ${buildId}`); + const { output } = JSON.parse(await fs.readFile(buildInfoOutputPath, 'utf-8'));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hardhat/hardhat-transpiler/tasks/transpile.ts` around lines 48 - 55, The non-null assertions on the results of hre.artifacts.getBuildInfoPath(...) and getBuildInfoOutputPath(...) can crash if they return undefined; update transpile.ts to explicitly check the returned paths (from getBuildInfoPath and getBuildInfoOutputPath) before calling fs.readFile, and throw or log a descriptive error (including the buildId) if a path is undefined or the file is missing; locate the code using symbols getBuildInfoPath, getBuildInfoOutputPath, and the variables input/solcVersion/output and perform early null/exists checks (fs.existsSync or try/catch around fs.readFile) so you read only valid file paths and provide helpful error messages instead of relying on the `!` operator.test/account/extensions/ERC7821.behavior.js (1)
53-53: ⚡ Quick winUse
0nfor the EntryPoint nonce equality check (consistency)
The nonce getters in this file are already asserted withequal(0)/equal(1)elsewhere, so the current test is likely fine; switching this one to0nis an optional consistency improvement and aligns with other bigint-based nonce assertions in the suite.Proposed fix
- await expect(this.ethers.predeploy.entrypoint.v09.getNonce(this.mock.target, 0)).to.eventually.equal(0); + await expect(this.ethers.predeploy.entrypoint.v09.getNonce(this.mock.target, 0)).to.eventually.equal(0n);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/account/extensions/ERC7821.behavior.js` at line 53, The test uses a number literal when asserting the EntryPoint nonce; change the assertion to use a bigint for consistency by comparing the result of this.ethers.predeploy.entrypoint.v09.getNonce(this.mock.target, 0) against 0n instead of 0 so it matches other bigint nonce assertions in the suite (look for getNonce and this.ethers.predeploy.entrypoint.v09 in the test).test/utils/Context.behavior.js (1)
3-3: 💤 Low valueUnnecessary
asynckeyword.The function
shouldBehaveLikeRegularContextis markedasyncbut contains noawaitstatements. The function body only has synchronousdescribeblocks. Consider removing theasynckeyword for clarity.♻️ Proposed fix
-export async function shouldBehaveLikeRegularContext() { +export function shouldBehaveLikeRegularContext() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/utils/Context.behavior.js` at line 3, The function shouldBehaveLikeRegularContext is erroneously declared async despite having no await usage; remove the async keyword from its declaration so the function is a plain synchronous function (locate the declaration "export async function shouldBehaveLikeRegularContext()" and change it to "export function shouldBehaveLikeRegularContext()") to make intent clearer and avoid misleading callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/checks.yml:
- Around line 106-108: The checkout step using actions/checkout@v6 currently
allows credential persistence; update the harnesses checkout step to explicitly
set persist-credentials: false so the workflow auth token is not carried forward
to subsequent git commands. Locate the step that uses actions/checkout@v6 (the
harnesses checkout) and add the persist-credentials: false key under its with:
block to disable token persistence.
In `@hardhat/hardhat-exposed/internal/expose.ts`:
- Around line 564-596: The switch on typeName.nodeType contains const bindings
(e.g., typeString, typeDef) that can leak across cases; wrap each case's
statements in a block scope { ... } so declarations like "const { typeString } =
typeName.typeDescriptions;" and "const typeDef = derefUserDefinedTypeName(...)"
are scoped to their case, including nested switch cases for typeDef.nodeType and
their own consts (e.g., the inner "const { typeString } =
typeDef.underlyingType.typeDescriptions;"), and ensure the default case and any
returns remain inside their respective blocks.
In `@hardhat/hardhat-exposed/tasks/generate-exposed-contracts.ts`:
- Around line 28-29: Replace string prefixes checks that use
rootPath.startsWith(hre.config.exposed.outDir) with a path-safe containment
test: resolve/normalize both rootPath and hre.config.exposed.outDir (use
path.resolve/path.normalize) and then use path.relative to determine if rootPath
is inside outDir (i.e., path.relative(outDirResolved, rootPathResolved) does not
start with '..' and is not equal to ''). Update all occurrences of this pattern
(the checks referencing rootPath and hre.config.exposed.outDir around the
current uses in generate-exposed-contracts.ts at the three spots flagged) so
they work correctly for absolute/relative paths and different path separators.
In `@scripts/checks/compare-gas-reports.js`:
- Around line 57-63: The static compare method can throw when update.contracts
or ref.contracts are missing; modify Report.compare (the static compare(update,
ref, opts) function) to guard those fields by defaulting update.contracts and
ref.contracts to {} (or returning an empty array early) before using
Object.entries and .filter; update references to update.contracts and
ref.contracts in this method so they are safely accessed (e.g., const uContracts
= update.contracts ?? {}; const rContracts = ref.contracts ?? {}) and then
operate on uContracts and rContracts in the existing logic.
In `@scripts/checks/inheritance-ordering.js`:
- Around line 28-29: The current check uses micromatch.all which requires the
input to match every pattern and breaks npm-style include/exclude semantics;
change the conditional that reads if (match.all(source.replace(/^project/, ''),
patterns)) to use match.isMatch(source.replace(/^project/, ''), patterns) so
that files are accepted if any include glob matches and negations (!) act as
exclusions; update any import/usage comments around this check (the loop over
findAll('ContractDefinition', solcOutput.sources[source].ast)) to reflect the
corrected matching behavior.
In `@scripts/solc-versions.js`:
- Around line 9-14: The compile function currently uses child_process.exec with
shell-interpolated command (`exec(\`forge build ${source} --use ${version} --out
...\`)`) which allows shell injection from values returned by
getContractsMetadata(); change compile to call a non-shell API
(child_process.execFile or child_process.spawn) with the command and an args
array (e.g., ['build', source, '--use', version, '--out',
`out/solc-${version}`]) so no shell interpolation occurs, preserve the
Promise-based resolve/reject behavior and existing error handling, and keep the
function name compile as the same exported symbol.
In `@scripts/upgradeable/transpile.sh`:
- Line 16: The npx hardhat transpile invocation uses --settings
$DIRNAME/transpile.config.json without quotes which can break on paths with
spaces or special chars; update the command in scripts/upgradeable/transpile.sh
(the line calling npx hardhat transpile --settings ...) to wrap the settings
path in double quotes (e.g., "--settings \"$DIRNAME/transpile.config.json\"") so
the shell treats it as a single argument referencing the $DIRNAME variable
correctly.
In `@test/account/utils/draft-ERC4337Utils.test.js`:
- Line 447: The test suite callback for describe('hash', async function () { is
incorrectly marked async; remove the async modifier from the describe callback
so the suite definition is synchronous, and move any asynchronous logic inside
this suite into individual it(...) tests or into Mocha hooks (before,
beforeEach, after, afterEach) as needed; locate the describe call in
draft-ERC4337Utils.test (the 'hash' suite) and change its signature to
describe('hash', function () { while relocating any await usage into the
appropriate it or hook functions (e.g., before(async () => { ... }) or it('...',
async function () { ... }).
In `@test/utils/cryptography/RSA.helper.js`:
- Line 5: The helper currently calls fs.readFileSync(file, 'utf8') directly
which makes parse() sensitive to the test runner CWD; update the parse function
in RSA.helper.js to detect non-absolute file paths and resolve them relative to
the helper module directory using import.meta.url and fileURLToPath (e.g.,
compute moduleDir = path.dirname(fileURLToPath(import.meta.url)) and join with
the passed file), then call fs.readFileSync on the resolved path (references:
parse function, fs.readFileSync call, variable/data reading).
---
Outside diff comments:
In `@test/helpers/erc7739.js`:
- Around line 42-44: The connect(provider) method on ERC7739Signer currently
calls this.#signer.connect(provider) but does not return the connected signer;
update ERC7739Signer.connect to return the connected signer (i.e., return the
result of this.#signer.connect(provider)) so it matches ethers' Signer.connect
chaining expectations and preserves AbstractSigner behavior. Ensure you
reference the private field `#signer` and the connect method on ERC7739Signer when
making the change.
In `@test/helpers/signers.js`:
- Around line 194-196: The comparator used when assigning this.#signers in
MultiERC7913SigningKey converts ethers.keccak256(...) hex strings to Numbers by
subtraction, causing unstable ordering; change the comparator to parse each
keccak256 hex string into a BigInt (e.g. BigInt(hash)) and compare those BigInt
values (return -1/0/1 or the difference as a Number derived from the BigInt
comparison) to produce a deterministic bytes32 ordering for the signers.sort
call.
---
Nitpick comments:
In `@hardhat/hardhat-oz-contracts-helpers/hook-handlers/network.ts`:
- Around line 17-33: The current Object.assign call overwrites any existing
connection.helpers; change it to merge with existing helpers instead: read the
existing connection.helpers and merge its entries with the new ones you
construct (including chain via getLocalChain(connection.provider),
impersonate(connection), storage helpers getSlot/getAddressInSlot/setSlot, and
time helpers clock/clockFromReceipt/increaseBy/increaseTo/duration) so you
preserve prior keys; update the Object.assign usage around connection.helpers
(or compute helpers = { ...connection.helpers, ...newHelpers } before assigning)
to ensure additive behavior without removing existing helper entries.
In `@hardhat/hardhat-transpiler/tasks/transpile.ts`:
- Around line 20-23: The comment above function transformKeys contains a typo
("key the keys intact"); update the comment to read "keep the keys intact" so it
accurately describes the function behavior; locate the comment immediately above
transformKeys and replace the erroneous phrase while leaving the rest of the
comment and the function signature unchanged.
- Around line 48-55: The non-null assertions on the results of
hre.artifacts.getBuildInfoPath(...) and getBuildInfoOutputPath(...) can crash if
they return undefined; update transpile.ts to explicitly check the returned
paths (from getBuildInfoPath and getBuildInfoOutputPath) before calling
fs.readFile, and throw or log a descriptive error (including the buildId) if a
path is undefined or the file is missing; locate the code using symbols
getBuildInfoPath, getBuildInfoOutputPath, and the variables
input/solcVersion/output and perform early null/exists checks (fs.existsSync or
try/catch around fs.readFile) so you read only valid file paths and provide
helpful error messages instead of relying on the `!` operator.
In `@test/account/extensions/ERC7821.behavior.js`:
- Line 53: The test uses a number literal when asserting the EntryPoint nonce;
change the assertion to use a bigint for consistency by comparing the result of
this.ethers.predeploy.entrypoint.v09.getNonce(this.mock.target, 0) against 0n
instead of 0 so it matches other bigint nonce assertions in the suite (look for
getNonce and this.ethers.predeploy.entrypoint.v09 in the test).
In `@test/helpers/access-manager.js`:
- Around line 50-55: Update the JSDoc `@requires` for prepareOperation to mention
the time helper dependency: the function prepareOperation(manager, { caller,
target, calldata, delay }) now uses this.helpers.time (calls timestamp() and
increaseTo.timestamp()), so add this.helpers.time to the `@requires` contract
alongside this.manager, this.caller, this.target, and this.calldata to document
the time helper dependency.
In `@test/helpers/storage.js`:
- Around line 36-40: The current upgradeableSlot helper swallows any
readArtifact error by using the second .then handler; change this to explicitly
handle only the "artifact not found" case and rethrow other errors. Replace the
dual-argument .then with a single .then + .catch on artifacts.readArtifact in
the upgradeableSlot function, and in the .catch inspect the thrown error (e.g.,
check a specific error.code or error.message pattern like /artifact not found/i)
to return offset only for that case and rethrow the error for all other failures
so unexpected issues (malformed JSON, permission errors) surface.
In `@test/metatx/ERC2771Forwarder.test.js`:
- Around line 148-163: Add explicit assertions that no ether balances changed
when the calls revert: in the 'bubbles out of gas' test (the it block invoking
this.forgeRequest with
this.receiver.interface.encodeFunctionData('mockFunctionOutOfGas') and executing
via this.forwarder.execute) wrap the failing call in a balance-check assertion
to assert no balances changed (e.g., use the test helper
expect(...).to.not.changeEtherBalances or equivalent) for the
relayer/sender/receiver addresses; apply the same pattern to the other
OOG/revert-path tests mentioned (the blocks at 165-193, 347-367, 369-398) to
ensure each revert path includes a no-balance-change assertion alongside the
revert and gas checks.
In `@test/utils/Context.behavior.js`:
- Line 3: The function shouldBehaveLikeRegularContext is erroneously declared
async despite having no await usage; remove the async keyword from its
declaration so the function is a plain synchronous function (locate the
declaration "export async function shouldBehaveLikeRegularContext()" and change
it to "export function shouldBehaveLikeRegularContext()") to make intent clearer
and avoid misleading callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cd7b305b-75b6-4121-92e4-70e235276661
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (288)
.changeset/spotty-teeth-show.md.codecov.yml.github/actions/gas-compare/action.yml.github/actions/setup/action.yml.github/actions/storage-layout/action.yml.github/workflows/checks.yml.github/workflows/formal-verification.ymlfoundry.tomlhardhat.config.jshardhat.config.tshardhat/env-artifacts.jshardhat/hardhat-exposed/hook-handlers/clean.tshardhat/hardhat-exposed/hook-handlers/config.tshardhat/hardhat-exposed/internal/build-info.tshardhat/hardhat-exposed/internal/expose.tshardhat/hardhat-exposed/internal/format-lines.tshardhat/hardhat-exposed/internal/types.tshardhat/hardhat-exposed/plugin.tshardhat/hardhat-exposed/tasks/build.tshardhat/hardhat-exposed/tasks/generate-exposed-contracts.tshardhat/hardhat-exposed/type-extensions.tshardhat/hardhat-oz-contracts-helpers/hook-handlers/hre.tshardhat/hardhat-oz-contracts-helpers/hook-handlers/network.tshardhat/hardhat-oz-contracts-helpers/hook-handlers/test.tshardhat/hardhat-oz-contracts-helpers/plugin.tshardhat/hardhat-oz-contracts-helpers/type-extensions.tshardhat/hardhat-transpiler/plugin.tshardhat/hardhat-transpiler/tasks/transpile.tshardhat/ignore-unreachable-warnings.jshardhat/remappings.jshardhat/skip-foundry-tests.jshardhat/task-test-get-files.jspackage.jsonremappings.txtscripts/checks/compare-gas-reports.jsscripts/checks/compare-layout.jsscripts/checks/coverage.shscripts/checks/extract-layout.jsscripts/checks/inheritance-ordering.jsscripts/checks/pragma-validity.jsscripts/fetch-common-contracts.jsscripts/generate/format-lines.jsscripts/generate/helpers/sanitize.jsscripts/generate/run.jsscripts/generate/templates/Arrays.jsscripts/generate/templates/Arrays.opts.jsscripts/generate/templates/Checkpoints.jsscripts/generate/templates/Checkpoints.opts.jsscripts/generate/templates/Checkpoints.t.jsscripts/generate/templates/Enumerable.opts.jsscripts/generate/templates/EnumerableMap.jsscripts/generate/templates/EnumerableSet.jsscripts/generate/templates/MerkleProof.jsscripts/generate/templates/MerkleProof.opts.jsscripts/generate/templates/Packing.jsscripts/generate/templates/Packing.opts.jsscripts/generate/templates/Packing.t.jsscripts/generate/templates/SafeCast.jsscripts/generate/templates/Slot.opts.jsscripts/generate/templates/SlotDerivation.jsscripts/generate/templates/SlotDerivation.t.jsscripts/generate/templates/StorageSlot.jsscripts/generate/templates/StorageSlotMock.jsscripts/generate/templates/TransientSlot.jsscripts/generate/templates/TransientSlotMock.jsscripts/generate/templates/conversion.jsscripts/get-contracts-metadata.jsscripts/helpers.jsscripts/minimize-pragma.jsscripts/prepack.shscripts/remove-ignored-artifacts.jsscripts/solc-versions.jsscripts/upgradeable/transpile.config.jsonscripts/upgradeable/transpile.shscripts/upgradeable/upgradeable.patchsolhint.config.cjstest/access/AccessControl.behavior.jstest/access/AccessControl.test.jstest/access/Ownable.test.jstest/access/Ownable2Step.test.jstest/access/extensions/AccessControlDefaultAdminRules.test.jstest/access/extensions/AccessControlEnumerable.test.jstest/access/manager/AccessManaged.test.jstest/access/manager/AccessManager.behavior.jstest/access/manager/AccessManager.predicate.jstest/access/manager/AccessManager.test.jstest/access/manager/AuthorityUtils.test.jstest/account/Account.behavior.jstest/account/Account.test.jstest/account/AccountECDSA.test.jstest/account/AccountEIP7702.t.soltest/account/AccountEIP7702.test.jstest/account/AccountERC7913.test.jstest/account/AccountMultiSigner.test.jstest/account/AccountMultiSignerWeighted.test.jstest/account/AccountP256.test.jstest/account/AccountRSA.test.jstest/account/AccountWebAuthn.test.jstest/account/examples/AccountEIP7702WithModulesMock.test.jstest/account/extensions/AccountERC7579.behavior.jstest/account/extensions/AccountERC7579.test.jstest/account/extensions/AccountERC7579Hooked.test.jstest/account/extensions/ERC7821.behavior.jstest/account/utils/EIP7702Utils.test.jstest/account/utils/draft-ERC4337Utils.test.jstest/account/utils/draft-ERC7579Utils.t.soltest/account/utils/draft-ERC7579Utils.test.jstest/crosschain/BridgeERC1155.behavior.jstest/crosschain/BridgeERC1155.test.jstest/crosschain/BridgeERC20.behavior.jstest/crosschain/BridgeERC20.test.jstest/crosschain/BridgeERC721.behavior.jstest/crosschain/BridgeERC721.test.jstest/crosschain/CrosschainExecutor.test.jstest/crosschain/ERC7786Recipient.test.jstest/finance/VestingWallet.behavior.jstest/finance/VestingWallet.test.jstest/finance/VestingWalletCliff.test.jstest/governance/Governor.test.jstest/governance/TimelockController.test.jstest/governance/extensions/GovernorCountingFractional.test.jstest/governance/extensions/GovernorCountingOverridable.test.jstest/governance/extensions/GovernorCrosschain.test.jstest/governance/extensions/GovernorERC721.test.jstest/governance/extensions/GovernorNoncesKeyed.test.jstest/governance/extensions/GovernorPreventLateQuorum.test.jstest/governance/extensions/GovernorProposalGuardian.test.jstest/governance/extensions/GovernorSequentialProposalId.test.jstest/governance/extensions/GovernorStorage.test.jstest/governance/extensions/GovernorSuperQuorum.test.jstest/governance/extensions/GovernorSuperQuorumGreaterThanQuorum.t.soltest/governance/extensions/GovernorTimelockAccess.test.jstest/governance/extensions/GovernorTimelockCompound.test.jstest/governance/extensions/GovernorTimelockControl.test.jstest/governance/extensions/GovernorVotesQuorumFraction.test.jstest/governance/extensions/GovernorVotesSuperQuorumFraction.test.jstest/governance/extensions/GovernorWithParams.test.jstest/governance/utils/ERC6372.behavior.jstest/governance/utils/Votes.behavior.jstest/governance/utils/Votes.test.jstest/governance/utils/VotesExtended.test.jstest/helpers/access-manager.jstest/helpers/account.jstest/helpers/chains.jstest/helpers/constants.jstest/helpers/deploy.jstest/helpers/eip712-types.jstest/helpers/eip712.jstest/helpers/enums.jstest/helpers/erc4337.jstest/helpers/erc7579.jstest/helpers/erc7739.jstest/helpers/governance.jstest/helpers/iterate.jstest/helpers/math.jstest/helpers/methods.jstest/helpers/precompiles.jstest/helpers/random.jstest/helpers/signers.jstest/helpers/storage.jstest/helpers/strings.jstest/helpers/time.jstest/helpers/trie.jstest/helpers/txpool.jstest/metatx/ERC2771Context.test.jstest/metatx/ERC2771Forwarder.t.soltest/metatx/ERC2771Forwarder.test.jstest/proxy/Clones.behaviour.jstest/proxy/Clones.t.soltest/proxy/Clones.test.jstest/proxy/ERC1967/ERC1967Proxy.test.jstest/proxy/ERC1967/ERC1967Utils.test.jstest/proxy/Proxy.behaviour.jstest/proxy/beacon/BeaconProxy.test.jstest/proxy/beacon/UpgradeableBeacon.test.jstest/proxy/transparent/ProxyAdmin.test.jstest/proxy/transparent/TransparentUpgradeableProxy.behaviour.jstest/proxy/transparent/TransparentUpgradeableProxy.test.jstest/proxy/utils/Initializable.test.jstest/proxy/utils/UUPSUpgradeable.test.jstest/sanity.test.jstest/token/ERC1155/ERC1155.behavior.jstest/token/ERC1155/ERC1155.test.jstest/token/ERC1155/extensions/ERC1155Burnable.test.jstest/token/ERC1155/extensions/ERC1155Crosschain.test.jstest/token/ERC1155/extensions/ERC1155Pausable.test.jstest/token/ERC1155/extensions/ERC1155Supply.test.jstest/token/ERC1155/extensions/ERC1155URIStorage.test.jstest/token/ERC1155/utils/ERC1155Holder.test.jstest/token/ERC1155/utils/ERC1155Utils.test.jstest/token/ERC20/ERC20.behavior.jstest/token/ERC20/ERC20.test.jstest/token/ERC20/extensions/ERC1363.test.jstest/token/ERC20/extensions/ERC20Burnable.test.jstest/token/ERC20/extensions/ERC20Capped.test.jstest/token/ERC20/extensions/ERC20Crosschain.test.jstest/token/ERC20/extensions/ERC20FlashMint.test.jstest/token/ERC20/extensions/ERC20Pausable.test.jstest/token/ERC20/extensions/ERC20Permit.test.jstest/token/ERC20/extensions/ERC20Votes.test.jstest/token/ERC20/extensions/ERC20Wrapper.test.jstest/token/ERC20/extensions/ERC4626.test.jstest/token/ERC20/extensions/draft-ERC20Bridgeable.test.jstest/token/ERC20/extensions/draft-ERC20TemporaryApproval.test.jstest/token/ERC20/utils/SafeERC20.test.jstest/token/ERC6909/ERC6909.behavior.jstest/token/ERC6909/ERC6909.test.jstest/token/ERC6909/extensions/ERC6909ContentURI.test.jstest/token/ERC6909/extensions/ERC6909Metadata.test.jstest/token/ERC6909/extensions/ERC6909TokenSupply.test.jstest/token/ERC721/ERC721.behavior.jstest/token/ERC721/ERC721.test.jstest/token/ERC721/ERC721Enumerable.test.jstest/token/ERC721/extensions/ERC721Burnable.test.jstest/token/ERC721/extensions/ERC721Consecutive.test.jstest/token/ERC721/extensions/ERC721Crosschain.test.jstest/token/ERC721/extensions/ERC721Pausable.test.jstest/token/ERC721/extensions/ERC721Royalty.test.jstest/token/ERC721/extensions/ERC721URIStorage.test.jstest/token/ERC721/extensions/ERC721Votes.test.jstest/token/ERC721/extensions/ERC721Wrapper.test.jstest/token/ERC721/utils/ERC721Holder.test.jstest/token/ERC721/utils/ERC721Utils.test.jstest/token/common/ERC2981.behavior.jstest/utils/Address.test.jstest/utils/Arrays.test.jstest/utils/Base58.test.jstest/utils/Base64.test.jstest/utils/Blockhash.t.soltest/utils/Blockhash.test.jstest/utils/Bytes.test.jstest/utils/CAIP.test.jstest/utils/Calldata.test.jstest/utils/Context.behavior.jstest/utils/Context.test.jstest/utils/Create2.test.jstest/utils/Create3.test.jstest/utils/ERC6372Utils.test.jstest/utils/LowLevelCall.test.jstest/utils/Memory.test.jstest/utils/Multicall.test.jstest/utils/Nonces.behavior.jstest/utils/Nonces.test.jstest/utils/NoncesKeyed.test.jstest/utils/Packing.test.jstest/utils/Panic.test.jstest/utils/Pausable.test.jstest/utils/RLP.test.jstest/utils/ReentrancyGuard.test.jstest/utils/RelayedCall.test.jstest/utils/ShortStrings.test.jstest/utils/SimulatedCall.test.jstest/utils/SlotDerivation.test.jstest/utils/StorageSlot.test.jstest/utils/Strings.test.jstest/utils/TransientSlot.test.jstest/utils/cryptography/ECDSA.test.jstest/utils/cryptography/EIP712.test.jstest/utils/cryptography/ERC1271.behavior.jstest/utils/cryptography/ERC7739.test.jstest/utils/cryptography/ERC7739Utils.test.jstest/utils/cryptography/MerkleProof.test.jstest/utils/cryptography/MessageHashUtils.test.jstest/utils/cryptography/P256.test.jstest/utils/cryptography/RSA.helper.jstest/utils/cryptography/RSA.test.jstest/utils/cryptography/SignatureChecker.test.jstest/utils/cryptography/TrieProof.test.jstest/utils/draft-InteroperableAddress.t.soltest/utils/draft-InteroperableAddress.test.jstest/utils/introspection/ERC165.test.jstest/utils/introspection/ERC165Checker.test.jstest/utils/introspection/SupportsInterface.behavior.jstest/utils/math/Math.test.jstest/utils/math/SafeCast.test.jstest/utils/math/SignedMath.t.soltest/utils/math/SignedMath.test.jstest/utils/structs/BitMap.test.jstest/utils/structs/Checkpoints.test.jstest/utils/structs/CircularBuffer.test.jstest/utils/structs/DoubleEndedQueue.test.jstest/utils/structs/EnumerableMap.behavior.jstest/utils/structs/EnumerableMap.test.jstest/utils/structs/EnumerableSet.behavior.jstest/utils/structs/EnumerableSet.test.jstest/utils/structs/Heap.test.jstest/utils/structs/MerkleTree.test.jstest/utils/types/Time.test.js
💤 Files with no reviewable changes (12)
- test/helpers/deploy.js
- scripts/fetch-common-contracts.js
- scripts/checks/coverage.sh
- hardhat/skip-foundry-tests.js
- test/account/AccountEIP7702.t.sol
- foundry.toml
- hardhat/ignore-unreachable-warnings.js
- hardhat/remappings.js
- hardhat.config.js
- hardhat/task-test-get-files.js
- hardhat/env-artifacts.js
- scripts/prepack.sh
| - uses: actions/checkout@v6 | ||
| with: | ||
| submodules: recursive |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the workflow file
ls -la .github/workflows || true
sed -n '1,220p' .github/workflows/checks.yml | nl -ba | sed -n '80,140p'
# Confirm whether persist-credentials is present anywhere in the file
rg -n "persist-credentials" .github/workflows/checks.yml || true
# Confirm the exact checkout step under "harnesses"
# Print a wider window around the "harnesses" job definition if present
rg -n "^[[:space:]]*harnesses:" .github/workflows/checks.yml || trueRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show checks.yml with line numbers around the referenced range (106-108)
awk 'NR>=90 && NR<=130 {printf "%4d:%s\n", NR, $0}' .github/workflows/checks.yml
# Confirm whether persist-credentials is present anywhere in the file
echo "---- persist-credentials matches ----"
rg -n "persist-credentials" .github/workflows/checks.yml || true
# Print the harnesses job section header(s) if present
echo "---- harnesses job matches ----"
rg -n "^[[:space:]]*harnesses:" .github/workflows/checks.yml || trueRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 1418
🌐 Web query:
actions/checkout v6 persist-credentials default value and behavior local git config
💡 Result:
For actions/checkout v6, the input persist-credentials default is effectively true (it’s documented as “Default: true” when configured/unspecified) and it persists the authentication token for later git commands during the job (it’s removed during post-job cleanup). [1][2] Local git config behavior (v6): - The token is persisted in the local git configuration so that subsequent steps can run authenticated git commands (e.g., additional git fetch/push). [1] - In v6, GitHub notes an improvement: persist-credentials stores credentials in a separate file under $RUNNER_TEMP instead of directly in.git/config, and it requires Actions Runner v2.329.0+ for Docker-container action scenarios. [1][3] Opt-out: - Set persist-credentials: false to avoid persisting the token. [1] Practical implication: - If you use actions/checkout v6 with persist-credentials not set (or left at its default), authenticated credentials are available for subsequent git commands in the job, but they should not remain after the job ends (post-job cleanup removes them). [1][2]
Citations:
- 1: https://github.com/actions/checkout/?tab=readme-ov-file
- 2: https://github.com/actions/checkout
- 3: actions/checkout@v5.0.1...v6.0.0
Disable credential persistence in the harnesses checkout step.
actions/checkout@v6 persists the workflow auth token by default (persist-credentials: true) so subsequent git commands in the same job can use it; add persist-credentials: false to avoid carrying the token forward (the token is later cleaned up after the job).
🔐 Suggested hardening
harnesses:
@@
- uses: actions/checkout@v6
with:
submodules: recursive
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v6 | |
| with: | |
| submodules: recursive | |
| - uses: actions/checkout@v6 | |
| with: | |
| submodules: recursive | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 106-108: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 106-106: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/checks.yml around lines 106 - 108, The checkout step using
actions/checkout@v6 currently allows credential persistence; update the
harnesses checkout step to explicitly set persist-credentials: false so the
workflow auth token is not carried forward to subsequent git commands. Locate
the step that uses actions/checkout@v6 (the harnesses checkout) and add the
persist-credentials: false key under its with: block to disable token
persistence.
| switch (typeName.nodeType) { | ||
| case 'ElementaryTypeName': | ||
| case 'ArrayTypeName': | ||
| const { typeString } = typeName.typeDescriptions; | ||
| assert(typeString != undefined); | ||
| return typeString; | ||
|
|
||
| case 'UserDefinedTypeName': | ||
| const typeDef = derefUserDefinedTypeName(deref, typeName); | ||
| switch (typeDef.nodeType) { | ||
| case 'UserDefinedValueTypeDefinition': | ||
| const { typeString } = typeDef.underlyingType.typeDescriptions; | ||
| assert(typeString != undefined); | ||
| return typeString; | ||
|
|
||
| case 'EnumDefinition': | ||
| assert(typeDef.members.length < 256); | ||
| return 'uint8'; | ||
|
|
||
| case 'ContractDefinition': | ||
| return 'address'; | ||
|
|
||
| case 'StructDefinition': | ||
| if (location === 'storage') { | ||
| throw new Error('Unexpected error'); // is treated separately in getFunctionArguments | ||
| } else { | ||
| return '(' + typeDef.members.map(v => getVarAbiType(v, context, deref, location)).join(',') + ')'; | ||
| } | ||
| } | ||
|
|
||
| default: | ||
| throw new Error('Unknown ABI type'); | ||
| } |
There was a problem hiding this comment.
Wrap switch case declarations in blocks to prevent variable leakage.
Static analysis correctly flags that const declarations in switch cases without block scoping can be accessed erroneously from other cases. Wrap each case body in braces.
🛡️ Proposed fix
function getAbiType(
typeName: TypeName,
context: ContractDefinition,
deref: ASTDereferencer,
location: StorageLocation | null,
): string {
switch (typeName.nodeType) {
- case 'ElementaryTypeName':
- case 'ArrayTypeName':
- const { typeString } = typeName.typeDescriptions;
- assert(typeString != undefined);
- return typeString;
-
- case 'UserDefinedTypeName':
- const typeDef = derefUserDefinedTypeName(deref, typeName);
- switch (typeDef.nodeType) {
- case 'UserDefinedValueTypeDefinition':
- const { typeString } = typeDef.underlyingType.typeDescriptions;
- assert(typeString != undefined);
- return typeString;
-
- case 'EnumDefinition':
- assert(typeDef.members.length < 256);
- return 'uint8';
-
- case 'ContractDefinition':
- return 'address';
-
- case 'StructDefinition':
- if (location === 'storage') {
- throw new Error('Unexpected error'); // is treated separately in getFunctionArguments
- } else {
- return '(' + typeDef.members.map(v => getVarAbiType(v, context, deref, location)).join(',') + ')';
- }
- }
-
- default:
- throw new Error('Unknown ABI type');
+ case 'ElementaryTypeName':
+ case 'ArrayTypeName': {
+ const { typeString } = typeName.typeDescriptions;
+ assert(typeString != undefined);
+ return typeString;
+ }
+
+ case 'UserDefinedTypeName': {
+ const typeDef = derefUserDefinedTypeName(deref, typeName);
+ switch (typeDef.nodeType) {
+ case 'UserDefinedValueTypeDefinition': {
+ const { typeString } = typeDef.underlyingType.typeDescriptions;
+ assert(typeString != undefined);
+ return typeString;
+ }
+
+ case 'EnumDefinition':
+ assert(typeDef.members.length < 256);
+ return 'uint8';
+
+ case 'ContractDefinition':
+ return 'address';
+
+ case 'StructDefinition':
+ if (location === 'storage') {
+ throw new Error('Unexpected error');
+ } else {
+ return '(' + typeDef.members.map(v => getVarAbiType(v, context, deref, location)).join(',') + ')';
+ }
+ }
+ }
+ // falls through if inner switch doesn't return
+
+ default:
+ throw new Error('Unknown ABI type');
}
}🧰 Tools
🪛 Biome (2.4.15)
[error] 567-567: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 572-572: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 575-575: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hardhat/hardhat-exposed/internal/expose.ts` around lines 564 - 596, The
switch on typeName.nodeType contains const bindings (e.g., typeString, typeDef)
that can leak across cases; wrap each case's statements in a block scope { ... }
so declarations like "const { typeString } = typeName.typeDescriptions;" and
"const typeDef = derefUserDefinedTypeName(...)" are scoped to their case,
including nested switch cases for typeDef.nodeType and their own consts (e.g.,
the inner "const { typeString } = typeDef.underlyingType.typeDescriptions;"),
and ensure the default case and any returns remain inside their respective
blocks.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| @@ -0,0 +1,27 @@ | |||
| import { ContractDefinition, ImportDirective, PragmaDirective, SourceUnit, UsingForDirective } from 'solidity-ast'; | |||
| import { Node, NodeType, NodeTypeMap } from 'solidity-ast/node'; | |||
| import { findAll, isNodeType } from 'solidity-ast/utils.js'; | ||
| import { NatSpec, parseNatspec } from '../utils/natspec'; | ||
| import { DocItemContext, DOC_ITEM_CONTEXT } from '../site'; | ||
| import { mapValues } from '../utils/map-values'; |
| import { mapValues } from '../utils/map-values'; | ||
| import { DocItem, docItemTypes } from '../doc-item'; | ||
| import { formatVariable } from './helpers'; | ||
| import { PropertyGetter } from '../templates'; |
| import { promises as fs } from 'fs'; | ||
| import { render } from './render'; | ||
| import { Build, buildSite } from './site'; | ||
| import { ensureArray } from './utils/ensure-array'; |
| @@ -0,0 +1,74 @@ | |||
| import Handlebars, { RuntimeOptions } from 'handlebars'; | |||
| import { Site, Page, DocItemWithContext, DOC_ITEM_CONTEXT } from './site'; | |||
| import path from 'path'; | ||
| import { ContractDefinition, SourceUnit } from 'solidity-ast'; | ||
| import { SolcOutput, SolcInput } from 'solidity-ast/solc'; | ||
| import { astDereferencer, ASTDereferencer, findAll, isNodeType, srcDecoder, SrcDecoder } from 'solidity-ast/utils.js'; |
a1a4ca2 to
15c2da4
Compare
Replaces #6317
testing
migrate processes that needs migrating