ZK-559: Add shielder-sdk-crypto package #77
Conversation
|
| @@ -0,0 +1,40 @@ | |||
| import eslint from "@eslint/js"; | |||
There was a problem hiding this comment.
can we share these config files among workspace members?
There was a problem hiding this comment.
moved to workspace, it seems to work.
ts/shielder-sdk-crypto/src/index.ts
Outdated
| initialDeposit: Scalar; | ||
| } | ||
|
|
||
| export interface NewAccountValues { |
There was a problem hiding this comment.
suggestion: rename Values to Inputs, PrivateInputs, ProverKnowledge, Advice
ts/shielder-sdk-crypto/src/index.ts
Outdated
| commitment: Scalar; | ||
| } | ||
|
|
||
| export type ShielderActionSecrets = { |
There was a problem hiding this comment.
do we want id here as well?
There was a problem hiding this comment.
id and nonce are parameters of the function, keeping it simple for now, no "mother-types".
|
|
||
| verifyWithdraw(proof: Proof, pubInputs: WithdrawPubInputs): Promise<boolean>; | ||
|
|
||
| proveAndVerifyMerkle(): Promise<Proof>; |
There was a problem hiding this comment.
do we still care about merkle?
|
|
||
| privateKeyToScalar(hex: `0x${string}`): Promise<Scalar>; | ||
|
|
||
| treeHeight(): Promise<number>; |
There was a problem hiding this comment.
I suggest using 100% clear name, like noteTreeHeight - in some time we might have other trees
There was a problem hiding this comment.
similar comment to arity - it's not clear if that's for note tree, poseidon width or e.g. some token operation
There was a problem hiding this comment.
moved to NoteTreeConfig interface
There was a problem hiding this comment.
nit: maybe just "height", for symmetry with "arity"?
There was a problem hiding this comment.
@kroist just a gentle reminder to leave a short response if you decide not to apply a suggestion
| WithdrawValues | ||
| } from "./index"; | ||
|
|
||
| export interface CryptoClient { |
There was a problem hiding this comment.
I don't know if that's feasible in ts, but if possible, I would split this interface - currently it mixes a lot of responsibilities:
- proving
- veryifying
- configuration information (arity, height)
- crypto primitives (hash)
- conversion
- secret management
I'm not saying that each of this bullet point shall be under a dedicated interface, but still, some separation might be useful and clarifying
There was a problem hiding this comment.
did split it into sub-interfaces
There was a problem hiding this comment.
If it were pure TypeScript, I would agree. But the purpose of CryptoClient is to aggregate the platform-specific functionality. Sometimes we'll inject a WASM crypto client, sometimes a React Native crypto client, and sometimes a mock. It feels simpler to do it once. Also, having just one gateway gives a single entry point for platform-specific initialization, if such initialization were needed. I think it might be better to keep these under 1 interface.
There was a problem hiding this comment.
indeed it can be done with an splitted interface.
There was a problem hiding this comment.
Ok, now it is ok. Max just grouped the new interfaces inside CryptoClient and I am satisfied with that.
| @@ -0,0 +1,102 @@ | |||
| /** | |||
There was a problem hiding this comment.
the same comment as in #76 please flag somehow (e.g. by deprecation marker or in Jira) that there is some code duplication that needs to be removed as soon as possible (to avoid any divergence)
There was a problem hiding this comment.
created issues in jira
| WithdrawValues | ||
| } from "./index"; | ||
|
|
||
| export interface CryptoClient { |
There was a problem hiding this comment.
Ok, now it is ok. Max just grouped the new interfaces inside CryptoClient and I am satisfied with that.
| @@ -0,0 +1,4 @@ | |||
| packages: | |||
There was a problem hiding this comment.
Could you fix Playwright tests pls?
|
|
||
| privateKeyToScalar(hex: `0x${string}`): Promise<Scalar>; | ||
|
|
||
| treeHeight(): Promise<number>; |
There was a problem hiding this comment.
nit: maybe just "height", for symmetry with "arity"?
Introducing a typescript package
shielder-sdk-cryptowhich purpose is to define an interface for platform-native cryptographic functions.Initial Setup and Configuration:
shielder-sdk-cryptopackage to the workspace (ts/pnpm-workspace.yaml). We will eventually transfer all typescript packages in monorepo to the workspace.Core Functionality Implementation:
ts/shielder-sdk-crypto/src/index.ts).CryptoClientinterface with methods for calling platform-native cryptography functions (ts/shielder-sdk-crypto/src/cryptoClient.ts).Scalarclass and utility functions for handling scalar values. It is cross-platform vanilla TS/JS code (ts/shielder-sdk-crypto/src/scalar.ts).