Replace libsodium-wrappers and tweetnacl with Sodium-JavaScript

[christianbundy]

This folds in the sodium-chloride and chloride-test modules so that their history is merged into chloride (and no longer used as outside dependencies). This gets Chloride using the Sodium-Friends API everywhere, removing libsodium-wrappers and tweetnacl as dependencies. This should be smaller, faster, and easier to use / maintain.

Resolves #18

cc: @arj03 @cinnamon-bun

(Folding in the other Git repositories is kinda weird, but I think it's useful because it preserves git blame and gives us all of the provenance metadata that we wouldn't get if I had just copied and pasted the files from the other repos.)

[christianbundy]

This is probably easiest to review by ignoring the diff and looking at the branch as if it were a new module:

https://github.com/christianbundy/chloride/tree/sodium-friends

[christianbundy]

Weird: I think the tests disagree on whether scalarmult should return null or throw an error instead. When I have some more time I can dig into this, because test/index.js expects null but test/generate.js expects an error. :upside_down_face:

[staltz]

It would be great to start using modules from sodium-friends! (E.g. for continued maintenance and security)

What was your reasoning for folding in sodium-chloride and chloride-test though? It feels separate, like these two things are not related or necessary for each other:

This folds in the sodium-chloride and chloride-test modules so that their history is merged into chloride (and no longer used as outside dependencies).

&

This gets Chloride using the Sodium-Friends API everywhere, removing libsodium-wrappers and tweetnacl as dependencies. This should be smaller, faster, and easier to use / maintain.

If possible, I would prefer to keep sodium-chloride and chloride-test separate and used as outside deps, because we wouldn't be rewriting history (this can confuse people who built up context in their head on how things work and where they are located), and also deviates from the small modules philosophy we've been following from Dominic's work.

I'd gladly approve a PR for using sodium-friends and removing libsodium-wrappers and tweetnacl.

[arj03]

I just skimmed this but it appears that you are replacing sodium-browserify with sodium-native & sodium-js? And as far as I can see none of those have wasm? Is that correct?

[christianbundy]

What was your reasoning for folding in sodium-chloride and chloride-test though

I suppose I'd like to reduce the scope of Chloride. Previously it was like FlumeDB, where the tests and plugins were their own repositories so that they could be independently versioned. It was configured to be this extensive pluggable system of libsodium providers, but it seems like everyone would just prefer the Sodium-Friends libraries with a libsodium.js-ish API.

Keeping core functionality (e.g. tests) in external modules that are independently versioned is a huge pain point for maintenance, and if there's only one module using chloride-test then it seems like they aren't separate modules. This approach works for test-flumelog/etc, but Chloride isn't an extensive family of similar modules that need reusable tests.

I understand keeping things how they are by default, but are there any other reasons these should be separate modules? IMO the "this makes dev and maintenance harder" cancels out the "this is the default". (Not to mention that Dominic isn't maintaining these modules, so we'd need to fork and maintain them anyway.)

[christianbundy]

And as far as I can see none of those have wasm?

Sodium-Native has C bindings, which is the fastest option, and Sodium-JavaScript is hand-written JS and Wasm that implements the same behavior. The Sodium-JavaScript repository doesn't mention Wasm but the dependencies (e.g. https://github.com/chm-diederichs/sha256-universal) often have an option to fall back to pure hand-written Wasm (e.g. https://github.com/chm-diederichs/sha256-wasm).

When I roll out of bed and walk to the office I'll put together some perf tests for this branch. 😂

[christianbundy]

More importantly: Thanks for the quick feedback! This is awesome and it feels good to not be left hanging with a big PR. 💗

[staltz]

IMO the "this makes dev and maintenance harder" cancels out the "this is the default".

@christianbundy I see, thanks, I'm warming up to this idea. But I still think it should be in its own PR, because this PR currently has 2 big changes, both of which are highly debatable.

[christianbundy]

Sure, I can split this into two patches if you'd like. I'd just say:

1. The first patch (remove chloride-test + sodium-browserify) makes less sense when it's on its own, since we'll still have libsodium-wrapper and tweetnacl.
2. If you merge the first patch please don't merge others until the second patch. I've made lots of changes and the merge conflicts are going to be hell.

(I think this is best as one PR, which is why I've proposed it that way, but I want to follow your workflow if that makes it easier to merge.)

[staltz]

How would it look like to have a PR only for replacing libsodium-wrapper and tweetnacl? There wouldn't be PR-dependencies, would there?

[christianbundy]

We'd still need PR dependencies, at least in sodium-chloride (because chloride-test is more pedantic than sodium-test on stuff like null vs undefined returns), but maybe that'd work? This branch was already a large effort, not to mention implementing our needs in sodium-javascript, so I haven't given lots of thought on how to slice and dice this patch into smaller bits.

[christianbundy]

Wait, I just thought of a terrible hack that might leave sodium-chloride out of this branch. It's inelegant, but if that's the way to get this merged then maybe I can fix the atrocious hackiness in a follow-up PR? I'm having a hard time trying to figure out how to take one cohesive piece of work and turn it into a bunch of bite-size pieces that are easy to review. :/

[christianbundy]

It sounds to me like this PR is unmergeable so I'm just going to close it. I've opened #22 with a subset of this patch, but this was a lot of work and I don't have the energy to slice it into a bunch of small PRs and then fix the inevitable merge conflicts.

[arj03]

I ran this in ssb-browser-core and did a full sync of my own feed. With this it takes 70 seconds versus 10 seconds before.

[christianbundy]

Interesting! Based on this perf bench I would've expected the opposite effect. Were you waiting for Wasm to be loaded in the old version? If so, maybe try a setTimeout() for a few seconds before starting the test? Otherwise I'd be very curious to see which APIs you're calling and how long they take. :thinking:

[arj03]

Maybe its because wasm it not really loaded before we try to use it and its stuck with js. Just a guess, the sodium-javascript doesn't really mention this but loading wasm is async. See the hook in sodium-browserify.

[arj03]

I replaced this with a setTimeout 2 secs and ran the test again same result. How do I know if wasm is loaded?

I need to go to bed now. Promised myself not another late night ;-)

[christianbundy]

Quick hack: https://github.com/christianbundy/chloride/tree/sodium-friends-again-ready

const chloride = require('chloride')
chloride.ready.then(() => {
  // do stuff
})

[christianbundy]

How do I know if wasm is loaded?

Easy if you have access to the underlying API, but Chloride specifically only exposes a small subset. :/

[arj03]

The ready thing is the same as setTimeout as I would have expected. If you want to test this yourself you can do a checkout of https://github.com/arj03/ssb-browser-demo & https://github.com/arj03/ssb-browser-core. Then modify the core as needed. In UI you just go to a profile, follow and then public -> sync. The terminal will show you when it is done. From there you can also do SSB.removeDB() to nuke the db for testing :)

[staltz]

I've opened #22 with a subset of this patch, but this was a lot of work and I don't have the energy to slice it into a bunch of small PRs and then fix the inevitable merge conflicts.

I've had this situation before in open source, and yeah it's frustrating, sorry for that. What has worked for me in the past is to either open an issue to inform or discuss an incoming large PR prior to writing the large PR, or develop the habit of writing tiny PRs, which are easier for the reviewer to merge, or in the case of not merging, the disappointment is also tiny, and manageable.