feat: Implement Ed25519 to Curve25519#54
Conversation
f-person
force-pushed
the
feat/ed25519-to-curve25519
branch
from
518d8ad to
f719d16
Compare
|
Thank you for the PR. I will try to review it within the next few days! |
|
The tests are mostly copied from other methods in |
There was a problem hiding this comment.
Hi. I finally found the time to take a look at your changes. Looks mostly fine, except that the crypto_sign_ed25519_sk_to_curve25519 converts a secret key to another secret key, which means the API should return a SecureKey instead of an Uint8List. Please take a look at my comments and fix the corresponding code.
Regarding the refactoring: Most of the code in this library looks very much alike, but with minor differences. For now, it is fine the way it is, as complex operations (like pointer management) have been extracted into their own classes and only the operations themselves remain. I personally would not extract the public key allocation, as it only moves the line somewhere else and at least in my opinion does not improve readability.
I am thinking about automating the code generation for the APIs on some abstract level, as that would streamline the implementations and allow me to easier implement the advanced APIs. However, as my time is limited, that is nothing I will investigate deeper in the near future.
Also, could you please rebase the PR? I think that should fix up the CI, so the pipeline can actually validate the PR is functionally correct ;)
| /// Provides crypto_sign_ed25519_sk_to_curve25519. | ||
| /// | ||
| /// See https://libsodium.gitbook.io/doc/advanced/ed25519-curve25519 | ||
| Uint8List skToCurve25519(SecureKey secretKey); |
There was a problem hiding this comment.
As this method converts a secret key to a secret key, it should return SecureKey as well
|
|
||
| final publicKey = | ||
| SodiumPointer<UnsignedChar>.alloc(sodium, count: publicKeyBytes); | ||
| final publicKey = _allocatePublicKey(); |
There was a problem hiding this comment.
I don't think extracting a method for this one-liner is needed, as it does not increase readability here.
| SodiumException.checkSucceededInt(result); | ||
|
|
||
| return Uint8List.fromList(curve25519PublicKey.asListView()); | ||
| } finally { |
There was a problem hiding this comment.
This does not work properly. If _allocatePublicKey throws, then ed25519PublicKey will never be disposed. Instead, you should build this up similar to https://github.com/Skycoder42/libsodium_dart_bindings/blob/main/packages/sodium/lib/src/ffi/api/sign_ffi.dart#L105
|
|
||
| return Uint8List.fromList(publicKey.asListView()); | ||
| } finally { | ||
| publicKey.dispose(); |
There was a problem hiding this comment.
It is fine here, as here there is only a single pointer to be disposed
| Uint8List skToCurve25519(SecureKey secretKey) { | ||
| validateSecretKey(secretKey); | ||
|
|
||
| final publicKey = _allocatePublicKey(); |
There was a problem hiding this comment.
As commented in the api file, this should be a SecureKeyFFI, not a Uint8List
| Uint8List skToCurve25519(SecureKey secretKey) { | ||
| validateSecretKey(secretKey); | ||
|
|
||
| return jsErrorWrap( |
There was a problem hiding this comment.
Again, return a SecretKeyJS here
| ]); | ||
| }); | ||
|
|
||
| test('returns the public key of the secret key', () { |
There was a problem hiding this comment.
... returns the Curve25519 secret key of the ...
| ); | ||
| }); | ||
|
|
||
| test('returns the curve25519 secret key of the ed25519 secret key', () { |
There was a problem hiding this comment.
Named correctly here, but still the wrong type
|
Hi! Somehow I missed your reply and only noticed it when configuring a shortcut(?) in the GitHub mobile app :D Thanks for the review! I'll take a look soon and fix the code! |
|
Hi @f-person. I wanted to ask if you need any help with the PR? |
|
Hi, Felix! |
|
@Skycoder42 are you working on this PR? If not I can try to find some time to help. |
|
@rhbrunetto No, I am currently not actively working on it. Feel free to continue the PR! |
3 participants
See https://libsodium.gitbook.io/doc/advanced/ed25519-curve25519