Passkey support #611
Passkey support #611
Conversation
ScreamingHawk
force-pushed
the
passkeys
branch
from
bb55001 to
a164344
Compare
ScreamingHawk
force-pushed
the
passkeys
branch
from
d967d81 to
28df7dd
Compare
| chainID: numberString(args.chainId), | ||
| signatures: filteredSignatures | ||
| }) | ||
| if (filteredSignatures.length === 0 || typeof args.signatures[0] === 'string') { |
There was a problem hiding this comment.
Why not use saveSignerSignatures2 every time? This check seems a bit buggy, as it assumes that the whole list of signatures will use the same type.
There was a problem hiding this comment.
I wanted to keep saveWitnesses backwards compatible.
I'll update this to check that the entire list matches a format. To make this error happen they would have to ignore the type string[] | SignerSignature[] though. We generally accept that the types are as specified.
There was a problem hiding this comment.
what does it mean backwards compatible? like being able to roll this sequence.js update before sessions is updated?
There was a problem hiding this comment.
Clients that use trackers in sequence.js would need to upgrade their implementation if they upgrade to this version. I wanted to avoid breaking changes where possible.
There was a problem hiding this comment.
But couldn't you recover the address if string[] and then use saveSignerSignatures2?
There was a problem hiding this comment.
I don't think so? Correct me if I'm wrong but we'd need the wallet config version, which this function doesn't have access to. We could assume, but that seems even more error prone than using the current method as is.
packages/sessions/src/tracker.ts
Outdated
| @@ -6,6 +6,7 @@ export type PresignedConfig = { | |||
| nextConfig: commons.config.Config | |||
| signature: string | |||
| referenceChainId?: string | |||
| validateBehavior?: 'ignore' | 'throw' | |||
There was a problem hiding this comment.
Shouldn't this be an argument of savePresignedConfiguration instead? Does PresignedConfigLink makes any use of it?
There was a problem hiding this comment.
Seems related to referenceChainId. Both are for validation purposes.
There was a problem hiding this comment.
Not really. referenceChainId specifies the chain to use for signature validation (without it, validation wouldn’t be possible), as it gives meaning to the signature. validateBehavior is not directly tied to the signature itself but acts as a rule for the process involving the signature. This “rule” shouldn’t inadvertently cross into other contexts.
| /** | ||
| * Build a validation signature for an undeployed contract signer. | ||
| */ | ||
| buildValidationSignature?(signatureBytes: ethers.BytesLike): Promise<ethers.BytesLike | undefined>; |
There was a problem hiding this comment.
Is this a 6492 signature or something else? if so, can we add 6492 to the comment?
There was a problem hiding this comment.
It is for now but I guess it doesn't have to be. If we extend support to other validation encodings (zk or something?) we could reuse this method.
Happy to rename if you think we won't reuse later.
There was a problem hiding this comment.
Let’s rename it for now. I’d prefer it to be obvious at a glance.
ScreamingHawk
force-pushed
the
passkeys
branch
2 times, most recently
from
c5ee3fe to
260bacb
Compare
ScreamingHawk
force-pushed
the
passkeys
branch
from
a22fd32 to
ad361cc
Compare
| decoded, | ||
| payload, | ||
| this.provider, | ||
| 'ignore' |
There was a problem hiding this comment.
Why are we passing "ignore" here if we do have a provider? aren't we risking contaminating the local database of signatures?
I guess the alternative would be to miss data, which may be worse.
|
Lets test this on |
Major changes:
/rpc/API/GetAuthToken2with reference chain id for auth/rpc/API/SaveSignerSignatures2with reference chain id for recording witnessesNote: All changes are backwards compatible.