Remove circomlibjs dependency in favor of zk-kit #1911
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
artwyman
commented
Oct 1, 2024
| @@ -19,6 +19,23 @@ | |||
| "port": 4321, | |||
| "restart": true, | |||
| "cwd": "${workspaceRoot}" | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Borrowed this from #1790.
It doesn't work for all utests (it depends on the path depth between the test file and package.json) but it works on many of them, and it's only useful if it's in the repo, so I think it's worth merging.
artwyman
changed the title
|
Note that there is still a devDependency on circomlibjs in @pcd/pod's unit tests, for the set of compatibility tests which validates that zk-kit and circomlibjs remain compatible with each other. I think it's good that these remain so we don't accidentally introduce any incompatibilities, and so we have the option to switch back should we ever decide it's worth it (e.g. for higher performance). |
robknight
approved these changes
Oct 1, 2024
|
Looks good to me! The bundle size impact will be great for new users. |
rrrliu
pushed a commit
that referenced
this pull request
Oct 8, 2024
Minimalist approach to switching away from circomlibjs in order to
eliminate its massive poseidon_constants from our bundle size. This
drops the bundle size in my measurements from 9.4MB to 6.4MB.
Functionality remains unchanged (validated by pre-existing backward
compatibility tests), with one known exception. The EdDSA PCD now only
supports "messages" (bigint arrays) of sizes in the set {1, 2, 3, 12,
13}. Other sizes will throw an error at proving time. This is because
poseidon-lite also has a set of constants (albeit much smaller than
circomlibjs) needed for each size. There are comments in the code
explaining why each size was selected.
Two things explicitly not done, given the minimalist approach:
- This PR doesn't attempt to unify/centralize uses of
zk-kit/poseidon-lite into a single package. Instead the old EdDSA PCDs
use the appropriate zk-kit/poseidon-lite libraries directly.
- This PR also doesn't change the format (in code, or in serialization)
of EdDSA keys to match the new form used by PODs. All externally-visible
formats are kept unchanged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Minimalist approach to switching away from circomlibjs in order to eliminate its massive poseidon_constants from our bundle size. This drops the bundle size in my measurements from 9.4MB to 6.4MB.
Functionality remains unchanged (validated by pre-existing backward compatibility tests), with one known exception. The EdDSA PCD now only supports "messages" (bigint arrays) of sizes in the set {1, 2, 3, 12, 13}. Other sizes will throw an error at proving time. This is because poseidon-lite also has a set of constants (albeit much smaller than circomlibjs) needed for each size. There are comments in the code explaining why each size was selected.
Two things explicitly not done, given the minimalist approach: