Add ERC7579 Validators #5891
Add ERC7579 Validators #5891
Conversation
🦋 Changeset detectedLatest commit: 3531217 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Reviewer's GuideThis PR implements a new ERC-7579 validator framework by introducing an abstract ERC7579Validator base and a concrete ERC7579Signature module that leverages ERC-7913 for address-less key verification, and complements these with comprehensive shared and integration tests plus documentation and changeset updates. Class diagram for ERC7579Validator and ERC7579Signature modulesclassDiagram
class IERC7579Module {
<<interface>>
}
class IERC7579Validator {
<<interface>>
}
class ERC7579Validator {
+isModuleType(moduleTypeId uint256) bool
+validateUserOp(userOp PackedUserOperation, userOpHash bytes32) uint256
+isValidSignatureWithSender(sender address, hash bytes32, signature bytes) bytes4
-_rawERC7579Validation(account address, hash bytes32, signature bytes) bool
}
class ERC7579Signature {
-_signers: mapping(address => bytes)
+signer(account address) bytes
+onInstall(data bytes)
+onUninstall(data bytes)
+setSigner(signer_ bytes)
-_setSigner(account address, signer_ bytes)
-_rawERC7579Validation(account address, hash bytes32, signature bytes) bool
}
IERC7579Module <|.. ERC7579Validator
IERC7579Validator <|.. ERC7579Validator
ERC7579Validator <|-- ERC7579Signature
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughIntroduces a new abstract validator module (ERC7579Validator) and a concrete implementation (ERC7579Signature) adding ERC-7913-based signature validation for ERC-7579 accounts. Updates documentation, adds changesets marking a minor release, and includes comprehensive tests for module lifecycle, signature validation, and multi-scheme signers (ECDSA, P256, RSA). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant EntryPoint
participant Account (ERC-7579)
participant ERC7579Signature as Validator (ERC7579Signature)
participant SignatureChecker
participant Verifier (ERC-7913)
rect rgba(230, 245, 255, 0.5)
note over Account,Validator: Installation
User->>Account: call module.onInstall(data)
Account->>Validator: onInstall(data)
Validator->>Validator: set signer bytes (verifier || key)
Validator-->>Account: installed
end
rect rgba(240, 255, 240, 0.5)
note over EntryPoint,Verifier: UserOp validation
EntryPoint->>Validator: validateUserOp(userOp, userOpHash)
Validator->>Validator: _rawERC7579Validation(msg.sender, userOpHash, sig)
Validator->>SignatureChecker: isValidSignatureNow(signer, userOpHash, sig)
SignatureChecker->>Verifier: verify(hash, sig, key)
Verifier-->>SignatureChecker: true/false
SignatureChecker-->>Validator: true/false
Validator-->>EntryPoint: SIG_VALIDATION_SUCCESS/FAILED
end
rect rgba(255, 245, 230, 0.5)
note over Account,Validator: Uninstallation
User->>Account: call module.onUninstall(...)
Account->>Validator: onUninstall(...)
Validator->>Validator: clear signer for account
Validator-->>Account: uninstalled
end
sequenceDiagram
autonumber
participant Caller
participant DApp/Wallet
participant Validator (ERC7579Signature)
note over DApp/Wallet,Validator: IERC1271 check with sender context
DApp/Wallet->>Validator: isValidSignatureWithSender(sender, hash, signature)
Validator->>Validator: _rawERC7579Validation(msg.sender, hash, signature)
Validator-->>DApp/Wallet: 0x1626ba7e or 0xffffffff
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `contracts/account/modules/ERC7579Signature.sol:59` </location>
<code_context>
+ }
+
+ /// @dev Sets the ERC-7913 signer (i.e. `verifier || key`) for the calling account.
+ function setSigner(bytes memory signer_) public virtual {
+ require(signer_.length >= 20, ERC7579SignatureInvalidSignerLength());
+ _setSigner(msg.sender, signer_);
+ }
</code_context>
<issue_to_address>
The minimum signer length check may not be sufficient for all ERC-7913 use cases.
ERC-7913 signers may require more specific validation based on verifier and key format. Please consider adding stricter checks or documenting the expected signer structure to avoid misconfiguration.
Suggested implementation:
```
/**
* @dev Sets the ERC-7913 signer for the calling account.
* The signer must be encoded as: `verifier (20 bytes) || key (variable length)`.
* Requirements:
* - `signer_` must be at least 20 bytes (verifier address).
* - `verifier` must be a valid non-zero address.
* - `key` must be present (length > 0).
*/
function setSigner(bytes memory signer_) public virtual {
require(signer_.length > 20, ERC7579SignatureInvalidSignerLength()); // 20 bytes verifier + at least 1 byte key
address verifier;
assembly {
verifier := mload(add(signer_, 20))
}
require(verifier != address(0), ERC7579SignatureInvalidVerifier());
_setSigner(msg.sender, signer_);
}
```
You will need to define the custom error `ERC7579SignatureInvalidVerifier()` if it does not already exist in your codebase, e.g.:
```solidity
error ERC7579SignatureInvalidVerifier();
```
Also, ensure that the expected structure of the signer (verifier || key) is documented wherever relevant in your codebase and documentation.
</issue_to_address>
### Comment 2
<location> `contracts/account/modules/ERC7579Signature.sol:79` </location>
<code_context>
+ * the account's stored signer. Derived contracts can override this to implement
+ * custom validation logic based on the sender.
+ */
+ function _rawERC7579Validation(
+ address account,
+ bytes32 hash,
+ bytes calldata signature
+ ) internal view virtual override returns (bool) {
+ return SignatureChecker.isValidSignatureNow(signer(account), hash, signature);
+ }
+}
</code_context>
<issue_to_address>
Signature validation relies on external SignatureChecker; ensure compatibility with all expected signer formats.
Document the expected signer formats and cryptographic schemes, and consider adding extension points to support custom signature validation if needed.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
function _rawERC7579Validation(
address account,
bytes32 hash,
bytes calldata signature
) internal view virtual override returns (bool) {
return SignatureChecker.isValidSignatureNow(signer(account), hash, signature);
}
}
=======
/**
* @dev Validates a `signature` for the given `account` and `hash`.
*
* This function uses SignatureChecker to validate the signature against the account's stored signer.
*
* Expected signer formats:
* - EOA (Externally Owned Account) addresses (ECDSA signatures)
* - ERC1271 contract-based signers (isValidSignature)
*
* Supported cryptographic schemes:
* - ECDSA (secp256k1)
* - ERC1271 contract signatures
*
* Derived contracts can override {_customSignatureValidation} to support additional formats or schemes.
*/
function _rawERC7579Validation(
address account,
bytes32 hash,
bytes calldata signature
) internal view virtual override returns (bool) {
// Extension point for custom validation logic
bool customValid = _customSignatureValidation(account, hash, signature);
if (customValid) {
return true;
}
return SignatureChecker.isValidSignatureNow(signer(account), hash, signature);
}
/**
* @dev Extension point for custom signature validation logic.
* Override in derived contracts to support additional signer formats or cryptographic schemes.
* Return true if the signature is valid, false otherwise.
*/
function _customSignatureValidation(
address account,
bytes32 hash,
bytes calldata signature
) internal view virtual returns (bool) {
// Default implementation: no custom validation
return false;
}
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| function setSigner(bytes memory signer_) public virtual { | ||
| require(signer_.length >= 20, ERC7579SignatureInvalidSignerLength()); |
There was a problem hiding this comment.
suggestion: The minimum signer length check may not be sufficient for all ERC-7913 use cases.
ERC-7913 signers may require more specific validation based on verifier and key format. Please consider adding stricter checks or documenting the expected signer structure to avoid misconfiguration.
Suggested implementation:
/**
* @dev Sets the ERC-7913 signer for the calling account.
* The signer must be encoded as: `verifier (20 bytes) || key (variable length)`.
* Requirements:
* - `signer_` must be at least 20 bytes (verifier address).
* - `verifier` must be a valid non-zero address.
* - `key` must be present (length > 0).
*/
function setSigner(bytes memory signer_) public virtual {
require(signer_.length > 20, ERC7579SignatureInvalidSignerLength()); // 20 bytes verifier + at least 1 byte key
address verifier;
assembly {
verifier := mload(add(signer_, 20))
}
require(verifier != address(0), ERC7579SignatureInvalidVerifier());
_setSigner(msg.sender, signer_);
}
You will need to define the custom error ERC7579SignatureInvalidVerifier() if it does not already exist in your codebase, e.g.:
error ERC7579SignatureInvalidVerifier();Also, ensure that the expected structure of the signer (verifier || key) is documented wherever relevant in your codebase and documentation.
| function _rawERC7579Validation( | ||
| address account, | ||
| bytes32 hash, | ||
| bytes calldata signature | ||
| ) internal view virtual override returns (bool) { | ||
| return SignatureChecker.isValidSignatureNow(signer(account), hash, signature); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: Signature validation relies on external SignatureChecker; ensure compatibility with all expected signer formats.
Document the expected signer formats and cryptographic schemes, and consider adding extension points to support custom signature validation if needed.
| function _rawERC7579Validation( | |
| address account, | |
| bytes32 hash, | |
| bytes calldata signature | |
| ) internal view virtual override returns (bool) { | |
| return SignatureChecker.isValidSignatureNow(signer(account), hash, signature); | |
| } | |
| } | |
| /** | |
| * @dev Validates a `signature` for the given `account` and `hash`. | |
| * | |
| * This function uses SignatureChecker to validate the signature against the account's stored signer. | |
| * | |
| * Expected signer formats: | |
| * - EOA (Externally Owned Account) addresses (ECDSA signatures) | |
| * - ERC1271 contract-based signers (isValidSignature) | |
| * | |
| * Supported cryptographic schemes: | |
| * - ECDSA (secp256k1) | |
| * - ERC1271 contract signatures | |
| * | |
| * Derived contracts can override {_customSignatureValidation} to support additional formats or schemes. | |
| */ | |
| function _rawERC7579Validation( | |
| address account, | |
| bytes32 hash, | |
| bytes calldata signature | |
| ) internal view virtual override returns (bool) { | |
| // Extension point for custom validation logic | |
| bool customValid = _customSignatureValidation(account, hash, signature); | |
| if (customValid) { | |
| return true; | |
| } | |
| return SignatureChecker.isValidSignatureNow(signer(account), hash, signature); | |
| } | |
| /** | |
| * @dev Extension point for custom signature validation logic. | |
| * Override in derived contracts to support additional signer formats or cryptographic schemes. | |
| * Return true if the signature is valid, false otherwise. | |
| */ | |
| function _customSignatureValidation( | |
| address account, | |
| bytes32 hash, | |
| bytes calldata signature | |
| ) internal view virtual returns (bool) { | |
| // Default implementation: no custom validation | |
| return false; | |
| } | |
| } |
![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 (11)
.changeset/yummy-ideas-stay.md (1)
5-5: Grammar polish for clarityAdd the missing article.
-`ERC7579Validator`: Add abstract validator module for ERC-7579 accounts that provides base implementation for signature validation. +`ERC7579Validator`: Add abstract validator module for ERC-7579 accounts that provides a base implementation for signature validation..changeset/solid-squids-cough.md (1)
5-5: Wording tweak for readabilityUse “using” instead of “through” and prefer “addressless”.
-`ERC7579Signature`: Add implementation of `ERC7579Validator` that enables ERC-7579 accounts to integrate with address-less cryptographic keys and account signatures through ERC-7913 signature verification. +`ERC7579Signature`: Add implementation of `ERC7579Validator` that enables ERC-7579 accounts to integrate with addressless cryptographic keys and account signatures using ERC-7913 signature verification.contracts/account/README.adoc (1)
10-11: Minor copy editsAdd the article and prefer “addressless”.
-* {ERC7579Validator}: Abstract validator module for ERC-7579 accounts that provides base implementation for signature validation. -* {ERC7579Signature}: Implementation of {ERC7579Validator} using ERC-7913 signature verification for address-less cryptographic keys and account signatures. +* {ERC7579Validator}: Abstract validator module for ERC-7579 accounts that provides a base implementation for signature validation. +* {ERC7579Signature}: Implementation of {ERC7579Validator} using ERC-7913 signature verification for addressless cryptographic keys and account signatures.test/account/modules/ERC7579Signature.test.js (5)
49-54: Simplify signer preparation (no need for prototype/call indirection)Bind directly to the signer instance to reduce cognitive load.
-function prepareSigner(prototype) { - this.signUserOp = userOp => - prototype.signTypedData - .call(this.signer, this.entrypointDomain, { PackedUserOperation }, userOp.packed) - .then(signature => Object.assign(userOp, { signature })); -} +function prepareSigner() { + this.signUserOp = userOp => + this.signer + .signTypedData(this.entrypointDomain, { PackedUserOperation }, userOp.packed) + .then(signature => Object.assign(userOp, { signature })); +}
121-129: Adjust caller after simplification aboveCall the simplified preparer without constructing an extra NonNativeSigner.
- this.signer = signerP256; - prepareSigner.call(this, new NonNativeSigner(this.signer.signingKey)); + this.signer = signerP256; + prepareSigner.call(this);
136-146: Adjust RSA setup similarlyKeep setup uniform across key types.
- this.signer = signerRSA; - prepareSigner.call(this, new NonNativeSigner(this.signer.signingKey)); + this.signer = signerRSA; + prepareSigner.call(this);
69-77: Optional: assert no event on second installIf “noop on reinstall” is a requirement, consider asserting that
ERC7579SignatureSignerSetis not emitted on the second call.
111-116: Consistency: invoke setSigner via the impersonated accountKeeps access pattern consistent with the successful path test; avoids accidental dependency on default signer.
- await expect(this.mock.setSigner('0x0123456789')).to.be.revertedWithCustomError( + await expect(this.mockFromAccount.setSigner('0x0123456789')).to.be.revertedWithCustomError( this.mock, 'ERC7579SignatureInvalidSignerLength', );test/account/modules/ERC7579Module.behavior.js (1)
30-35: Prefer await chaining for clarity (no functional change).Using await + then is unusual in tests. Split into two awaits for readability.
- const userOp = await this.mockAccount.createUserOp(this.userOp).then(op => this.signUserOp(op)); + const op = await this.mockAccount.createUserOp(this.userOp); + const userOp = await this.signUserOp(op);contracts/account/modules/ERC7579Signature.sol (2)
24-26: Event can be more indexable.Consider emitting verifier separately for easier off-chain filtering (optional).
- event ERC7579SignatureSignerSet(address indexed account, bytes signer); + event ERC7579SignatureSignerSet(address indexed account, address indexed verifier, bytes key);Follow-up: parse verifier = address(bytes20(signer_)) and key = signer_[20:].
17-20: Minor doc typo.- * Ethereum address.A smart account with this module installed can keep an emergency key as a + * Ethereum address. A smart account with this module installed can keep an emergency key as a
📜 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 (8)
.changeset/solid-squids-cough.md(1 hunks).changeset/yummy-ideas-stay.md(1 hunks)contracts/account/README.adoc(2 hunks)contracts/account/modules/ERC7579Signature.sol(1 hunks)contracts/account/modules/ERC7579Validator.sol(1 hunks)test/account/modules/ERC7579Module.behavior.js(1 hunks)test/account/modules/ERC7579Signature.test.js(1 hunks)test/account/modules/ERC7579Validator.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/account/modules/ERC7579Validator.test.js (3)
test/account/modules/ERC7579Module.behavior.js (7)
require(1-1)require(2-2)require(3-3)userOp(31-31)userOp(38-38)signature(50-50)signature(58-58)test/helpers/account.js (1)
impersonate(7-10)test/helpers/erc7579.js (1)
MODULE_TYPE_VALIDATOR(3-3)
test/account/modules/ERC7579Signature.test.js (3)
test/account/modules/ERC7579Module.behavior.js (7)
require(1-1)require(2-2)require(3-3)userOp(31-31)userOp(38-38)signature(50-50)signature(58-58)test/helpers/account.js (1)
impersonate(7-10)test/helpers/erc7579.js (1)
MODULE_TYPE_VALIDATOR(3-3)
test/account/modules/ERC7579Module.behavior.js (3)
test/account/modules/ERC7579Signature.test.js (9)
require(1-1)require(2-2)require(3-3)require(5-5)require(6-6)require(7-7)require(8-8)require(9-9)require(11-11)test/account/modules/ERC7579Validator.test.js (7)
require(1-1)require(2-2)require(4-4)require(5-5)require(6-6)require(7-7)require(9-9)test/helpers/erc4337.js (1)
SIG_VALIDATION_FAILURE(4-4)
🪛 LanguageTool
.changeset/yummy-ideas-stay.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...lidity': minor --- ERC7579Validator: Add abstract validator module for ERC-7579 ...
(QB_NEW_EN)
[grammar] ~5-~5: There might be a mistake here.
Context: ...dator module for ERC-7579 accounts that provides base implementation for signature valid...
(QB_NEW_EN)
[grammar] ~5-~5: There might be a mistake here.
Context: ...implementation for signature validation.
(QB_NEW_EN)
⏰ 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). (8)
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
- GitHub Check: slither
- GitHub Check: tests-upgradeable
- GitHub Check: coverage
- GitHub Check: tests-foundry
- GitHub Check: tests
🔇 Additional comments (7)
test/account/modules/ERC7579Validator.test.js (1)
11-46: Fixture setup looks solidE2E wiring for ECDSA case is coherent: deploy mock, build 4337 domain, create signer, pack install data, impersonate account, and expose context consumed by shared behaviors.
contracts/account/README.adoc (1)
28-33: Good docs structureDedicated “Validators” section and anchors are consistent with the rest of the README layout.
test/account/modules/ERC7579Module.behavior.js (2)
12-16: Lifecycle happy-path looks good.
37-43: Your scripts are running; I’ll review the implementations forpackedandcreateUserOpto confirm ifpackedis dynamically computed or pre-stored.contracts/account/modules/ERC7579Signature.sol (1)
79-85: SignatureChecker overload for bytes-signer confirmed—no action requiredThe
SignatureCheckerlibrary indeed defines an overloadfunction isValidSignatureNow( bytes memory signer, bytes32 hash, bytes memory signature ) internal view returns (bool) { … }in
contracts/utils/cryptography/SignatureChecker.sol(lines 105–113), which matches the call inERC7579Signature.solboth in parameter types and order.contracts/account/modules/ERC7579Validator.sol (2)
62-70: validateUserOp wiring looks correct.
79-88: isValidSignatureWithSender behavior documented and consistent.Ignoring the sender param and using msg.sender matches the test harness pattern. Ensure tests always call via the account context (mockFromAccount).
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| 'openzeppelin-solidity': minor | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify package key matches the workspace configuration
Ensure openzeppelin-solidity is the correct package name in this repo’s Changesets config; otherwise use the actual workspace package (often @openzeppelin/contracts).
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Packages matching expected names:"
fd -a package.json | xargs -I{} sh -c 'jq -r ".name + \" @ \" + (.version // \"unknown\") + \" :: {}\"" "{}"' | rg '@openzeppelin/contracts|openzeppelin-solidity' -n || true
echo
echo "Changesets referencing package keys:"
rg -n "openzeppelin-solidity|@openzeppelin/contracts" .changeset || trueLength of output: 2180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Root package.json metadata:"
jq '{name, version, private, workspaces}' package.json
echo
echo "Changesets config:"
jq '.' .changeset/config.jsonLength of output: 590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for pnpm-workspace.yaml..."
if [ -f pnpm-workspace.yaml ]; then
echo "Found pnpm-workspace.yaml"
else
echo "pnpm-workspace.yaml not found"
fi
echo
echo "Checking for lerna.json..."
if [ -f lerna.json ]; then
echo "Found lerna.json"
else
echo "lerna.json not found"
fi
echo
echo "Searching for 'workspaces' key in package.json..."
grep -R '"workspaces"' -n package.json || echo "No 'workspaces' key in package.json"
echo
echo "Searching for 'packages' key in .changeset/config.json..."
grep -R '"packages"' -n .changeset/config.json || echo "No 'packages' key in .changeset/config.json"Length of output: 890
Update Changesets to target the published package (@openzeppelin/contracts)
Your current setup is bumping the private root package (name: "openzeppelin-solidity") instead of the actual npm package in contracts/package.json (@openzeppelin/contracts). To fix this, you need to tell Changesets about the subpackage and update all existing changesets to use its key.
• In .changeset/config.json, add the packages field to point at the contracts folder:
{
"$schema": "https://unpkg.com/@changesets/config@2.3.0/schema.json",
- "changelog": [
+ "packages": ["contracts"],
+ "changelog": [
"@changesets/changelog-github",
{
"repo": "OpenZeppelin/openzeppelin-contracts"
}
],
"commit": false,
"access": "public",
"baseBranch": "master"
}• Update each existing changeset in .changeset/*.md so that the first line uses the published package name. For example, in yummy-ideas-stay.md:
- 'openzeppelin-solidity': minor
+ '@openzeppelin/contracts': minor• Repeat for all files under .changeset/ (e.g. witty-hats-flow.md, whole-cats-find.md, etc.).
After these updates, running Changesets will correctly bump and publish @openzeppelin/contracts (version 5.4.0) instead of the private root.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'openzeppelin-solidity': minor | |
| '@openzeppelin/contracts': minor |
🤖 Prompt for AI Agents
In .changeset/yummy-ideas-stay.md around line 2 and across other .changeset/*.md
files, the changeset is targeting the private root package
('openzeppelin-solidity') instead of the published package; change the first
line of this and every changeset file to use the published package key (for
example "@openzeppelin/contracts: minor") so Changesets will bump the correct
package, and add a packages field to .changeset/config.json pointing to the
contracts folder (e.g. "packages": ["contracts"]) so Changesets knows to target
the subpackage.
| /// @dev Sets the ERC-7913 signer (i.e. `verifier || key`) for the calling account. | ||
| function setSigner(bytes memory signer_) public virtual { | ||
| require(signer_.length >= 20, ERC7579SignatureInvalidSignerLength()); | ||
| _setSigner(msg.sender, signer_); | ||
| } |
There was a problem hiding this comment.
Custom error with require will not compile; use revert.
require(..., CustomError()) is invalid. Switch to an explicit revert.
- function setSigner(bytes memory signer_) public virtual {
- require(signer_.length >= 20, ERC7579SignatureInvalidSignerLength());
- _setSigner(msg.sender, signer_);
- }
+ function setSigner(bytes memory signer_) public virtual {
+ if (signer_.length < 20) revert ERC7579SignatureInvalidSignerLength();
+ _setSigner(msg.sender, signer_);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @dev Sets the ERC-7913 signer (i.e. `verifier || key`) for the calling account. | |
| function setSigner(bytes memory signer_) public virtual { | |
| require(signer_.length >= 20, ERC7579SignatureInvalidSignerLength()); | |
| _setSigner(msg.sender, signer_); | |
| } | |
| /// @dev Sets the ERC-7913 signer (i.e. `verifier || key`) for the calling account. | |
| function setSigner(bytes memory signer_) public virtual { | |
| if (signer_.length < 20) revert ERC7579SignatureInvalidSignerLength(); | |
| _setSigner(msg.sender, signer_); | |
| } |
🤖 Prompt for AI Agents
In contracts/account/modules/ERC7579Signature.sol around lines 58 to 62, the
require call using a custom error will not compile; replace the require(...,
CustomError()) with an explicit check and revert: check signer_.length < 20 in
an if statement and revert ERC7579SignatureInvalidSignerLength() when true, then
call _setSigner(msg.sender, signer_); keep visibility and function signature
unchanged.
| it('returns failure value for invalid signature', async function () { | ||
| const hash = ethers.hashMessage('Hello, world!'); | ||
| const signature = this.invalidSignature || '0x00'; | ||
| await expect(this.mock.isValidSignatureWithSender(this.other, hash, signature)).to.eventually.equal( | ||
| INVALID_VALUE, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use mockFromAccount here for consistency with msg.sender-based validation.
isValidSignatureWithSender relies on msg.sender (the account). Using this.mock may mask issues.
- await expect(this.mock.isValidSignatureWithSender(this.other, hash, signature)).to.eventually.equal(
+ await expect(this.mockFromAccount.isValidSignatureWithSender(this.other, hash, signature)).to.eventually.equal(
INVALID_VALUE,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('returns failure value for invalid signature', async function () { | |
| const hash = ethers.hashMessage('Hello, world!'); | |
| const signature = this.invalidSignature || '0x00'; | |
| await expect(this.mock.isValidSignatureWithSender(this.other, hash, signature)).to.eventually.equal( | |
| INVALID_VALUE, | |
| ); | |
| it('returns failure value for invalid signature', async function () { | |
| const hash = ethers.hashMessage('Hello, world!'); | |
| const signature = this.invalidSignature || '0x00'; | |
| await expect(this.mockFromAccount.isValidSignatureWithSender(this.other, hash, signature)).to.eventually.equal( | |
| INVALID_VALUE, | |
| ); | |
| }); |
🤖 Prompt for AI Agents
test/account/modules/ERC7579Module.behavior.js around lines 56 to 61: the test
calls isValidSignatureWithSender on this.mock which doesn’t simulate msg.sender
correctly; replace the call to use the mock instance created for the account
(mockFromAccount) so msg.sender-based validation is executed. Specifically,
obtain the mock bound to this.other (or create mockFromAccount(this.other)) and
call isValidSignatureWithSender on that mock instance with the same hash and
signature, keeping the assertion expecting INVALID_VALUE.
Smaller version of #5888 for validator modules
PR Checklist
npx changeset add)Summary by Sourcery
Introduce a new abstract ERC-7579 validator module and a concrete ERC-7913–based signature validator with accompanying documentation and tests
New Features:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores