Replace noble-ed25519 with tweetnacl

[bgins]

Summary

This PR fixes/implements the following bugs/features

• Replace noble-ed25519 with tweetnacl
• Update tsconfig target to ES2018

Explain the motivation for making this change. What existing problem does the pull request solve?

The noble-ed25519 library uses big integer literals which only work in es2020 or newer environments. This PR replaces noble-ed25519 with tweetnacl to support older environments. See #276 for more details.

We use the verify method from noble-ed25519. This PR replaces it with sign.detached.verify from tweetnacl.

The function signatures are nearly the same. noble-ed25519 permits more types and tweetnacl requires Uint8Arrays. This is enforced here:

https://github.com/dchest/tweetnacl-js/blob/3389b7c9f00545e516a0fdafb324b7857cf1527d/nacl.js#L949-L954

In any case, our wrapper function enforces Uint8Arrays so we should be fine for types.

noble-ed25519 returns Promise<bool> and tweetnacl returns bool. The tweetnacl result is wrapped in a promise here to keep the original behavior.

Test plan (required)

The two environments that lack es2020 support that we are aware of are Create React App and SvelteKit. See sveltejs/kit#2220 for a discussion regarding SvelteKit.

There are bundler issues with SvelteKit that make this difficult to test. Create React App can be tested as follows:

create-react-app my-app
cd my-app
npm install webnative

Import webnative in src/App.js

import * as wn from 'webnative';

Build and run the app locally (with http-server or something else)

npm run build
http-server build/

This should fail with a Uncaught TypeError: can't convert BigInt to number runtime error.

Now replace webnative with the version from this PR, build again, and run it. The app should run without an error.

⚠️ We should test this more somehow! Getting it to work with SvelteKit would be nice, but there are likely other things we can do to check that the libraries behave in the same way.

Edit: These changes pass in our test suites, and beyond that we can rely on these libraries to work equivalently. See #277 (comment) for a summary of where this PR stands.

Closing issues

Closes #276

After Merge

• Does this change invalidate any docs or tutorials? If so ensure the changes needed are either made or recorded
• Does this change require a release to be made? Is so please create and deploy the release

[matheus23]

There are bundler issues with SvelteKit that make this difficult to test

What exactly are these issues?

[matheus23]

I was just wondering about performance differences. This is from the noble-ed25519 readme:

# noble-ed25519
[...]
sign x 3,247 ops/sec @ 307μs/op
[...]
# tweetnacl-fast@1.0.3
[...]
sign x 519 ops/sec @ 2ms/op

No comparison for verify, though. There's some mentions about

verify 100 messages per second on a laptop

In tweetnacl's readme, but the numbers on noble's readme are from an Apple M1:

verify x 726 ops/sec @ 1ms/op

[bgins]

There are bundler issues with SvelteKit that make this difficult to test

What exactly are these issues?

Thinking about it some more, these problems might be the result of how I am testing this locally. Let me explain a bit about how I am testing this with SvelteKit.

SvelteKit has this notion of adapters that are meant to target different environments like node, netlify, vercel, or a static build. For our purposes, I have been using adapter-static with with server-side rendering disabled by setting the ssr to false in the Svelte config. The svelte.config.js I am using looks like:

import adapter from '@sveltejs/adapter-static';

/** @type {import('@sveltejs/kit').Config} */
const config = {
	kit: {
		adapter: adapter({
			// default options are shown
			pages: 'build',
			assets: 'build',
			fallback: null
		}),
		ssr: false,
		// hydrate the <div id="svelte"> element in src/app.html
		target: '#svelte'
	}
};

export default config;

To get a similar setup

npm init svelte@next my-app
cd my-app
npm install
npm i -D @sveltejs/adapter-static@next

select the "SvelteKit demo app", "no" to the remaining questions, and the replace svelte.config.js with the one above.

Import webnative in src/routes/__layout.svelte

<script>
	import * as wn from 'webnative'
	
	import Header from '$lib/header/Header.svelte';
	import '../app.css';
</script>

When I install my local build of webnative with npm install ../webnative (i.e. the relative path to it in my filesystem) and then build the project, I get this error:

> Using @sveltejs/adapter-static
> Cannot find package 'ipfs-message-port-protocol' imported from /home/brian/code/fission/my-app/.svelte-kit/output/server/app.js
Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'ipfs-message-port-protocol' imported from /home/brian/code/fission/my-app/.svelte-kit/output/server/app.js
    at new NodeError (node:internal/errors:371:5)
    at packageResolve (node:internal/modules/esm/resolve:712:9)
    at moduleResolve (node:internal/modules/esm/resolve:753:18)
    at Loader.defaultResolve [as _resolve] (node:internal/modules/esm/resolve:867:11)
    at Loader.resolve (node:internal/modules/esm/loader:89:40)
    at Loader.getModuleJob (node:internal/modules/esm/loader:242:28)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:40)
    at link (node:internal/modules/esm/module_job:75:36)

I see ipfs-message-port-protocol down in node_modules/webnative/node_modules but not at the top level. Maybe this error is a quirk of testing locally?

[bgins]

I was just wondering about performance differences.

Yeah, noble-ed25519 is way faster at signing!

Do you have a sense for how many verify operations we might be doing? Would this lead to a performance issue for us?

[bgins]

Hmm, looks like this didn't work out in our GH action test

import { sign } from "tweetnacl";
         ^^^^
SyntaxError: Named export 'sign' not found. The requested module 'tweetnacl' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'tweetnacl';
const { sign } = pkg;

[bgins]

Adding a comparison of webntative bundle sizes using noble-ed25519 and tweetnacl:

noble-ed25519
- index.umd.min.js: 569.3 kB
- index.umd.min.js.gz: 167.9 kB
- index.umd.min.js.map: 2.4 MB

tweetnacl
- index.umd.min.js: 589.8 kB
- index.umd.min.js.gz: 176.1 kB
- index.umd.min.js.map: 2.5 MB

The noble-ed25519 version is smaller by ~20.5 kB and ~8.2 kB when gzipped.

[bgins]

Fixed a few things on this PR to get it ready.

Merging this PR does not fulfill the original motivation of supporting Create React App because we have other bundler issues that prevent it. But this change is still worthwhile because:

• tweetnacl has been audited and has seen much more real-world use than noble-ed25519
• It supports a wider range of environments because tweetnacl does not use big integer literals

The tradeoffs are basically larger bundle size as noted in a comment above.

Note that noble-ed25519 was our only dependency with big integer literals, and we can set our tsconfig target to ES2018 with that removed.

Our ed25519.browser.test.ts test suite it passing, and I think that beyond that, we can trust that these libraries are verifying signatures equivalently.

[matheus23]

Looking good! Quite a simple change for what we get out of it. Ship it :) 🚢

[matheus23]

I wonder whats up with these changes. Most likely nothing to worry about, though! (And definitely doesn't prevent merging :)

[bgins]

Yeah, that's odd. I sometimes use npm instead of yarn out of habit, but doesn't seem like that would update yarn.lock. 🤔

[matheus23]

🎉

[walkah]

🚢 I agree with @matheus23 nice wins for a relatively minor change 👍