feat(account-modules): Add CoSignerValidator for server co-signing#57
feat(account-modules): Add CoSignerValidator for server co-signing#57pcarranzav wants to merge 7 commits intomainfrom
Conversation
- ERC-7579 validator requiring agent key + server co-signer - Per-account admin trust model with global co-signer sets - Supports both validateUserOp and isValidSignatureWithSender - ERC-7201 namespaced storage for upgrade safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 32 tests covering installation, validation, key management - Tests dual signature validation (UserOp and ERC-1271 paths) - Tests admin/coSigner rotation with grace period - Tests multi-account scenarios - All tests passing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Uses Safe Singleton Deployer for deterministic CREATE2 deployment - Same address across all chains - Includes predict() function to compute address before deployment - Predicted address: 0x972F1d56Ec65248f57CF1204b2F012a3a2d7f82B Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| address account = msg.sender; | ||
|
|
||
| // Prevent double initialization | ||
| require(!$.accountInitialized[account], "Already initialized"); |
There was a problem hiding this comment.
absolute nit, but custom errors have been really nice for us debugging horizon issues
| { | ||
| CoSignerStorage storage $ = _getStorage(); | ||
|
|
||
| // Decode signature as (bytes agentSig, bytes coSignerSig) |
There was a problem hiding this comment.
Probably worth putting the signature decodification and validation into an internal fn, this function and the previous one pretty much duplicate this code
| * @notice ERC-7579 validator requiring dual signatures (agent key + server co-signer) | ||
| * @dev Provides server-enforced spend limits and programmable policies for agent operations | ||
| */ | ||
| contract CoSignerValidator is ICoSignerValidator, ERC7579ValidatorBase, ReentrancyGuard { |
There was a problem hiding this comment.
ReentrancyGuard is never used in the contract
| /** | ||
| * @notice Called when module is uninstalled for an account | ||
| */ | ||
| function onUninstall(bytes calldata) external override(IModule) { |
There was a problem hiding this comment.
we probably want to emit some events here like AgentKeyRemoved
| * @dev Can only be called by the account itself | ||
| * @param key The agent key address to add | ||
| */ | ||
| function addAgentKey(address key) external { |
There was a problem hiding this comment.
Thinking about batched ops for adding/removing agent keys. Probably adding multiuple keys at once wont be used much but removing them sounds more plausible? Worth considering.
| * @param account The account address | ||
| * @return The admin address | ||
| */ | ||
| function getAdmin(address account) external view returns (address) { |
There was a problem hiding this comment.
might be worth adding a view function to do account -> co-signer? isValidCoSignerForAccount() or something like that, otherwise it's a couple of contract calls i think.
|
Seems like agent+co-signer can currently call Suggestion: Add an function setAdmin(address newAdmin, bytes calldata ownerSig) external {
bytes32 hash = keccak256(abi.encode(account, "setAdmin", newAdmin, nonce++));
require($.owners[account].contains(ECDSA.recover(hash, ownerSig)), "Not owner");
// ...
}Include |
|
|
||
| // ============ Constructor ============ | ||
|
|
||
| constructor() {} |
| require($.accountInitialized[account], "Not initialized"); | ||
|
|
||
| $.agentKeys[account].remove(key); | ||
| emit AgentKeyRemoved(account, key); |
There was a problem hiding this comment.
will emit even if $.agentKeys[account].remove(key); is falsy?
There was a problem hiding this comment.
oh nice one, .remove() does not fail but returns false if the key does not exist no?
| function removeCoSigner(address coSigner) external { | ||
| CoSignerStorage storage $ = _getStorage(); | ||
| $.coSigners[msg.sender].remove(coSigner); | ||
| emit CoSignerRemoved(msg.sender, coSigner); |
I think this is a necessary aspect of how this works. Agent + co-signer can execute user ops and sign arbitrary messages, so they could already add/remove owners on the OwnableValidator, drain the account, etc. The server is being trusted to add any necessary guardrails to the agent key. It's essentially a 2/2 multisig, so if both keys are compromised the account is compromised. |
Right, I missed that. Thanks for clarifying. |
- Switch to custom errors for better debugging (CoSignerValidator_ prefix) - Remove ReentrancyGuard (not used) - Remove empty constructor - Extract _validateDualSignature() internal helper (DRY) - Add events in onUninstall (emit AgentKeyRemoved for each key) - Check remove() return value before emitting events - Add batched operations: addAgentKeys() and removeAgentKeys() - Add isValidCoSignerForAccount() helper view function - Update tests for custom errors - Add tests for batched operations and helper function All 35 tests passing ✅ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add CoSignerValidatorInstalled event in onInstall - Add CoSignerValidatorUninstalled event in onUninstall - Update tests to expect new events Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
matiasedgeandnode
left a comment
matiasedgeandnode
left a comment
There was a problem hiding this comment.
Maybe add a test for uninstall → reinstall flow? Also the same address can be registered as both an agent key and a co-signer, but maybe this is fine?
Another nice test would be a test for duplicate agent key in onInstall.
In fact, wouldn't it make sense to have a private helper for _addAgentKeys that onInstall, addAgentKey, and addAgentKeys can call? And the same for _removeAgentKeys
| error CoSignerValidator_AlreadyInitialized(); | ||
| error CoSignerValidator_NotInitialized(); | ||
| error CoSignerValidator_InvalidAddress(); | ||
| error CoSignerValidator_InvalidAgentKey(); |
There was a problem hiding this comment.
CoSignerValidator_InvalidAgentKey and CoSignerValidator_InvalidCoSigner are declared but never used anywhere?
| address key = keys[i]; | ||
| require(key != address(0), CoSignerValidator_InvalidAddress()); | ||
| $.agentKeys[account].add(key); | ||
| emit AgentKeyAdded(account, key); |
There was a problem hiding this comment.
will emit even if key was already there? Is that intentional?
|
|
||
| CoSignerStorage storage $ = _getStorage(); | ||
| $.coSigners[msg.sender].add(coSigner); | ||
| emit CoSignerAdded(msg.sender, coSigner); |
There was a problem hiding this comment.
will emit even if coSigner was already there? Is that intentional?
| address key = initialAgentKeys[i]; | ||
| require(key != address(0), CoSignerValidator_InvalidAddress()); | ||
| $.agentKeys[account].add(key); | ||
| emit AgentKeyAdded(account, key); |
| /** | ||
| * @notice Called when module is uninstalled for an account | ||
| */ | ||
| function onUninstall(bytes calldata) external override(IModule) { |
There was a problem hiding this comment.
Maybe exit early with an error if not installed?
| * @dev msg.sender is the admin adding to their own co-signer set | ||
| * @param coSigner The co-signer address to add | ||
| */ | ||
| function addCoSigner(address coSigner) external { |
There was a problem hiding this comment.
maybe require($.accountInitialized[account], CoSignerValidator_NotInitialized()); here and in removeCoSigner?
There was a problem hiding this comment.
not really, this is for admins, not for accounts
Contract changes: - Remove unused errors (InvalidAgentKey, InvalidCoSigner) - Extract _addAgentKey/_removeAgentKey private helpers (DRY for onInstall, addAgentKey, addAgentKeys, removeAgentKey, removeAgentKeys) - Check .add() return value - only emit AgentKeyAdded if actually added - Check .add() return value in addCoSigner - only emit if actually added - Add require(accountInitialized) guard in onUninstall (exit early) New tests: - Uninstall -> reinstall flow - Duplicate agent key in onInstall (deduplicates via EnumerableSet) - Duplicate addAgentKey does not emit - Duplicate addCoSigner does not emit - onUninstall reverts if not initialized All 40 tests passing ✅ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
Adds CoSignerValidator - a new ERC-7579 validator module that enables 2-of-2 co-signing (agent key + server key) for smart account operations.
Changes
Architecture
validateUserOp(ERC-4337) andisValidSignatureWithSender(ERC-1271)Deployment
0x972F1d56Ec65248f57CF1204b2F012a3a2d7f82BTesting
All 32 tests passing ✅
Next Steps
Related
tmp/server_cosigning_spec.mdtmp/server_cosigning_implementation_plan.md🤖 Generated with Claude Code