remove buffer-dep, replace with bufferFrom #445

dghelm · 2022-04-22T16:17:28Z

PULL REQUEST

Update

Changed dependencies:

dropped sjcl for pbkdf2
dropped buffer for buffer-from (already used in a dep)
updated @skynetlabs/[email protected] to update its buffer-from dep
dropped randomBytes and instead used tweetnacl's method
(and sjcl's hex conversion method was no longer necessary since pbkdf2 used a buffer that is easily cast as the necessary Uint8Array)

Overview

I'm looking into other dependencies and found this...

In a recent PR @peterjan made to ethers-js, the lead dev mentioned buffer adding lots of overhead in browser builds. This made me wonder if we needed it, and I found bufferFrom, which removes the need for the much larger Buffer library.

We already are using bufferFrom in tus-js-client dependency. So all we should be doing is dropping an unused dependency without actually adding an additional one.

Figured I'd submit the PR.

https://www.npmjs.com/package/buffer-from

kwypchlo · 2022-04-22T16:43:44Z

Looks GREAT!

mrcnski · 2022-04-27T02:02:42Z

src/crypto.ts

@@ -1,5 +1,4 @@
 import { misc, codec } from "sjcl";
-import { Buffer } from "buffer";


As weird as it looks, having this line in this file, though unused, was preventing a bug in the browser. This should be tested before we merge it in - I can do that by running the test-skapp tests when I have time.

With latest commits, I think this is fixed.

mrcnski

Don't merge yet.

mrcnski · 2022-04-27T19:18:57Z

Another thing, can you please remove package-lock.json?

mrcnski · 2022-04-27T19:22:40Z

When I try to webpack a skapp now I get this:

ERROR in ./node_modules/skynet-js/node_modules/safe-buffer/index.js 3:13-30
Module not found: Error: Can't resolve 'buffer' in '/Users/marcin/Sync/Repos/github.com/m-cat/identity-test-skapp/node_modules/skynet-js/node_modules/safe-buffer'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
	- add a fallback 'resolve.fallback: { "buffer": require.resolve("buffer/") }'
	- install 'buffer'
If you don't want to include a polyfill, you can use an empty module like this:
	resolve.fallback: { "buffer": false }

So it seems like it would require some more configuration on the part of the skapp author now...

dghelm · 2022-04-27T19:40:23Z

Ha, that's the exact error I was trying to run down, but for crypto. Not sure what safe-buffer is, but we may need to continue assessing.

mrcnski · 2022-04-27T19:44:38Z

Ha, that's the exact error I was trying to run down, but for crypto. Not sure what safe-buffer is, but we may need to continue assessing.

safe-buffer is a dependency of some of our dependencies, so not something that is totally trivial to remove.

dghelm · 2022-04-27T20:20:25Z

Ha, that's the exact error I was trying to run down, but for crypto. Not sure what safe-buffer is, but we may need to continue assessing.

safe-buffer is a dependency of some of our dependencies, so not something that is totally trivial to remove.

Ah okay -- yeah, I couldn't find an easy way to drop crypto without changing genKeyPairFromSeed to an async method, so I think the library is stuck displaying this warning for webpack 5 either way. The tests still pass, but maybe this is more of a consideration for libkernel/next SDK.

kwypchlo · 2022-04-28T10:31:58Z

@dghelm I did some digging and the only dependency that uses safe-buffer is randombytes and we could replace it with https://github.com/consento-org/get-random-values-polypony (npm https://www.npmjs.com/package/get-random-values) - this should allow us to drop the crypto package too (I think)

dghelm · 2022-04-28T15:18:45Z

@dghelm I did some digging and the only dependency that uses safe-buffer is randombytes

Good catch. tweetnacl has a randomBytes method, so the latest commit drops this dependency as well.

dghelm · 2022-04-28T15:25:52Z

Also, buffer-from is not de-deduped from the tus-js-client fork, since it uses 0.1.2 -- we'd maybe want to upgrade the dep in our fork if the breaking changes are minimal.

Also, crypto is still used in sjcl afaik, and alternatives to the key-gen method it uses all seem to be async methods. Digging into that is what started this PR, and I made a post on Monday in #skynet-js channel digging a bit more into it.

Current output of npm ls --prod true:

[email protected] /home/dgh/dev/temp/skynet-js
├─┬ @skynetlabs/[email protected]
│ ├── [email protected]
│ ├─┬ [email protected]
│ │ ├── [email protected]
│ │ └─┬ [email protected]
│ │   ├─┬ [email protected]
│ │   │ └─┬ [email protected]
│ │   │   └── [email protected]
│ │   └─┬ [email protected]
│ │     ├── [email protected]
│ │     └── [email protected]
│ ├── [email protected]
│ ├── [email protected]
│ ├── [email protected]
│ ├─┬ [email protected]
│ │ ├── [email protected]
│ │ └── [email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ └── [email protected]
├─┬ [email protected]
│ └── [email protected]
├── [email protected]
├─┬ [email protected]
│ └── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├─┬ [email protected]
│ └── [email protected] deduped
├── [email protected]
├── [email protected]
└─┬ [email protected]
  ├── [email protected]
  └── [email protected]

kwypchlo · 2022-04-28T18:02:28Z

Also, buffer-from is not de-deduped from the tus-js-client fork, since it uses 0.1.2 -- we'd maybe want to upgrade the dep in our fork if the breaking changes are minimal.

@m-cat updated upstream on our fork and seems like we're using buffer-from 1.1.2 now too so that should use the same version now https://github.com/SkynetLabs/tus-js-client/blob/master/package.json#L79

kwypchlo · 2022-04-28T18:14:07Z

Also, crypto is still used in sjcl afaik, and alternatives to the key-gen method it uses all seem to be async methods. Digging into that is what started this PR, and I made a post on Monday in #skynet-js channel digging a bit more into it.

So afaik scjs is used only here:
https://github.com/SkynetLabs/skynet-js/blob/master/src/crypto.ts#L1

One method is pbkdf2 and we could use https://www.npmjs.com/package/pbkdf2 instead - it has sync alternative:

pbkdf2.pbkdf2Sync('password', 'salt', 1, 32, 'sha512')

Second method is codec.hex which looks like it's quite simple function that does not rely on crypto and maybe it could be copied over or replaced by something different:
https://github.com/bitwiseshiftleft/sjcl/blob/master/core/codecHex.js#L14-L20

dghelm · 2022-04-28T20:00:46Z

One method is pbkdf2 and we could use https://www.npmjs.com/package/pbkdf2 instead - it has sync alternative

I reviewed that library last week, but afaik it still depends on crypto but maybe because it's part of the crypto-browserify it handles it in a way that doesn't throw errors. See:

This library provides the functionality of PBKDF2 with the ability to use any supported hashing algorithm returned from crypto.getHashes()

I'll give it a go and see what happens though.

kwypchlo · 2022-04-28T20:15:17Z

One method is pbkdf2 and we could use https://www.npmjs.com/package/pbkdf2 instead - it has sync alternative

I reviewed that library last week, but afaik it still depends on crypto but maybe because it's part of the crypto-browserify it handles it in a way that doesn't throw errors. See:

This library provides the functionality of PBKDF2 with the ability to use any supported hashing algorithm returned from crypto.getHashes()

I'll give it a go and see what happens though.

as far as I can tell, the browser build (browser.js entrypoint) does not require "crypto" package - the nodejs build (index.js entrypoint) does require it but that's fine, skynet-js should correctly pull the browser version for the browser build and nodejs version for the nodejs build (I guess it should)

dghelm · 2022-04-28T23:24:41Z

Now getting no errors or warnings on Create React App 5.0.1, which uses webpack 5.

src/crypto.ts

mrcnski

Hmm I'm still getting this when I try to build a plain skapp (non-React) with Webpack:

ERROR in ./node_modules/skynet-js/node_modules/safe-buffer/index.js 3:13-30
Module not found: Error: Can't resolve 'buffer' in '/Users/marcin/Sync/Repos/github.com/m-cat/identity-test-skapp/node_modules/skynet-js/node_modules/safe-buffer'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
	- add a fallback 'resolve.fallback: { "buffer": require.resolve("buffer/") }'
	- install 'buffer'
If you don't want to include a polyfill, you can use an empty module like this:
	resolve.fallback: { "buffer": false }

dghelm · 2022-04-29T21:31:45Z

Hmm I'm still getting this when I try to build a plain skapp (non-React) with Webpack:

And just to confirm -- you were also seeing this with webpack 5 and our current production version, right?

If so, I think this is an improvement and we should identify the webpack configuration differences as a follow-up.

mrcnski · 2022-05-02T16:53:49Z

@dghelm Yeah, I'm running

"webpack": "^5.16.0",

I think the situation is actually worse than before because I get errors now, whereas before I was only getting warners. Manually listing buffer as a dependency in skynet-js was preventing those errors.

kwypchlo · 2022-05-25T08:56:23Z

@dghelm @m-cat I added a commit that replaces pbkdf2 package with pbkdf2-hmac package here 2ba6606

Package pbkdf2-hmac uses native browser crypto function when running in browser and native node crypto package when running in node environment. Unfortunately, since browser native implementation is async, it means a breaking change in genKeyPairFromSeed and genKeyPairAndSeed - they need to be async. Let me know your thoughts. I think this is worth it, making skynet-js lighter (buffer package is 26.8kB and pbkdf2-hmac is 2.4kB), faster (native browser implementation is definitely faster than javascript one) and independent of nodejs packages (we will be able to use webpack without polyfills and warnings) is a good path forward. If this is not on the table, we can revert my commit.

Implementation details: https://github.com/juanelas/pbkdf2-hmac/blob/master/src/ts/index.ts#L55-L79

PAlexanderFranklin · 2022-06-07T16:22:05Z

Svelte is having trouble with randombytes when instantiating the skynet client. This is on Svelte version 3.48.0.

Example Repo https://github.com/PAlexanderFranklin/caller-kit

dghelm · 2022-06-07T18:20:14Z

Unfortunately, since browser native implementation is async, it means a breaking change in genKeyPairFromSeed and genKeyPairAndSeed - they need to be async.

I think this is off the table for right now -- if we planned to move forward with skynet-js this seems great (there are also browser crypto methods I think we could take advantage of by moving more towards async), but I think we should document it separately from this PR, which seems increasingly important as more devs are running into issues with React and Svelte.

kwypchlo · 2022-06-08T14:02:10Z

Unfortunately, since browser native implementation is async, it means a breaking change in genKeyPairFromSeed and genKeyPairAndSeed - they need to be async.

I think this is off the table for right now -- if we planned to move forward with skynet-js this seems great (there are also browser crypto methods I think we could take advantage of by moving more towards async), but I think we should document it separately from this PR, which seems increasingly important as more devs are running into issues with React and Svelte.

I think it's still pretty reasonable to bump the major to remove those deps, that's really going to help with package size and compatibility with webpack 5. I bet ether.js maintainers would be happy with that too.

mrcnski · 2022-06-08T15:06:47Z

@kwypchlo I would be happy with that too, but we would need @DavidVorick to sign off on it.

remove buffer-dep, replace with bufferFrom

1359335

dghelm requested review from kwypchlo and mrcnski April 22, 2022 16:17

kwypchlo previously approved these changes Apr 22, 2022

View reviewed changes

mrcnski reviewed Apr 27, 2022

View reviewed changes

mrcnski suggested changes Apr 27, 2022

View reviewed changes

remove package-lock.json

829032e

dghelm dismissed kwypchlo’s stale review via 829032e April 27, 2022 19:42

replace randombytes usage with tweetnacl method

486a6ec

Merge branch 'master' into drop-buffer

c8cd0cc

dghelm added 3 commits April 28, 2022 15:31

replace sjcl with pbkdf2

d30265b

fix length and hash-algo

ab42e70

Merge branch 'master' into drop-buffer

ea4dd45

dghelm requested review from mrcnski and kwypchlo April 28, 2022 23:24

kwypchlo reviewed Apr 29, 2022

View reviewed changes

src/crypto.ts Outdated Show resolved Hide resolved

Merge branch 'master' into drop-buffer

35b9394

kwypchlo previously approved these changes Apr 29, 2022

View reviewed changes

kwypchlo assigned dghelm Apr 29, 2022

mrcnski suggested changes Apr 29, 2022

View reviewed changes

kwypchlo added 2 commits May 5, 2022 10:28

Merge branch 'master' into drop-buffer

06e4821

Merge remote-tracking branch 'origin/master' into drop-buffer

2ffc356

kwypchlo dismissed their stale review via bed3a00 May 25, 2022 08:48

replace pbkdf2 package with async pbkdf2-hmac

2ba6606

kwypchlo force-pushed the drop-buffer branch from bed3a00 to 2ba6606 Compare May 25, 2022 08:57

kwypchlo added 3 commits June 8, 2022 17:19

Merge remote-tracking branch 'origin/master' into drop-buffer

525315b

fix jest-environment-jsdom import

1bc7aa5

fix jest 27 to 28 migration issue

ebf3a12

kwypchlo marked this pull request as draft June 28, 2022 12:17

kwypchlo added 2 commits October 4, 2022 09:56

Merge remote-tracking branch 'origin/master' into drop-buffer

c469a1d

Merge remote-tracking branch 'origin/master' into drop-buffer

9860fc7

kwypchlo marked this pull request as ready for review October 4, 2022 08:43

kwypchlo closed this Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove buffer-dep, replace with bufferFrom #445

remove buffer-dep, replace with bufferFrom #445

dghelm commented Apr 22, 2022 •

edited

Loading

kwypchlo commented Apr 22, 2022

mrcnski Apr 27, 2022

dghelm Apr 28, 2022

mrcnski left a comment

mrcnski commented Apr 27, 2022

mrcnski commented Apr 27, 2022

dghelm commented Apr 27, 2022

mrcnski commented Apr 27, 2022

dghelm commented Apr 27, 2022

kwypchlo commented Apr 28, 2022

dghelm commented Apr 28, 2022

dghelm commented Apr 28, 2022 •

edited

Loading

kwypchlo commented Apr 28, 2022

kwypchlo commented Apr 28, 2022

dghelm commented Apr 28, 2022

kwypchlo commented Apr 28, 2022

dghelm commented Apr 28, 2022

mrcnski left a comment

dghelm commented Apr 29, 2022

mrcnski commented May 2, 2022

kwypchlo commented May 25, 2022 •

edited

Loading

PAlexanderFranklin commented Jun 7, 2022 •

edited

Loading

dghelm commented Jun 7, 2022

kwypchlo commented Jun 8, 2022

mrcnski commented Jun 8, 2022

		@@ -1,5 +1,4 @@
		import { misc, codec } from "sjcl";
		import { Buffer } from "buffer";

remove buffer-dep, replace with bufferFrom #445

remove buffer-dep, replace with bufferFrom #445

Conversation

dghelm commented Apr 22, 2022 • edited Loading

PULL REQUEST

Update

Overview

kwypchlo commented Apr 22, 2022

mrcnski Apr 27, 2022

Choose a reason for hiding this comment

dghelm Apr 28, 2022

Choose a reason for hiding this comment

mrcnski left a comment

Choose a reason for hiding this comment

mrcnski commented Apr 27, 2022

mrcnski commented Apr 27, 2022

dghelm commented Apr 27, 2022

mrcnski commented Apr 27, 2022

dghelm commented Apr 27, 2022

kwypchlo commented Apr 28, 2022

dghelm commented Apr 28, 2022

dghelm commented Apr 28, 2022 • edited Loading

kwypchlo commented Apr 28, 2022

kwypchlo commented Apr 28, 2022

dghelm commented Apr 28, 2022

kwypchlo commented Apr 28, 2022

dghelm commented Apr 28, 2022

mrcnski left a comment

Choose a reason for hiding this comment

dghelm commented Apr 29, 2022

mrcnski commented May 2, 2022

kwypchlo commented May 25, 2022 • edited Loading

PAlexanderFranklin commented Jun 7, 2022 • edited Loading

dghelm commented Jun 7, 2022

kwypchlo commented Jun 8, 2022

mrcnski commented Jun 8, 2022

dghelm commented Apr 22, 2022 •

edited

Loading

dghelm commented Apr 28, 2022 •

edited

Loading

kwypchlo commented May 25, 2022 •

edited

Loading

PAlexanderFranklin commented Jun 7, 2022 •

edited

Loading