Changed function signature to pass all arguments as hexadecimal strings

[tex0l]

To solve this issue : #21

[ggozad]

I agree it would be nice to unify I/O to scrypt. The choice of utf-8 for password and bytes for salt was based on what was default on most scrypt implementations I knew of at least at the time of writing this.
I have to say though that hex is a weird choice. Why would it be default?
Should we perhaps add an additional function that accepts and returns hex?

Also I would like to see tests for this in the example app, using the same arguments as we have already.

[tex0l]

Hi @ggozad ,

I'd be happy to modify the interfaces to ensure retro-compatibility or to provide other options. By the way, the example app has already been updated and tested with the hex encoding.

I'd suggest doing base64 or hex for all I/O from the JS to/from the native code (I chose hexbecause that was already the chosen output encoding, but I really don't mind), and provide other encoding options in Javascript with the buffer package with the following interface :

const result = await scrypt(passwd, salt[, N=16384, r=8, p=1, dkLen=64, encoding='legacy'])

Where encoding can be one of the following:

• legacy: it keeps the current behaviour, meaning passwd as an utf-8 string, salt as a bitArray and the result as a hex string ;
• hex: it considers everything to be strings with hex encoding ;
• base64: it considers everything to be strings with base64 encoding ;
• buffer: it considers everything to be Buffers in the sense of the buffer package.

With such a behaviour — and using buffer to do the dirty encoding work — we could make this a non-breaking change, and provide other options, without having to re-implement everything.

What do you think? And if you think it's a good solution, what default encoding do you want (my laziness would recommend hex encoding because it's done already, but actually I think base64 is better)? If this solution suits you, I'll start implementing it.

[ggozad]

This is very much the proper way of going about it, I will be happy to merge.
If you have the time, it would be awesome if there's a way to actually provide proper tests (but not sure how to do this and use the native ios/android implementations). If not please modify the example so that known test vectors are used for each of the encodings. The one I had has been tested, but perhaps better if we use some of the standard test vectors of scrypt. Let me know if you need help finding those.

I have a very busy couple of weeks, so please excuse the latency in replying :)

[tex0l]

Hi @ggozad ,

I made the changes I proposed with a unified 'hex' strings native I/O and multiple encodings from the JS interface.

I tried adding tests, but the Jest testing framework and others I have tried all boil down to the same issue : they mock native dependencies...

To workaround, I made something simple and dirty : there is now a "start tests" button in the example app which executes RN-Scrypt against 23+1 test vectors (23 in UTF-8, and one non-UTF-8) that I got from SJCL and the RFC. Note that the biggest test vector cannot be executed with SJCL, it makes the JS executor crash (it is not included in SJCL's test vectors), but it works with RN Scrypt :).

I updated the README.md. Tell me what you think!

[tex0l]

The other solution for testing is to configure Selenium or Appium, I didn't have the courage to do so.

I know you told me you were busy, when do you think you'll be able to take a look at the PR? I have a package.json with a github URL in it rather than a clean version number :)

[ggozad]

I am sorry, I have been struck with a bad case of flu and have been out for a week now :(
I will do my best to see this through this week once I recover a bit...
No need for appium, I might have to do that at some stage.

[ggozad]

This is excellent work, again thank you.

[ggozad]

@tex0l I forgot to ask, could you please make another pr where you put your name/emal in the contributors and add your id to changelog? Version should be 1.2.0 in package json and all the rest.

[tex0l]

@ggozad thank you for merging my PR! I've created another PR with the changes you asked: #25