Store Credential id with passkey #270
Conversation
The odds that the parsing are correct is basically zero, but right now I can't even build this locally so hopefully CI has better luck?
Parsing it from the auth data would only work on creation, not on get
can't figure out how to update pnpm lockfile
Also allows checking if the passkey exists so you can avoid adding the same passkey to multiple accounts on the same domain
cpb8010
force-pushed
the
passkey-by-id
branch
from
c125c79 to
43dd3e2
Compare
Annoying to do between two repos with the lockfile
For cross-origin support it would be possible to try the same key on another domain. You also might want to check if you have an account already without knowing your address and just your passkey
cpb8010
changed the title
This should help sync the contracts to the SSO repo when interating on contracts
Without the extra layer it would have been possible to hide someone's key in the map or to make it look like the map did exist when it shouldn't have.
leftover from debugging
| mapping(string originDomain => mapping(bytes credentialId => mapping(address accountAddress => bytes32 publicKey))) | ||
| public lowerKeyHalf; | ||
| mapping(string originDomain => mapping(bytes credentialId => mapping(address accountAddress => bytes32 publicKey))) | ||
| public upperKeyHalf; |
There was a problem hiding this comment.
Let's combine these into a single mapping that maps to e.g. bytes32[2]
There was a problem hiding this comment.
This is supported on mainnet now!?
| (bytes32[2] memory key32, string memory originDomain) = abi.decode(key, (bytes32[2], string)); | ||
| bytes32 initialLowerHalf = lowerKeyHalf[originDomain][msg.sender]; | ||
| bytes32 initialUpperHalf = upperKeyHalf[originDomain][msg.sender]; | ||
| function removeValidationKey(bytes calldata credentialId, string calldata domain) external { |
There was a problem hiding this comment.
Is there a reason for having an internal/external pair of functions instead of having one public one?
There was a problem hiding this comment.
I call the internal one from onUninstall! So there's a memory/calldata difference
| if (uint256(initialLowerHalf) != 0 || uint256(initialUpperHalf) != 0) { | ||
| return false; | ||
| } | ||
| if (keyExistsOnDomain[originDomain][credentialId] != address(0)) { |
There was a problem hiding this comment.
since it is impossible (from what I see) to have a zero key, is this check (and hence mapping) really necessary?
There was a problem hiding this comment.
If possible, I would leave it here, as this one could replace AAFactory.accountMappings mapping for resolving account address from credentialId (and effectively remove it from AAFactory).
There was a problem hiding this comment.
This check is more of a sanity check to prevent keyExistsOnDomain from falling out of sync with the public key storage. I can remove if we think there's no possibility of that happening
Description
Support multiple passkeys per domain by looking up the credential id from the authenticator data. This is more efficient than just making a list and provides more information to passkey and account management.
Additional context
We also want to update the SDK to support this new init format and to use this credential id in this module instead of storing it as the unique id within the factory.