node-scrypt icon indicating copy to clipboard operation
node-scrypt copied to clipboard

Add bindings dependency

Open fanatid opened this issue 8 years ago • 12 comments

Probably will solve many issues, like https://github.com/barrysteyn/node-scrypt/issues/135 https://github.com/ethereumjs/testrpc/issues/204#issuecomment-262956174

fanatid avatar Nov 26 '16 08:11 fanatid

Anyway to test this / prove it fixes something?

BrandonZacharie avatar Nov 26 '16 18:11 BrandonZacharie

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?

dbryan0516 avatar Jun 21 '18 13:06 dbryan0516

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.

ml1nk avatar Jun 21 '18 14:06 ml1nk

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

dbryan0516 avatar Jun 21 '18 14:06 dbryan0516

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

ml1nk avatar Jun 21 '18 14:06 ml1nk

@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

chrisveness avatar Jul 07 '18 10:07 chrisveness

@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).

demurgos avatar Jul 09 '18 11:07 demurgos

@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.

chrisveness avatar Jul 09 '18 13:07 chrisveness

@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.

demurgos avatar Jul 09 '18 14:07 demurgos

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.

dwalintukan avatar Jul 12 '18 12:07 dwalintukan

@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 avatar Jul 13 '18 10:07 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!

chrisveness avatar Jul 13 '18 10:07 chrisveness