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

Move Scalar to JS #935

Merged
merged 30 commits into from
Jun 8, 2023
Merged

Move Scalar to JS #935

merged 30 commits into from
Jun 8, 2023

Conversation

mitschabaude
Copy link
Member

@mitschabaude mitschabaude commented May 31, 2023

closes #925

bindings: o1-labs/o1js-bindings#30

This PR continues the work of #902 by moving the Scalar class from OCaml to JS. The OCaml class is removed entirely. Apart from minor API changes like introduction of Scalar.toConstant() (useful for tests), this keeps the same API intact. The internal representation of Scalar stays the same as before because the Group class relies on the structure and the PR to move that is in flight.

Some other things we also did, which stick to the theme of cleaning up the OCaml side:

  • Remove back and forth conversion of private keys from the OCaml side
  • Changed PrivateKey.to/fromBase58 to be the pure TS implementation
  • Moved private key conversions from the Ledger module to the Test module, because they are only used for tests now
  • Added a file where we will add tests for consistency that are no longer tested elsewhere as we move to pure TS implementations in snarkyjs
  • Remove the signFeePayer method from the global exports and a couple of exposed functions that it depended on

@mitschabaude mitschabaude marked this pull request as ready for review June 1, 2023 15:53
src/lib/signature.ts Show resolved Hide resolved
src/lib/scalar.ts Show resolved Hide resolved
Copy link
Member

@Trivo25 Trivo25 left a comment

Choose a reason for hiding this comment

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

very nice!

import { Field as InternalField } from './field.js';
import { Group as InternalGroup } from './group.js';
import { Scalar as ScalarBigint } from '../provable/curve-bigint.js';
import { mod } from '../bindings/crypto/finite_field.js';
import { Scalar } from './scalar.js';
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add the function constructor for the Scalar as well? I am a bit unsure - while it would be uniform, Scalar is also not really a primitive like Field or Bool, especially since you cant prove operations like addition 🤔

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 point. I think the need is not pressing and requires refactoring the current constructor, so would save it for a future PR

Copy link
Member

@bkase bkase left a comment

Choose a reason for hiding this comment

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

Looks great! Exploring these files are going to be a great way for SnarkyJS devs to learn more about our crypto stack

return Scalar.from(z);
}

// TODO don't leak 'shifting' to the user and remove these methods
Copy link
Member

Choose a reason for hiding this comment

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

+1

@mitschabaude mitschabaude merged commit 9f08936 into main Jun 8, 2023
9 checks passed
@mitschabaude mitschabaude deleted the refactor/scalar branch June 8, 2023 14:35
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.

Move Scalar to JS
3 participants