Add ERC-6900 account extension #80
Conversation
| * @dev Extracts the validator from the user operation. | ||
| * | ||
| */ | ||
| function _extractUserOpValidator(PackedUserOperation calldata userOp) internal pure virtual returns (ModuleEntity) { |
There was a problem hiding this comment.
_extractSignatureValidator already does that. Maybe we can drop this function, and only use _extractSignatureValidator
| const hooks = [ethers.hexlify(ethers.randomBytes(32))]; | ||
| await expect(this.mockFromEntrypoint.installValidation(validationConfig, selectors, installData, hooks)) | ||
| .to.be.revertedWithCustomError(this.mock, 'ERC6900ModuleInterfaceNotSupported') | ||
| .withArgs(moduleId, '0x46c0c1b4'); |
There was a problem hiding this comment.
this value should be defined as a constant
| if (_validationFlags.isGlobalValidation()) { | ||
| Execution storage execution = _executions[executionSelector]; | ||
| // The account MUST consider the validation applicable to any module | ||
| // execution function with the allowGlobalValidation flag set to true | ||
| require( | ||
| !execution.allowGlobalValidation, | ||
| ERC6900Utils.ERC6900ExecutionSelectorNotAllowedForGlobalValidation( | ||
| validationModuleEntity, | ||
| executionSelector | ||
| ) | ||
| ); | ||
| } else { | ||
| // validation functions have a configurable range of applicability. | ||
| // This can be configured with selectors installed to a validation | ||
| require( | ||
| !validation.selectors.contains(executionSelector), | ||
| ERC6900Utils.ERC6900MissingValidationForSelector(executionSelector) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This part of the ERC is somewhat ambiguous to me. Should a validation that has the isGlobal flag be able to also specify selectors for which it is valid? Which approach do you think is better?
Note: if we do go forward with this interpretation, maybe we should disallow adding validation selectors when the isGlobal flag is true? https://github.com/jeremyjams/openzeppelin-community-contracts/blob/a0c58324195ab1bff1f4b8d8d9c4e655c6a8ec8d/contracts/account/extensions/AccountERC6900.sol#L280-L285
There was a problem hiding this comment.
Ok, about your question "Should a validation that has the isGlobal flag be able to also specify selectors for which it is valid?"
We can see from the ERC:
-
The standard supports a isGlobalValidation flag for validation functions, which means that this function is added to a global validation pool and can validate any of the execution functions that expose themselves to global validation via allowGlobalValidation flag.
-
validation functions [...] can be configured with selectors installed to a validation
From than I understand that:
- Any execution function being declared as allowed for global validation can be validated by a validation set as global
- and Any execution function can be validated by a validation having the execution selector installed
From that I understand that a same validation could validate both rules, then I feel we should not "disallow adding validation selectors when the isGlobal flag is true". What do think, do you still stick to your first impression?
There was a problem hiding this comment.
I think we could go either way. The reference impl actually does something similar to what we have here already (either global or selector based, not both). Go with whatever you think makes more sense for how validation functions will be utilized.
| * if the module specified by the first 20 bytes of the nonce key is installed. Falls back to | ||
| * {AccountCore-_validateUserOp} otherwise. |
There was a problem hiding this comment.
Is the fallback currently implemented?
There was a problem hiding this comment.
Thank you, indeed fallback was not implemented at all, please see Init fallback now.
There was a problem hiding this comment.
Ah, I was referring to the fallback to super mentioned in _validateUserOp. Sorry for the confusion.
| function _rawSignatureValidation( | ||
| bytes32 hash, | ||
| bytes calldata signature | ||
| ) internal view virtual override returns (bool) { |
There was a problem hiding this comment.
Given #80 (comment) it seems like super is not being called, as such, when is this function utilized?
There was a problem hiding this comment.
The AccountERC6900 being an ERC7739, this function is at least called from there, what do you think?
https://eips.ethereum.org/EIPS/eip-6900#validation-call-flow
Signature validation happens within the account’s implementation of the function isValidSignature, defined in ERC-1271.
There was a problem hiding this comment.
@arr00 mentioned to me that ERC-6900 requires 1271 to be implemented using validation modules, and that having ERC-7739 + _rawSignatureValidation is not the right approach.
However, having a quick look at ERC-6900 and I am not seeing that. @arr00, could you please direct me to the relevant part ?
Edit: possible missunderstanding
There was a problem hiding this comment.
Note: considering the signature validation mechanism (calling validateSignature on a module with address(this) as one of the arguments), I'm not convinced we want to use 7739 in the case of an ERC6900 account.
There was a problem hiding this comment.
Here is the paragraph on the validation call flow. https://eips.ethereum.org/EIPS/eip-6900#validation-call-flow
There was a problem hiding this comment.
Sorry I was a little confused here. I think the callflow is sound. That said, do you think it might make more sense to implement ERC7739 on the module side as opposed to the account side?
There was a problem hiding this comment.
"not convinced we want to use 7739 in the case of an ERC6900 account"
I think I get your point, preventing signature replays should be the responsibility of the validator module. What you suggest is aligned with related reference validator module.
Please see new "Expose ERC-1271 signature validation" commit.
| require( | ||
| ERC165Checker.supportsInterface(module, type(IERC6900Module).interfaceId), | ||
| ERC6900Utils.ERC6900ModuleInterfaceNotSupported(module, type(IERC6900Module).interfaceId) | ||
| ); |
There was a problem hiding this comment.
We can't always call supportsInterface since the module may be an EOA (or other non-module) used for direct validation.
There was a problem hiding this comment.
I'm not sure to understand, can a module be an EOA?
There was a problem hiding this comment.
See the paragraph on direct call validation in the ERC. Yes, you can install a validation function with the module being an EOA.
There was a problem hiding this comment.
Hey all, I wanted to mention here that the ERC-6900 spec does not require that this check happens onchain, and we realized that having this as non-standard behavior in the reference implementation is probably confusing. So, we've decided to remove it from the RI here: erc6900/reference-implementation#214
Per our discussion with @Amxx and @ernestognw, we're still requiring this to be implemented by modules, and moving the responsibility of checking to be off-chain (i.e. by wallets) prior to installing a module.
Co-authored-by: Hadrien Croubois <[email protected]>
| bytes4[] calldata selectors, | ||
| bytes calldata installData, | ||
| bytes[] calldata hooks | ||
| ) public virtual onlyEntryPointOrSelf { |
There was a problem hiding this comment.
onlyEntryPointOrSelf doesn't appear to be the right modifier to use here, at least according to the reference implementation
In progress works: