-
Notifications
You must be signed in to change notification settings - Fork 197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Passkey support #808
Passkey support #808
Changes from 4 commits
061321e
55e24a6
6702b3d
a74b809
33fd454
d20dc53
dac782f
f1ef57e
a61e70c
a547b98
b2ac8df
eff8ab5
3d524ef
3aa0fdd
06cdedf
f3cf8ab
694488b
94cb8a0
dd0dddc
625b668
801f8f2
7ba2e47
378c03e
70e09af
4977cf6
2e284eb
7d58f60
200c6b2
a4a45de
6cbd3f6
d5f32ec
61a1280
102c3b6
9ff82fc
a51e837
3dd6595
1b91381
64cd846
8fbbded
1ba2989
b875321
eb1ce1a
36ef47f
13c8ef5
c31b351
d40e489
2081134
d3da1e0
1c22d9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ import ModuleManager from './managers/moduleManager' | |
import OwnerManager from './managers/ownerManager' | ||
import { | ||
AddOwnerTxParams, | ||
AddPasskeyOwnerTxParams, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why two separate types? Could we use an Union type instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
} |
||
ConnectSafeConfig, | ||
CreateTransactionProps, | ||
PredictedSafeProps, | ||
|
@@ -38,7 +39,8 @@ import { | |
SigningMethod, | ||
SigningMethodType, | ||
SwapOwnerTxParams, | ||
SafeModulesPaginated | ||
SafeModulesPaginated, | ||
passkeyArgType | ||
} from './types' | ||
import { | ||
EthSafeSignature, | ||
|
@@ -54,24 +56,28 @@ import { | |
generatePreValidatedSignature, | ||
generateSignature, | ||
preimageSafeMessageHash, | ||
preimageSafeTransactionHash | ||
preimageSafeTransactionHash, | ||
adjustVInSignature | ||
} from './utils' | ||
import EthSafeTransaction from './utils/transactions/SafeTransaction' | ||
import { SafeTransactionOptionalProps } from './utils/transactions/types' | ||
import { | ||
encodeMultiSendData, | ||
isAddPasskeyOwnerTxParams, | ||
standardizeMetaTransactionData, | ||
standardizeSafeTransactionData | ||
} from './utils/transactions/utils' | ||
import { isSafeConfigWithPredictedSafe } from './utils/types' | ||
import { | ||
getCompatibilityFallbackHandlerContract, | ||
getMultiSendCallOnlyContract, | ||
getProxyFactoryContract | ||
getProxyFactoryContract, | ||
getSafeWebAuthnSignerFactoryContract | ||
} from './contracts/safeDeploymentContracts' | ||
import SafeMessage from './utils/messages/SafeMessage' | ||
import semverSatisfies from 'semver/functions/satisfies' | ||
import SafeProvider from './SafeProvider' | ||
import PasskeySigner from './utils/passkeys/PasskeySigner' | ||
|
||
const EQ_OR_GT_1_4_1 = '>=1.4.1' | ||
const EQ_OR_GT_1_3_0 = '>=1.3.0' | ||
|
@@ -114,10 +120,38 @@ class Safe { | |
async #initializeProtocolKit(config: SafeConfig) { | ||
const { provider, signer, isL1SafeSingleton, contractNetworks } = config | ||
|
||
this.#safeProvider = new SafeProvider({ | ||
provider, | ||
signer | ||
}) | ||
const isPasskeySigner = signer && typeof signer !== 'string' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disclaimer: The comment has nothing to do with this line but wasnt the protocol kit supposed to have a |
||
|
||
if (isPasskeySigner) { | ||
const safeProvider = new SafeProvider({ | ||
provider | ||
}) | ||
const chainId = await safeProvider.getChainId() | ||
const customContracts = contractNetworks?.[chainId.toString()] | ||
|
||
const safeWebAuthnSignerFactoryContract = await getSafeWebAuthnSignerFactoryContract({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be retrieved from the safeProvider right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, but I updated this code to use the |
||
safeProvider, | ||
safeVersion: '1.4.1', | ||
customContracts | ||
}) | ||
|
||
const passkeySigner = await PasskeySigner.init( | ||
signer, | ||
safeWebAuthnSignerFactoryContract, | ||
safeProvider.getExternalProvider() | ||
) | ||
|
||
this.#safeProvider = new SafeProvider({ | ||
provider, | ||
signer: passkeySigner | ||
}) | ||
} else { | ||
this.#safeProvider = new SafeProvider({ | ||
provider, | ||
signer | ||
}) | ||
} | ||
|
||
if (isSafeConfigWithPredictedSafe(config)) { | ||
this.#predictedSafe = config.predictedSafe | ||
this.#contractManager = await ContractManager.init( | ||
|
@@ -421,13 +455,37 @@ class Safe { | |
return this.#moduleManager.isModuleEnabled(moduleAddress) | ||
} | ||
|
||
/** | ||
* Checks if a specific address or passkey is an owner of the current Safe. | ||
* | ||
* @param owner - The owner address or a passkey object | ||
* @returns TRUE if the account is an owner | ||
*/ | ||
async isOwner(owner: string | passkeyArgType): Promise<boolean> { | ||
const isOwnerAddress = typeof owner === 'string' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case we need to know if the |
||
|
||
if (isOwnerAddress) { | ||
return this.#isOwnerAddress(owner) | ||
} | ||
|
||
// passkey flow | ||
const webAuthnSignerFactoryContract = this.#contractManager.safeWebAuthnSignerFactoryContract | ||
const provider = this.#safeProvider.getExternalProvider() | ||
|
||
const passkeySigner = await PasskeySigner.init(owner, webAuthnSignerFactoryContract, provider) | ||
|
||
const ownerAddress = await passkeySigner.getAddress() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be wrong but isnt all this chunk just the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the |
||
|
||
return this.#isOwnerAddress(ownerAddress) | ||
} | ||
|
||
/** | ||
* Checks if a specific address is an owner of the current Safe. | ||
* | ||
* @param ownerAddress - The account address | ||
* @returns TRUE if the account is an owner | ||
*/ | ||
async isOwner(ownerAddress: string): Promise<boolean> { | ||
async #isOwnerAddress(ownerAddress: string): Promise<boolean> { | ||
if (this.#predictedSafe?.safeAccountConfig.owners) { | ||
return Promise.resolve( | ||
this.#predictedSafe?.safeAccountConfig.owners.some((owner: string) => | ||
|
@@ -727,6 +785,27 @@ class Safe { | |
throw new Error('Transactions can only be signed by Safe owners') | ||
} | ||
|
||
// passkey flow | ||
const isPasskeySigner = await this.#safeProvider.isPasskeySigner() | ||
if (isPasskeySigner) { | ||
const txHash = await this.getTransactionHash(transaction) | ||
const signedHash = await this.#safeProvider.signMessage(txHash) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We usually do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
|
||
const signatureAdjusted = adjustVInSignature( | ||
SigningMethod.ETH_SIGN, | ||
signedHash, | ||
txHash, | ||
signerAddress | ||
) | ||
|
||
const signature = new EthSafeSignature(signerAddress, signatureAdjusted, true) | ||
|
||
const signedSafeTransaction = await this.copyTransaction(transaction) | ||
signedSafeTransaction.addSignature(signature) | ||
|
||
return signedSafeTransaction | ||
} | ||
|
||
const safeVersion = await this.getContractVersion() | ||
if ( | ||
signingMethod === SigningMethod.SAFE_SIGNATURE && | ||
|
@@ -810,7 +889,6 @@ class Safe { | |
throw new Error('Transaction hashes can only be approved by Safe owners') | ||
} | ||
|
||
// TODO: fix this | ||
return this.#contractManager.safeContract.approveHash(hash, { | ||
from: signerAddress, | ||
...options | ||
|
@@ -999,9 +1077,17 @@ class Safe { | |
* @throws "Threshold cannot exceed owner count" | ||
*/ | ||
async createAddOwnerTx( | ||
{ ownerAddress, threshold }: AddOwnerTxParams, | ||
params: AddOwnerTxParams | AddPasskeyOwnerTxParams, | ||
options?: SafeTransactionOptionalProps | ||
): Promise<SafeTransaction> { | ||
const isPasskey = isAddPasskeyOwnerTxParams(params) | ||
|
||
if (isPasskey) { | ||
return this.#createAddPasskeyOwnerTx(params, options) | ||
} | ||
|
||
const { ownerAddress, threshold } = params | ||
|
||
const safeTransactionData = { | ||
to: await this.getAddress(), | ||
value: '0', | ||
|
@@ -1014,6 +1100,59 @@ class Safe { | |
return safeTransaction | ||
} | ||
|
||
/** | ||
* Returns the Safe transaction to add a passkey as owner and optionally change the threshold. | ||
* | ||
* @param params - The transaction params | ||
* @param options - The transaction optional properties | ||
* @returns The Safe transaction ready to be signed | ||
* @throws "Invalid owner address provided" | ||
* @throws "Address provided is already an owner" | ||
* @throws "Threshold needs to be greater than 0" | ||
* @throws "Threshold cannot exceed owner count" | ||
*/ | ||
async #createAddPasskeyOwnerTx( | ||
{ passkey, threshold }: AddPasskeyOwnerTxParams, | ||
options?: SafeTransactionOptionalProps | ||
): Promise<SafeTransaction> { | ||
const webAuthnSignerFactoryContract = this.#contractManager.safeWebAuthnSignerFactoryContract | ||
const provider = this.#safeProvider.getExternalProvider() | ||
|
||
const passkeySigner = await PasskeySigner.init(passkey, webAuthnSignerFactoryContract, provider) | ||
|
||
const ownerAddress = await passkeySigner.getAddress() | ||
const isPasskeySignerDeployed = await this.#safeProvider.isContractDeployed(ownerAddress) | ||
|
||
// The passkey Signer is a contract compliant with EIP-1271 standards, we need to check if it has been deployed. | ||
if (isPasskeySignerDeployed) { | ||
return this.createAddOwnerTx({ ownerAddress, threshold }, options) | ||
} | ||
|
||
// If it has not been deployed, we need to create a batch that includes both the Signer contract deployment and the addOwner transaction | ||
|
||
// First transaction of the batch: The Deployment of the Signer | ||
const createSignerTransaction = { | ||
to: await passkeySigner.safeWebAuthnSignerFactoryContract.getAddress(), | ||
value: '0', | ||
data: passkeySigner.encodeCreateSigner() | ||
} | ||
|
||
// Second transaction of the batch: The AddOwner transaction | ||
const addOwnerTransaction = { | ||
to: await this.getAddress(), | ||
value: '0', | ||
data: await this.#ownerManager.encodeAddOwnerWithThresholdData(ownerAddress, threshold) | ||
} | ||
|
||
// transactions for the batch | ||
const transactions = [createSignerTransaction, addOwnerTransaction] | ||
|
||
return await this.createTransaction({ | ||
transactions, | ||
options | ||
}) | ||
} | ||
|
||
/** | ||
* Returns the Safe transaction to remove an owner and optionally change the threshold. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ import { | |
} from '@safe-global/protocol-kit/types' | ||
import { SafeVersion } from '@safe-global/safe-core-sdk-types' | ||
import SafeProvider from '@safe-global/protocol-kit/SafeProvider' | ||
import PasskeySigner from './utils/passkeys/PasskeySigner' | ||
|
||
class SafeFactory { | ||
#contractNetworks?: ContractNetworksConfig | ||
|
@@ -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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider on extracting this to a local util since its reused. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done! I addressed this comment in this other PR. I created a util in called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After your suggestion in this comment, I create an |
||
|
||
if (isPasskeySigner) { | ||
const safeProvider = new SafeProvider({ | ||
provider | ||
}) | ||
const chainId = await safeProvider.getChainId() | ||
const customContracts = contractNetworks?.[chainId.toString()] | ||
|
||
const safeWebAuthnSignerFactoryContract = | ||
await safeProvider.getSafeWebAuthnSignerFactoryContract({ | ||
safeVersion: '1.4.1', | ||
customContractAddress: customContracts?.safeWebAuthnSignerFactoryAddress, | ||
customContractAbi: customContracts?.safeWebAuthnSignerFactoryAbi | ||
}) | ||
|
||
const passkeySigner = await PasskeySigner.init( | ||
signer, | ||
safeWebAuthnSignerFactoryContract, | ||
safeProvider.getExternalProvider() | ||
) | ||
|
||
this.#safeProvider = new SafeProvider({ | ||
provider, | ||
signer: passkeySigner | ||
}) | ||
} else { | ||
this.#safeProvider = new SafeProvider({ | ||
provider, | ||
signer | ||
}) | ||
} | ||
this.#safeVersion = safeVersion | ||
this.#isL1SafeSingleton = isL1SafeSingleton | ||
this.#contractNetworks = contractNetworks | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid the cast ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! I created a
createSafeProvider
util function to create a properSafeProvider
object based on thesigner
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.