-
Notifications
You must be signed in to change notification settings - Fork 220
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
feat: devex improvements to noirjs #3044
Conversation
We would only instantiate if the backend hadn't already been instantiated previously. This change saves us checking whether a variable is defined or not but means we give up the option of lazy initialization. I don't think that this is a good trade. |
This requires us to always have a backend when interacting with the |
I also think this is not ideal. We should give users the tools to break out of the standard workflow if necessary (we've already run into a couple of cases where this is necessary). |
On the other hand it kind of relies on the developer to store the instance if they're browsing away to other routes. But I can revert to lazy initialisation if you prefer.
I see what you mean, but I don't think passing the circuit twice is good practice. We want to target both the "standard" flow, and the more advanced usage. Just as you suggested, I can change to make backend an optional parameter (and do some error handling) and tackle both situations.
I most definitely agree with you! However, this doesn't void the user of the possibility to use these packages, as he can always import the If we export them as an API, they'd be clogging the inexperienced developer's workflow with use-cases that are probably not the majority. |
This kinda defeats the point of noir.js, no? If we're expecting users to manage versions of the underlying packages to keep them in sync with each other, and as well as that with the versions included in noir.js then we've just made the problem worse. |
I actually raise the same point, but with a different argumentation... If In other words, We're not excluding advanced developers from using our tools, we're including everybody else. |
I didn't say that it's the only purpose but a significant motivator for |
cc #2910 |
So talking to @kevaundray we decided to export everything (and shut up the docs autogenerator) since we're not sure which methods will be used elsewhere, there's some work going on in Aztec that would be using some acvm methods. Once that's cleared up, we can narrow them as part of #3078 Let me just make some changes and I'll push & resolve conflicts. Thanks @TomAFrench for the discussion |
Thanks @signorecello! Do request for re-reviews when ready. |
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 PR is muddling what the Noir
class represents, it encapsulates a Noir program and should expose methods to interact with this program. new Noir()
then doesn't really have any meaning and any method calls on Noir()
could just be regular functions.
I think making the Backend
be the source of truth on the circuit in question is also a misstep. The Backend
only needs the circuit to determine how much of the CRS to load and is otherwise generic, ideally I should be able to reuse the same backend object for creating proofs for two different circuits but this PR takes us further away from this.
// Initial inputs to your program | ||
async execute( | ||
inputs: abi.InputMap, | ||
circuit?: CompiledCircuit, |
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's a better API to instantiate the Noir
class with a CompiledCircuit
and use that in this function rather than having to pass in a circuit each time.
This is inconsistent when Noir
"knows" about the circuit you're interacting with when you're proving/verifying but not for execution.
const solvedWitnessString = await new Noir(assert_lt_json).execute(inputsString, assert_lt_program); | ||
const solvedWitnessNumber = await new Noir(assert_lt_json).execute(inputsNumber, assert_lt_program); |
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.
These are broken. You're passing the circuit in twice.
I have similar observations on the surface to Tom. Seems that we do not need |
So I've talked with @TomAFrench and it seems like we could drop this in favour of lazy initialization of the CRS which would be a more dynamic way of achieving the same (not passing the circuit to both instances). I'll be spinning off of this to add the destroy method and the options object in another PR. Thanks @kobyhallx for the review |
Description
Working on autogenerated docs at #3035 I came up with some improvements to UX and Devex for NoirJS.
Problem*
Not really a problem, just addressing some DevEx concerns and UX. Before this PR, standard usage was:
This could be improved
Summary*
circuit
parameter to the newexecute
function instead of the Noir class - this avoids creating a new Noir class everytime we want to execute something, plus removes the need to always pass a circuit twice (to backend, and to Noir)Makes it not export acvm, abi, generateWitness, and others from the index file of noir_js, as a precursor to chore: adding autogenerated docs for noirjs #3035It will still export these and we'll figure it out with the docs autogenerator