Passkey support #808

DaniSomoza · 2024-05-08T16:26:34Z

What it solves

Resolves #789 & #793 by adding support for passkeys in the protocol-kit.

How this PR fixes it

`protocol-kit` instantiation using passkeys

You can create an instance of the protocol-kit using a passkey as a Signer. The passkey Signer is a contract compliant with EIP-1271 standards.

the passkey argument is an object with 2 keys:

rawId: The raw identifier of the passkey. See rawId docs
publicKey: The passkey’s public key, generated via the getPublicKey() method, necessary for generating X & Y coordinates. See getPublicKey() method docs

  const passkeyCredential = await navigator.credentials.create({ ... });

  const rawId = passkeyCredential.rawId;
  const publicKey = passkeyCredential.response.getPublicKey();

  const passkey = {
    rawId,
    publicKey
  };

  const safeWithPasskey = await Safe.create({
    provider: RPC_URL,
    signer: passkey,
    safeAddress
  });

Add a passkey owner

Now you can use the createAddOwnerTx method present in the protocol-kit using a passkey:

  const safeTransaction = await safe.createAddOwnerTx({
    passkey,
  });

This function returns the Safe transaction to add a passkey as owner and optionally change the threshold. If the passkey Signer contract has not been deployed, this function creates a batch that includes both the Signer contract deployment and the addOwner transaction.

`isOwner` function

The isOwner function now checks if a specific address or passkey is an owner of the current Safe:

  const safe = await Safe.create({
    provider: RPC_URL,
    safeAddress,
  });

  await safe.isOwner(passkey); // returns true or false

Sign a transaction using passkeys

  const safeWithPasskey = await Safe.create({
    provider: RPC_URL,
    signer: passkey,
    safeAddress,
  });

  const dumpTransaction = {
    to: safeAddress,
    value: "0",
    data: "0x",
  };

  const safeTransaction = await safeWithPasskey.createTransaction({
    transactions: [dumpTransaction],
  });

  const signedTransaction = await safeWithPasskey.signTransaction(
    safeTransaction
  );


// execution need to be handled via EOA

Added the WebAuthn Signer Factory Contract

This SafeWebAuthnSignerFactory contract is used to create a Signer and to get the address of a Signer.

yagopv · 2024-05-14T10:38:54Z

packages/account-abstraction-kit/src/AccountAbstraction.ts

-    const safeProvider = new SafeProvider({ provider: this.#provider, signer: this.#signer })
+    const safeProvider = new SafeProvider({
+      provider: this.#provider,
+      signer: this.#signer as string


Can we avoid the cast ?

done! I created a createSafeProvider util function to create a proper SafeProvider object based on the signer provided by the user.

I added this in the other PR (passkey-4337-support) to be able to use it also in the relay-kit. With this change the protocol-kit, the relay-kit and the account-abstraction-kit is compatible with passkeys.

yagopv · 2024-05-14T10:41:43Z

packages/protocol-kit/src/Safe.ts

@@ -29,6 +29,7 @@ import ModuleManager from './managers/moduleManager'
 import OwnerManager from './managers/ownerManager'
 import {
  AddOwnerTxParams,
+  AddPasskeyOwnerTxParams,


Why two separate types? Could we use an Union type instead?

I added two separate types to create guards:

export function isAddOwnerTxParams( params: AddOwnerTxParams | AddPasskeyOwnerTxParams ): params is AddOwnerTxParams { return (params as AddOwnerTxParams).ownerAddress !== undefined } export function isAddPasskeyOwnerTxParams( params: AddOwnerTxParams | AddPasskeyOwnerTxParams ): params is AddPasskeyOwnerTxParams { return (params as AddPasskeyOwnerTxParams).passkey !== undefined }

packages/protocol-kit/src/Safe.ts

yagopv · 2024-05-14T10:50:02Z

packages/protocol-kit/src/Safe.ts

+    const isPasskeySigner = await this.#safeProvider.isPasskeySigner()
+    if (isPasskeySigner) {
+      const txHash = await this.getTransactionHash(transaction)
+      const signedHash = await this.#safeProvider.signMessage(txHash)


We usually do this.signHash(txHash) for signing tx hashes and will adjust the V and return the EthSafeSignature object

true! I updated this in the 4337 support PR:

const isPasskeySigner = await this.#safeProvider.isPasskeySigner() if (isPasskeySigner) { const txHash = await this.getTransactionHash(transaction) signature = await this.signHash(txHash)

packages/protocol-kit/src/types/passkeys.ts

yagopv · 2024-05-14T11:23:19Z

packages/protocol-kit/src/Safe.ts

+      const chainId = await safeProvider.getChainId()
+      const customContracts = contractNetworks?.[chainId.toString()]
+
+      const safeWebAuthnSignerFactoryContract = await getSafeWebAuthnSignerFactoryContract({


This can be retrieved from the safeProvider right?

yep, but I updated this code to use the createSafeProvider util function to wrap all the logic about to create the safeProvider object to reuse it in all packages.

yagopv · 2024-05-14T11:26:27Z

packages/protocol-kit/src/types/safeConfig.ts

@@ -40,7 +41,7 @@ type SafeConfigWithPredictedSafeProps = {

 export type SafeConfigProps = {
  provider: SafeProviderConfig['provider']
-  signer?: SafeProviderConfig['signer']
+  signer?: string | passkeyArgType


Why this change? SafeProviderConfig signer should be the correct one right?

true! I restored it in the other PR: https://github.com/safe-global/safe-core-sdk/pull/841/files#diff-d7c347ab5fed07174eb38d1a9731c90191e250ca1afabe36c35924940823a8cbR44

yagopv · 2024-05-14T11:28:31Z

packages/protocol-kit/src/utils/passkeys/PasskeySigner.ts

+  constructor(
+    passkeyRawId: ArrayBuffer,
+    coordinates: PasskeyCoordinates,
+    safeWebAuthnSignerFactoryContract: SafeWebAuthnSignerFactoryContractImplementationType,


What about using the safeProvider as a param and retrieve the contract and external provider from there?

I think that the best option is to avoid this, because the user can use the custom contracts and we have all this logic in the Safe.init() function.

So to avoid to include this customContracts logic inside of the SafeProvider, I think that we should keep this:

const safeWebAuthnSignerFactoryContract = await getSafeWebAuthnSignerFactoryContract({ safeProvider, safeVersion: '1.4.1', customContracts }) const passkeySigner = await PasskeySigner.init( signer, safeWebAuthnSignerFactoryContract, safeProvider.getExternalProvider() ) new SafeProvider({ provider, signer: passkeySigner })

...t/src/contracts/SafeWebAuthnSignerFactory/v1.4.1/SafeWebAuthnSignerFactoryContract_v1_4_1.ts

packages/protocol-kit/src/contracts/safeDeploymentContracts.ts

yagopv

I think we need an utility (buildPasskey) to build the object used with navigator.credentials.create

{
      pubKeyCredParams: [
        {
          // ECDSA w/ SHA-256: https://datatracker.ietf.org/doc/html/rfc8152#section-8.1
          alg: -7,
          type: 'public-key',
        },
      ],
      challenge: crypto.getRandomValues(new Uint8Array(32)),
      rp: {
        name: 'Safe Wallet',
      },
      user: {
        displayName: 'Safe Owner',
        id: crypto.getRandomValues(new Uint8Array(32)),
        name: 'safe-owner',
      },
      timeout: 60000,
      attestation: 'none',
    }

leonardotc · 2024-05-31T07:25:26Z

packages/protocol-kit/src/Safe.ts

+   * @returns TRUE if the account is an owner
+   */
+  async isOwner(owner: string | passkeyArgType): Promise<boolean> {
+    const isOwnerAddress = typeof owner === 'string'


The this.#safeProvider has an util to detect if this is an address. Consider to just query it. I dont think it will match eip3770 though.

In this case we need to know if the owner param is a string or a passkeyArgType not an address.

leonardotc · 2024-05-31T07:36:48Z

packages/protocol-kit/src/Safe.ts

+
+    const passkeySigner = await PasskeySigner.init(owner, webAuthnSignerFactoryContract, provider)
+
+    const ownerAddress = await passkeySigner.getAddress()


I may be wrong but isnt all this chunk just the this.#safeProvider.getSignerAddress()?

the this.#safeProvider might have been initialized with an EOA instead of a passkey. We need to ensure that the owner's address is created from the passkey provided by the user in the isOwner function parameter, not from the one in the this.#safeProvider.

leonardotc · 2024-05-31T07:50:56Z

packages/protocol-kit/src/Safe.ts

-      provider,
-      signer
-    })
+    const isPasskeySigner = signer && typeof signer !== 'string'


Disclaimer: The comment has nothing to do with this line but wasnt the protocol kit supposed to have a getPasskeyCreationOptions or something like that for the people to have a schema on how to create a "safe-compliant" passkey or something?

leonardotc · 2024-05-31T08:10:09Z

packages/protocol-kit/src/SafeFactory.ts

@@ -62,7 +63,38 @@ class SafeFactory {
  }: SafeFactoryInitConfig) {
    this.#provider = provider
    this.#signer = signer
-    this.#safeProvider = new SafeProvider({ provider, signer })
+    const isPasskeySigner = signer && typeof signer !== 'string'


Consider on extracting this to a local util since its reused.

done! I addressed this comment in this other PR. I created a util in called createSafeProvider to reuse all the logic behind this:

After your suggestion in this comment, I create an static async init method in the SafeProvider instead of the createSafeProvider

leonardotc · 2024-05-31T08:14:27Z

packages/protocol-kit/src/types/safeProvider.ts


 export type SafeProviderConfig = {
  /** signerOrProvider - Ethers signer or provider */
  provider: Eip1193Provider | HttpTransport | SocketTransport
-  signer?: HexAddress | PrivateKey
+  signer?: HexAddress | PrivateKey | passkeyArgType


Isnt this the SafeSigner above?

nope, in the SafeSigner the PasskeySigner is a different type

leonardotc · 2024-05-31T08:24:40Z

packages/protocol-kit/src/SafeProvider.ts

-      return this.#externalProvider.getSigner(this.signer)
+    if (typeof this.signer === 'string') {
+      // If the signer is not an Ethereum address, it should be a private key
+      if (this.signer && !ethers.isAddress(this.signer)) {


I think if it reached this point, we can assume that this.signer is defined. Please, remove the left-hand side conditionhere and below.

leonardotc · 2024-05-31T08:29:52Z

...t/src/contracts/SafeWebAuthnSignerFactory/v1.4.1/SafeWebAuthnSignerFactoryContract_v1_4_1.ts

+
+    super(chainId, safeProvider, defaultAbi, safeVersion, customContractAddress, customContractAbi)
+
+    this.safeVersion = safeVersion


Any particular reason why its not just this.safeVersion = '1.4.1'?

It is because super(...) call must be the first statement in the constructor. after super(...) call we can set the this.safeVersion value

leonardotc · 2024-05-31T11:33:02Z

packages/protocol-kit/src/utils/passkeys/PasskeySigner.ts

+      }
+    })) as PublicKeyCredential & { response: AuthenticatorAssertionResponse }
+
+    if (!assertion || !assertion?.response?.authenticatorData) {


I dont think you need the first verification since you are using safe navigation/elvis operator.

leonardotc · 2024-05-31T11:38:38Z

packages/protocol-kit/src/utils/passkeys/PasskeySigner.ts

+      publicKey: {
+        challenge: data,
+        allowCredentials: [{ type: 'public-key', id: this.passkeyRawId }],
+        userVerification: 'required'


I imagine this would throw that weird prompt in the user's face and that is not something that the Dapp necessarily opted-in (since its not an option here). It should probably be added to the js-docs.

leonardotc · 2024-05-31T14:06:40Z

packages/protocol-kit/src/utils/passkeys/PasskeySigner.ts

+  }
+
+  signTypedData(): Promise<string> {
+    throw new Error('Passkey Signers cannot sign signTypedData, they can only sign data.')


Consider on changing the isTypedDataSigner as well (although that will probably change the error).

* Fix existing unit tests * Test case for initializing Safe4337Pack with a passkey signer * Tests for signing a SafeOperation with passkey + sending to bundler * Extend webauthnShim to initialize with a static private key * Passkey private key must be specified via env variable * Move logic to create a mocked passkey object to utils folder in protocol-kit * Fix passkey tests in protocol-kit and use `createMockPasskey` function from utils * Remove `.only` from test * Add comment * Add env variable `PASSKEY_PRIVATE_KEY` * Fix `createMockPasskey` function JSDoc * Allow to create mocked passkeys with a static private key * Fix test for latest changes Passkey signer is not being added to the owners of a 4337 Safe, but only the SafeWebAuthnSharedSigner address. * Fix tests * Remove duplicated file for passkey test utils * Remove unnecessary `await` * Fix account-abstraction-kit test --------- Co-authored-by: Daniel <25051234+dasanra@users.noreply.github.com>

coveralls · 2024-06-17T09:21:49Z

Pull Request Test Coverage Report for Build 9545149597

Details

50 of 51 (98.04%) changed or added relevant lines in 4 files are covered.
5 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.6%) to 78.437%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/relay-kit/src/packs/safe-4337/Safe4337Pack.ts	32	33	96.97%

Files with Coverage Reduction	New Missed Lines	%
packages/relay-kit/src/packs/safe-4337/utils.ts	5	80.95%

Totals
Change from base Build 9485617547:	0.6%
Covered Lines:	817
Relevant Lines:	969

💛 - Coveralls

* Added SAFE_WEBAUTHN_SHARED_SIGNER_ADDRESS as parameter in the addDummySignature

coveralls · 2024-06-19T12:40:31Z

Pull Request Test Coverage Report for Build 9582107333

Details

50 of 51 (98.04%) changed or added relevant lines in 4 files are covered.
5 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.6%) to 78.437%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/relay-kit/src/packs/safe-4337/Safe4337Pack.ts	32	33	96.97%

Files with Coverage Reduction	New Missed Lines	%
packages/relay-kit/src/packs/safe-4337/utils.ts	5	80.95%

Totals
Change from base Build 9485617547:	0.6%
Covered Lines:	817
Relevant Lines:	969

💛 - Coveralls

* Add createSwapOwnerTx passkey tests * Add remove owner tests

* add SafeWebAuthnSharedSigner Contract * added passkey shared signer support to signHash * Added unit tests

coveralls · 2024-07-10T12:16:54Z

Pull Request Test Coverage Report for Build 10524084006

Details

55 of 59 (93.22%) changed or added relevant lines in 3 files are covered.
5 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.3%) to 77.534%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/relay-kit/src/packs/safe-4337/Safe4337Pack.ts	38	42	90.48%

Files with Coverage Reduction	New Missed Lines	%
packages/relay-kit/src/packs/safe-4337/utils.ts	5	80.95%

Totals
Change from base Build 10510384545:	0.3%
Covered Lines:	978
Relevant Lines:	1188

💛 - Coveralls

yagopv · 2024-07-10T21:51:43Z

packages/protocol-kit/src/types/safeProvider.ts


 export type SafeProviderConfig = {
  /** signerOrProvider - Ethers signer or provider */
  provider: Eip1193Provider | HttpTransport | SocketTransport
-  signer?: HexAddress | PrivateKey
+  signer?: HexAddress | PrivateKey | PasskeySigner | PasskeyArgType


Confused here with the added PasskeySigner. This is to instantiate the SafeProvider with some signer from outside as an address, a privateKey or a passkey args object. Can this be instantiate with the PasskeySigner class?

In any case this should be added as well to the SafeSigner type above

yagopv · 2024-07-10T21:57:17Z

packages/protocol-kit/src/SafeProvider.ts

-
-  constructor({ provider, signer }: SafeProviderConfig) {
+  provider: SafeProviderConfig['provider']
+  signer?: SafeSigner


I would use the SafeProviderConfig['signer'] in all places and remove the SafeSigner type or add a new SafeEthereumProvider (for example) instead, so we would use:

provider: SafeProviderConfig['provider'], signer: SafeProviderConfig['signer']

or

provider: SafeEthereumProvider, signer: SafeSigner

This is only to make it cleaner as this two props are used in multiple places

* refactor in Safe.ts to remove passkey shared signer logic * Add getPasskeyOwnerAddress as util fn * add createPasskeyDeploymentTransaction as a util function * fix: use safeProvider instance * fix isOwner test for v1.0.0 * add getPasskeyOwnerAddress tests * Add dinamic addresses to the getPasskeyOwnerAddress tests

…safe-deployments type

dasanra · 2024-08-12T14:59:01Z

packages/protocol-kit/src/utils/passkeys/createPasskeyDeploymentTransaction.ts

@@ -0,0 +1,50 @@
+import Safe from '../../Safe'


We are missing many global routes @safe-global/protocol-kit/Safe

* use contract addresses from safe-modules-deployments * upated safe passkey address in fixtures

DaniSomoza changed the base branch from main to development May 8, 2024 16:26

DaniSomoza changed the base branch from development to Abitype-1_3_0-safe-contract May 8, 2024 16:27

DaniSomoza requested review from yagopv, leonardotc, tmjssz and dasanra May 9, 2024 14:18

DaniSomoza self-assigned this May 9, 2024

DaniSomoza marked this pull request as ready for review May 9, 2024 14:19

This was linked to issues May 10, 2024

[Passkeys] Add compatibility to the protocol-kit #789

Closed

[Passkeys] Add passkey owner to an already existing Safe #793

Closed

yagopv reviewed May 14, 2024

View reviewed changes

tmjssz reviewed May 14, 2024

View reviewed changes

...t/src/contracts/SafeWebAuthnSignerFactory/v1.4.1/SafeWebAuthnSignerFactoryContract_v1_4_1.ts Outdated Show resolved Hide resolved

tmjssz reviewed May 14, 2024

View reviewed changes

packages/protocol-kit/src/contracts/safeDeploymentContracts.ts Outdated Show resolved Hide resolved

yagopv reviewed May 14, 2024

View reviewed changes

DaniSomoza added 4 commits May 16, 2024 14:01

Add passkey support

061321e

update createAddOwnerTx to receive passkey param

55e24a6

fix init issue in relay-kit

6702b3d

fix issue in BaseContract

a74b809

dasanra force-pushed the passkey-support branch from 5817de9 to a74b809 Compare May 16, 2024 12:17

dasanra changed the base branch from Abitype-1_3_0-safe-contract to development May 16, 2024 12:17

leonardotc reviewed May 31, 2024

View reviewed changes

PasskeyArgType type as Uppercase

33fd454

DaniSomoza and others added 2 commits June 12, 2024 18:15

feat(relay-kit): add dummy signature as a passkey signature (#857)

d5f32ec

DaniSomoza and others added 2 commits June 18, 2024 13:38

Update passkey type (#859)

102c3b6

* Added SAFE_WEBAUTHN_SHARED_SIGNER_ADDRESS as parameter in the addDummySignature

Set alpha.1 version

9ff82fc

leonardotc and others added 5 commits June 20, 2024 10:40

feat(protocol-kit): Tests for swap and remove passkey owners (#861)

a51e837

* Add createSwapOwnerTx passkey tests * Add remove owner tests

[Passkeys] Detect Shared Signer owner (#875)

3dd6595

* add SafeWebAuthnSharedSigner Contract * added passkey shared signer support to signHash * Added unit tests

Merge branch 'development' into passkey-support

1b91381

Merge branch 'development' into passkey-support

64cd846

Fix safe-kit reconnections

8fbbded

yagopv reviewed Jul 10, 2024

View reviewed changes

DaniSomoza and others added 5 commits August 12, 2024 13:09

Merge branch 'development' into passkey-support

b875321

fix(protocol-kit): build not finishing because of breaking change in …

eb1ce1a

…safe-deployments type

Set alpha.2 version

36ef47f

add comment to use external util

13c8ef5

dasanra reviewed Aug 12, 2024

View reviewed changes

dasanra and others added 5 commits August 14, 2024 16:12

Merge branch 'development' into passkey-support

c31b351

use contract addresses from safe-modules-deployments (#950)

d40e489

* use contract addresses from safe-modules-deployments * upated safe passkey address in fixtures

Merge branch 'development' into passkey-support

2081134

chore: remove bytecode from webauthn ABIs

d3da1e0

update passkeys contract names from v1_4_1 to v0.2.1 (#953)

1c22d9c

dasanra approved these changes Aug 23, 2024

View reviewed changes

DaniSomoza merged commit aaaf44c into development Aug 23, 2024
20 checks passed

DaniSomoza deleted the passkey-support branch August 23, 2024 11:24

github-actions bot locked and limited conversation to collaborators Aug 23, 2024

dasanra restored the passkey-support branch August 23, 2024 11:25


		const passkeySigner = await PasskeySigner.init(owner, webAuthnSignerFactoryContract, provider)

		const ownerAddress = await passkeySigner.getAddress()


		super(chainId, safeProvider, defaultAbi, safeVersion, customContractAddress, customContractAbi)

		this.safeVersion = safeVersion

Passkey support #808

Passkey support #808

Conversation

DaniSomoza commented May 8, 2024 • edited Loading

What it solves

How this PR fixes it

protocol-kit instantiation using passkeys

Add a passkey owner

isOwner function

Sign a transaction using passkeys

Added the WebAuthn Signer Factory Contract

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniSomoza Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yagopv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardotc May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 17, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9545149597

Details

💛 - Coveralls

coveralls commented Jun 19, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9582107333

Details

💛 - Coveralls

coveralls commented Jul 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10524084006

Details

💛 - Coveralls

yagopv Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

yagopv Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniSomoza commented May 8, 2024 •

edited

Loading

`protocol-kit` instantiation using passkeys

`isOwner` function

DaniSomoza Jun 4, 2024 •

edited

Loading

leonardotc May 31, 2024 •

edited

Loading

coveralls commented Jun 17, 2024 •

edited

Loading

coveralls commented Jun 19, 2024 •

edited

Loading

coveralls commented Jul 10, 2024 •

edited

Loading

yagopv Jul 10, 2024 •

edited

Loading

yagopv Jul 10, 2024 •

edited

Loading