Browser build and package split #34

jbr · 2022-05-28T00:50:35Z

The biggest change in this PR is switching away from using the galois library and instead just vendoring the parts of galois that we need. It turned out that we did not actually use the assemblyscript because none of our moduli were in the range that their optimizations applied to, so trying to figure out how to upgrade and maintain that part of the library was entirely unnecessary. Instead, we vendor just the TS implementation, which is what we were executing anyway.

This PR currently uses an unpublished version of the HPKE library; PRing and publishing that will allow us to depend on it and remove the vendored tgz.

branlwyd

Nice work! Just a few "administrative" comments.

branlwyd · 2022-07-08T22:27:04Z

package.json

-    "typedoc": "^0.22.17",
-    "typescript": "^4.7.4",
-    "typescript-eslint-language-service": "^5.0.0"
+    "hpke": "file:hpke-0.2.0.tgz"


As you suggested in offline discussion, let's cut a release of hpke-dispatch before merging to avoid the need to vendor the HPKE library.

branlwyd · 2022-07-08T22:56:57Z

packages/dap/src/nonce.ts

@@ -0,0 +1,25 @@
+import { Buffer } from "buffer";


nit: I think this file ended up having its contents copied instead of being git mv'ed somehow -- at least, GitHub doesn't seem to understand it was moved from its previous location.

branlwyd · 2022-07-08T23:03:30Z

packages/field/src/PrimeField.ts

@@ -0,0 +1,1444 @@
+/// source: https://github.com/GuildOfWeavers/galois


We'll probably want to vendor new versions of this library eventually; IMO it'd be very helpful to include some instructions on how to update this library. (i.e. grab the repo, copy these parts over, make these manual changes, ...)

An automated script to "regenerate" the vendored library would be ideal, but depending on how complex the vendoring process is, may be more work than is warranted. And it'd be easy for updates to the galois library to break an automated script.

I think we should consider this a hard fork, and plan to independently maintain all of the code in perpetuity. Part of the motivation behind this change was realizing we were going to end up doing that either way, since upstream seems mostly unmaintained. Part of that should probably be removing any code we don't use, and making sure we have better test coverage than in this PR

Thinking about this some more, I think you're right that this is the best option. Needing to maintain a finite-field library is not ideal, but given that upstream is unmaintained, we might not have much choice.

branlwyd · 2022-07-11T23:33:29Z

Also, it looks like there's a non-flaky test failure. IIUC, it's because the new field library doesn't have sufficient coverage. Since we are vendoring this library, perhaps we can either reduce the required coverage or exclude the library from coverage requirements entirely?

coveralls · 2022-07-12T22:40:11Z

Pull Request Test Coverage Report for Build 3138461862

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

414 of 438 (94.52%) changed or added relevant lines in 28 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.7%) to 91.609%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/vdaf/src/index.ts	8	9	88.89%
packages/common/src/index.ts	16	21	76.19%
packages/field/src/PrimeField.ts	227	245	92.65%

Totals
Change from base Build 3047637308:	-1.7%
Covered Lines:	978
Relevant Lines:	1038

💛 - Coveralls

jbr · 2022-07-18T18:23:58Z

This is pending divviup/hpke-dispatch#12

branlwyd

LGTM modulo waiting for an hpke-dispatch release.

divergentdave

We've got three different standalone files in the scripts directory -- are those okay where they are, or would you recommend giving them a separate package? (They also need some export/import work to fix them, but that can wait)

There are still three files underneath src. I think those should go away, with the possible exception of src/index.ts if we need to re-export items from the root package.

Nice work, I'm excited to get this cooking!

packages/vdaf/src/index.ts

divergentdave · 2022-09-27T18:02:56Z

package.json

-    "typedoc": "^0.23.10",
-    "typescript": "^4.7.4",
-    "typescript-eslint-language-service": "^5.0.0"
+    "@parcel/packager-ts": "^2.7.0",


We should add parcel as a top-level devDependency as well. I tried running npm i && npm run build, and it failed on the parcel build command prior to installing it.

divergentdave · 2022-09-27T18:17:52Z

packages/common/package.json

+    "format": "prettier -w src",
+    "test:coverage": "nyc --cwd=../.. --no-clean --reporter=lcov --reporter=text --reporter=json npm test",
+    "ci": "npm run lint && npm run test:coverage",
+    "benchmark": "ts-node scripts/benchmarks.ts"


nit: this command is defined on several packages, but we only have the one benchmark script hanging off of the top-level package.

divergentdave · 2022-09-27T18:36:16Z

packages/common/tsconfig.json

+    "rootDir": "src",
+    "outDir": "dist"
+  },
+  "include": ["src"]


nit: some directories have "include": ["src"], while others have "include": ["./src/*.ts"]. Should these all use the *.ts glob version instead?

I think this is because some have subdirectories within src, so "src" is the more general variant

jbr marked this pull request as ready for review July 7, 2022 16:49

jbr force-pushed the browser-build-and-package-split branch from 7732d2a to 84e1a10 Compare July 7, 2022 17:14

branlwyd self-requested a review July 8, 2022 21:39

branlwyd suggested changes Jul 8, 2022

View reviewed changes

jbr force-pushed the browser-build-and-package-split branch 2 times, most recently from b6d3324 to cc52e12 Compare July 12, 2022 20:23

jbr requested a review from branlwyd July 18, 2022 18:23

branlwyd approved these changes Jul 19, 2022

View reviewed changes

This was referenced Aug 31, 2022

Importing divviup-ts from Node #89

Closed

Add container and script for interoperation tests #95

Merged

jbr force-pushed the browser-build-and-package-split branch from 7fcdc31 to e7a2e19 Compare September 22, 2022 22:10

This was referenced Sep 22, 2022

Rename PrepareMessage to PrepareState #103

Merged

Upgrade deps #104

Closed

use wasm-bindgen's fs.readFileSync loader in node divviup/hpke-dispatch#17

Merged

Sum also accepts bigints #111

Merged

divergentdave reviewed Sep 27, 2022

View reviewed changes

split package into smaller packages, browser build

4125471

jbr force-pushed the browser-build-and-package-split branch from 0fca2d3 to 4125471 Compare September 30, 2022 19:13

jbr merged commit 6d332aa into main Sep 30, 2022

jbr deleted the browser-build-and-package-split branch September 30, 2022 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Browser build and package split #34

Browser build and package split #34

jbr commented May 28, 2022 •

edited

Loading

branlwyd left a comment

branlwyd Jul 8, 2022

branlwyd Jul 8, 2022

branlwyd Jul 8, 2022

jbr Jul 8, 2022

branlwyd Jul 11, 2022

branlwyd commented Jul 11, 2022

coveralls commented Jul 12, 2022 •

edited

Loading

jbr commented Jul 18, 2022

branlwyd left a comment

divergentdave left a comment

divergentdave Sep 27, 2022

divergentdave Sep 27, 2022

divergentdave Sep 27, 2022

jbr Sep 27, 2022 •

edited

Loading

		@@ -0,0 +1,1444 @@
		/// source: https://github.com/GuildOfWeavers/galois

Browser build and package split #34

Browser build and package split #34

Conversation

jbr commented May 28, 2022 • edited Loading

branlwyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branlwyd commented Jul 11, 2022

coveralls commented Jul 12, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3138461862

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

jbr commented Jul 18, 2022

branlwyd left a comment

Choose a reason for hiding this comment

divergentdave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbr Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

jbr commented May 28, 2022 •

edited

Loading

coveralls commented Jul 12, 2022 •

edited

Loading

jbr Sep 27, 2022 •

edited

Loading