Fixed bug with incorrect names in generated comment for Multisig account. #720
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThis PR updates the OpenZeppelin Wizard to fix incorrect names in generated comments for Multisig accounts by renaming the SignerMultisig type family to MultiSignerERC7913 across test snapshots and updating the signerName derivation logic in account.ts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/solidity/src/account.ts (1)
247-250: Excellent fix for the Multisig naming bug!The logic correctly maps Multisig signers to their actual OpenZeppelin contract names (
MultiSignerERC7913andMultiSignerERC7913Weighted) instead of the incorrectSignerMultisigpattern. This ensures generated comments reference the correct contract names.Optional: Use strict equality for consistency.
Consider using
===instead of==on line 248 for consistency with TypeScript best practices:- const signerBaseName = opts.signer.includes('Multisig') - ? `MultiSignerERC7913${opts.signer == 'MultisigWeighted' ? 'Weighted' : ''}` - : `Signer${opts.signer}`; + const signerBaseName = opts.signer.includes('Multisig') + ? `MultiSignerERC7913${opts.signer === 'MultisigWeighted' ? 'Weighted' : ''}` + : `Signer${opts.signer}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/core/solidity/src/account.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
.changeset/huge-ties-find.md(1 hunks)packages/core/solidity/src/account.test.ts.md(24 hunks)packages/core/solidity/src/account.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ernestognw
Repo: OpenZeppelin/contracts-wizard PR: 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: ernestognw
Repo: OpenZeppelin/contracts-wizard PR: 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
Repo: OpenZeppelin/contracts-wizard PR: 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
Repo: OpenZeppelin/contracts-wizard PR: 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
Repo: OpenZeppelin/contracts-wizard PR: 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/account.test.ts.md
📚 Learning: 2025-08-15T23:23:13.097Z
Learnt from: ernestognw
Repo: OpenZeppelin/contracts-wizard PR: 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/account.test.ts.md
📚 Learning: 2025-08-19T01:15:58.945Z
Learnt from: ernestognw
Repo: OpenZeppelin/contracts-wizard PR: 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/account.test.ts.md
📚 Learning: 2025-08-29T22:26:44.931Z
Learnt from: ernestognw
Repo: OpenZeppelin/contracts-wizard PR: 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.tspackages/core/solidity/src/account.test.ts.md
📚 Learning: 2025-08-20T20:23:30.259Z
Learnt from: ericglau
Repo: OpenZeppelin/contracts-wizard PR: 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.ts
📚 Learning: 2025-08-21T19:44:06.797Z
Learnt from: ericglau
Repo: OpenZeppelin/contracts-wizard PR: 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.ts
📚 Learning: 2025-08-15T22:49:25.653Z
Learnt from: ernestognw
Repo: OpenZeppelin/contracts-wizard PR: 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:
.changeset/huge-ties-find.md
📚 Learning: 2025-09-12T15:07:08.673Z
Learnt from: ericglau
Repo: OpenZeppelin/contracts-wizard PR: 663
File: packages/core/cairo_alpha/src/custom.test.ts.md:12-12
Timestamp: 2025-09-12T15:07:08.673Z
Learning: In the OpenZeppelin contracts-wizard cairo_alpha package changelog (packages/core/cairo_alpha/CHANGELOG.md), each alpha version gets its own separate entry under the "Unreleased" section rather than updating a single entry. This allows tracking of changes across different alpha releases (e.g., v3.0.0-alpha.0, v3.0.0-alpha.1, v3.0.0-alpha.2 all have separate entries).
Applied to files:
.changeset/huge-ties-find.md
🧬 Code graph analysis (1)
packages/core/solidity/src/account.ts (1)
packages/core/solidity/src/options.ts (1)
upgradeableName(6-12)
⏰ 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: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
.changeset/huge-ties-find.md (1)
1-5: LGTM!The changeset correctly categorizes this as a minor version bump for a bug fix. The description clearly states the issue being resolved.
Comment on lines
+6244
to
+6245
| // IMPORTANT: Make sure MultiSignerERC7913 is most derived than AccountERC7579␊ | ||
| // in the inheritance chain (i.e. contract ... is AccountERC7579, ..., MultiSignerERC7913)␊ |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Multisig comment rename to MultiSignerERC7913 looks correct.
Snapshot guidance now matches OZ naming (MultiSignerERC7913 and MultiSignerERC7913Weighted, incl. Upgradeable). Nice cleanup. One ask: in weighted sections, confirm the override list intentionally references MultiSignerERC7913 (base) vs Weighted if Weighted doesn’t override _rawSignatureValidation.
Also applies to: 6310-6311, 6381-6382, 6453-6454, 6900-6901, 6983-6984, 7070-7071, 7158-7159, 7625-7626, 7705-7706, 7786-7787, 8166-8167, 8240-8241, 8319-8320, 8399-8400, 8909-8910, 9000-9001, 9095-9096, 9191-9192, 9645-9646, 9729-9730, 9817-9818, 9906-9907
🏁 Script executed:
#!/bin/bash
# 1) Ensure no stale SignerMultisig identifiers remain (except in EIP712 titles/UI text if intended).
rg -nP 'SignerMultisig(Weighted)?(Upgradeable)?' -g '!**/node_modules/**'
# 2) Confirm expected MultiSigner names are present.
rg -nP 'MultiSignerERC7913(Weighted)?(Upgradeable)?' packages/
# 3) Double‑check weighted snapshots’ override lines use the intended base types.
rg -nP -C2 'override\(MultiSignerERC7913(?:Upgradeable)?[, ]' packages/core/solidity/src/account.test.ts.mdLength of output: 73012
Fix weighted section override clauses to match guidance comments.
The snapshot comments correctly specify inheritance order for MultiSignerERC7913Weighted variants (e.g., lines 8166-8167: "Make sure MultiSignerERC7913Weighted is most derived..."), but the corresponding override clauses incorrectly reference only the base MultiSignerERC7913. For weighted sections:
- Lines 8173, 8247, 8326, 8406:
override(MultiSignerERC7913, ...)should beoverride(MultiSignerERC7913Weighted, ...) - Lines 8916, 9007, 9102, 9198, 9652, 9736, 9824, 9913:
override(MultiSignerERC7913Upgradeable, ...)should beoverride(MultiSignerERC7913WeightedUpgradeable, ...)
Also applies to: 8240-8241, 8319-8320, 8399-8400, 9000-9001, 9095-9096, 9191-9192, 9645-9646, 9729-9730, 9817-9818, 9906-9907
🤖 Prompt for AI Agents
packages/core/solidity/src/account.test.ts.md around the noted ranges (weighted
sections): update the incorrect override clauses to reference the weighted
most-derived contracts as per the comments; specifically replace
override(MultiSignerERC7913, ...) with override(MultiSignerERC7913Weighted, ...)
at lines 8173, 8247, 8326, 8406 (and the corresponding nearby occurrences at
8240-8241, 8319-8320, 8399-8400), and replace
override(MultiSignerERC7913Upgradeable, ...) with
override(MultiSignerERC7913WeightedUpgradeable, ...) at lines 8916, 9007, 9102,
9198, 9652, 9736, 9824, 9913 (and the corresponding nearby occurrences at
9000-9001, 9095-9096, 9191-9192, 9645-9646, 9729-9730, 9817-9818, 9906-9907);
ensure each override list includes the weighted variant as the most-derived
override target to match the inheritance comment.
ericglau
requested changes
Nov 5, 2025
Co-authored-by: Eric Lau <[email protected]>
CoveMB
approved these changes
Nov 5, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #712