-
Notifications
You must be signed in to change notification settings - Fork 153
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
refactor(zk-kit-poseidon): remove old circomlib dependencies in crypto #943
Conversation
✅ Deploy Preview for maci-typedoc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
9694618
to
fcfae59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ctrlc03 thanks, just one comment/question.
crypto/ts/types.ts
Outdated
export type Ciphertext = bigint[]; | ||
// we define a bignumber as either a bigint or a string | ||
// which is what we use the most in MACI | ||
export type BigNumber = bigint | string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this type can have conflicts if it's using with BigNumber from ethers of from bignumber.js. Maybe it makes sense to reuse BigNumberish type from ethers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good shout, I can use that instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xmad What do you think about simply changing the type name? so we don't import from ethers and make further changes to the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it works as well.
0930543
to
b6e5508
Compare
b6e5508
to
0340f79
Compare
Description
This PR removes an old import (ciromlib) from the crypto library and instead imports the same functions from zk-kit. zk-kit provides lightweight TypeScript libraries for zk developers, allowing MACI's packages to be slimmer and more up to date.
Additional Notes
This is the first step into removing an old dependency, currently we still use it inside the circom circuits, and will be tackled in another PR.
@cedoor MACI 🤝 zk-kit
Related issue(s)
Re: #772
Additional Info
Let's merge #944 first
Confirmation