Improve CircuitValue #269

mitschabaude · 2022-06-30T09:44:16Z

this makes significant changes to the CircuitValue class, some non-controversial and some potentially controversial:

change the types of static methods on CircuitValue -- they weren't inferred correctly before, now they are, so type-safe extending of these methods works now
might be controversial: make the constructor on CircuitValue generic.
- the benefit is that now users of CircuitValue don't have to write a (very boiler-plate-y) constructor.
- a downside is that, with default TS configs, people not using a constructor on classes that extend CircuitValue will get a TS error on their props:
  "Property has no initializer and is not definitely assigned in the constructor"
  this can be fixed locally by adding a ! to the prop, e.g. @prop value!: Field. it can also be fixed globally in the project by adding a TS config option, which we should probably do in the CLI template project
- another small downside is that overriding the constructor becomes a bit more error-prone - you have to pass on your constructor arguments to super(). as such, this change probably breaks a couple of existing code, in a way that fixing it might not be very obvious. if we go with this, I think going forward we should not encourage the pattern of overriding the constructor in examples, but instead add static from methods which add any additional logic and invoke the original constructor. e.g., see how I changed the sudoku example

mitschabaude

some comments:

mitschabaude · 2022-06-30T10:10:10Z

src/lib/circuit_value.ts

 abstract class CircuitValue {
+  constructor(...props: any[]) {


fun fact: this function was written verbatim, completely correctly, by github copilot on the first try!!!

That's pretty amazing!

That is really cool! I'm going to try out copilot.

mitschabaude · 2022-06-30T10:11:46Z

src/lib/circuit_value.ts

@@ -122,21 +166,6 @@ abstract class CircuitValue {
  }
 }

-(CircuitValue as any).check = function (v: any) {


was able to remove this hack because I got the types working on CircuitValue

mitschabaude · 2022-06-30T10:12:24Z

src/lib/int.ts

-    super();
-    this.magnitude = magnitude;
-    this.sgn = sgn;
+    super(magnitude, sgn);


I left this constructor because it adds something nice -- a default value for one of the arguments

mitschabaude · 2022-06-30T10:13:35Z

tsconfig.json

@@ -14,6 +14,7 @@
    "useDefineForClassFields": false, // ensure correct behaviour of class fields with decorators

    "strict": true, // for the full TypeScript experience
+    "strictPropertyInitialization": false, // to enable generic constructors, e.g. on CircuitValue


if we go with this PR, this should be added in the zkapp CLI template projects as well

cc @ymekuria has a heads up 👀

I agree we should fix this globally in the CLI and push users to do the same instead fixing locally by adding a ! to the prop, e.g. @prop value!: Field.

mitschabaude · 2022-06-30T10:31:58Z

src/examples/sudoku/sudoku-zkapp.ts

@@ -84,6 +83,7 @@ class SudokuZkapp extends SmartContract {

    // finally, we check that the sudoku is the one that was originally deployed
    let sudokuHash = this.sudokuHash.get(); // get the hash from the blockchain
+    this.sudokuHash.assertEquals(sudokuHash);


this is a fix that's unrelated to the rest of the PR

MartinMinkov

The changes look good to me as long as everyone agrees on this controversial change :D

I added an issue in the zkapp-cli repo to update the Sudoku example if this lands in it's current state.

MartinMinkov · 2022-06-30T18:24:14Z

src/lib/circuit_value.ts

+      const value = (v as any)[key];
+      if (propType.check === undefined)
+        throw Error('bug: circuit value without .check()');
+      propType.check(value);


Does this error get thrown if a CircuitValue class does not define a check function for itself?

no! it would get thrown if one of our primitives was missing check -- which isn't the case --, or maybe if a developer uses something which isn't a CircuitValue,

but probably won't get thrown at all :D

MartinMinkov · 2022-06-30T18:25:46Z

src/lib/circuit_value.ts

 abstract class CircuitValue {
+  constructor(...props: any[]) {


That's pretty amazing!

ymekuria

The changes LGTM assuming we all decide to accept the controversial changes. I added an issue to the CLI similar to @MartinMinkov 's to update the TS Config if this PR lands.

I generally like this approach of reducing boilerplate in the constructor for users. The TS error downside is minimal and can be mitigated fairly easily. Introducing a new pattern of adding a static from method will be more involved. It would involve changing existing code, updating docs, and user eduction. I think the upside is probably worth all this.

mitschabaude · 2022-06-30T20:52:38Z

Introducing a new pattern of adding a static from method will be more involved. It would involve changing existing code, updating docs, and user eduction. I think the upside is probably worth all this.

Hmm ok, we also don't have to do this. If you don't prefer it then we can also stick with constructors in our examples, or just don't single out one way of doing it

ymekuria · 2022-06-30T21:46:19Z

Introducing a new pattern of adding a static from method will be more involved. It would involve changing existing code, updating docs, and user eduction. I think the upside is probably worth all this.

Hmm ok, we also don't have to do this. If you don't prefer it then we can also stick with constructors in our examples, or just don't single out one way of doing it

I'm ok to do this if we all agree that this pattern is better. We should be clear and consistent on our recommendations of best practices.

bkase

This looks great, I think the tradeoffs are all worth it 💯

mitschabaude added 9 commits June 23, 2022 19:25

type CircuitValue.check in a way that works

b1c5dd6

Merge branch 'main' into feature/circuit-value-tweaks

eb4259b

fix types of static methods on CircuitValue

b0d8e5a

fixup type

d0a6c41

add generic constructor to CircuitValue

a405f1d

remove / tweak CircuitValue constructors in /lib

be1d24b

remove / tweak CircuitValue constructors in examples

928206e

allow not explicitly initialized props in tsconfig

0a52594

fix sudoku example

6cc99fa

mitschabaude commented Jun 30, 2022

View reviewed changes

mitschabaude mentioned this pull request Jun 30, 2022

Snarkyjs: fix Circuit.if MinaProtocol/mina#11417

Merged

MartinMinkov mentioned this pull request Jun 30, 2022

Update existing examples to use new SnarkyJS CircuitValue improvements o1-labs/zkapp-cli#227

Closed

MartinMinkov approved these changes Jun 30, 2022

View reviewed changes

ymekuria self-requested a review June 30, 2022 19:28

ymekuria mentioned this pull request Jun 30, 2022

Update TS config to allow extending CircuitValue class with no constructor o1-labs/zkapp-cli#228

Closed

ymekuria approved these changes Jun 30, 2022

View reviewed changes

bkase approved these changes Jul 5, 2022

View reviewed changes

Merge branch 'main' into feature/circuit-value-tweaks

c3eb8c6

mitschabaude merged commit 98f2c4b into main Jul 7, 2022

mitschabaude deleted the feature/circuit-value-tweaks branch July 7, 2022 09:27

This was referenced Aug 3, 2022

fetchEvents for LocalBlockchain #323

Merged

Update template and examples with preconditions and new CircuitValue improvements [Do Not Merge Until New Published Version of SnarkyJS is Added] o1-labs/zkapp-cli#238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CircuitValue #269

Improve CircuitValue #269

mitschabaude commented Jun 30, 2022 •

edited

Loading

mitschabaude left a comment

mitschabaude Jun 30, 2022 •

edited

Loading

MartinMinkov Jun 30, 2022

ymekuria Jun 30, 2022

mitschabaude Jun 30, 2022

mitschabaude Jun 30, 2022

mitschabaude Jun 30, 2022

MartinMinkov Jun 30, 2022

ymekuria Jun 30, 2022

mitschabaude Jun 30, 2022

MartinMinkov left a comment •

edited

Loading

MartinMinkov Jun 30, 2022

mitschabaude Jun 30, 2022

mitschabaude Jun 30, 2022

MartinMinkov Jun 30, 2022

ymekuria left a comment

mitschabaude commented Jun 30, 2022 •

edited

Loading

ymekuria commented Jun 30, 2022

bkase left a comment

Improve CircuitValue #269

Improve CircuitValue #269

Conversation

mitschabaude commented Jun 30, 2022 • edited Loading

mitschabaude left a comment

Choose a reason for hiding this comment

mitschabaude Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinMinkov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymekuria left a comment

Choose a reason for hiding this comment

mitschabaude commented Jun 30, 2022 • edited Loading

ymekuria commented Jun 30, 2022

bkase left a comment

Choose a reason for hiding this comment

mitschabaude commented Jun 30, 2022 •

edited

Loading

mitschabaude Jun 30, 2022 •

edited

Loading

MartinMinkov left a comment •

edited

Loading

mitschabaude commented Jun 30, 2022 •

edited

Loading