search

adorsys / didcomm-mediator-rs

Simple mediator for DIDComm Messaging v2
Apache License 2.0
2 stars 0 forks source link

Comments on did-utils — 962ff8d #15

Closed IngridPuppet closed 10 months ago

IngridPuppet commented 10 months ago

Below are point-to-point questions and suggestions.

IngridPuppet commented 10 months ago

Is this really necessary? Cargo flags this as unused.

https://github.com/ADORSYS-GIS/didcomm-mediator-rs/blob/962ff8d13bdf54a03b62706c5c3ac50ec7152691/did-utils/Cargo.toml#L28-L30

IngridPuppet commented 10 months ago

This will panic if the incoming slice is not of the right length. Maybe you'd want to use the same logic as with from_secret_key.

https://github.com/ADORSYS-GIS/didcomm-mediator-rs/blob/962ff8d13bdf54a03b62706c5c3ac50ec7152691/did-utils/src/crypto/ed25519.rs#L43-L53

IngridPuppet commented 10 months ago

There is a nicer API for this line: sk.verifying_key().

https://github.com/ADORSYS-GIS/didcomm-mediator-rs/blob/962ff8d13bdf54a03b62706c5c3ac50ec7152691/did-utils/src/crypto/ed25519.rs#L58

IngridPuppet commented 10 months ago

Vec::new() looks like a sentinel value, which does not align with Rust philosophy. Why not return an Option?

https://github.com/ADORSYS-GIS/didcomm-mediator-rs/blob/962ff8d13bdf54a03b62706c5c3ac50ec7152691/did-utils/src/crypto/ed25519.rs#L68-L81

IngridPuppet commented 10 months ago

L83: Why not emit a meaningful error instead of panicking? L86: I noticed there is an Error::SignatureError variant. Does it not belong in here?

https://github.com/ADORSYS-GIS/didcomm-mediator-rs/blob/962ff8d13bdf54a03b62706c5c3ac50ec7152691/did-utils/src/crypto/ed25519.rs#L82-L88

IngridPuppet commented 10 months ago

This should test instead for keypair.verify. Isn't it?

https://github.com/ADORSYS-GIS/didcomm-mediator-rs/blob/962ff8d13bdf54a03b62706c5c3ac50ec7152691/did-utils/src/crypto/ed25519.rs#L165-L168

IngridPuppet commented 10 months ago

ChatGPT was definitely not great here. The key_exchange method should be used at least twice for both parties, in lieu of diffie_hellman, which is the goal IMO.

https://github.com/ADORSYS-GIS/didcomm-mediator-rs/blob/962ff8d13bdf54a03b62706c5c3ac50ec7152691/did-utils/src/crypto/x25519.rs#L117-L160

@francis-pouatcha : Did not get your point here. Can you explain?

IngridPuppet commented 10 months ago

Maybe substitute an assertion for the print?

https://github.com/ADORSYS-GIS/didcomm-mediator-rs/blob/962ff8d13bdf54a03b62706c5c3ac50ec7152691/did-utils/src/didcore.rs#L259-L264

francis-pouatcha commented 10 months ago

All fixed except this: https://github.com/ADORSYS-GIS/didcomm-mediator-rs/issues/15#issuecomment-1701190321

Did not get the point.

francis-pouatcha commented 10 months ago

Please review and move to done.