Add an initializer to non upgradeable AccountERC7579 variants #653
Conversation
Reviewer's GuideRefactors non-upgradeable AccountERC7579 variants to use an initializer function and disable initializers on implementation contracts, updating both test fixtures and the contract builder accordingly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughCentralizes account initialization to Initializable-based initialize functions for ERC-7579 accounts, replaces constructor branches, and introduces AccountFactory-gated constructor locking. Updates account generation logic in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant AF as AccountFactory
participant Impl as Account Implementation (Initializable)
participant Proxy as Proxy/Deployed Account
participant Module as ERC-7579 Module
Dev->>AF: requestAccountDeployment(params)
AF->>Impl: deploy implementation (constructor runs)
note right of Impl: _disableInitializers() called
AF->>Proxy: deploy proxy pointing to Impl
AF->>Proxy: call initialize(moduleTypeId, module, initData)
Proxy->>Impl: delegatecall initialize(...)
Impl->>Impl: validate moduleTypeId
Impl->>Module: _installModule(moduleTypeId, module, initData)
Module-->>Impl: initialized
Impl-->>Proxy: initialization complete
Proxy-->>AF: ready
AF-->>Dev: account address
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/metal-ghosts-listen.md (1)
5-6: Remove stray trailing character to avoid changeset parse issues.There's an orphaned "6" on Line 6 that will break the changeset YAML fence parsing.
Apply this diff:
Add initializers to ERC-7579 non-upgradeable accounts -6
🧹 Nitpick comments (7)
.changeset/metal-ghosts-listen.md (1)
5-5: Consider clarifying the scope in the summary.Suggest: “Add initializer entry points to non-upgradeable AccountERC7579 variants for factory deployments.”
packages/core/solidity/src/signer.ts (1)
70-74: Use the constant to avoid string drift between the check and insertion.No functional change, but using the same variable reduces risk if the sentinel ever changes.
Apply this diff:
- c.addConstructorCode(`_disableInitializers();`); + c.addConstructorCode(disableInitializers);packages/core/solidity/src/account.ts (2)
181-191: Nit: fix comment phrasing.“executor of validation” reads oddly; suggest “executor or validator”.
Apply this diff:
- // Accounts that use ERC7579 without a signer must be constructed with at least one module (executor of validation) + // Accounts that use ERC7579 without a signer must be constructed with at least one module (executor or validator)
243-261: Optional: prefer upgradeable signer bases even for non-upgradeable accounts to remove constructor args.Using initializer-only flows for signers (via Signer*Upgradeable + __init calls) avoids constructor parameters on the implementation, simplifying factory deployments.
I can draft a targeted change in signer.ts to use upgradeable signer parents when opts.signer is set and the account itself is non-upgradeable.
packages/core/solidity/src/account.test.ts.md (3)
228-228: Use non-upgradeable Initializable import for non-upgradeable variants.These non-upgradeable ERC-7579 snapshots import Initializable from contracts-upgradeable. Prefer the non-upgradeable path for consistency and to avoid mixing packages in generated code.
- import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; + import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol";Also applies to: 282-282, 334-334, 390-390
231-237: Consider dropping the oz-upgrades “unsafe-allow-reachable constructor” tag in non-upgradeable clones.These contracts aren’t upgradeable; the annotation is unnecessary and may confuse readers. The pattern of disabling initializers is correct; just omit the tag in non-upgradeable snapshots.
- /// @custom:oz-upgrades-unsafe-allow-reachable constructor constructor() EIP712("MyAccount", "1") { // Accounts are typically deployed and initialized as clones during their first user op, // therefore, initializers are disabled for the implementation contract _disableInitializers(); }Also applies to: 285-291, 337-343, 393-399
239-245: Tighten initialize() validation and add reason strings.
- Add explicit revert reasons for moduleTypeId.
- Optionally guard against zero module address to catch misconfiguration early (cheap and clear).
- function initialize(uint256 moduleTypeId, address module, bytes calldata initData) + function initialize(uint256 moduleTypeId, address module, bytes calldata initData) public initializer { - require(moduleTypeId == MODULE_TYPE_VALIDATOR || moduleTypeId == MODULE_TYPE_EXECUTOR); + require( + moduleTypeId == MODULE_TYPE_VALIDATOR || moduleTypeId == MODULE_TYPE_EXECUTOR, + "Invalid moduleTypeId" + ); + require(module != address(0), "Module is zero address"); _installModule(moduleTypeId, module, initData); }Also applies to: 293-299, 345-351, 401-407
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/core/solidity/src/account.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
.changeset/metal-ghosts-listen.md(1 hunks)packages/core/solidity/src/account.test.ts.md(12 hunks)packages/core/solidity/src/account.ts(3 hunks)packages/core/solidity/src/signer.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
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.
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.
📚 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/signer.ts.changeset/metal-ghosts-listen.mdpackages/core/solidity/src/account.tspackages/core/solidity/src/account.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:
.changeset/metal-ghosts-listen.mdpackages/core/solidity/src/account.tspackages/core/solidity/src/account.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:
.changeset/metal-ghosts-listen.mdpackages/core/solidity/src/account.tspackages/core/solidity/src/account.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/account.tspackages/core/solidity/src/account.test.ts.md
🧬 Code graph analysis (1)
packages/core/solidity/src/account.ts (2)
packages/core/solidity/src/contract.ts (1)
ContractBuilder(78-238)packages/core/solidity/src/signer.ts (1)
addLockingConstructorAllowReachable(67-75)
⏰ 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 (6)
packages/core/solidity/src/account.ts (5)
198-201: LGTM: initializer added for non-upgradeable ERC7579 accounts without a signer.This unblocks factory-based deployments of non-upgradeable variants.
239-239: Good: constructor locking comment centralizes factory-deployment guidance.Ensures implementations are locked and Upgrades Plugins validations are happy.
339-345: Nice helper extraction.Reusing the locking-constructor with standardized comments keeps the generation consistent.
192-196: Align Initializable import path strategy with the rest of the file.Elsewhere (Lines 231-236) the path is selected conditionally by upgradeability; here it’s hardcoded to “contracts-upgradeable”. Make it consistent to avoid accidentally requiring the upgradeable package when not needed.
Apply this diff:
- c.addParent({ - name: 'Initializable', - path: '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol', - }); + c.addParent({ + name: 'Initializable', + path: opts.upgradeable + ? '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol' + : '@openzeppelin/contracts/proxy/utils/Initializable.sol', + });⛔ Skipped due to learnings
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.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.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.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.
231-236: Ignore duplicate Initializable import in account.ts
The twoc.addParentcalls are intentional: the first always imports the upgradeableInitializablefor ERC-4337 account requirements, and the second handles conditional non-upgradeable vs upgradeable imports.Likely an incorrect or invalid review comment.
packages/core/solidity/src/account.test.ts.md (1)
239-245: Confirm single-module initialize() doesn’t violate 7579 “validator + executor” prerequisites.Given PR #609’s requirement for at least one validator and one executor, please confirm that accounts initialized with only one module won’t fail their first user operation, and document/encode the expected two-step setup if needed. If failure is possible, consider an overload that installs both in one call.
|
AFAIK, accounts that are deployed as implementation and then initialized as clone are already supported through the "upgrade transparent" options. Non-upgradeable is for standalone accounts that have no proxy/clone on top. While not the usecase we expect the most, I still think it is valid. If we really want to hide it, then I would rather remove the "non-upgradeable" option from the wizard UI, and just have a toggle to enable/disable the UUPS part. |
Description
In #609, we introduced a change that forces the installation of at least a validator and an executor in AccountERC7579. However, non upgradeable accounts are deployed by factories and must use an initializer as all other variants.
Currently, these account variants only work if they're not deployed from a factory. This PR clarifies how they should be used
Summary by Sourcery
Switch non-upgradeable AccountERC7579 variants to use OpenZeppelin Initializable patterns so they can be deployed via factories. Always add Initializable, disable initializers in the implementation constructor, and generate initialize() functions for module setup, extracting common logic into a new helper.
Bug Fixes:
Enhancements:
Tests: