Unify upgradeability of Account with the other types of contract #658
Conversation
Reviewer's GuideThis PR reverts the upgradeability transformation introduced in #609 by consolidating multiple upgrade flags into a single “upgradeable” property, restoring non-upgradeable accounts to use ordinary constructors (without Initializable or disableInitializer) and simplifying proxy deployment support. It updates how contracts are printed, how upgradeable logic is applied, and removes scattered helper flags in favor of unified handling. Class diagram for Signer logic simplificationclassDiagram
class Signer {
+addSigner(c: ContractBuilder, signer: SignerOptions, upgradeable: boolean)
}
class SignerOptions {
<<enumeration>>
false
ERC7702
ECDSA
P256
RSA
Multisig
MultisigWeighted
}
Signer --> SignerOptions
Signer --> ContractBuilder
%% Shows that signer logic no longer distinguishes upgradeable/non-upgradeable for parent selection and constructor/initializer.
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough✨ Finishing Touches🧪 Generate unit tests
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The repeated use of
transpiled: falseacross many imports suggests it might be cleaner to introduce a small helper or default behavior so you don’t have to sprinkle that flag everywhere manually. - The
printConstructorfunction now handles both a locking constructor and a separate initializer, which adds significant branching; consider refactoring that into two focused helper methods (one for constructors, one for initializers) to simplify the logic. - Since you’ve collapsed three boolean flags into a single
upgradeableproperty that drives import paths, modifiers, and deployment code, you might introduce a thin wrapper or config object to centralize upgradeable-vs-non-upgradeable decisions instead of scatteringif (upgradeable)checks throughout the codebase.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated use of `transpiled: false` across many imports suggests it might be cleaner to introduce a small helper or default behavior so you don’t have to sprinkle that flag everywhere manually.
- The `printConstructor` function now handles both a locking constructor and a separate initializer, which adds significant branching; consider refactoring that into two focused helper methods (one for constructors, one for initializers) to simplify the logic.
- Since you’ve collapsed three boolean flags into a single `upgradeable` property that drives import paths, modifiers, and deployment code, you might introduce a thin wrapper or config object to centralize upgradeable-vs-non-upgradeable decisions instead of scattering `if (upgradeable)` checks throughout the codebase.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Amxx
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/core/solidity/src/options.ts (2)
6-12: upgradeableName: solid, add explicit return typeFunction logic looks good and handles Initializable correctly. Add an explicit return type for clarity.
-export function upgradeableName(n: string) { +export function upgradeableName(n: string): string {
14-26: upgradeableImport: path/name transform is correct; consider guarding non-OZ pathsThe transform is idempotent for already-upgradeable paths and uses posix separators. Optionally no-op early for non-@OpenZeppelin paths to avoid touching local imports.
export function upgradeableImport(p: ImportContract): ImportContract { const { dir, ext, name } = path.parse(p.path); - // Use path.posix to get forward slashes + // Only transform OZ imports. Use path.posix to get forward slashes. + if (!/^@openzeppelin\/contracts\b/.test(dir)) return p; return { ...p,packages/core/solidity/src/account.ts (1)
279-282: Base import for multisig-weightedImporting MultiSignerERC7913 to document inheritance is fine. If unused by the printer, consider dropping later to keep imports minimal.
packages/core/solidity/src/contract.ts (1)
31-31: Document semantics of ReferencedContract.transpiled.Please add a short comment clarifying default behavior when undefined (e.g., “undefined => default transform policy; true => eligible for transform; false => never transform”). This avoids ambiguity across contributors.
export interface ReferencedContract { name: string; - transpiled?: boolean; + /** + * Controls import/name transpilation for upgradeable builds. + * - undefined: follow default policy + * - true: eligible for upgradeable transform + * - false: never transform + */ + transpiled?: boolean; }packages/core/solidity/src/zip-foundry.ts (1)
110-114: Script generation is consistent; minor DX tweak suggestion.Consider also commenting out vm.startBroadcast()/vm.stopBroadcast() when args.length==0 but the user may still want to dry-run without broadcasting. Optional; current behavior is fine.
- const deploymentLines = [ - 'vm.startBroadcast();', + const deploymentLines = [ + // For dry-run without sending txs, comment out start/stopBroadcast. + 'vm.startBroadcast();', ...getAddressVariables(c, args), ...getDeploymentCode(c, args), `console.log("${c.upgradeable ? 'Proxy' : 'Contract'} deployed to %s", address(instance));`, 'vm.stopBroadcast();', ];Also applies to: 123-125, 137-156, 160-166
packages/core/solidity/src/zip-hardhat.ts (1)
138-139: Optional: log implementation/admin for upgradeable deploymentsHelps users find both addresses without extra steps.
Apply this diff inside the script template:
- console.log(`${c.upgradeable ? 'Proxy' : 'Contract'} deployed to ${await instance.getAddress()}`); + const addr = await instance.getAddress(); + console.log(`${c.upgradeable ? 'Proxy' : 'Contract'} deployed to ${addr}`); + ${c.upgradeable ? `// Extra visibility for proxies + const impl = await upgrades.erc1967.getImplementationAddress(addr); + console.log(\`Implementation deployed to \${impl}\`); + const admin = await upgrades.erc1967.getAdminAddress(addr); + console.log(\`Admin at \${admin}\`);` : ''}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/core/solidity/src/account.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/zip-foundry.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/zip-hardhat.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
packages/core/solidity/src/account.ts(11 hunks)packages/core/solidity/src/contract.ts(2 hunks)packages/core/solidity/src/helpers.ts(0 hunks)packages/core/solidity/src/options.ts(2 hunks)packages/core/solidity/src/print.ts(2 hunks)packages/core/solidity/src/set-upgradeable.ts(3 hunks)packages/core/solidity/src/signer.ts(2 hunks)packages/core/solidity/src/zip-foundry.test.ts(1 hunks)packages/core/solidity/src/zip-foundry.test.ts.md(10 hunks)packages/core/solidity/src/zip-foundry.ts(10 hunks)packages/core/solidity/src/zip-hardhat.test.ts(0 hunks)packages/core/solidity/src/zip-hardhat.test.ts.md(0 hunks)packages/core/solidity/src/zip-hardhat.ts(6 hunks)
💤 Files with no reviewable changes (3)
- packages/core/solidity/src/zip-hardhat.test.ts
- packages/core/solidity/src/helpers.ts
- packages/core/solidity/src/zip-hardhat.test.ts.md
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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/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: 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-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/zip-foundry.tspackages/core/solidity/src/options.tspackages/core/solidity/src/signer.tspackages/core/solidity/src/print.tspackages/core/solidity/src/zip-foundry.test.ts.mdpackages/core/solidity/src/set-upgradeable.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/zip-foundry.tspackages/core/solidity/src/options.tspackages/core/solidity/src/set-upgradeable.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: 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/set-upgradeable.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/print.tspackages/core/solidity/src/zip-foundry.test.ts.mdpackages/core/solidity/src/set-upgradeable.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/zip-foundry.test.ts.mdpackages/core/solidity/src/set-upgradeable.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/zip-foundry.test.ts.md
📚 Learning: 2025-08-29T22:26:44.931Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#653
File: packages/core/solidity/src/account.ts:189-201
Timestamp: 2025-08-29T22:26:44.931Z
Learning: In ERC-7579, accounts support multiple execution paths: the standard ERC-4337 path requiring validators for UserOp validation, and the module execution path where executor modules can directly call executeFromExecutor to execute transactions on behalf of the account. This means accounts initialized with only an executor module are functional and valid according to the ERC-7579 specification.
Applied to files:
packages/core/solidity/src/account.ts
🧬 Code graph analysis (4)
packages/core/solidity/src/zip-hardhat.ts (1)
packages/core/solidity/src/contract.ts (1)
Contract(3-14)
packages/core/solidity/src/options.ts (2)
packages/core/solidity/src/contract.ts (3)
ImportContract(24-26)ReferencedContract(28-31)Contract(3-14)packages/core/solidity/src/infer-transpiled.ts (1)
inferTranspiled(3-5)
packages/core/solidity/src/print.ts (1)
packages/core/solidity/src/infer-transpiled.ts (1)
inferTranspiled(3-5)
packages/core/solidity/src/account.ts (2)
packages/core/solidity/src/options.ts (1)
upgradeableName(6-12)packages/core/solidity/src/signer.ts (2)
signerFunctions(88-98)signers(40-65)
⏰ 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 (36)
packages/core/solidity/src/options.ts (2)
33-36: Helpers surface change LGTMExposing upgradeable and transformImport on Helpers aligns with the new flow.
39-46: withHelpers: transformation order is rightApplying upgradeableImport before opts.transformImport is the right precedence. No issues.
packages/core/solidity/src/account.ts (11)
76-79: PackedUserOperation marked non-transpiled: good catchAvoids false-positive transpilation since it doesn’t start with “I”. LGTM.
88-91: Account parent non-transpiled + override: LGTMKeeping Account non-upgradeable is correct and prevents accidental transpilation.
104-112: ERC7739 non-transpiled: consistent with intent to avoid upgradeable overheadGood choice given the draft status and ABI expectations.
131-137: ERC721Holder non-transpiled: acceptableUsing non-upgradeable holder utils is fine as they carry no storage.
141-147: ERC1155Holder non-transpiled: acceptableSame rationale as ERC721Holder. LGTM.
153-162: ERC7821 non-transpiled and override: LGTMKeeps the account tree consistent. No issues.
183-188: Constructor path for ERC7579 modulesRelying on setUpgradeableAccount to convert constructor args/code into an initializer for upgradeable builds is fine. Just ensure setUpgradeableAccount handles the _installModule call when upgradeable=true.
191-203: ERC7739 fallback to AccountERC7579[Upgradeable]Logic makes sense. It depends on the availability and import of AccountERC7579Upgradeable (see earlier verification). No other issues.
242-252: _rawSignatureValidation override when no signer/modulesOverride target set to non-upgradeable Account is correct for this branch. LGTM.
167-179: Confirmed availability of AccountERC7579Upgradeable; no changes required.
256-266: Keep AbstractSigner non-transpiled import
NoAbstractSignerUpgradeable.solexists in the upgradeable contracts package’s signers directory, so the explicittranspiled: falseimport and override are required.Likely an incorrect or invalid review comment.
packages/core/solidity/src/zip-foundry.test.ts (1)
177-179: Gate --force by c.upgradeable: LGTMMatches the unified upgradeable flag. No issues.
packages/core/solidity/src/signer.ts (1)
13-13: Override specifier is correct
MultiSignerERC7913Weighted extends MultiSignerERC7913, which implements_rawSignatureValidation, so usingMultiSignerERC7913as the override target is valid.packages/core/solidity/src/contract.ts (2)
13-13: Single source of truth for upgradeability looks good.
78-78: Defaulting upgradeable=false on builders is correct.packages/core/solidity/src/set-upgradeable.ts (3)
19-19: Correctly toggling c.upgradeable.
23-23: Removetranspiled: truesuggestion
The import path for Initializable is automatically rewritten to the upgradeable variant viahelpers.transformImportwhenc.upgradeableis true; there’s notranspiledoption onImportContract.Likely an incorrect or invalid review comment.
62-65: Add tests for account + signer upgradeability paths
- In packages/core/solidity/src/account.test.ts, add a test for non-upgradeable account + signer ensuring no
@openzeppelin/contracts-upgradeable/.../Initializable.solimport, no_disableInitializers(), and no publicinitialize(...).- Add a complementary test for upgradeable account + signer asserting the presence of upgradeable imports and initializer calls.
⛔ Skipped due to learnings
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.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: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.packages/core/solidity/src/zip-foundry.test.ts.md (1)
236-239: Snapshot updates align with new upgradeable gating (LGTM).
- Install/remappings now switch to upgradeable packages only when upgradeable=true.
- This mirrors the generator logic in zip-foundry.ts.
No action needed.
Also applies to: 254-256, 478-481, 496-498, 669-672, 687-689, 1000-1003, 1018-1020, 1313-1316, 1331-1333
packages/core/solidity/src/zip-foundry.ts (5)
19-23: Gate Upgrades import behind c.upgradeable (LGTM).Keeps non-upgradeable tests/scripts lean.
64-66: initialOwner var generation avoids shadowing (LGTM).The conditional prevents duplicate declarations when initializer also has initialOwner.
280-281: Readme: conditional --force flag (LGTM).Matches upgradeable flow that enables FFI/build_info.
Also applies to: 288-289
40-58: Add tests covering all deployment paths
No test files exercisingUpgrades.deployTransparentProxy,Upgrades.deployUUPSProxy, or directnew ContractName(...)could be found; ensure coverage for each path.
208-213: Ignore remapping verification for setup.sh – nosetup.shexists in the repository; that check isn’t applicable.Likely an incorrect or invalid review comment.
packages/core/solidity/src/print.ts (3)
113-121: Non-upgradeable branch (single constructor) remains correct (LGTM).Parents with initializers are invoked in the constructor as before.
161-166: inferTranspiled usage is appropriate; ensures __X_init only under upgradeable.This prevents accidental __X_init in non-upgradeable builds.
85-111: Constructor/initializer split is correct as-is.
No non-transpiled parent will carry dynamic constructor args into the parameterless constructor path, so no additional filtering is needed.packages/core/solidity/src/zip-hardhat.ts (7)
9-13: Good: upgradeability-gated plugin import in Hardhat configConditionally importing @openzeppelin/hardhat-upgrades only when needed keeps non-upgradeable projects lean. LGTM.
121-124: Unified deploy path reads clearlySwitching between upgrades.deployProxy and ContractFactory.deploy based on c.upgradeable is straightforward and correct.
171-172: Readme wording toggle is consistentThe copy now correctly reflects script vs Ignition based on upgradeability.
190-191: Deploy command selection looks rightScript for upgradeable, Ignition for non-upgradeable is a sensible split.
196-199: Runtime imports toggle is correctImporting upgrades from "hardhat" only when upgradeable avoids unused imports in non-upgradeable zips.
218-223: Zip contents match deploy pathIncluding scripts/deploy.ts only for upgradeable and Ignition module otherwise is consistent with the README and tests.
225-225: Correct: pass upgradeable flag into hardhat.config.ts emitterKeeps the config and runtime imports in sync with the chosen upgrade model.
Amxx
force-pushed
the
fix/non-upgradeable-accounts-dont-need-initializers
branch
from
22961b5 to
55e4114
Compare
Amxx
force-pushed
the
fix/non-upgradeable-accounts-dont-need-initializers
branch
from
55e4114 to
2674a1c
Compare
|
If we're still interested in doing #576, I would suggest finishing that one first instead of going back and forth |
Amxx
force-pushed
the
fix/non-upgradeable-accounts-dont-need-initializers
branch
from
f0f9db3 to
36b2d0c
Compare
|
I think 576 is lower priority. I'd first make sure that the Accounts are generated "properly" before adding something extra |
| @@ -133,6 +128,7 @@ function addERC721Holder(c: ContractBuilder, opts: AccountOptions): void { | |||
| c.addParent({ | |||
| name: 'ERC721Holder', | |||
| path: '@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol', | |||
| transpiled: false, | |||
There was a problem hiding this comment.
There is an upgradeable variant of this. Any reason not to use it?
There was a problem hiding this comment.
I would argue there should not be an upgradeable version. This is stateless, and I think we should at some point remove the upgradeable version.
That being said, we can remove the transpiled: false flag, and use the upgradeable version. Result logic will be the same. One of the difference though, is that you'll have a __ERC721Holder_init() in your constructor that you don't need. So I think its clearner to use the non-upgradeable version, but I'd be ok with using the upgradeable version.
All the above apply to ERC1155Holder.
It is a priority for the Open Accounts Framework as all accounts would require an initializer. Again, please name at least one popular use case for accounts that have a constructor instead of an initializer. In fact, I consider this current implementation more strange and way more misleading than the original one (according to your own categorization). I have a strong opinion so won't approve. For me, the correct implementation is still #653 |
There was a problem hiding this comment.
Looks good.
To summarize the discussion behind this PR, we had a conversation about whether the Accounts should have an initializer by default even in the non-upgradeable version.
The initial assumption was that initializable was fundamentally different from upgradeable since developers perceive a feeling of bloat when using the upgradeable variant, and because ERC-4337 assumes that accounts will be deployed through a factory, which implies that they need an initializer.
While the initializer is still required in ERC-4337. We decided to keep the wizard output consistent with previous iterations, where we recommended the upgradeable variant for initializable clones.
LGTM, though, we should strive to provide an option for factories as suggested in #576
Amxx
deleted the
fix/non-upgradeable-accounts-dont-need-initializers
branch
This PR addresses an inconsistency introduced in #609
This is done by reverting a significant portion the changes to upgradeability transformation introduced in #609
About the produced solidity code
Non-upgradeable accounts, that include a signer, are currently quite strange:
_disableInitializer()In fact they are in a mixed state between beeing non upgradeable, and being an implementation for proxies.
With this PR, non-upgradeable accounts are
_disableInitializercallHow its implemented
contract.tsto its state before Update migrated imports from community to vanilla contracts #609. In particularshouldAutoTranspileImports,shouldInstallContractsUpgradeableandshouldUseUpgradesPluginsForProxyDeploymentare removed in favor of the oldupgradealbesetUpgradeable.ts, and use a simpler version ofsetUpgradeableAccount(similar to the existing variants for generic and governor)ReferencedContract.transpiled: boolean | undefinedto explicit which parents should and shouldn't be transpiled.Summary by Sourcery
Revert the experimental upgradeability transformation from #609 and restore the original upgradeable/non-upgradeable account behavior using a single
upgradeableflag.Enhancements:
upgradeableboolean in contract definitions and remove the specializedshouldAutoTranspileImports,shouldInstallContractsUpgradeable, andshouldUseUpgradesPluginsForProxyDeploymentflagsset-upgradeable.tsandcontract.tsto their pre-Update migrated imports from community to vanilla contracts #609 state and simplify the upgradeable account patternInitializable, call_disableInitializers(), or expose public initializers