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

chore: clean up #74

Merged
merged 7 commits into from
Nov 18, 2023
Merged

chore: clean up #74

merged 7 commits into from
Nov 18, 2023

Conversation

0xmad
Copy link
Collaborator

@0xmad 0xmad commented Nov 8, 2023

  • Add checks for ci actions
  • Run prettier, clippy, fmt commands for all the files
  • Move circom circuits to a circom folder
  • Get rid of js var statements
  • Speed up circom tests by moving them to separate files and run concurrently

0xmad and others added 3 commits November 8, 2023 17:50
- [x] Add checks for ci actions
- [x] Run prettier, clippy, fmt commands for all the files
- [x] Move circom circuits to a circom folder
- [x] Get rid of js var statements
@Divide-By-0 Divide-By-0 requested a review from skaunov November 15, 2023 10:25
@Divide-By-0
Copy link
Member

This looks great! Until @skaunov reviews it, do you want to send your wallet address and I can send a bounty?

@0xmad
Copy link
Collaborator Author

0xmad commented Nov 15, 2023

@Divide-By-0 sorry, but I work at PSE :)

@Divide-By-0
Copy link
Member

@Divide-By-0 sorry, but I work at PSE :)

Oh you know what, that makes a lot of sense LOL

Copy link
Collaborator

@skaunov skaunov left a comment

Choose a reason for hiding this comment

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

I stumbled through, looking into what caught my attention. GA upgrades are very cool, lots of useful settings/setups, and so on.
The one thing that I would highlight is replacing Yarn for pnpm. I don't follow JS really closely, and my knowledge of it is pretty basic; I mean no defense of Yarn from my side, heh. The sole reason I highlight the fact is that with mine casual exposure to JS I never came across pnpm AFAIR. On the other hand I guess @0xmad wouldn't put it here if it weren't developer friendly.

That's all from me, other added comments are truly minor.

.github/workflows/rust.yml Outdated Show resolved Hide resolved
circuits/package-lock.json Outdated Show resolved Hide resolved
@0xmad
Copy link
Collaborator Author

0xmad commented Nov 16, 2023

@skaunov thanks for the review. I added pnpm just because it's faster and it's easier to use it as monorepo tool (I'll add workspaces later to get rid of relative paths inside circuits/circom).

@0xmad 0xmad requested a review from skaunov November 18, 2023 06:50
Copy link
Collaborator

@skaunov skaunov left a comment

Choose a reason for hiding this comment

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

All items which caught my attention was addressed; so I have nothing to add to the previous review!

@skaunov skaunov merged commit 79c6026 into plume-sig:main Nov 18, 2023
11 checks passed
@skaunov
Copy link
Collaborator

skaunov commented Nov 18, 2023

I just noticed that @Divide-By-0 asked actually to also merge this one. %)

@0xmad 0xmad deleted the chore/clean-up branch November 18, 2023 17:50
@skaunov
Copy link
Collaborator

skaunov commented Nov 22, 2023

Just noticed that we have path with repetition now: <circuits/circom/test/circuits>. I personally try to avoid this due to minor discomfort it brings, so want to attract your attention to this. Though if everybody fine with it, then...

@0xmad
Copy link
Collaborator Author

0xmad commented Nov 22, 2023

@skaunov what's the problem with it? It's not going to be exported and it uses internally inside test folder.

@skaunov
Copy link
Collaborator

skaunov commented Dec 1, 2023

Path at

Then, navigate to the `circuits/` directory and install the dependencies there:
should also corrected, right? @0xmad

@skaunov
Copy link
Collaborator

skaunov commented Dec 1, 2023

I also received when was quickly running the instructions.

devbox@pop-os:~/zk-nullifier-sig/circuits/circom$ pnpm run flatten-deps && pnpm run test
 ERR_PNPM_NO_SCRIPT  Missing script: flatten-deps

Command "flatten-deps" not found.

@0xmad
Copy link
Collaborator Author

0xmad commented Dec 1, 2023

circuits/ path yes, should be corrected.
You don't need to run flatten-deps with test. It's automatically run after pnpm install.

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.

3 participants