noble-ed25519 icon indicating copy to clipboard operation
noble-ed25519 copied to clipboard

Move the crypto selection out of the ed25519 logic.

Open Miaourt opened this issue 2 years ago • 8 comments

Kept the changes minimal and the selection logic the same to avoid too big refactor of the building pipeline.

Fix #40

Tested with the Jest test included, and a full keygen/sign/verify in Deno/ESM.

(Since I'm moving all the ed25519's logic from index.ts to ed25519.ts Github doesn't really show useful diff for the pull request, so have a look at the last commit for revelant changes)

Note for the future : pretty sure we can remove the selection from a runtime one to a more simple performant one in the building pipeline but would require more changes that I'm willing to make right now.

Miaourt avatar Feb 27 '22 20:02 Miaourt

why are you renaming index.ts? keep it as is.

paulmillr avatar Feb 27 '22 20:02 paulmillr

I'm renaming it because it's mandatory for keeping it as the default entrypoint.

Unless, of course, you'd like to make a new "nodejs+browser tsc pipeline" entrypoint loading the logic from an index.ts that will be unable to live on it's own and might introduce breaking changes...

I think this is the most elegant solution to keep index.ts the default entrypoint to prevent any potential breaking change from weird direct imports in some projects.

You need to get rid of the crypto import inside the logic if you want it to be compatible with Deno, like we discussed in #40.

Miaourt avatar Feb 27 '22 20:02 Miaourt

Did you test this with ESM modules? Common.js modules?

paulmillr avatar Feb 27 '22 22:02 paulmillr

Tested ESM modules in nodejs & vite contexts

import * as ed from "../noble-ed25519/lib/esm/index.js"; // path to my local branch

(async () => {
  // keys, messages & other inputs can be Uint8Arrays or hex strings
  // Uint8Array.from([0xde, 0xad, 0xbe, 0xef]) === 'deadbeef'
  const privateKey = ed.utils.randomPrivateKey();
  const message = Uint8Array.from([0xab, 0xbc, 0xcd, 0xde]);
  const publicKey = await ed.getPublicKey(privateKey);
  const signature = await ed.sign(message, privateKey);
  const isValid = await ed.verify(signature, message, publicKey);

  console.log(isValid)
})();

Tested Common.js in nodejs context

let ed = require("../noble-ed25519/lib/index.js"); // path to my local branch

(async () => {
  // keys, messages & other inputs can be Uint8Arrays or hex strings
  // Uint8Array.from([0xde, 0xad, 0xbe, 0xef]) === 'deadbeef'
  const privateKey = ed.utils.randomPrivateKey();
  const message = Uint8Array.from([0xab, 0xbc, 0xcd, 0xde]);
  const publicKey = await ed.getPublicKey(privateKey);
  const signature = await ed.sign(message, privateKey);
  const isValid = await ed.verify(signature, message, publicKey);

  console.log(isValid)
})();

Tested in browser context with the rollup's generated noble-ed25519.js with this

<script type="module">
import "./noble-ed25519.js";

(async () => {
  // keys, messages & other inputs can be Uint8Arrays or hex strings
  // Uint8Array.from([0xde, 0xad, 0xbe, 0xef]) === 'deadbeef'
  const privateKey = window.nobleEd25519.utils.randomPrivateKey();
  const message = Uint8Array.from([0xab, 0xbc, 0xcd, 0xde]);
  const publicKey = await window.nobleEd25519.getPublicKey(privateKey);
  const signature = await window.nobleEd25519.sign(message, privateKey);
  const isValid = await window.nobleEd25519.verify(signature, message, publicKey);

  console.log(isValid)
})();
</script>

They all output true without any errors And as I said in the first post I tested with the Jest test included, and did the same "isValid" verify test with Deno with the new mod.ts

If you have more tests ideas I can test them.

Miaourt avatar Feb 27 '22 23:02 Miaourt

Webpack? Rollup? Browserify? Sorry for messing with your impl, and for lack of tests - just need to be sure it's rock solid.

paulmillr avatar Feb 27 '22 23:02 paulmillr

I will try to test them the same way during the week... but I highly doubt it will change much since thoses bundlers you suggested are quite tested and understand pretty much any valid js...

I understand don't worry, I think it could be great to have CI doing this kind of testing, if you've been doing that for every releases you did, lol.

Miaourt avatar Feb 27 '22 23:02 Miaourt

An alternative solution for this (which can also be added to @noble/secp256k1 where the same applies), would be -

  1. Remove the import * as nodeCrypto from 'crypto'; from index.ts

  2. Adjust the utils in index.ts with -

export const utils = {
  crypto: {
    node: undefined,
    // applies to Deno and Browser
    web: typeof self === 'object' && 'crypto' in self ? self.crypto : undefined,
  } as { node?: any; web?: any },
  ...
  1. Now replace all usages of crypto. with utils.crypto., i.e if (utils.crypto.web) { utils.crypto.web.getRandomValues(...) in index.ts.

  2. Add a specific Node.js entry point, node.ts -

import * as nodeCrypto from 'crypto';

import { utils } from './index';

// inject the node crypto version only in this environment
utils.crypto.node = nodeCrypto;

export * from './index';
  1. Add an export map entry for Node to package.json (all current LTS versions of Node will respect this, exports maps have been available since mid-Node 12, with this the browser entries all all unchanged) -
"exports": {
    ".": {
      "types": "./lib/index.d.ts",
      "node": {
        "import": "./lib/esm/node.js",
        "default": "./lib/node.js"
      },
      "import": "./lib/esm/index.js",
      "default": "./lib/index.js"
    }
  }

The above could be easier to reason about, test and review.

jacogr avatar Jul 03 '22 10:07 jacogr

Export maps are not supported in typescript. https://github.com/microsoft/TypeScript/issues/33079

paulmillr avatar Jul 03 '22 10:07 paulmillr

2.0 will likely support native webcrypto instead of this

paulmillr avatar Feb 04 '23 08:02 paulmillr