Add missing imports for ERC-7579 modular accounts without a signer #661
Conversation
consolidate imports
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a changeset for a patch release. Updates account constructor logic to validate ERC7579 module types and install a module at construction, and introduces import-only constants. Refactors import printing to group by path. Reorganizes and expands multiple Solidity test snippets toward upgradeable/UUPS patterns, adds bridging and role-based access in stablecoin tests, and reorders imports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Deployer
participant Account as Account.constructor
participant ERC7579 as IERC7579 (consts)
participant Modules as ModuleRegistry
Deployer->>Account: new Account(moduleTypeId, module, initData)
Account->>ERC7579: Read MODULE_TYPE_VALIDATOR / EXECUTOR
Account->>Account: require(moduleTypeId in {VALIDATOR, EXECUTOR})
alt moduleType valid
Account->>Modules: _installModule(moduleTypeId, module, initData)
Modules-->>Account: installation complete
Account-->>Deployer: constructed
else invalid type
Account-->>Deployer: revert
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/solidity/src/account.ts (1)
185-190: Critical: constructor-only module install will break proxy/clone deployments — move module install into the initializeraccount.ts currently adds constructor args and runs require(...) + _installModule(moduleTypeId, module, initData) in the constructor; minimal clones/proxies do not execute constructors and the generated initialize() does not install modules.
Action: move the require(...) and _installModule(...) calls into the account initializer (or have the builder emit the same args+code into the initializer for upgradeable/clone deployments).
Location: packages/core/solidity/src/account.ts (around lines 185–189).
🧹 Nitpick comments (3)
.changeset/sour-poems-stop.md (1)
5-5: Changelog note is good; consider mentioning the import formatting change.Since imports are now grouped by path and ordered by path (not by symbol), add a brief note so downstream snapshot diffs aren’t surprising.
packages/core/solidity/src/print.ts (1)
281-294: Deduplicate symbols per path and avoid O(n^2) scans.If the same symbol is added multiple times for a path, this will emit duplicates and break Solidity imports. Also prefer strict equality. Refactor to build a map of path → Set and emit once.
Apply:
- const transformedImports = imports.map(p => helpers.transformImport(p)); - - return transformedImports - .map(p => p.path) - .filter((path, i, paths) => paths.indexOf(path) == i) - .sort() - .map(path => `import {${ - transformedImports - .filter(p => p.path === path) - .map(p => p.name) - .sort() - .join(', ') - }} from "${path}";`); + const transformedImports = imports.map(p => helpers.transformImport(p)); + + const byPath = new Map<string, Set<string>>(); + for (const { path, name } of transformedImports) { + const set = byPath.get(path) ?? new Set<string>(); + set.add(name); + byPath.set(path, set); + } + + return [...byPath.keys()] + .sort() + .map(path => { + const names = [...(byPath.get(path)!)].sort(); + return `import {${names.join(', ')}} from "${path}";`; + });packages/core/solidity/src/account.ts (1)
178-179: Nit: apostrophe in comments."its not recognized" → "it's not recognized".
- transpiled: false, // name doesn't start with "I" so its not recognized as an "interface object" + transpiled: false, // name doesn't start with "I" so it's not recognized as an "interface object"Also applies to: 183-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
packages/core/solidity/src/account.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/custom.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/erc1155.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/erc20.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/erc721.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/governor.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/stablecoin.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/zip-foundry.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/zip-hardhat.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
.changeset/sour-poems-stop.md(1 hunks)packages/core/solidity/src/account.ts(1 hunks)packages/core/solidity/src/custom.test.ts.md(2 hunks)packages/core/solidity/src/erc1155.test.ts.md(9 hunks)packages/core/solidity/src/erc20.test.ts.md(21 hunks)packages/core/solidity/src/erc721.test.ts.md(11 hunks)packages/core/solidity/src/governor.test.ts.md(19 hunks)packages/core/solidity/src/print.ts(1 hunks)packages/core/solidity/src/stablecoin.test.ts.md(8 hunks)packages/core/solidity/src/zip-foundry.test.ts.md(7 hunks)packages/core/solidity/src/zip-hardhat.test.ts.md(5 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-08-29T22:26:44.931Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#653
File: packages/core/solidity/src/account.ts:189-201
Timestamp: 2025-08-29T22:26:44.931Z
Learning: In ERC-7579, accounts support multiple execution paths: the standard ERC-4337 path requiring validators for UserOp validation, and the module execution path where executor modules can directly call executeFromExecutor to execute transactions on behalf of the account. This means accounts initialized with only an executor module are functional and valid according to the ERC-7579 specification.
Applied to files:
packages/core/solidity/src/account.ts
📚 Learning: 2025-08-15T23:23:13.097Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#609
File: packages/core/solidity/src/signer.ts:31-38
Timestamp: 2025-08-15T23:23:13.097Z
Learning: In OpenZeppelin contracts-wizard, the `setUpgradeableAccount` function deliberately sets `c.upgradeable = false` after upgradeable setup to exclude EIP712Upgradeable and ERC7739Upgradeable variants (avoiding per-call SLOAD overhead). This architectural pattern necessitates manual `_disableInitializers()` setup in both account.ts and signer.ts since the standard upgradeable constructor logic doesn't apply when upgradeability is disabled post-setup.
Applied to files:
packages/core/solidity/src/account.tspackages/core/solidity/src/custom.test.ts.mdpackages/core/solidity/src/erc1155.test.ts.mdpackages/core/solidity/src/zip-hardhat.test.ts.mdpackages/core/solidity/src/erc721.test.ts.mdpackages/core/solidity/src/zip-foundry.test.ts.mdpackages/core/solidity/src/erc20.test.ts.md
📚 Learning: 2025-08-20T20:23:30.259Z
Learnt from: ericglau
PR: OpenZeppelin/contracts-wizard#609
File: packages/core/solidity/src/set-upgradeable.ts:0-0
Timestamp: 2025-08-20T20:23:30.259Z
Learning: In OpenZeppelin contracts-wizard, upgradeable contracts use two different strategies for Initializable imports: Account contracts directly import from contracts-upgradeable/proxy/utils/Initializable.sol for explicit clarity, while other upgradeable contracts (ERC20, ERC721, Governor, etc.) use helpers to automatically transpile imports to upgradeable versions. This architectural separation is intentional due to the special ERC-4337 requirements of account contracts.
Applied to files:
packages/core/solidity/src/custom.test.ts.mdpackages/core/solidity/src/governor.test.ts.mdpackages/core/solidity/src/erc1155.test.ts.mdpackages/core/solidity/src/zip-hardhat.test.ts.mdpackages/core/solidity/src/erc721.test.ts.mdpackages/core/solidity/src/zip-foundry.test.ts.mdpackages/core/solidity/src/erc20.test.ts.md
📚 Learning: 2025-08-21T19:44:06.797Z
Learnt from: ericglau
PR: OpenZeppelin/contracts-wizard#609
File: packages/core/solidity/src/account.ts:191-204
Timestamp: 2025-08-21T19:44:06.797Z
Learning: Initializable is available in both openzeppelin/contracts and openzeppelin/contracts-upgradeable packages, so conditional imports like `openzeppelin/${opts.upgradeable ? 'contracts-upgradeable' : 'contracts'}/proxy/utils/Initializable.sol` are valid and will resolve correctly.
Applied to files:
packages/core/solidity/src/custom.test.ts.mdpackages/core/solidity/src/governor.test.ts.mdpackages/core/solidity/src/erc1155.test.ts.mdpackages/core/solidity/src/zip-hardhat.test.ts.mdpackages/core/solidity/src/erc721.test.ts.mdpackages/core/solidity/src/zip-foundry.test.ts.mdpackages/core/solidity/src/erc20.test.ts.md
📚 Learning: 2025-08-15T22:52:34.129Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#609
File: packages/core/solidity/src/account.ts:89-0
Timestamp: 2025-08-15T22:52:34.129Z
Learning: In OpenZeppelin contracts-wizard, non-upgradeable accounts still require initializer logic (Initializable, _disableInitializers(), and initialize() function) because ERC-4337 accounts are typically deployed by factories as minimal clone proxies, which cannot use constructors effectively for initialization. This is the intended deployment pattern for ERC-4337 account abstraction, even for non-upgradeable accounts.
Applied to files:
packages/core/solidity/src/custom.test.ts.mdpackages/core/solidity/src/erc1155.test.ts.mdpackages/core/solidity/src/zip-hardhat.test.ts.mdpackages/core/solidity/src/erc721.test.ts.mdpackages/core/solidity/src/zip-foundry.test.ts.mdpackages/core/solidity/src/erc20.test.ts.md
📚 Learning: 2025-08-19T01:15:58.945Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#609
File: packages/core/solidity/src/signer.ts:45-49
Timestamp: 2025-08-19T01:15:58.945Z
Learning: The OpenZeppelin contracts-upgradeable package includes upgradeable versions of signer contracts like SignerECDSAUpgradeable, SignerP256Upgradeable, SignerRSAUpgradeable, MultiSignerERC7913Upgradeable, and MultiSignerERC7913WeightedUpgradeable in the contracts/utils/cryptography/signers directory.
Applied to files:
packages/core/solidity/src/zip-hardhat.test.ts.mdpackages/core/solidity/src/erc721.test.ts.mdpackages/core/solidity/src/zip-foundry.test.ts.mdpackages/core/solidity/src/erc20.test.ts.md
📚 Learning: 2025-08-19T01:15:58.945Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#609
File: packages/core/solidity/src/signer.ts:45-49
Timestamp: 2025-08-19T01:15:58.945Z
Learning: All OpenZeppelin signer contracts have upgradeable variants available in the contracts-upgradeable package: SignerECDSAUpgradeable, SignerP256Upgradeable, SignerRSAUpgradeable, MultiSignerERC7913Upgradeable, and MultiSignerERC7913WeightedUpgradeable all exist in contracts/utils/cryptography/signers/ directory.
Applied to files:
packages/core/solidity/src/zip-hardhat.test.ts.mdpackages/core/solidity/src/erc721.test.ts.mdpackages/core/solidity/src/zip-foundry.test.ts.md
📚 Learning: 2025-08-15T22:49:25.653Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#609
File: .changeset/sour-hats-grow.md:2-6
Timestamp: 2025-08-15T22:49:25.653Z
Learning: In OpenZeppelin contracts-wizard, breaking changes that have concrete migration paths (like dependency migrations from Community Contracts to OpenZeppelin Contracts) can be handled as minor version bumps instead of major bumps, per maintainer ernestognw's versioning policy.
Applied to files:
packages/core/solidity/src/zip-foundry.test.ts.md
🧬 Code graph analysis (1)
packages/core/solidity/src/print.ts (1)
packages/core/solidity/src/contract.ts (1)
imports(108-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (39)
packages/core/solidity/src/custom.test.ts.md (1)
82-82: LGTM: import reordering matches new path-grouped printer.Also applies to: 172-172
packages/core/solidity/src/account.ts (1)
175-184: LGTM: add missing IERC7579 constants for module type checks.Importing MODULE_TYPE_VALIDATOR and MODULE_TYPE_EXECUTOR with transpiled: false is correct here.
packages/core/solidity/src/governor.test.ts.md (1)
16-16: LGTM: TimelockController and related imports reordered/deduplicated per path-grouped import output.Also applies to: 106-106, 192-192, 284-284, 376-376, 465-465, 560-560, 646-646, 732-732, 913-913, 999-999, 1261-1261, 1350-1350, 1436-1436
packages/core/solidity/src/erc1155.test.ts.md (1)
16-16: LGTM: import grouping and ordering now by path; snapshots updated accordingly.Also applies to: 39-39, 138-138, 162-162, 204-204, 330-330, 365-365, 448-448, 449-449, 542-542, 543-543
packages/core/solidity/src/erc20.test.ts.md (1)
64-64: LGTM: import paths consolidated and ordered by path (Ownable, ERC1363[Upgradeable], ERC20Bridgeable[Upgradeable], Initializable/UUPS).Snapshots reflect the new printer behavior; no functional changes implied.
Also applies to: 185-185, 325-325, 380-380, 548-548, 576-576, 579-579, 608-608, 613-613, 648-648, 691-691, 719-719, 746-746, 749-749, 783-783, 829-829, 860-860, 863-863, 897-897, 900-900, 1027-1027, 1028-1028, 1031-1031, 1036-1036, 1136-1136, 1139-1139, 1224-1224, 1225-1225, 1227-1227, 1322-1322, 1323-1323, 1325-1325
packages/core/solidity/src/stablecoin.test.ts.md (1)
48-48: LGTM: reordered imports (Ownable, ERC1363, core ERC20/Permit, Community extensions, Bridgeable) to match path-grouped import printer.Also applies to: 169-169, 246-246, 301-301, 335-336, 370-371, 416-417, 594-595, 598-598, 604-604
packages/core/solidity/src/zip-foundry.test.ts.md (8)
336-346: Imports grouped by path look correct; ERC20 upgradeable set is consistent.
- Adding OwnableUpgradeable, Initializable, and UUPSUpgradeable here aligns with the upgradeable pattern used elsewhere.
- Keeping ERC20BridgeableUpgradeable under draft- path is fine for OZ ^5.4.0; just ensure the targeted OZ version still exposes this path.
If you want, I can run a quick repo scan to confirm no other snapshots still import these from split lines or different paths.
371-376: Guard + minting logic unchanged; import additions unblock compilation.The require and conditional mint remain intact; added imports introduce no behavioral changes.
581-582: Consolidated ERC20Upgradeable/Permit imports — good.Grouping by path improves readability and matches the PR goal.
766-770: ERC721 upgradeable sample: Initializable and ERC721Upgradeable imports.This matches the base list in the contract; ordering-by-path is consistent.
933-933: ERC1155 import moved next to Ownable — OK.Reordering to path-based grouping is fine and non-functional.
1099-1100: ERC1155Transparent: Initializable and ERC1155Upgradeable imports added.Consistent with initializer flow; no issues.
1264-1267: Account ECDSA: Add EIP712 and ERC7739 imports.These imports were missing when using a modular account without a signer; adding them matches the inheritance list.
1426-1435: Account ECDSA UUPS: Upgradeable signer + non-upgradeable EIP712 combo is intentional.Importing Initializable/UUPSUpgradeable and SignerECDSAUpgradeable while keeping EIP712 (non-upgradeable) aligns with the account-specific pattern to avoid extra SLOAD overhead.
Confirm generator logic always pairs EIP712 (non-upgradeable) with accounts, including UUPS, to prevent accidental drift.
packages/core/solidity/src/zip-hardhat.test.ts.md (14)
143-152: UUPS ERC20: Added upgradeability imports and grouped by path.OwnableUpgradeable, Initializable, UUPSUpgradeable, and draft ERC20BridgeableUpgradeable are correctly imported and ordered.
Verify OZ version used in tests exposes draft-ERC20BridgeableUpgradeable under the same path.
310-313: ERC721 upgradeable: Initializable and ERC721Upgradeable imports.Matches inheritance; ordering by path is consistent.
415-416: ERC1155 basic: Import order normalized.Non-functional, improves consistency.
501-504: Account ECDSA: Add EIP712 and ERC7739 imports.Fixes missing imports for modular accounts without signer.
577-584: Account ECDSA UUPS: Add upgradeability imports and keep EIP712 non-upgradeable.Consistent with intended pattern; ordering is by path.
445-445: Votes sample: Add EIP712 import.Required due to EIP712 base; good catch.
479-479: Votes + blocknumber: Add EIP712 import.Matches inheritance; fine.
513-513: Votes + timestamp: Add EIP712 import.Consistent; no functional change.
554-561: Full upgradeable transparent: Add OwnableUpgradeable, Initializable, EIP712Upgradeable imports.Aligns with contract bases and path-grouped ordering.
627-635: Full upgradeable UUPS: Add OwnableUpgradeable, Initializable, UUPSUpgradeable, EIP712Upgradeable imports.Looks correct and consistent.
709-717: Full upgradeable UUPS + managed: Add Initializable, UUPSUpgradeable, EIP712Upgradeable imports.Contract’s inheritance requires these; ordering matches path rule.
296-296: Pausable sample: Reintroduced Ownable import.Matches Ownable usage in constructor/guards.
335-335: Mintable sample: ERC721 import placement adjusted.Non-functional; consistent with path ordering.
417-417: Mintable + incremental: ERC721 import moved under path ordering.LGTM.
packages/core/solidity/src/erc721.test.ts.md (11)
142-142: Mintable + URI Storage: Add Ownable import.Required by inheritance; resolves missing import.
190-190: Mintable + URI Storage + incremental: Add Ownable import.Consistent with constructor and onlyOwner guard.
296-296: Pausable: Add Ownable import.Matches onlyOwner usage.
335-335: Mintable: ERC721 import position normalized.Path-ordered grouping; OK.
417-417: Mintable + incremental: ERC721 import ordering.Non-functional cleanup.
445-445: Votes: Add EIP712 import.Required by base EIP712; good.
479-479: Votes + blocknumber: Add EIP712 import.Correct and necessary.
513-513: Votes + timestamp: Add EIP712 import.Matches usage of EIP712 base.
554-556: Full upgradeable transparent: Added OwnableUpgradeable, Initializable, EIP712Upgradeable imports.All bases are now correctly imported and grouped by path.
Also applies to: 561-561
627-629: Full upgradeable UUPS: Added OwnableUpgradeable, Initializable, UUPSUpgradeable, EIP712Upgradeable.Imports align with inheritance; ordering-by-path applied.
Also applies to: 635-635
709-710: Full upgradeable UUPS + managed: Initializable, UUPSUpgradeable, EIP712Upgradeable imports added.Consistent with AccessManagedUpgradeable pattern; no concerns.
Also applies to: 716-716
| c.addImportOnly({ | ||
| name: 'MODULE_TYPE_VALIDATOR', | ||
| path: '@openzeppelin/contracts/interfaces/draft-IERC7579.sol', | ||
| transpiled: false, // name doesn't start with "I" so its not recognized as an "interface object" | ||
| }); | ||
| c.addImportOnly({ | ||
| name: 'MODULE_TYPE_EXECUTOR', | ||
| path: '@openzeppelin/contracts/interfaces/draft-IERC7579.sol', | ||
| transpiled: false, // name doesn't start with "I" so its not recognized as an "interface object" | ||
| }); |
There was a problem hiding this comment.
afaik we have a test that checks whether the contracts compile in packages/core/solidity/src/test.ts, how come it missed it?
There was a problem hiding this comment.
solidity/src/test.ts does not check the compilation.
This zip tests do check compilation ...and a bit more (deployment)
So I tried to add zip tests that include a account with 7579 and no module. It was a pain and in the end was blocked by the tests script failling because they need a valid ERC7579 module to run the construction/initialization.
AFAIK, we don't have tests that check compilation without also deploying the contract (and proxy).
There was a problem hiding this comment.
solidity/src/test.ts does compilation tests, but the signer option was missing the false case in the generator.
Added it (by using exported options defined elsewhere, so any new values in the future won't be missed) in 88cf4c8.
However, this now reveals compile warnings due to the below, which fails the tests. For example, this occurs when no options are selected:
Warning: Function state mutability can be restricted to pure
function _rawSignatureValidation(bytes32 hash, bytes calldata signature)
internal
view
override
returns (bool)
{
// Custom validation logic
return false;
}Should we ignore these warnings in the tests?
There was a problem hiding this comment.
My bad, I got confused between test.ts and account.test.ts
There was a problem hiding this comment.
there are multiple options to make that warning go away, I'm not sure which one is best:
-
make the function pure: if someone start filling the code, they may get error. Is it obvious that they can safelly change pure to view ?
-
make the function "virtual override" instead of just "override". Will people be confused by the virtual keyword ?
-
Add a line to is view, and does nothing. Not example
msg.sender;. Possibly with a comment. I think its probably more confusing than options 1 and 2. -
Just refuse to build a contract that doesn't support module AND doesn't have a signer.
-
Somehow disable that warning
There was a problem hiding this comment.
or selectively ignore the warnings our tests by filtering them in hardhat.config.ts. However, users will still see the warning in their projects and might be confused.
There was a problem hiding this comment.
- Just refuse to build a contract that doesn't support module AND doesn't have a signer.
Is there any use case for a contract that doesn't support module AND doesn't have a signer? If not, then I think we should go with this option.
There was a problem hiding this comment.
I just noticed the constructors are receiving the arguments in calldata, which is not possible. Here's an example:
// SPDX-License-Identifier: MIT
// Compatible with OpenZeppelin Contracts ^5.4.0
pragma solidity ^0.8.27;
import {Account} from "@openzeppelin/contracts/account/Account.sol";
import {ERC7821} from "@openzeppelin/contracts/account/extensions/draft-ERC7821.sol";
contract MyAccount is Account, ERC7821 {
function _erc7821AuthorizedExecutor(address caller, bytes32 mode, bytes calldata executionData)
internal
view
override
returns (bool)
{
return caller == address(entryPoint()) || super._erc7821AuthorizedExecutor(caller, mode, executionData);
}
function _rawSignatureValidation(bytes32 hash, bytes calldata signature)
internal
view
override
returns (bool)
{
// Custom validation logic
return false;
}
}
This PR also changes the way imports are written, consolidating multiple items are imported from the same file. This changes the way imports are ordered (by path instead of by name)