Add WebAuthn signer to Accounts #718
Conversation
|
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 WalkthroughThis PR adds WebAuthn as a new signer option for smart accounts. The change propagates across schema definitions, code generation, signer implementation, test cases, UI controls, and AI function definitions, with WebAuthn positioned between P256 and RSA in all signer option lists. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/solidity/src/account.test.ts.md (1)
4660-4660: Add snapshot path to Gitleaks allowlist to suppress false positive security warnings.Verified: Lines 4660 and 5819 contain Solidity contract code in generated test snapshots (account.test.ts.md), not secrets. These are benign lines from contract inheritance declarations:
- Line 4660:
contract CustomAccountWithSignerWebAuthnERC721Holder is Account, EIP712, ...- Line 5819:
contract CustomAccountWithSignerWebAuthnERC721Holder is Initializable, Account, EIP712, ...Update Gitleaks configuration to allowlist
packages/core/solidity/src/account.test.ts.mdto prevent recurring false positives from snapshot files.
📜 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 (10)
.changeset/young-readers-cover.md(1 hunks)packages/common/src/ai/descriptions/solidity.ts(1 hunks)packages/core/solidity/src/account.test.ts(1 hunks)packages/core/solidity/src/account.test.ts.md(1 hunks)packages/core/solidity/src/account.ts(1 hunks)packages/core/solidity/src/generate/account.ts(1 hunks)packages/core/solidity/src/signer.ts(4 hunks)packages/mcp/src/solidity/schemas.ts(1 hunks)packages/ui/api/ai-assistant/function-definitions/solidity.ts(1 hunks)packages/ui/src/solidity/AccountControls.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
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-09-18T20:18:23.799Z
Learnt from: ericglau
Repo: OpenZeppelin/contracts-wizard PR: 652
File: packages/ui/api/ai-assistant/function-definitions/confidential.ts:32-38
Timestamp: 2025-09-18T20:18:23.799Z
Learning: In OpenZeppelin Wizard, the AI Assistant defines its own function definitions separately and does not use the MCP tools. This means the AI Assistant function definitions and MCP schemas can have different shapes without causing validation conflicts.
Applied to files:
packages/ui/api/ai-assistant/function-definitions/solidity.ts
📚 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/ui/api/ai-assistant/function-definitions/solidity.tspackages/core/solidity/src/account.tspackages/core/solidity/src/account.test.tspackages/mcp/src/solidity/schemas.tspackages/core/solidity/src/generate/account.tspackages/core/solidity/src/signer.tspackages/common/src/ai/descriptions/solidity.ts.changeset/young-readers-cover.mdpackages/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/ui/api/ai-assistant/function-definitions/solidity.tspackages/core/solidity/src/account.tspackages/core/solidity/src/account.test.tspackages/mcp/src/solidity/schemas.tspackages/core/solidity/src/generate/account.tspackages/core/solidity/src/signer.tspackages/common/src/ai/descriptions/solidity.ts.changeset/young-readers-cover.mdpackages/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.tspackages/mcp/src/solidity/schemas.tspackages/core/solidity/src/generate/account.tspackages/core/solidity/src/signer.ts.changeset/young-readers-cover.mdpackages/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.tspackages/core/solidity/src/account.test.ts.md
📚 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.tspackages/core/solidity/src/account.test.ts.md
📚 Learning: 2025-08-15T22:52:34.129Z
Learnt from: ernestognw
Repo: OpenZeppelin/contracts-wizard PR: 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/account.test.ts.md
📚 Learning: 2025-09-18T20:17:09.709Z
Learnt from: ericglau
Repo: OpenZeppelin/contracts-wizard PR: 652
File: packages/mcp/src/confidential/schemas.ts:40-46
Timestamp: 2025-09-18T20:17:09.709Z
Learning: MCP tool schemas for confidential contracts can be more restrictive than the core API. For the votes field in confidential fungible tokens, the MCP schema only accepts 'blocknumber' or 'timestamp' strings because undefined behaves functionally equivalent to false, eliminating the need to explicitly accept boolean values in the tool definition.
Applied to files:
packages/mcp/src/solidity/schemas.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/young-readers-cover.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/young-readers-cover.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.test.ts.md
🧬 Code graph analysis (1)
packages/core/solidity/src/account.ts (1)
packages/core/solidity/src/signer.ts (1)
signerFunctions(121-131)
🪛 Gitleaks (8.28.0)
packages/core/solidity/src/account.test.ts.md
[high] 4660-4660: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 5819-5819: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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 (17)
.changeset/young-readers-cover.md (1)
1-7: LGTM!The changeset properly documents the addition of WebAuthn as a new signer option with appropriate minor version bumps for all affected packages.
packages/ui/src/solidity/AccountControls.svelte (1)
219-225: LGTM!The WebAuthn radio option is properly integrated into the UI with consistent formatting, appropriate help text, and a documentation link.
packages/core/solidity/src/account.test.ts (1)
64-64: LGTM!WebAuthn is properly added to the test signer matrix, enabling comprehensive test coverage across all upgrade variants and feature combinations.
packages/core/solidity/src/generate/account.ts (1)
11-11: LGTM!WebAuthn is correctly added to the signer options for account generation, positioned consistently between P256 and RSA.
packages/common/src/ai/descriptions/solidity.ts (1)
73-73: LGTM!The WebAuthn description is clear, properly positioned, and explains that it builds on top of P256, which provides helpful context for understanding its relationship to other signers.
packages/mcp/src/solidity/schemas.ts (1)
131-131: LGTM!WebAuthn is properly added to the account schema with consistent positioning and validation pattern.
packages/ui/api/ai-assistant/function-definitions/solidity.ts (1)
214-214: LGTM!WebAuthn is correctly added to the AI assistant function definition for account signers, maintaining consistent positioning across all integration points.
packages/core/solidity/src/account.ts (1)
249-256: Review comment is incorrect.SignerWebAuthn inherits from SignerP256, which itself extends AbstractSigner. The conditional logic in account.ts (lines 249–256) that skips the AbstractSigner import/override for WebAuthn is not because WebAuthn lacks AbstractSigner in its inheritance chain; rather, it reflects special handling specific to the ERC7579 account context (lines 246–257). This is distinct from the general signer handling in signer.ts (lines 41–61), where WebAuthn explicitly includes AbstractSigner imports and overrides.
Likely an incorrect or invalid review comment.
packages/core/solidity/src/signer.ts (4)
6-15: LGTM!WebAuthn is correctly positioned in the SignerOptions array, maintaining logical grouping near P256 given the inheritance relationship.
78-81: LGTM!The WebAuthn entry in the signers registry follows the established pattern and uses the correct path convention for OpenZeppelin contracts.
106-109: LGTM!The constructor arguments for WebAuthn correctly match P256's arguments (qx, qy coordinates), which is appropriate since WebAuthn uses P256 curves for key verification. Additional WebAuthn-specific data (authenticator data, client data JSON) would be passed at signature validation time, not during construction.
46-61: The WebAuthn implementation pattern is correct and intentional.SignerWebAuthn is documented as an "Implementation of SignerP256 that supports WebAuthn authentication assertions" that "enables signature validation using WebAuthn authentication assertions" while "allowing for both WebAuthn and raw P256 signature validation."
The dual override pattern and explicit AbstractSigner import reflect this design:
- The contract leverages the P256 public key while enabling both WebAuthn and raw P256 signature validation, requiring overrides at both the AbstractSigner and SignerP256 levels
- The conditional handling in account.ts (AbstractSigner import only when
opts.signer !== 'WebAuthn') confirms WebAuthn receives special treatment, validating the explicit AbstractSigner import in signer.tsNo issues found.
packages/core/solidity/src/account.test.ts.md (5)
4585-4596: ERC1271 variant parity with other signers.isValidSignature pattern matches ECDSA/P256. No issues.
5412-5451: ERC‑7579 + WebAuthn override precedence.Override list includes AccountERC7579Upgradeable alongside SignerWebAuthn/AbstractSigner/SignerP256Upgradeable; fallback isValidSignature logic is consistent. LGTM.
5619-5669: Override pattern is correct and consistently applied.All contracts inheriting from
AccountERC7579HookedUpgradeablecorrectly useoverride(AccountERC7579Upgradeable, ERC7739)throughout the codebase, since the Hooked variant inherits from the baseAccountERC7579Upgradeable. No changes required.
5045-5071: Initializer ordering verified across all WebAuthn upgradeable variants — confirmed correct and consistent.The verification confirms that
__SignerP256_init()is consistently called before__SignerWebAuthn_init()across all 21+ generated WebAuthn upgradeable account variants in the test snapshots. This pattern holds uniformly regardless of context (standalone, with ERC7579, with hooked variants, etc.). No deviations or inconsistencies were found.
4545-4567: Override list order contradicts inheritance placement—review reasoning is incorrect.The code is correct, but the review comment mischaracterizes the relationship. SignerWebAuthn appears last in the inheritance chain (e.g.,
is Account, EIP712, ERC7739, SignerP256, SignerWebAuthn), but correctly appears first in the override list (override(SignerWebAuthn, AbstractSigner, SignerP256)). In Solidity, override order determines super call resolution and must be inverse to inheritance order for proper C3 linearization. This pattern mirrors ECDSA/P256/RSA consistently across all snapshots. The code is sound, but the explanation conflates inheritance placement with override list ordering.Likely an incorrect or invalid review comment.
After Solidity Contracts 5.5, the WebAuthn Signer is available and we were missing the option for it.