Skip to content

Conversation

@dchest
Copy link
Owner

@dchest dchest commented May 10, 2015

Now there are three general ways to call scrypt:

scrypt(password, salt, logN, r, dkLen, interruptStep, callback, [encoding])

Derives a key from password and salt and calls callback with derived key
as the only argument. The calculations are interrupted with zero
setTimeout at the given interruptSteps to avoid freezing the browser.
Encoding is optional.

scrypt(password, salt, logN, r, dkLen, callback, [encoding])

Same as first, but uses default interruptStep (1000). Encoding is
optional.

scrypt(password, salt, logN, r, dkLen, [encoding]) -> returns result

Synchronous: doesn't interrupt calculations and returns the result
instead of passing it to callback. Encoding is optional. Perfect for use
in web workers.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.92%) to 98.81% when pulling 4499f69 on sync into 5c066a5 on master.

@dchest
Copy link
Owner Author

dchest commented May 10, 2015

@evilaliv3 what do you think?

@evilaliv3
Copy link
Contributor

let me say that i've a lot of criticism (constructive). i do not like the approach you had in creating the function in order to permit it to be called with some arguments or others. no javascript function is generally builded this way so that they accept some parameters and the mix them like you do internally with callback = interruptstep. this make code less clear (think that you code could be reused or grow up with more functionality.

this is what i suggest to do in obtain the same goal:
i would refactor the code packaging all inside an anonymous function and exporting using the notation this.name1 = , this.name2 =.
i would then export two function: scrypt, script_sync defined using the schema shown below:

    (function() {
        'use strict';
        function SHA256(m) {
        }
        .....
        this.scrypt = function () {
            // Note: step argument for interruptedFor must be divisible by
              // two, since smixStepX work in increments of 2.
              if (!interruptStep) interruptStep = 1000;

              smixStart();
              interruptedFor(0, N, interruptStep*2, smixStep1, function() {
                interruptedFor(0, N, interruptStep*2, smixStep2, function () {
                  smixFinish();
                  var result = PBKDF2_HMAC_SHA256_OneIter(password, B, dkLen);
                  if (encoding == "base64")
                    callback(bytesToBase64(result));
                  else if (encoding == "hex")
                    callback(bytesToHex(result));
                  else
                    callback(result);
                });
              });
        }
        this.scrypt_sync = function() {
            // the new simplified code with respect to this.scrypt_above
        }
    })();

@evilaliv3
Copy link
Contributor

you can see here a packaging identical to the one i'm proposing: https://github.com/EvanHahn/HumanizeDuration.js/blob/master/humanize-duration.js

so that we can package the library with the name scrypt-async-js and have:

  • scrypt-async-js.scrypt()
  • scrypt-async-js.scrypt_sync()

@dchest
Copy link
Owner Author

dchest commented May 10, 2015

Well, handling optional arguments like this is pretty common, however I agree that redefining how the function returns result based on this is uncommon. Another option that I like more than having two functions is to namespace the sync version under scrypt: that is, we'll have scrypt() and scrypt.sync():

scrypt(password, salt, logN, r, dkLen, [interruptStep], callback, [encoding])
scrypt.sync(password, salt, logN, r, dkLen, [encoding]) -> returns result

@evilaliv3
Copy link
Contributor

i do not like so much the word sync to mean a module but also a function :)

@evilaliv3
Copy link
Contributor

why you do not like calling them scrypt and scrypt_sync ?

@dchest
Copy link
Owner Author

dchest commented May 10, 2015

It should be scrypt and scryptSync then :-) Having them in a separate namespace would break backwards compatibility, having two exported functions is excessive IMHO :-) I think it's a common pattern, I certainly saw a few of such (something and something.sync) on npm.

@evilaliv3
Copy link
Contributor

i'm sure some fairies will die if you do that. do you mind?

@dchest
Copy link
Owner Author

dchest commented May 10, 2015

It's JavaScript, all fairies are already dead :-D

@evilaliv3
Copy link
Contributor

damn. look a unicorn!

@evilaliv3
Copy link
Contributor

anyway ok i see your point, this is why i liked more the idea of packaging all inside a module called "scrypt-async-js" with two public functions:

  • scrypt-async-js.scrypt()
  • scrypt-async-js.scryptSync()

i do not see any issue in the retrocompatibility, as it's a one line fix and frankly i do not think a lot of project are using the library right now.

dchest added 4 commits May 13, 2015 14:42
Now there are three general ways to call scrypt:

scrypt(password, salt, logN, r, dkLen, interruptStep, callback, [encoding])

Derives a key from password and salt and calls callback with derived key
as the only argument. The calculations are interrupted with zero
setTimeout at the given interruptSteps to avoid freezing the browser.
Encoding is optional.

scrypt(password, salt, logN, r, dkLen, callback, [encoding])

Same as first, but uses default interruptStep (1000). Encoding is
optional.

scrypt(password, salt, logN, r, dkLen, [encoding]) -> returns result

Synchronous: doesn't interrupt calculations and returns the result
instead of passing it to callback. Encoding is optional. Perfect for use
in web workers.
If interruptStep is <= 0, avoid calling setTimeout, and calculate result
immediately, passing it to callback.
@dchest
Copy link
Owner Author

dchest commented May 13, 2015

All right, let's drop this sync thing for now and just merge setTimeout avoidance: #15.

@dchest dchest closed this May 13, 2015
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.

4 participants