Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: PrivateKey type #1232

Merged
merged 17 commits into from
Jul 27, 2023
Merged

feat: PrivateKey type #1232

merged 17 commits into from
Jul 27, 2023

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Jul 27, 2023

Description

  1. Fixes Create a type with validity checks for private key #1226.
  2. Separated classes in yarn-project/circuits.js/src/types/partial_contract_address.ts to individual files.
  3. Refactored curves to not pass buffers for both public and private key.
  4. In the curves I renamed the scalar param to privateKey because having scalar being type PrivateKey is weird and we don't ever use these classes with scalars which are not a representation of private key. For this reason I think it's ok to have the parameter name be less generic. Another alternative I considered was having a Scalar type and PrivateKey being a type alias of it but given that we don't use non-private-key scalar anywhere in our TS codebase I think that's unnecessary complication.
  5. Renamed e2e-account-contract to e2e-account-contracst so that the job name is consistent with the test name.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@netlify
Copy link

netlify bot commented Jul 27, 2023

Deploy Preview for preeminent-bienenstitch-606ad0 canceled.

Name Link
🔨 Latest commit 2e8f0ed
🔍 Latest deploy log https://app.netlify.com/sites/preeminent-bienenstitch-606ad0/deploys/64c27d20ffa0860008c3512f

@benesjan benesjan marked this pull request as draft July 27, 2023 09:30
@benesjan benesjan changed the title feat: PrivateKey type feat: PrivateKey type Jul 27, 2023
@Maddiaa0
Copy link
Member

merging master to check the l1 contracts ci issue is fixed

@benesjan benesjan force-pushed the janb/private-key-type branch 2 times, most recently from ea80040 to 7bf6ed0 Compare July 27, 2023 13:25
@benesjan benesjan marked this pull request as ready for review July 27, 2023 13:37
@benesjan benesjan enabled auto-merge (squash) July 27, 2023 13:38
@@ -9,8 +9,8 @@ import {
createAztecRpcClient,
getAccountWallet,
} from '@aztec/aztec.js';
import { PrivateKey } from '@aztec/circuits.js';
Copy link
Member

Choose a reason for hiding this comment

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

you can export this from aztec.js to avoid bringing in the extra dependency.
I can also do it in my PR as I've done a few of these already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would make sense to export that through types package (that one is in deps). Will do it like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 12cc87f

@benesjan benesjan disabled auto-merge July 27, 2023 13:56
@benesjan benesjan force-pushed the janb/private-key-type branch from 7bf6ed0 to 12cc87f Compare July 27, 2023 13:59
Copy link
Member

@spypsy spypsy left a comment

Choose a reason for hiding this comment

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

Looks good, I think you may have to add PrivateKey to getHttpRpcServer & createAztecRpcClient and ensure that it has toString & fromString implementations

@benesjan benesjan force-pushed the janb/private-key-type branch from f7b889c to 2e8f0ed Compare July 27, 2023 14:20
@benesjan
Copy link
Contributor Author

Looks good, I think you may have to add PrivateKey to getHttpRpcServer & createAztecRpcClient and ensure that it has toString & fromString implementations

@spypsy Good catch! Addressed in 2e8f0ed

@benesjan benesjan enabled auto-merge (squash) July 27, 2023 14:24
@benesjan benesjan merged commit 6bbbb65 into master Jul 27, 2023
@benesjan benesjan deleted the janb/private-key-type branch July 27, 2023 14:32
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Create a type with validity checks for private key
3 participants