Feature: Implement a Passkey `SignatureValidator` factory #306

mmv08 · 2024-03-05T17:47:24Z

This PR is a partial solution for #288. What needs to be added is a proper test suite for the factory. It only has one migrated end-to-end test from the 4337 module package.

What was done:

Introduce ICustomECSignerFactory and ICustom256BitECSignerFactory interfaces for custom signer factories. The first is generic and should work with signing algorithms of any key length, while the second is optimized for EC signing and 256-bit key sizes. The first one is not used in the project and is kept as an example or for future reference.
Add the WebAuthnVerifier and make it use the new P256Library introduced in Introduce P-256 Verifier Contract #298
Migrate the SafeSignerLaunchpad contract and make it implement the ICustom256BitECSignerFactory interface thus making it Safe256BitECSignerLaunchpad
Migrate and adjust the end-to-end test from the 4337 module for the launchpad

To be done:

Remove the migrated contracts from the 4337 package

coveralls · 2024-03-05T17:53:03Z

Pull Request Test Coverage Report for Build 8177592072

Details

4 of 95 (4.21%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-62.4%) to 14.474%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
modules/passkey/contracts/SignatureValidator.sol	0	4	0.0%
modules/passkey/contracts/verifiers/WebAuthnVerifier.sol	1	12	8.33%
modules/passkey/contracts/WebAuthnSigner.sol	0	21	0.0%
modules/passkey/contracts/4337/Safe256BitECSignerLaunchpad.sol	3	58	5.17%

Totals
Change from base Build 8160381332:	-62.4%
Covered Lines:	17
Relevant Lines:	108

💛 - Coveralls

mmv08 · 2024-03-06T12:48:28Z

modules/4337/contracts/experimental/WebAuthnSigner.sol

@@ -5,7 +5,6 @@ pragma solidity >=0.8.0;
 import {SignatureValidatorConstants} from "./SignatureValidatorConstants.sol";
 import {IUniqueSignerFactory} from "./SafeSignerLaunchpad.sol";
 import {SignatureValidator} from "./SignatureValidator.sol";
-import {SignatureValidatorConstants} from "./SignatureValidatorConstants.sol";


removed a duplicated import

mmv08 · 2024-03-06T12:48:40Z

examples/4337-gas-metering/tsconfig.json

  },
-  "exclude": ["node_modules"],
+  "exclude": ["node_modules"]


the formatter wanted me to do it

modules/4337/package.json

nlordell · 2024-03-06T13:49:05Z

modules/passkey/contracts/interfaces/ICustomSignerFactory.sol

+ * @dev Interface for creating and verifying ECDSA signers. This is a generalized interface that should be
+ *  compatible with curves of any order size. Currently not used in the project and exists here for reference.
+ */
+interface ICustomECSignerFactory {


nit, this isn't an EC signer - in particular, it would support an interface to passkey with RSA key.

nlordell · 2024-03-06T13:51:30Z

modules/passkey/.env.sample

@@ -0,0 +1,2 @@
+# Entrypoint address to use for the Safe 4337 module deployment in 4337 tests. Default is the canonical EntryPoint v0.7 address
+TESTS_4337_MODULE_DEPLOYMENT_ENTRY_POINT_ADDRESS="0x0000000071727De22E5E9d8BAf0edAc6f37da032"


nit: missing trailing newline.

nlordell · 2024-03-06T13:52:31Z

modules/passkey/contracts/4337/Safe256BitECSignerLaunchpad.sol

+    }
+}
+
+interface SafeSetup {


nit: Can we move this to an ISafe file?

nlordell · 2024-03-06T13:55:48Z

modules/passkey/contracts/interfaces/ICustomSignerFactory.sol

+ * @title ICustom256BitECSignerFactory
+ * @dev Interface for creating and verifying ECDSA signers using 256-bit elliptic curves.
+ */
+interface ICustom256BitECSignerFactory {


Seeing as ICustomECSignerFactory isn't used at all, I would actually suggest removing it. It feels a bit like we are trying to future proof, even if there is no demand for using passkeys with algorithms other than ECDSA over the P-256 curve.

Yeah I mentioned in the @dev natspec that it's just kept for the future reference. I can remove it.

nlordell · 2024-03-06T14:08:08Z

modules/passkey/docker/bundler/Dockerfile

@@ -0,0 +1,12 @@
+FROM docker.io/library/node:18


You can ln-s this file, so it is always the same as the 4337 file. It is a lightweight way to share the docker file between both projects 🤷

I was researching whether it works on Windows and can't find a concrete answer. Example: https://unix.stackexchange.com/questions/63172/does-windows-recognize-linuxs-symbolic-links

Do you know if it works across different OS? I know we don't have anyone on the team on Windows but for the sake of good open-source etiquette wanna check it

modules/passkey/hardhat.config.ts

nlordell · 2024-03-06T14:11:15Z

modules/passkey/src/deploy/4337.ts

+  } else if (!ENTRY_POINT) {
+    throw new Error('TESTS_4337_MODULE_DEPLOYMENT_ENTRY_POINT_ADDRESS must be set')


Since this is for testing, do we ever even need this? Might be simpler to always deploy the EntryPoint.

Oh... we need it for the launchpad contract. In that case, I think it should be DEPLOYMENT_ENTRY_POINT, since when we deploy it for real on Polygon for example, it feels weird to use a TEST_* variable.

good idea, removed

Ah, I didn't read the second comment 🤦 will restore it

modules/passkey/contracts/4337/Safe256BitECSignerLaunchpad.sol

mmv08 · 2024-03-06T15:39:39Z

I'm not sure why e2e tests fail; they work on my machine (including with a fresh dependency installation). I will debug it later today,

nlordell · 2024-03-06T16:07:56Z

Maybe related to fresh vs cached docker compose image?

mmv08 · 2024-03-06T19:04:12Z

Maybe related to fresh vs cached docker compose image?

I fixed the first one (I could reproduce it in the end; it required removing all the build files and reinstalling dependencies from scratch), but now there's a new one with a SERVER_ERROR I can't seem to reproduce: https://github.com/safe-global/safe-modules/actions/runs/8177290950/job/22358636900?pr=306. I'm not sure what it can be 🤷

mmv08 force-pushed the 288-implement-a-passkey-signaturevalidator-factory branch 6 times, most recently from d59f4f5 to cd78a75 Compare March 6, 2024 12:08

mmv08 marked this pull request as ready for review March 6, 2024 12:09

mmv08 requested a review from a team as a code owner March 6, 2024 12:09

mmv08 requested review from nlordell, akshay-ap and remedcu and removed request for a team March 6, 2024 12:09

mmv08 force-pushed the 288-implement-a-passkey-signaturevalidator-factory branch 6 times, most recently from e273052 to a966519 Compare March 6, 2024 12:31

mmv08 commented Mar 6, 2024

View reviewed changes

nlordell mentioned this pull request Mar 6, 2024

Verify Multiple P-256 Verifiers Are Supported #307

Merged