Add bindings dependency

[fanatid]

Probably will solve many issues, like #135 trufflesuite/ganache-cli-archive#204 (comment)

[BrandonZacharie]

Anyway to test this / prove it fixes something?

[dbryan0516]

I spent HOURS trying to solve this issue:

When running npm start

ERROR in ./node_modules/scrypt/index.js
Module not found: Error: Can't resolve './build/Release/scrypt' in 'C:\Users\dylan\Documents\Workspace\project\node_modules\scrypt'
 @ ./node_modules/scrypt/index.js 3:19-52
 @ ./node_modules/scrypt.js/node.js
 @ ./node_modules/web3-eth-accounts/src/index.js
 @ ./node_modules/web3-eth/src/index.js
 @ ./node_modules/web3/src/index.js
 @ ./src/utils/getWeb3.js
 @ ./src/utils/kaleidoKards.js
 @ ./src/components/buttons/PurchaseBtn.js
 @ ./src/components/footer/Footer.js
 @ ./src/App.js
 @ ./src/index.js
 @ multi (webpack)-dev-server/client?http://localhost:8080 ./src/index.js

Until I finally stumbled upon this PR. I tried the changes and it fixed it. This should be merged. Why isn't it?

[ml1nk]

I don't think barrysteyn is currently working on this project, there are at least two other pulls that should be merged, too. If you want you can try to use my fork.

[dbryan0516]

Thanks @ml1nk But if this repo isn't being maintained anymore than npm install scrypt should be updated to point to an up to date version

[ml1nk]

That's true but only barrysteyn can do it, at least a scoped package with a current version makes switching rather easy.

[chrisveness]

@dbryan0516, @ml1nk if you are using Node v10.5.0 (current), scrypt has finally been released as part of the crypto module: I have written a zero-dependency npm module scrypt-kdf which you might find interesting as it has none of the issues of native abstractions, and which I would be glad for you to try out.

It has a slightly simplified API compared with this module (it is promise-based and interfaces with strings rather than buffers), but it operates on the same stored password hashes.

If you try it, let me know your experiences! @chrisveness

[demurgos]

@chrisveness
Thanks for the notification, it's a great news!
I took a quick look at your lib, it looks pretty good but I haven't had the time to test it yet.
I'd just recommend to not swallow or wrap system errors if you can't deal with them (kdf, verify).

[chrisveness]

@demurgos are you referring to the 'localise error to this function' section (L110)? My intention was to pass on all relevant error information without spurious stack trace data from deep within Node.js, but if I'm losing any useful information, I can remove the try/catch.

[demurgos]

@chrisveness
I was thinking about L110 and L179.
In my opinion, L179 should definitely throw instead of swallowing: something wrong happened and there's no clear way to recover from this.

L110 is more complicated and opinion-based. I see the goal to help localizing the error to your lib (a good thing), but I'm always a bit uneasy with this because you may loose context (for example if there is some extra data beyond what you get with String(e)). Lately I tend to throw custom errors with a .cause property pointing to the original errors and an overridden toString to print the error chain, but it may be a bit too much for this use case.

So L110 may be an overreaction from me, but it'd be better to throw something at L179.

[dwalintukan]

I am having this same issue. I tried changing my node version to 10.5.0 in NVM and set the engine to 10.5.0 as well, but the crypto module is missing the scrypt and scryptSync functions.

[chrisveness]

@demurgos you are quite correct about L179. I've not been able to think of any case where that exception would get thrown, but I was lazy in ignoring it: I've now corrected it to throw in the same way as L110.

The .cause idea sounds interesting, but I think I'll leave it until I can find a specific example of where it might be helpful.

[chrisveness]

@dwalintukan I can't think of any reason crypto.scrypt would be missing from Node.js v10.5.0: it sounds like the sort of territory where I resort to rebooting!