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

Refactor provable core #1507

Merged
merged 29 commits into from
Mar 26, 2024
Merged

Refactor provable core #1507

merged 29 commits into from
Mar 26, 2024

Conversation

mitschabaude
Copy link
Member

@mitschabaude mitschabaude commented Mar 14, 2024

The general goal here is to make the structure of core provable code more logical.

closes #1504

  • get rid of several import cycles surrounding Field, by introducing a low-level stub module field-constructor.ts
  • move FieldVar into its own lower level module
  • define the one and only internal version of exists()
  • Fix vulnerability: missing boolean check in Field.isEven()
  • move Provable<T> to its own file
    • Simplify doccomments on Provable<T> 48b897c
    • Make Provable<T> a type instead of interace, because we don't use interfaces in general and they behave slightly different than types in subtle ways 82fe43b
    • also make other interfaces types, for consistency
  • move some provable types into provable-types/
  • move some provable types logic from bindings to provable-types/
  • move some logic mostly used by mina-signer into mina-signer
  • break up circuit-value.ts into separate files for CircuitValue, Struct and Unconstrained

bindings: o1-labs/o1js-bindings#258

Base automatically changed from feature/ts-gadgets to main March 17, 2024 08:52
@mitschabaude mitschabaude marked this pull request as ready for review March 25, 2024 14:25
@mitschabaude mitschabaude requested a review from a team as a code owner March 25, 2024 14:25
Copy link
Member Author

Choose a reason for hiding this comment

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

this was moved here from bindings/lib/provable-bigint

Copy link
Member Author

Choose a reason for hiding this comment

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

this was moved here from bindings/lib/provable-snarky

Copy link
Member Author

Choose a reason for hiding this comment

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

this code is the CircuitValue part of what used to be src/lib/circuit-value.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

this whole code used to be in field.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

these interfaces used to be in snarky.d.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

this code is the Unconstrained part of what used to be src/lib/circuit-value.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

Grouping things into the provable-types folder and creating the unconstrained file makes things easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

apart from existsAsync(), this now also has the exists() function we use internally a lot and that Provable.witness uses

import { Field } from './core.js';
import { Field as Fp } from '../provable/field-bigint.js';
import { Fp } from '../bindings/crypto/finite-field.js';
Copy link
Contributor

@ymekuria ymekuria Mar 25, 2024

Choose a reason for hiding this comment

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

Just curious why you decided to move this to the bindings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! The Field module from field-bigint.ts is a wrapper around the Fp module which was always in bindings/crypto. field-bigint.ts adds other stuff to it like toBytes and toJSON methods which have nothing to do with the core finite field arithmetic. It's basically the version of Field used by mina-signer.

It's hard to track dependencies when logic is wrapped and reexported multiple times. I wanted to make it more explicit where this Fp (bigint field arithmetic) logic actually comes from, and remove the dependency on the mina-signer middle-man code where not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok great. That logic makes a lot of sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Grouping things into the provable-types folder and creating the unconstrained file makes things easier to understand.

@mitschabaude mitschabaude merged commit a55491c into main Mar 26, 2024
16 checks passed
@mitschabaude mitschabaude deleted the feature/consolidate-exists branch March 26, 2024 09:21
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.

Fix dependency cycles between field and core gadgets
2 participants