Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to dalek 2.x crates #180

Merged

Conversation

andrewwhitehead
Copy link
Contributor

No description provided.

Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
@andrewwhitehead andrewwhitehead marked this pull request as ready for review September 28, 2023 19:08
Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor comments about unwrapping.

cbc = { version = "0.1", default-features = false, optional = true }
chacha20 = { version = "0.9" } # should match dependency of chacha20poly1305
chacha20 = { version = "0.9" } # should match dependency of chacha20poly1305
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a change you introduced, but chacha20 does not match chacha20poly1305. Maybe we should remove the comment? The latest chacha20 is 0.9.1 and chacha20poly1305 is on 0.10.1.

Copy link
Contributor Author

@andrewwhitehead andrewwhitehead Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, although the comment could maybe be clearer. Because we use both both of these crates (and there isn't a re-export available), it's important to make sure that the version of chacha20 matches the version range depended upon by chacha20poly1305.

@@ -97,25 +93,14 @@ impl Ed25519KeyPair {
/// Verify a signature against the public key
pub fn verify_signature(&self, message: &[u8], signature: &[u8]) -> bool {
if let Ok(sig) = Signature::try_from(signature) {
self.public.verify_strict(message, &sig).is_ok()
let vk = VerifyingKey::from_bytes(&self.public).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this infallable or should we return a Result<bool> for this?

Copy link
Contributor Author

@andrewwhitehead andrewwhitehead Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is infallible if the public key is valid, which is verified when the struct is created. Added a comment.

Ok(Self::from_secret_key(
SecretKey::from_bytes(sk.as_ref()).unwrap(),
))
Ok(Self::from_secret_key((&sk).try_into().unwrap()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still infallible? If so, might be good to leave the comment here.

Copy link
Contributor Author

@andrewwhitehead andrewwhitehead Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, it would only fail if &ArrayKey<U32> can't be turned into &[u8; 32]

@andrewwhitehead andrewwhitehead merged commit 7b5c363 into openwallet-foundation:main Oct 4, 2023
27 checks passed
@andrewwhitehead andrewwhitehead deleted the upd/dalek2 branch October 4, 2023 18:03
jamshale pushed a commit to jamshale/askar that referenced this pull request Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants