Conversation
franziskuskiefer
left a comment
franziskuskiefer
left a comment
There was a problem hiding this comment.
The general approach looks good but we need a) benchmarks to show that this is actually better than before, and b) more tests (especially for all the encode and decode functions). Also see comments inline.
| for _ in 0..n { | ||
| match d.u8()? { | ||
| 0 => uniq!("IdentityKey::public_key", public_key, PublicKey::decode(d)?), | ||
| 0 => uniq!( |
There was a problem hiding this comment.
This macro really sholdn't exist
There was a problem hiding this comment.
Agreed, but that would require refactoring on a larger scale because it is not specific to this PR.
| match version { | ||
| 1 => Ok(IdentityKeyPair { | ||
| version, | ||
| secret_key: to_field!(secret_key_v1_option, "IdentityKeyPair::secret_key_v2"), |
There was a problem hiding this comment.
Ugh, another macro that needs to go 😬
| @@ -243,7 +284,7 @@ impl PreKeyBundle { | |||
|
|
|||
| pub fn signed(ident: &IdentityKeyPair, key: &PreKey) -> PreKeyBundle { | |||
| let ratchet_key = key.key_pair.public_key.clone(); | |||
There was a problem hiding this comment.
Why does this need cloning?
There was a problem hiding this comment.
DHPublicKey doesn't implement Copy because it depends on GroupElement
There was a problem hiding this comment.
Well, I guess my main question was why this key needs to be copied. That implies that it is changed in some way, which is not really what we want. The clone should probably happen further down when the key is copied to the PreKeyBundle. Not crucial though, also nothing you actually touched 😉
| // DHPublicKey //////////////////////////////////////////////////////////////// | ||
|
|
||
| #[derive(PartialEq, Eq, Debug, Clone)] | ||
| pub struct DHPublicKey { |
There was a problem hiding this comment.
Wouldn't a type be sufficient here?
There was a problem hiding this comment.
I guess I wanted to keep the symmetry with the other keys
|
|
3 participants
No description provided.