fix: let a selected EIP-712 definition win name clashes with non-selected sources - #8581
Conversation
popescuoctavian
added
no changeset needed
|
There was a problem hiding this comment.
Pull request overview
Fixes Hardhat’s EIP-712 type canonicalization conflict detection so that when a struct name is defined both in a selected source and in a non-selected source, the selected definition deterministically wins even if the name is reachable as a dependency—avoiding a spurious HHE818 abort.
Changes:
- Adjust deferred name-clash throwing to only error for reachable conflicts when no selected definition exists.
- Improve
HHE818dependency-conflict remediation guidance to mention resolving by including the canonical dependency source. - Update/add regression tests to assert selected-vs-non-selected reachable clashes resolve deterministically (including an aave-v4-shaped case).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/hardhat/src/internal/builtin-plugins/solidity-test/eip712/canonicalize.ts | Updates conflict detection rule and remediation text so selected definitions win reachable clashes vs non-selected sources. |
| packages/hardhat/test/internal/builtin-plugins/solidity-test/eip712/canonicalize.ts | Updates existing test expectation and adds regression coverage for selected winning reachable clashes and the aave-v4-shaped scenario. |
Suppressed comments (1)
packages/hardhat/test/internal/builtin-plugins/solidity-test/eip712/canonicalize.ts:587
- Same as above: prefer sorting a copy (
[...expected].sort()) to avoid mutating shared expectation arrays in-place.
assert.deepEqual(
canonicalizeStructs([...collected].reverse(), selected).sort(),
[...expected.sort()],
);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| assert.deepEqual(canonicalizeStructs(collected, selected).sort(), [ | ||
| ...expected.sort(), | ||
| ]); |
|
/bench scenarios=aave-v4 |
|
🚀 Starting regression benchmark for |
|
❌ Regression benchmark failed for |
hardhatTotal size of the bundle: List of dependencies (sorted by size) |
642df4b to
39979b6
Compare
|
/bench scenarios=aave-v4 |
|
🚀 Starting regression benchmark for |
|
❌ Regression benchmark failed for |
Running
test solidityon the aave-v4 migration branch (pinned at27c95ea) aborts with:The two definitions. The project's
hardhat.config.tsselects a single file,eip712Types.include: ['tests/helpers/mocks/EIP712Types.sol'], which defines both the signed rootSetUserPositionManagersand, right below it, the dependency it references,PositionManagerUpdate {address positionManager; bool approve;}— the type whose name is baked into the on-chain typehash, so it cannot be renamed. The clash comes from a non-selected production interface that reuses the name for an unrelated configuration payload:IAaveV4ConfigEngine.PositionManagerUpdate {ISpokeConfigurator spokeConfigurator; address spoke; address positionManager; bool active;}. Neither side is reasonably renameable (one is typehash-compatible with the deployed contracts, the other is audited production source shared with the Foundry build), and the config already does exactly what our docs prescribe.Why the collector throws.
canonicalizeStructsindexes struct definitions by name, selected sources first, so a selected definition deterministically wins any name clash. Conflicting non-selected definitions are deferred, and the deferred path threw whenever the clashing name was reachable from a selected struct — herePositionManagerUpdateis reachable because selectedSetUserPositionManagershas aPositionManagerUpdate[]member. The throw's rationale ("which copy got inlined would depend on iteration order") is only true when the name has no selected definition; when one exists, it always wins, so the abort was spurious. It also made behavior inconsistent: the same clash was documented as harmless when the selected struct was only a root, but became fatal the moment a sibling selected struct referenced it. #8344 fixed the unreferenced-clash case; this is the remaining mixed (selected-vs-non-selected, reachable) case its repro didn't exercise.Fix. The deferred conflict now throws only when the name is reachable AND has no selected definition (
reachable.has(name) && !selectedNames.has(name)). This bounds the registry the wayincludepromises (and matches Foundry's include-scopedbind-jsonregistry, where non-included sources never contribute definitions), whileHHE818still fires on the genuinely ambiguous cases: two selected definitions of the same name, and clashes among non-selected definitions that a selected struct depends on. TheHHE818remediation text now also mentions resolving a dependency clash by including the source with the canonical definition.Tests. The case asserting the old throw now asserts the selected definition wins (order-independently), plus a new regression test mirroring the aave-v4 shape exactly (one included file defines both the root and the dependency; a non-selected file redefines the dependency's name). All EIP-712 suites pass (89 tests); package lint/build pass; changeset included.