Update migrated imports from community to vanilla contracts #609
Conversation
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on: |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThreads upgradeability through account generation, signer wiring, tooling, UI, schemas, and zip generators; replaces a single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI (AccountControls)
participant MCP as MCP Tool
participant Gen as Account Builder
participant Upg as setUpgradeableAccount
participant Print as Printer
participant Zip as Zip Generator
UI->>MCP: opts { signer, upgradeable, ... }
MCP->>Gen: buildAccount(opts)
Gen->>Upg: setUpgradeableAccount(c, opts.upgradeable)
alt upgradeable == false
Gen->>Gen: wire non-upgradeable imports/paths\nattach signer via constructor args
else upgradeable in {'uups','transparent'}
Gen->>Gen: set flags (install upgradeable, plugins, transpile)\nadd initialize(...) and initializer wiring\nadapt base names/overrides to *Upgradeable
end
Gen->>Print: emit constructor or initialize (shouldUseInitializers)
Gen-->>Zip: contract c with flags
alt shouldUseUpgradesPluginsForProxyDeployment
Zip->>Zip: generate plugin-based proxy deployment
else
Zip->>Zip: generate direct contract deployment
end
sequenceDiagram
autonumber
participant Gen as Account Builder
participant S as addSigner
participant OZ as OpenZeppelin Contracts 5.4.0
Gen->>S: addSigner(c, signer, upgradeable)
alt signer == ERC7702 and upgradeable != false
S-->>Gen: throw OptionsError (upgradeable not supported)
else signer in {ECDSA,P256,RSA}
S->>OZ: reference SignerX.sol (contracts path)
S-->>Gen: attach constructor args from signerArgs
else signer in {MultiSigner, MultiSignerWeighted}
alt upgradeable != false
S->>OZ: derive *Upgradeable path (contracts-upgradeable)\nadd initializer and locking constructor
else
S->>OZ: use non-upgradeable path and constructor args
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
✨ 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 (
|
User descriptionTODO
PR TypeEnhancement Description
Diagram Walkthroughflowchart LR
A["Community Contracts"] -- "migrate" --> B["Vanilla Contracts 5.4.0"]
C["Non-upgradeable Signers"] -- "add support" --> D["Upgradeable Signers"]
E["Initializer Pattern"] -- "replace with" --> F["Constructor Pattern"]
B --> G["Updated Account Generation"]
D --> G
F --> G
|
| Relevant files | |||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Enhancement | 7 files
| ||||||||||||||
| Tests | |||||||||||||||
| Configuration changes | |||||||||||||||
| Documentation | |||||||||||||||
| Additional files | 1 files
|
![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
🧹 Nitpick comments (1)
packages/core/solidity/package.json (1)
25-25: Keep and explicitly pin@openzeppelin/community-contractsin devDependenciesAfter verification,
@openzeppelin/community-contractsis indeed required by several parts of thecore/soliditypackage (stablecoin fixtures, thepreparescript, import‐library utilities, and associated tests). To ensure reproducible installs and clear auditability, please update the devDependency to pin a full 40-character commit hash or an official release tag rather than the current 7-character short SHA.Locations to update:
packages/core/solidity/package.json(devDependencies entry)Proposed diff:
- "@openzeppelin/community-contracts": "git+https://github.com/OpenZeppelin/openzeppelin-community-contracts.git#2d607bd", + "@openzeppelin/community-contracts": "git+https://github.com/OpenZeppelin/openzeppelin-community-contracts.git#<full-40-char-commit-or-tag>"
📜 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)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
packages/core/solidity/package.json(1 hunks)
⏰ 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). (10)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: build (stellar, compile)
- GitHub Check: build (stellar, default)
- GitHub Check: build (stylus, default)
- GitHub Check: build (cairo, default)
- GitHub Check: build (solidity, default)
- GitHub Check: mcp
- GitHub Check: format-lint
- GitHub Check: deno-check
- GitHub Check: check
|
@coderabbitai The |
|
Tip For best results, initiate chat on the files or code changes. @ericglau Thank you for the clarification! I understand now that the 🧠 Learnings used |
Amazing, thanks @ericglau, sure.
Yeah, good catch. I see we're consistently using it like that in all other upgradeable variants so looks good to me.
Got it. Also all good here.
Also great catch. So these 2 contracts are stateless (no storage variables) so it's safe to use either. I think we should've marked this as |
| contract MyAccount is Initializable, Account, EIP712, ERC7739, AccountERC7579Upgradeable, UUPSUpgradeable {␊ | ||
| /// @custom:oz-upgrades-unsafe-allow-reachable constructor␊ | ||
| constructor() EIP712("MyAccount", "1") {␊ | ||
| _disableInitializers();␊ | ||
| }␊ | ||
| ␊ | ||
| function isValidSignature(bytes32 hash, bytes calldata signature)␊ | ||
| public␊ | ||
| view␊ | ||
| override(AccountERC7579Upgradeable, ERC7739)␊ | ||
| returns (bytes4)␊ | ||
| {␊ | ||
| // ERC-7739 can return the ERC-1271 magic value, 0xffffffff (invalid) or 0x77390001 (detection).␊ | ||
| // If the returned value is 0xffffffff, fallback to ERC-7579 validation.␊ | ||
| bytes4 erc7739magic = ERC7739.isValidSignature(hash, signature);␊ | ||
| return erc7739magic == bytes4(0xffffffff) ? AccountERC7579Upgradeable.isValidSignature(hash, signature) : erc7739magic;␊ | ||
| }␊ | ||
| ␊ | ||
| function _authorizeUpgrade(address newImplementation)␊ | ||
| internal␊ | ||
| override␊ | ||
| onlyEntryPointOrSelf␊ | ||
| {}␊ | ||
| ␊ | ||
| // The following functions are overrides required by Solidity.␊ | ||
| ␊ | ||
| function _validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash)␊ | ||
| internal␊ | ||
| override(Account, AccountERC7579Upgradeable)␊ | ||
| returns (uint256)␊ | ||
| {␊ | ||
| return super._validateUserOp(userOp, userOpHash);␊ | ||
| }␊ | ||
| }␊ |
There was a problem hiding this comment.
This accoutn needs an initializer that installs a first module. Otherwise, its just bricked at construction.
This is particularly visible in this case, but I believe its all upgradeable accounts that are missing an initializer. In most cases the initializer would be empty, so we don't really care, but in cases where the initializer is actually needed that is more serious.
There was a problem hiding this comment.
Note: we have the same issue with the non-upgradeable version. If we don't have a signer, and just use 7579, the constructor doesn't install any "initial module"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/solidity/src/signer.ts (2)
33-49: Upgradeable path is correctly wired; ensure deps get installed
- Initializable + locking constructor + initializer + Upgradeable parent are correctly applied.
- Minor: confirm the generator ensures
@openzeppelin/contracts-upgradeableis installed in zip scenarios when this path is hit (either via import scanning or by toggling a flag). If needed, setc.shouldInstallContractsUpgradeable = truehere.If you rely on a flag, minimally:
c.addParent({ name: 'Initializable', path: '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol', }); + c.shouldInstallContractsUpgradeable = true;
62-77: Placement of the “unsafe-allow-reachable constructor” tag — add a contract-level tag for robustnessThe constructor-body doc comment may be missed by some tooling. Consider also emitting a contract-level natspec tag to be extra safe.
Apply in
addLockingConstructorAllowReachable:export function addLockingConstructorAllowReachable(c: ContractBuilder, bodyComments?: string[]): void { const disableInitializers = '_disableInitializers();'; if (!c.constructorCode.includes(disableInitializers)) { - c.addConstructorComment('/// @custom:oz-upgrades-unsafe-allow-reachable constructor'); + c.addConstructorComment('/// @custom:oz-upgrades-unsafe-allow-reachable constructor'); + c.addNatspecTag('@custom:oz-upgrades-unsafe-allow-reachable', 'constructor'); bodyComments?.forEach(comment => c.addConstructorCode(comment)); c.addConstructorCode(disableInitializers); } }
📜 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 (2)
packages/core/solidity/src/account.ts(9 hunks)packages/core/solidity/src/signer.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
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/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.
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: 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.
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.
📚 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/signer.tspackages/core/solidity/src/account.ts
📚 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/signer.tspackages/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/signer.tspackages/core/solidity/src/account.ts
📚 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/signer.tspackages/core/solidity/src/account.ts
📚 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/signer.tspackages/core/solidity/src/account.ts
📚 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/signer.tspackages/core/solidity/src/account.ts
📚 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/signer.tspackages/core/solidity/src/account.ts
🧬 Code graph analysis (2)
packages/core/solidity/src/signer.ts (2)
packages/core/solidity/src/contract.ts (1)
ContractBuilder(78-238)packages/core/solidity/src/set-upgradeable.ts (1)
Upgradeable(8-8)
packages/core/solidity/src/account.ts (3)
packages/core/solidity/src/set-upgradeable.ts (1)
setUpgradeableAccount(64-78)packages/core/solidity/src/signer.ts (5)
addSigner(9-60)addLockingConstructorAllowReachable(69-77)signerArgs(106-125)signerFunctions(127-137)signers(79-104)packages/core/solidity/src/contract.ts (2)
functions(119-121)ContractBuilder(78-238)
⏰ 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). (4)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: build (stellar, compile)
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (19)
packages/core/solidity/src/signer.ts (7)
9-9: Signature update to addSigner looks goodAdding the
upgradeableparameter is consistent with the PR goals and enables clean branching.
20-27: Good guard: block ERC7702 when upgradeable is trueClear error message, aligned with “EOAs can upgrade by redelegation.”
41-42: Initializer modifier present — good catchThe
initializermodifier prevents re-initialization; this addresses the earlier risk.
50-55: Non-upgradeable constructor flow is correctPassing constructor args through to the parent is the right approach for vanilla signers.
79-104: Signer import map updated to vanilla Contracts — correct pathsPaths point to the new
contracts/utils/cryptography/signerslocations introduced in 5.4.0.
106-125: Signer args match 5.4.0 APIs (types/ordering)
- ECDSA: address
- P256: qx, qy as bytes32
- RSA: e, n as bytes
- Multisig: bytes[] signers + uint64 threshold
- MultisigWeighted: bytes[] signers + uint64[] weights + uint64 threshold
These align with OZ 5.4.0.
12-16: Override base is correct
I verified in the OZ 5.4.0 upgradeable sources that_rawSignatureValidationis declared only in MultiSignerERC7913Upgradeable (at line 237) and not re-declared in MultiSignerERC7913WeightedUpgradeable. The current override usingsigners.Multisig.namecovers the sole declaration and will not cause any “does not override anything” errors. No changes needed.packages/core/solidity/src/account.ts (12)
7-15: LGTM - Clean import structure for signer functionality.The imports from
./signerare well-organized and provide all necessary signer-related functionality including types, utilities, and the mainaddSignerfunction. The addition ofaddLockingConstructorAllowReachablealigns with the upgradeable account patterns.
70-70: Excellent integration of upgradeable account setup.The call to
setUpgradeableAccountproperly integrates the upgradeability configuration from the common options. This follows the established pattern from other contract types and ensures consistency across the codebase.
87-87: Successful migration to vanilla OpenZeppelin contracts.The base Account import has been correctly migrated from community contracts to the standard
@openzeppelin/contracts/account/Account.solpath. This aligns with the PR objectives to use OpenZeppelin Contracts 5.4.0 instead of community contracts.
96-97: Proper signer integration with upgradeability support.The
addSignercall now correctly passes the upgradeable flag, and the subsequentaddSignerInitializercall handles the initialization logic. This design properly separates concerns between signer setup and initialization patterns.
163-166: Well-implemented dynamic naming for upgradeable variants.The conditional naming logic correctly handles both upgradeable and non-upgradeable variants:
baseERC7579AccountName: AccountERC7579 vs AccountERC7579UpgradeablefullERC7579AccountName: Includes the specific module type with Upgradeable suffixpackageName: Correctly switches between contracts and contracts-upgradeableThis follows the established pattern and addresses the objective to support both deployment modes.
178-203: Robust initialization pattern for ERC7579 without signers.The logic properly handles the requirement that ERC7579 accounts without signers must have at least one module. The conditional branching between upgradeable and non-upgradeable flows is correct:
- Upgradeable: Uses initializer pattern with
InitializableandaddLockingConstructorAllowReachable- Non-upgradeable: Uses constructor arguments and constructor code
The validation requiring either validator or executor module types is appropriate for ERC7579 compliance.
205-222: Sophisticated signature validation override logic.The
isValidSignatureoverride handling is well-designed:
- Correctly overrides the base ERC7579 account
- For ERC7739: Implements fallback logic where ERC7739 returning
0xfffffffffalls back to ERC7579 validation- Dynamic naming ensures the correct base contract is referenced
The comment explaining the ERC7739 magic values (0xffffffff for invalid, 0x77390001 for detection) is helpful for maintainability.
224-267: Well-architected initializer pattern for non-upgradeable accounts.The
addSignerInitializerfunction properly implements the ERC-4337 deployment pattern where accounts are deployed as clones by factories. Key strengths:
- Correct architectural understanding: Uses initializers even for non-upgradeable accounts because ERC-4337 accounts are deployed as minimal clone proxies
- Proper Initializable import: Conditionally imports from the correct package (contracts vs contracts-upgradeable)
- Good constructor comments: Explains why initializers are disabled for the implementation
- Complete signer initialization: Handles all signer types with appropriate initialization calls
The early returns for upgradeable accounts (handled in signer.ts) and ERC7702 (no initialization needed) are appropriate.
315-343: Complex but correct signature validation disambiguation.The logic for handling conflicts between Signer and AccountERC7579 implementations is sophisticated:
- Dynamic naming: Correctly constructs upgradeable variant names
- Import management: Properly imports AbstractSigner for override resolution
- Inheritance guidance: Provides clear comments about inheritance order requirements
- MultisigWeighted handling: Correctly imports the base Multisig contract with dynamic path resolution
The comment about ensuring correct inheritance order is crucial for proper function resolution in Solidity's C3 linearization.
385-392: Correct type updates for threshold and weights.The function signatures have been updated to use
uint64for threshold and weight parameters, which aligns with the OpenZeppelin Contracts 5.4.0 signer implementations. This type change is more gas-efficient thanuint256for these use cases.
151-151: The next step is to confirm which OpenZeppelin Contracts version you’re targeting so we can verify that the path exists in that release. I’ve added a script to extract the installed version from your package.json—once we have that, we can check GitHub for the corresponding file.
109-109: Ensure draft-ERC7739.sol is present in your OpenZeppelin packageIt looks like searching your codebase for “ERC7739” only turned up references in your tests and schema definitions, but didn’t confirm that the actual Solidity file exists in the installed OpenZeppelin Contracts. You should:
- Verify that your project’s installed version of @openzeppelin/contracts (in package.json) includes utils/cryptography/signers/draft-ERC7739.sol.
- If it isn’t present, either upgrade to an OpenZeppelin release that ships this draft or adjust the import to point at the correct path.
- Double-check your node_modules or the tarball of @openzeppelin/contracts to ensure the file name and directory structure match exactly.
Please confirm that the import path
path: '@openzeppelin/contracts/utils/cryptography/signers/draft-ERC7739.sol',actually resolves to a file in your dependencies.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/core/solidity/src/helpers.ts (2)
1-1: Use POSIX path utils to avoid backslashes in Solidity importsNode’s default path API is OS-dependent and can emit backslashes on Windows. Use path.posix to guarantee forward slashes in import specifiers.
Apply:
-import path from "path"; +import { posix as path } from "node:path";
20-21: Make the contracts segment replacement more robustIf '/contracts/' isn’t present, the replacement is a no-op; that’s fine. Consider being explicit to avoid accidental matches in other segments.
Example:
- dir: dir.replace('/contracts/', '/contracts-upgradeable/'), + dir: dir.includes('/contracts/') ? dir.replace('/contracts/', '/contracts-upgradeable/') : dir,packages/core/solidity/src/account.ts (1)
233-239: Remove unreachable ternary for Initializable importThis branch only executes when opts.upgradeable is false (early return above), so the ternary’s true side is unreachable. Simplify.
Apply:
- path: opts.upgradeable - ? '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol' - : '@openzeppelin/contracts/proxy/utils/Initializable.sol', + path: '@openzeppelin/contracts/proxy/utils/Initializable.sol',
📜 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 selected for processing (3)
packages/core/solidity/src/account.ts(9 hunks)packages/core/solidity/src/helpers.ts(1 hunks)packages/core/solidity/src/signer.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
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/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: 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.
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: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.
📚 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/helpers.tspackages/core/solidity/src/account.tspackages/core/solidity/src/signer.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/signer.ts
📚 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/account.tspackages/core/solidity/src/signer.ts
📚 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/account.tspackages/core/solidity/src/signer.ts
📚 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/account.tspackages/core/solidity/src/signer.ts
📚 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/account.tspackages/core/solidity/src/signer.ts
📚 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/account.tspackages/core/solidity/src/signer.ts
🧬 Code graph analysis (2)
packages/core/solidity/src/account.ts (4)
packages/core/solidity/src/set-upgradeable.ts (1)
setUpgradeableAccount(64-78)packages/core/solidity/src/signer.ts (5)
addSigner(10-59)addLockingConstructorAllowReachable(68-76)signerArgs(105-124)signerFunctions(126-136)signers(78-103)packages/core/solidity/src/helpers.ts (1)
makeUpgradeable(12-25)packages/core/solidity/src/contract.ts (2)
functions(119-121)ContractBuilder(78-238)
packages/core/solidity/src/signer.ts (3)
packages/core/solidity/src/contract.ts (1)
ContractBuilder(78-238)packages/core/solidity/src/set-upgradeable.ts (1)
Upgradeable(8-8)packages/core/solidity/src/helpers.ts (1)
makeUpgradeable(12-25)
🪛 Biome (2.1.2)
packages/core/solidity/src/helpers.ts
[error] 18-18: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 GitHub Actions: Test
packages/core/solidity/src/helpers.ts
[warning] 1-1: Prettier formatting issue detected in this file. Run 'prettier --write' to fix.
packages/core/solidity/src/account.ts
[warning] 1-1: Prettier formatting issue detected in this file. Run 'prettier --write' to fix.
packages/core/solidity/src/signer.ts
[warning] 1-1: Prettier formatting issue detected in this file. Run 'prettier --write' to fix.
⏰ 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). (2)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (11)
packages/core/solidity/src/helpers.ts (1)
1-25: Formatting: run PrettierCI flagged a Prettier issue. Please run: prettier --write packages/core/solidity/src/helpers.ts
packages/core/solidity/src/account.ts (5)
164-176: Dynamic ERC-7579 base selection looks correctmakeUpgradeable is applied consistently to both names and paths for AccountERC7579 and Hooked variants. This aligns with OZ 5.4 layout.
205-218: isValidSignature fallback logic is soundERC-7739 fallback to ERC-7579 on 0xffffffff is correctly implemented, and overrides reference the upgradeability-adjusted base name.
315-340: Good disambiguation for _rawSignatureValidation with MultisigWeightedImporting MultiSigner (base of Weighted) for override disambiguation is appropriate. No action needed.
71-71: Upgradeability wiring is placed correctlysetUpgradeableAccount is invoked before imports/overrides that depend on upgradeable state. Good sequencing.
1-394: Formatting: run PrettierCI flagged a Prettier issue. Please run: prettier --write packages/core/solidity/src/account.ts
packages/core/solidity/src/signer.ts (5)
35-47: Upgradeable signer path and initializer setup LGTMUses Initializable, locks constructor, adds initializer, and imports the correct Upgradeable signer via makeUpgradeable.
49-54: Non-upgradeable constructor flow LGTMConstructor args are forwarded to vanilla signer parents; aligns with OZ 5.4 constructors.
13-17: Override target name selection is correct for Weighted multisigTargeting MultiSigner for the override (and applying makeUpgradeable) matches the inheritance (Weighted extends MultiSigner).
68-76: Idempotent locking constructor helper is well-factoredaddLockingConstructorAllowReachable avoids duplicate inserts and adds the correct validation tag.
1-137: Formatting: run PrettierCI flagged a Prettier issue. Please run: prettier --write packages/core/solidity/src/signer.ts
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/solidity/src/signer.ts (1)
46-51: Good: no constructor args in upgradeable branch; args only for non-upgradeableThis implements the proxy-safe pattern and resolves the earlier concern about constructors in upgradeable flows.
🧹 Nitpick comments (3)
packages/core/solidity/src/signer.ts (3)
16-24: Reorder: throw before mutating the builder in the ERC7702 pathAvoid partial mutations if an error is thrown by moving the parent addition after the error check.
case 'ERC7702': - c.addParent(signers[signer]); - if (upgradeable) { + if (upgradeable) { throw new OptionsError({ erc7702: 'EOAs can upgrade by redelegating to a new account', upgradeable: 'EOAs can upgrade by redelegating to a new account', }); - } + } + c.addParent(signers[signer]); break;
41-44: Use makeUpgradeable for the name too (minor consistency)Keeps name/path transformation in sync and avoids manual template strings.
- c.addParent({ - name: `${signers[signer].name}Upgradeable`, + c.addParent({ + name: makeUpgradeable(signers[signer].name, upgradeable), path: makeUpgradeable(signers[signer].path, upgradeable), });
65-72: Guard constructor comment when auto-transpiling is enabledaddConstructorComment throws if shouldAutoTranspileImports is true. Skip the comment in that case to avoid generation failures while still locking initializers.
export function addLockingConstructorAllowReachable(c: ContractBuilder, bodyComments?: string[]): void { const disableInitializers = '_disableInitializers();'; if (!c.constructorCode.includes(disableInitializers)) { - c.addConstructorComment('/// @custom:oz-upgrades-unsafe-allow-reachable constructor'); + if (!c.shouldAutoTranspileImports) { + c.addConstructorComment('/// @custom:oz-upgrades-unsafe-allow-reachable constructor'); + } bodyComments?.forEach(comment => c.addConstructorCode(comment)); c.addConstructorCode(disableInitializers); } }
📜 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 selected for processing (3)
packages/core/solidity/src/account.ts(9 hunks)packages/core/solidity/src/helpers.ts(1 hunks)packages/core/solidity/src/signer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/solidity/src/helpers.ts
- packages/core/solidity/src/account.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
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/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: 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.
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: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.
📚 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/signer.ts
📚 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/signer.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/signer.ts
📚 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/signer.ts
📚 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/signer.ts
📚 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/signer.ts
📚 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/signer.ts
🧬 Code graph analysis (1)
packages/core/solidity/src/signer.ts (4)
packages/core/solidity/src/contract.ts (1)
ContractBuilder(78-238)packages/core/solidity/src/index.ts (3)
ContractBuilder(5-5)Upgradeable(10-10)OptionsError(17-17)packages/core/solidity/src/set-upgradeable.ts (1)
Upgradeable(8-8)packages/core/solidity/src/helpers.ts (1)
makeUpgradeable(12-26)
⏰ 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). (4)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: build (stellar, compile)
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
packages/core/solidity/src/signer.ts (3)
13-15: Override target for Weighted multisig is correctUsing MultiSignerERC7913 as the override base when the selected signer is MultisigWeighted is appropriate: MultiSignerERC7913Weighted extends MultiSignerERC7913 and does not re-declare the low-level _rawSignatureValidation; the override should reference the base that defines it. (docs.openzeppelin.com)
31-45: Upgradeable wiring looks solid (constructor lock + initializer)
- Initializable parent import is correct.
- Locking constructor via _disableInitializers plus oz-upgrades-unsafe-allow-reachable annotation matches recommended upgrades validations.
- initialize calls the correct __X_init with the right arg order for each signer.
102-121: Arg shapes and types align with OZ 5.4 signers
- ECDSA(address), P256(bytes32,bytes32), RSA(bytes,bytes), Multisig(bytes[],uint64), Weighted(bytes[],uint64[],uint64) match the 5.4 APIs. (github.com)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/solidity/src/helpers.ts (1)
17-24: Ack: prior linter warning about declarations in switch cases is addressed
Cases are wrapped in a block and a default branch exists, resolving the earlier Biome warning.
🧹 Nitpick comments (2)
packages/core/solidity/src/helpers.ts (2)
25-28: Include the unknown option value in the error for easier debuggingApply:
- throw new Error('Unknown upgradeable option'); + throw new Error(`Unknown upgradeable option: ${String(upgradeable)}`);
4-11: Tighten wording in docs: be explicit about accepted valuesApply:
-// If upgradeable is true-ish, translate a contract name or contract path into its corresponding upgradeable variant. +// If upgradeable is 'uups' or 'transparent', translate a contract name or contract path into its corresponding upgradeable variant.
📜 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 selected for processing (1)
packages/core/solidity/src/helpers.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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/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: 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.
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: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.
📚 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/helpers.ts
📚 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/helpers.ts
⏰ 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). (4)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: build (stellar, compile)
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
packages/core/solidity/src/helpers.ts (1)
12-29: LGTM: exhaustive switch +neverguard is solid
Theconst _: never = upgradeablemakes this future-proof at compile time. Nice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/solidity/src/signer.ts (2)
31-45: Set contracts-upgradeable install flag to avoid missing dependency in zipsYou import from
@openzeppelin/contracts-upgradeablehere but don’t mark the builder to install it. In other flows this is often set centrally; since you’re bypassing the helper, set it locally to be safe.Apply this diff inside the
if (upgradeable)branch:if (upgradeable) { + // Ensure consumers install @openzeppelin/contracts-upgradeable + c.shouldInstallContractsUpgradeable = true; c.addParent({ name: 'Initializable', path: '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol', });
67-74: Make duplicate-lock detection more robustThe guard checks for the exact string
_disableInitializers();. Minor variations (whitespace/comments) could bypass it. Consider tracking a boolean or checking via regex.- const disableInitializers = '_disableInitializers();'; - if (!c.constructorCode.includes(disableInitializers)) { + const disableInitializers = /_disableInitializers\(\)\s*;/; + if (!c.constructorCode.some(l => disableInitializers.test(l))) { c.addConstructorComment('/// @custom:oz-upgrades-unsafe-allow-reachable constructor'); bodyComments?.forEach(comment => c.addConstructorCode(comment)); - c.addConstructorCode(disableInitializers); + c.addConstructorCode('_disableInitializers();'); }
📜 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 selected for processing (1)
packages/core/solidity/src/signer.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
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/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: 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.
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: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.
📚 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/signer.ts
📚 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/signer.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/signer.ts
📚 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/signer.ts
📚 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/signer.ts
📚 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/signer.ts
📚 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/signer.ts
🧬 Code graph analysis (1)
packages/core/solidity/src/signer.ts (3)
packages/core/solidity/src/contract.ts (1)
ContractBuilder(78-238)packages/core/solidity/src/set-upgradeable.ts (1)
Upgradeable(8-8)packages/core/solidity/src/helpers.ts (1)
makeUpgradeable(12-30)
⏰ 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). (4)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: build (stellar, compile)
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
packages/core/solidity/src/signer.ts (5)
13-15: Override target for MultisigWeighted looks correctUsing the base
MultiSignerERC7913for the override when the selected signer isMultisigWeightedmatches the inheritance in OZ 5.4.0. LGTM.
17-25: Blocking ERC7702 + upgradeable is correctThrowing
OptionsErrorwhenupgradeableis set with ERC7702 is the right constraint. LGTM.
77-103: Signer import migration to vanilla paths is correctPaths point to OZ Contracts 5.4.0 signers. LGTM.
104-123: Constructor/initializer arg shapes match OZ 5.4.0Arg names and types align with signer constructors/init functions (ECDSA, P256, RSA, Multisig, Weighted). LGTM.
38-41: No duplicate initializer in any single contractOur search across packages/core/solidity/src found three
initializemodifiers: one in signer.ts (line 38) and two in account.ts (lines 199 and 248)【scriptOutput】. However, the two in account.ts are guarded by anif/elseand thus only one initializer is ever emitted per build, and signer.ts generates a separate contract entirely. There is no scenario where more than oneinitializemodifier ends up in the same contract, so no change is required here.Likely an incorrect or invalid review comment.
|
@SocketSecurity ignore npm/[email protected] |
|
@SocketSecurity ignore npm/[email protected] |
Fixes #618