feat: expose the public key on the CredentialRef#2164
Conversation
| self.0.public_key_hash().to_vec() | ||
| } | ||
|
|
||
| /// Get the public key associated with this credential ref. |
There was a problem hiding this comment.
Nit: I would say "this credential", rather than "this credential ref" because the ref is just pointing to the credential and the ref is just a thin proxy for the credential.
| Self { | ||
| client_id: credential.client_id().to_owned(), | ||
| public_key_hash: Sha256Hash::hash_from(credential.signature_key_pair.public()), | ||
| public_key: credential.signature_key_pair.to_public_vec().clone(), |
There was a problem hiding this comment.
Now that I am looking at this, I don't like the fact that we're computing SHA-256 every time we construct a credential ref. Now that we have the public key with the credential ref, it is wasteful to compute and store the hash like that. It would be better to have the clients call a function to get the hash, if and when they need it and not store the hash at all.
There was a problem hiding this comment.
I don't know if and when consumers should need the hash. Storing the hash was just an optimization (see WPB-22950). So we might as well just revert what we did in that item entirely.
| pub struct CredentialRef { | ||
| client_id: ClientId, | ||
| public_key_hash: Sha256Hash, | ||
| public_key: Vec<u8>, |
There was a problem hiding this comment.
Do we really want to keep the public key in the credential ref struct?
Note WPB-22950.
We could instead provide a getter for the public key on the CoreCrypto instance instead.
There was a problem hiding this comment.
This is a good point, especially having in mind the coming PQ algorithms, where we will have pubkeys larger than 1kb (e.g. ML-KEM-768 pubkey is 1184 bytes, and that's likely the minimum we will encounter with PQ ciphersuites).
If the public key and hash are relatively rare operations, I would suggest to make both of them lazy, i.e. do not load/compute them until they are actually requested. I suspect most usage of credential refs will be purely "here's a handle to a credential, add/remove it from the conversation" so I think doing these operations lazily will be fine. If it later turns out that's not sufficiently quick, we can re-evaluate. At this point we should prioritize credential refs being minimal and quick to create.
typfel
force-pushed
the
jacob/feat/expose-public-key
branch
from
434881c to
91bd1b5
Compare
typfel
force-pushed
the
jacob/feat/expose-public-key
branch
from
91bd1b5 to
1700c59
Compare
3 participants
What's new in this PR
Expose the public key on the CredentialRef
PR Submission Checklist for internal contributors
SQPIT-764feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.