Kan/passkeys compatibility#868
Conversation
Kay-Zee
requested review from
chasefleming,
janezpodhostnik,
jribbink and
nvdtf
as code owners
tarakby
left a comment
tarakby
left a comment
There was a problem hiding this comment.
- If my understanding is correct, the current transaction IDs in tests should remain the same after the change. Test should pass without updating the transaction IDs.
- It would be good to add a new test where at least one transaction signature is of the webauthn scheme
Co-authored-by: Bastian Müller <bastian@turbolent.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds initial support for passkeys compatibility by introducing ExtensionData field support in transaction signatures. It maintains backward compatibility by creating legacy canonical forms for existing signatures while preparing for future passkey authentication.
- Adds ExtensionData field to TransactionSignature and related structures
- Implements legacy transaction decoding fallback for backward compatibility
- Updates dependency versions in examples/go.mod
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| transaction_legacy.go | New file implementing legacy transaction signature canonical forms and decoding |
| transaction.go | Adds ExtensionData field support and legacy compatibility logic |
| examples/go.mod | Updates various dependency versions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Checks if the scheme is plain authentication scheme, and indicate that it | ||
| // is required to use the legacy canonical form. | ||
| // We check for a valid scheme identifier, as this should be the only case | ||
| // where the extension data can be left out of the cannonical form. |
There was a problem hiding this comment.
Typo in comment: 'cannonical' should be 'canonical'.
| // Until we deprecate the old TransactionSignature format, we need to have two canonical forms. | ||
| // int is not RLP-serializable, therefore s.SignerIndex and s.KeyIndex are converted to uint | ||
| if s.shouldUseLegacyCanonicalForm() { | ||
| // This is the legacy cononical form, mainly here for backward compatibility |
There was a problem hiding this comment.
Typo in comment: 'cononical' should be 'canonical'.
| if err != nil { | ||
| return nil, err | ||
| // If the transaction is in the legacy format, convert it to the canonical form | ||
| if strings.Contains(err.Error(), "too few elements") { // since the rlp library does not have this error type, just check string |
There was a problem hiding this comment.
Using string matching for error handling is fragile. Consider defining a custom error type or using error wrapping/unwrapping patterns for more robust error handling.
| if err != nil { | ||
| return nil, err | ||
| // If the transaction is in the legacy format, convert it to the canonical form | ||
| if strings.Contains(err.Error(), "too few elements") { // since the rlp library does not have this error type, just check string |
There was a problem hiding this comment.
Is there a better check than this?
There was a problem hiding this comment.
not really, this is working off the go ethereum error: https://github.com/ethereum/go-ethereum/blob/master/rlp/decode.go#L108-L112
which is
type decodeError struct {
msg string
typ reflect.Type
ctx []string
}
so only really gives you msg to work with. There is no additional ctx for the error we're working with.
|
going to merge, to unblock somethings in flow-go, but if there are other comments, i will come back and address |
|
Took a peek and cant see anything from our side. |
|
all good from my side too, super excited for passkeys |
Closes: #???
Description
Adds initial understanding of the ExtensionData field for transaction signatures
Does not yet allow adding of ExtensionData to the signature, i.e. no passkeys support yet in this PR.
For contributor use:
masterbranchFiles changedin the Github PR explorer