Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version 3 #125

Merged
merged 29 commits into from
Sep 12, 2024
Merged

Version 3 #125

merged 29 commits into from
Sep 12, 2024

Conversation

paulmillr
Copy link
Collaborator

@paulmillr paulmillr commented Sep 3, 2024

  • aes: change from native async to noble-ciphers sync
  • new bls module: bls12-381 operations
  • new bn module: bn254 operations
  • new math module: modPow and modInv operations
  • Update dependencies
  • Add support for node.js v14
  • Improve typescript compatibility by emitting separate types for cjs / esm

Backwards-incompatible changes:

  1. utils: crypto var had been removed
  2. aes: async methods became sync

@paulmillr
Copy link
Collaborator Author

cc @legobeat please review

@alcuadrado
Copy link
Member

Is the intention to run the two builds before publishing? I guess so, but just double-checking.

Should we add info about the new modules to the readme?

@paulmillr
Copy link
Collaborator Author

@alcuadrado yes, in fact, we have been doing two builds for a long time, using npm run build:

    "build:tsc": "tsc --project tsconfig.prod.json && tsc --project tsconfig.prod.esm.json",

Should we add info about the new modules to the readme?

Yes, and tests for them.

@paulmillr paulmillr changed the title v2.3.0: Update deps v3 Sep 4, 2024
@paulmillr paulmillr changed the title v3 Version 3 Sep 4, 2024
@legobeat
Copy link
Contributor

legobeat commented Sep 5, 2024

cc @legobeat please review

It's not clear to me if Node.js v14 compatibility is intended for this release or if that's a question in scope for this at all (here is BTW where an engines.node field is useful to set expectations straight)? Feel free to disregard points you think out of scope here.

  • Dependency versions are aligned (no inconsistencies or dupes) 👍
  • Seems to run fine on Node 16~22 👍
  • v3 indicates breaking changes to me - what should be considered here? I am probably missing something.
  • The aes module uses crypto.subtle.importKey, which is available in Node.js >=v16, not v14. (aes tests fail with TypeError: Cannot read property 'importKey' of undefined). It does not look like a regression, though.

Improve typescript compatibility by emitting separate types for cjs / esm

  • While the implementations are expected to differ, is there some reason why we can't have a unified interface (making the ESM/CJS difference invisible from an API perspective) and the same .d.ts files? They *.d.ts and esm/*.d.ts files are currently identical from what I can tell?
  • index.d.ts and esm/index.d.ts are present but empty - remove, or add explicit re-exports of public interfaces?

@legobeat
Copy link
Contributor

legobeat commented Sep 5, 2024

Aside related to the two builds: It's not rare to see projects get caught inadvertently breaking either of CJS or ESM in a subtle but impacting change after transitioning to dual builds. While things look good here now, there is always the risk of future discrepancies (or even something we missed here).

The current mocha tests seem to be instrumented as commonjs from what I can tell. Could be worth looking at instrumenting the same tests twice, under esm and commonjs "modes", to catch any future discrepancies?

@paulmillr
Copy link
Collaborator Author

paulmillr commented Sep 6, 2024

why we can't have a unified interface (making the ESM/CJS difference invisible from an API perspective) and the same .d.ts files

https://arethetypeswrong.github.io

https://arethetypeswrong.github.io/?p=%40metamask%2Feth-sig-util%407.0.3

index.d.ts and esm/index.d.ts are present but empty - remove, or add explicit re-exports of public interfaces?

ts auto-produces them, I don't think it's necessary to have script which rms them. Not sure what we can do here. explicit re-exports is no-go. does it matter? The files are small and don't enlarge the bundle massively.

v3 indicates breaking changes to me - what should be considered here? I am probably missing something.

I will be removing crypto export from utils. I also wanted to replace aes with something sync (from noble ciphers), while moving old version to aes-compat or aes-webcrypto, but it's not certain.

@paulmillr
Copy link
Collaborator Author

paulmillr commented Sep 9, 2024

AES has been changed from native (async) to (audited) noble-ciphers (sync).

I believe this change should be done, because async methods are a big deal. They force all app code to become async. Native aes is also not available in all environments.

Ethers, for example, uses sync AES.

@legobeat
Copy link
Contributor

legobeat commented Sep 9, 2024

LGTM!

Still curious on what the breaking change(s) beyond aes going sync are, if any.

v3 indicates breaking changes to me - what should be considered here? I am probably missing something.

@paulmillr
Copy link
Collaborator Author

Still curious on what the breaking change(s) beyond aes going sync are, if any.

  1. crypto removed from utils
  2. aes sync

that's it. nothing else

@paulmillr
Copy link
Collaborator Author

@legobeat do you know why nodejs v14 is failing on linux? The error is weird

npm ERR! Cannot read property '@noble/ciphers' of undefined

@legobeat legobeat mentioned this pull request Sep 11, 2024
@legobeat
Copy link
Contributor

legobeat commented Sep 11, 2024

@paulmillr Looks to be because of node 14 shipping with incompatible npm v6.

@legobeat legobeat mentioned this pull request Sep 11, 2024
README.md Outdated

See [browser usage](#browser-usage) for information on using the package with major Javascript bundlers. It is
tested with **Webpack, Rollup, Parcel and Browserify**.
We support all major platforms and runtimes.
Copy link
Contributor

@legobeat legobeat Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Would be nice to list what's explicitly supported (node.js, bun, deno, browsers?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, also would be good to somehow test in bun, deno

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together, @paulmillr! And @legobeat for also reviewing it :)

@paulmillr paulmillr merged commit 8d2e9be into main Sep 12, 2024
14 checks passed
@paulmillr paulmillr deleted the v23 branch September 12, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement transparent builds Add BN254 operations Add powMod Add BLS12-381 operations
3 participants