⚡️ Optimise gas usage when enabling p256 validator, add a few comments (#52)

* ⚡️ Optimise gas usage when enabling p256 validator, add a few comments

- Reducing the number of indexed variable inside a log highly decrease his gas usage, in the p256 validator, we only matter about the kernel account as index (it's 375 gas per topic, so per indexed props, so reducing the index on both key reduce the enabling gas cost by 375 * 4 -> 1500 gas)
 - Add a few reflexion todo comment, do you rly need to send the previous key in the event? Since it's cost with a `sload`& also in the event itself

* ⚡️ Remove the oldKeys for the event signature

* ⚡️ Only compare to raw msg signing instead of eth signed message for p256

Author: KONFeature

src/Kernel.sol

@@ -29,7 +29,7 @@ contract Kernel is EIP712, Compatibility, KernelStorage {
     /// @dev Selector of the `DisabledMode()` error, to be used in assembly, 'bytes4(keccak256(bytes("DisabledMode()")))', same as DisabledMode.selector()
     uint256 private constant _DISABLED_MODE_SELECTOR = 0xfc2f51c5;
 
-    /// @dev Current kernel name and version, todo: Need to expose getter for this variables? 
+    /// @dev Current kernel name and version, todo: Need to expose getter for this variables?
     string public constant name = KERNEL_NAME;
     string public constant version = KERNEL_VERSION;
 

src/validator/P256Validator.sol

@@ -9,55 +9,72 @@ import {ValidationData} from "../common/Types.sol";
 import {SIG_VALIDATION_FAILED} from "../common/Constants.sol";
 import {P256} from "p256-verifier/P256.sol";
 
+/// @title P256Validator
+/// @notice This validator uses the P256 curve to validate signatures.
 contract P256Validator is IKernelValidator {
-    event P256PublicKeysChanged(address indexed kernel, PublicKey indexed oldKeys, PublicKey indexed newKeys);
+    /// @notice Emitted when a bad key is provided.
+    error BadKey();
+
+    /// @notice Emitted when the public key of a kernel is changed.
+    event P256PublicKeysChanged(address indexed kernel, PublicKey newKeys);
 
+    /// @notice The P256 public key of a kernel.
     struct PublicKey {
         uint256 x;
         uint256 y;
     }
 
-    error BadKey();
-
-    mapping(address => PublicKey) public p256PublicKey;
+    /// @notice The P256 public keys of a kernel.
+    mapping(address kernel => PublicKey PublicKey) public p256PublicKey;
 
+    /// @notice Enable this validator for a kernel account.
+    /// @dev The kernel account need to be the `msg.sender`.
+    /// @dev The public key is encoded as `abi.encode(PublicKey)` inside the data, so (uint256,uint256).
     function enable(bytes calldata _data) external payable override {
         PublicKey memory key = abi.decode(_data, (PublicKey));
         if (key.x == 0 || key.y == 0) {
             revert BadKey();
         }
-        PublicKey memory oldKey = p256PublicKey[msg.sender];
+        // Update the key (so a sstore)
         p256PublicKey[msg.sender] = key;
-        emit P256PublicKeysChanged(msg.sender, oldKey, key);
+        // And emit the event
+        emit P256PublicKeysChanged(msg.sender, key);
     }
 
+    /// @notice Disable this validator for a kernel account.
+    /// @dev The kernel account need to be the `msg.sender`.
     function disable(bytes calldata) external payable override {
         delete p256PublicKey[msg.sender];
     }
 
-    function validateUserOp(UserOperation calldata _userOp, bytes32 _userOpHash, uint256) external payable override returns (ValidationData validationData) {
+    /// @notice Validate a user operation.
+    function validateUserOp(UserOperation calldata _userOp, bytes32 _userOpHash, uint256)
+        external
+        payable
+        override
+        returns (ValidationData validationData)
+    {
         (uint256 r, uint256 s) = abi.decode(_userOp.signature, (uint256, uint256));
         PublicKey memory key = p256PublicKey[_userOp.sender];
-        bytes32 hash = ECDSA.toEthSignedMessageHash(_userOpHash);
-        if (P256.verifySignature(hash, r, s, key.x, key.y)) {
+        if (P256.verifySignature(_userOpHash, r, s, key.x, key.y)) {
             return ValidationData.wrap(0);
-        } 
-        if (!P256.verifySignature(_userOpHash, r, s, key.x, key.y)) {
-            return SIG_VALIDATION_FAILED;
         }
+        return SIG_VALIDATION_FAILED;
     }
 
-    function validateSignature(bytes32 hash, bytes calldata signature) external view override returns (ValidationData) {
+    /// @notice Validate a signature.
+    function validateSignature(bytes32 hash, bytes calldata signature)
+        external
+        view
+        override
+        returns (ValidationData)
+    {
         (uint256 r, uint256 s) = abi.decode(signature, (uint256, uint256));
         PublicKey memory key = p256PublicKey[msg.sender];
         if (P256.verifySignature(hash, r, s, key.x, key.y)) {
             return ValidationData.wrap(0);
         }
-        bytes32 ethHash = ECDSA.toEthSignedMessageHash(hash);
-        if (!P256.verifySignature(ethHash, r, s, key.x, key.y)) {
-            return SIG_VALIDATION_FAILED;
-        }
-        return ValidationData.wrap(0);
+        return SIG_VALIDATION_FAILED;
     }
 
     function validCaller(address _caller, bytes calldata) external view override returns (bool) {

test/foundry/KernelLiteECDSA.t.sol

@@ -97,7 +97,8 @@ contract KernelECDSATest is KernelTestBase {
 
     function test_transfer_ownership() external {
         address newOwner = makeAddr("new owner");
-        UserOperation memory op = buildUserOperation(abi.encodeWithSelector(KernelLiteECDSA.transferOwnership.selector, newOwner));
+        UserOperation memory op =
+            buildUserOperation(abi.encodeWithSelector(KernelLiteECDSA.transferOwnership.selector, newOwner));
         vm.expectEmit(true, true, true, false, address(entryPoint));
         emit UserOperationEvent(entryPoint.getUserOpHash(op), address(kernel), address(0), op.nonce, false, 0, 0);
         performUserOperationWithSig(op);

test/foundry/KernelTestBase.sol

@@ -272,7 +272,9 @@ abstract contract KernelTestBase is Test {
     function test_disable_mode() external {
         vm.warp(1000);
         bytes memory empty;
-        UserOperation memory op = buildUserOperation(abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty));
+        UserOperation memory op = buildUserOperation(
+            abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty)
+        );
         performUserOperationWithSig(op);
         assertEq(uint256(bytes32(IKernel(address(kernel)).getDisabledMode())), 1 << 224);
         assertEq(uint256(IKernel(address(kernel)).getLastDisabledTime()), block.timestamp);
@@ -336,7 +338,8 @@ abstract contract KernelTestBase is Test {
     function test_revert_when_mode_disabled() external {
         vm.warp(1000);
         bytes memory empty;
-        UserOperation memory op = buildUserOperation(abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty)
+        UserOperation memory op = buildUserOperation(
+            abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty)
         );
         performUserOperationWithSig(op);
 

test/foundry/validator/KillSwitchValidator.t.sol

@@ -68,7 +68,8 @@ contract KillSwitchValidatorTest is KernelECDSATest {
     }
 
     function test_force_unblock() external {
-        UserOperation memory op = buildUserOperation(abi.encodeWithSelector(Kernel.execute.selector, owner, 0, "", Operation.Call));
+        UserOperation memory op =
+            buildUserOperation(abi.encodeWithSelector(Kernel.execute.selector, owner, 0, "", Operation.Call));
 
         op.signature = bytes.concat(bytes4(0), entryPoint.signUserOpHash(vm, ownerKey, op));
         performUserOperation(op);

test/foundry/validator/P256Validator.t.sol

@@ -16,17 +16,14 @@ import {P256} from "p256-verifier/P256.sol";
 import {FCL_ecdsa_utils} from "FreshCryptoLib/FCL_ecdsa_utils.sol";
 import {IKernel} from "src/interfaces/IKernel.sol";
 
-
 using ERC4337Utils for IEntryPoint;
 
 contract P256ValidatorTest is KernelTestBase {
     P256Verifier p256Verifier;
     P256Validator p256Validator;
 
     // Curve order (number of points)
-    uint256 constant n =
-        0xFFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551;
-
+    uint256 constant n = 0xFFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551;
 
     uint256 x;
     uint256 y;
@@ -55,7 +52,7 @@ contract P256ValidatorTest is KernelTestBase {
         return abi.encodePacked(bytes4(0x00000000), abi.encode(r, s));
     }
 
-    function getOwners() internal virtual override returns (address[] memory _owners){
+    function getOwners() internal virtual override returns (address[] memory _owners) {
         _owners = new address[](1);
         _owners[0] = address(0);
         return _owners;
@@ -69,7 +66,7 @@ contract P256ValidatorTest is KernelTestBase {
         return abi.encodeWithSelector(KernelStorage.initialize.selector, p256Validator, abi.encode(x, y));
     }
 
-    function test_default_validator_enable() external override{
+    function test_default_validator_enable() external override {
         UserOperation memory op = buildUserOperation(
             abi.encodeWithSelector(
                 IKernel.execute.selector,
@@ -106,7 +103,7 @@ contract P256ValidatorTest is KernelTestBase {
     function test_external_call_execute_success() external override {
         vm.skip(true);
     }
-    
+
     function test_external_call_execute_delegatecall_success() external override {
         vm.skip(true);
     }
@@ -150,7 +147,7 @@ contract P256ValidatorTest is KernelTestBase {
         vm.assume(privateKey != 0);
         (uint256 x1, uint256 y1) = generatePublicKey(privateKey);
         (uint256 r, uint256 s) = generateSignature(privateKey, hash);
-        
+
         vm.assume(x1 != 0);
         vm.assume(y1 != 0);
         vm.assume(r != 0);
@@ -161,7 +158,7 @@ contract P256ValidatorTest is KernelTestBase {
     function test_validate_signature() external override {
         Kernel kernel2 = Kernel(payable(factory.createAccount(address(kernelImpl), getInitializeData(), 3)));
         bytes32 hash = keccak256(abi.encodePacked("hello world"));
-        
+
         bytes32 digest = keccak256(
             abi.encodePacked(
                 "\x19\x01", ERC4337Utils._buildDomainSeparator(KERNEL_NAME, KERNEL_VERSION, address(kernel)), hash