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

Specifying a custom dir with "dirCircuits" #33

Closed
cedoor opened this issue Nov 28, 2023 · 6 comments · Fixed by #34
Closed

Specifying a custom dir with "dirCircuits" #33

cedoor opened this issue Nov 28, 2023 · 6 comments · Fixed by #34
Labels
bug Something isn't working

Comments

@cedoor
Copy link

cedoor commented Nov 28, 2023

If I specify a custom directory for my circuit templates (e.g. src) and run circomkit compile it still creates a circuits/main/circuit.circom folder for its component. It fails because the relative path generated within that circuit is wrong. "../circuit.circom" doesn't exist in that folder.

I think the right folder for components should be src/main/circuit.circom.

The following line could be the problem.

const targetDir = `./circuits/${directory}`;

Does it make sense? Or am I missing something?

@erhant
Copy link
Owner

erhant commented Nov 28, 2023

Hello, thanks for the issue!

You are right, I believe I'd initially started as an opinionated repo where all circuits lay below circuits folder but then I added the configs, apparently forgot to connect this particular setting to instantiate.

There is a test for a custom path but there is no test for custom dir, so I think I will add a test for this as well.

I will fix it by exposing that directory config to instantiate + use that for targetDir. Probably tomorrow 🙏🏻
I will @ you when new version is published to NPM.

@erhant erhant added the bug Something isn't working label Nov 28, 2023
@cedoor
Copy link
Author

cedoor commented Nov 28, 2023

@erhant Thanks for your quick reply, I'll wait for it. And great work, I really like this tool!

@erhant
Copy link
Owner

erhant commented Nov 29, 2023

@cedoor heya, I believe the issue is fixed now & is published at v0.0.19

The test workflow failed because apparently the Circom installation fails in the workflow (#35 ) but the tests were passing locally™ for me. Please let me know if the issue is fixed on your end.

Also I wanted to ask for an opinion: the fix is as we have first discussed where Circomkit has a single dirCircuits and expects all circuits to be under there, and the rest of the path should be made via the file field of a circuit config. Would it make sense to also have an optional dirCircuits-like field for each circuit?

EDIT: Workflow bug is fixed & tests are pacito ✅

@cedoor
Copy link
Author

cedoor commented Nov 29, 2023

@erhant It works 👍🏽 Thank you so much!

Also I wanted to ask for an opinion: the fix is as we have first discussed where Circomkit has a single dirCircuits and expects all circuits to be under there, and the rest of the path should be made via the file field of a circuit config. Would it make sense to also have an optional dirCircuits-like field for each circuit?

Yes, I think it makes sense and I'd probably need it soon :D

@erhant
Copy link
Owner

erhant commented Nov 29, 2023

No problem!

Truly happy to hear that you enjoyed the lib & a pleasure to see PSE using it ❤️

I've created an issue about the aforementioned feature #37, I will look at it sometime soon, please @ me in that issue if you happen to need that urgently 👍🏻

@erhant erhant closed this as completed Nov 29, 2023
@cedoor
Copy link
Author

cedoor commented Nov 29, 2023

I've created an issue about the aforementioned feature #37, I will look at it sometime soon, please @ me in that issue if you happen to need that urgently 👍🏻

Sure! I think it's not urgent tho 👍🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants