Account implementation and identities #5182
Conversation
|
| bytes private _e; | ||
| bytes private _n; | ||
|
|
||
| function signer() public view virtual returns (bytes memory e, bytes memory n) { | ||
| return abi.decode(Clones.fetchCloneArgs(address(this)), (bytes, bytes)); | ||
| return (_e, _n); |
There was a problem hiding this comment.
This is going in storage. That is not complying with 4337 storage accesses.
There was a problem hiding this comment.
Right, even in immutable arguments it accesses storage via EIP712Readable. It also discards upgradeability and most other account providers I've reviewed don't identify this as an issue or just don't care about it.
I asked about this in Alto (Pimlico) and it seems like these transactions will be processed privately but not broadcasted to the decentralized mempool (whatever that is). So far, Accounts can read their own storage or external "associated storage" (i.e. keccak(account_address||bytes32)+n where n is up to 128) so that enables to validate multiple contract signatures using IERC1271 through a validator.
With that approach, a MultisigValidator is just a multiplexer of multiple accounts checked with SignatureChecker.isValidSignatureNow and is still compatible during the validation phase. However, any upgradeable version of the account or associated modules breaks the rules again (checks non-associated storage when reading the implementation slot).
From what I've seen (e.g. Safe), Multisigs frequently work by collecting the signatures and then packing them into a transaction that someone else's execute, for that reason I think it's fine that multisig-approved calls are not validated during the execution phase, and rather executed by another account or relayer.
Bottom line is: Our default recommendation is an AccountERC7579 that requires at least 1 validator module. Validator modules shouldn't be upgradeable or they'll break storage access rules although the account can still be executed from another account or through a private bundler (e.g Pimlico whitelists Safe proxy accounts even though they read the master copy from storage).
| bytes32 private immutable _qx; | ||
| bytes32 private immutable _qy; | ||
|
|
||
| function signer() public view virtual returns (bytes32 qx, bytes32 qy) { | ||
| return abi.decode(Clones.fetchCloneArgs(address(this)), (bytes32, bytes32)); | ||
| return (_qx, _qy); |
There was a problem hiding this comment.
Any reason for that change ? My understanding is that it requires redeploying an entier instnace for every new account (that include redeploying the P256 library).
IMO clonesWithArgs are way better here.
There was a problem hiding this comment.
Yes. I still think we should document how to use signer clones since they are usable for upgradeable accounts where the validation is in its own storage (compliant with the storage validation rules). However, the easiest approach is to use a validator module if it's not upgradeable.
Imo the clones with immutable arguments shouldn't be packaged into the library and rather a section in our documentation where we describe what "immutable signers" are useful for.
Alternative to #5119
PR Checklist
npx changeset add)