Skip to content
This repository has been archived by the owner on Jan 20, 2023. It is now read-only.

KeyPair functions should return an object rather than a map #8

Closed
rmtmckenzie opened this issue Sep 7, 2018 · 6 comments
Closed

KeyPair functions should return an object rather than a map #8

rmtmckenzie opened this issue Sep 7, 2018 · 6 comments

Comments

@rmtmckenzie
Copy link
Contributor

rmtmckenzie commented Sep 7, 2018

Any functions returning a map would be better off returning an object with members. As it stands, the developer needs to figure out that the public key and secret key are returned under the keys 'pk' and 'sk', and perform their own check that those parameters are actually returned.

At the least the map should be documented and there should be constants for the various keys.

If this were to be changed it is a breaking change, but one that could be mitigated either completely or partially by making the class being returned either implement Map<String,Uint8List> or at least override operator [](String key).

The same goes for the functions that reutrn tx/rx keys and nonce+encrypted key or key+nonce.

@rmtmckenzie
Copy link
Contributor Author

By the way - while I found this annoying, the library is otherwise excellent.

@kozw
Copy link
Contributor

kozw commented Sep 7, 2018

Fair point on documentation and constants for the various keys, this should be added.

The Sodium class contains a 1:1 mapping of Dart to native libsodium functions. My thinking here is that the API should closely match the libsodium signatures using core Dart classes and types. I wanted to leave out any opinionated, custom type from this low-level Sodium API.

To address your concern, a high-level opinionated API has been created, which does contain KeyPair, SessionKeys, and other return types for using libsodium in a Dart-friendly manner. See CryptoBox.generateKeyPair for example, it does return a typed KeyPair instance.

As far as breaking changes is concerned, this plugin is pre-v1, and I'm still toying with the API. I'm not too concerned about introducing breaking changes.

@rmtmckenzie
Copy link
Contributor Author

Ah, didn't see that, I was just looking at the Sodium class for the most part and made the invalid assumption that the dart API and sodium API would be mostly incompatible (as the libsodium-jni library tends to be - I've used it in the past and have been less than impressed). I'll close this as the wrapper classes seem to resolve any concerns I may have had.

Documentation might be advisable but as an open-source project I 100% understand (I maintain a couple of small libraries myself and realize it's a lot of work - if I get a chance I will try to add a PR). Hopefully this issue will help someone else at least =).

And just out of curiosity have you looked at the LazySodium library as an alternative to libsodium-jni? Their java/android api is a whole lot better than libsodium-jni, although I don't think they compile to quite as many platforms right now and it seems as though you mostly use the native api anyways.

@kozw
Copy link
Contributor

kozw commented Sep 10, 2018

I did a few tests with LazySodium. I like what they're doing, but I got some incorrect results from various crypto functions. Decided to stick with libsodium_jni for now.

@gurpreet-
Copy link

Hi @kozw,

Maintainer of Lazysodium here. I have fixed a few bugs in the library related to password hashing. Perhaps that solves your incorrect results problem?

Please let me know if it works, you can find my email in my bio. Happy to discuss any problems with lazysodium you're having.

Sorry for resurrecting an old thread :)

@kozw
Copy link
Contributor

kozw commented Dec 17, 2018

Cool, I'll give it another try. Will keep you in the loop.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants