Tidy up note package - #271
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #271 +/- ##
==========================================
- Coverage 82.57% 79.06% -3.51%
==========================================
Files 5 10 +5
Lines 241 922 +681
==========================================
+ Hits 199 729 +530
- Misses 30 106 +76
- Partials 12 87 +75 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| // NewVerifier returns a verifier for the given key, if the key's algo is known. | ||
| func NewVerifier(key string) (note.Verifier, error) { |
There was a problem hiding this comment.
nit, put this next to the Verifier struct?
| errVerifierAlg = errors.New("unknown verifier algorithm") | ||
| errInvalidHash = errors.New("invalid key hash") | ||
| errMalformedSig = errors.New("malformed signature") | ||
| errSignerHash = errors.New("invalid verifier hash") |
There was a problem hiding this comment.
Actually this is correct as is - the hash is over the verifier key, even when it's being checked as part of creating a new signer. (Also, this is how upstream names it: https://cs.opensource.google/go/x/mod/+/refs/tags/v0.38.0:sumdb/note/note.go;l=335)
| case algEd25519: | ||
| // We "re-implement" this here so as to be able to return a local Signer instance (as | ||
| // opposed to a note.Signer). | ||
| // This has the benefit that all signers from this packge carry a Verifier() getter. |
| } | ||
|
|
||
| s.sign = func(msg []byte) ([]byte, error) { | ||
| return ed25519.Sign(key, msg), nil |
There was a problem hiding this comment.
This is different than NewSignerForCosignatureV1, this is intentional, right?
There was a problem hiding this comment.
Yup, this is in the case algEd25519:, so equivalent to https://cs.opensource.google/go/x/mod/+/refs/tags/v0.38.0:sumdb/note/note.go;l=320-322
| switch keyBytes[0] { | ||
| case algECDSAWithSHA256: | ||
| return NewECDSAVerifier(key) | ||
| case algEd25519CosignatureV1, algMLDSA44: |
There was a problem hiding this comment.
I understand this code has just been migrated from note/note_verifier.go, so it might be out of scope for this, PR, but do we also need a case algEd25519?
There was a problem hiding this comment.
That's handled in the default: case (by upstream sumdb/note).
The reason we can do that here (and not in NewSigner) is that we're not trying to widen the note.Verifier interface, only the note.Signer one.
|
Ta, PTAL |
5 participants
This PR tidies up the note package a bit:
note.gofileNewSignerandNewVerifierfunctions to natively handleed25519keys via the localSignerinterface which gives access to the associated verifier.