privacy-scaling-explorations / zk-kit

A monorepo of reusable libraries for zero-knowledge technologies.
https://zkkit.pse.dev
MIT License
284 stars 69 forks source link

Restrict types supported by functions in the EdDSA Poseidon package #188

Closed cedoor closed 6 months ago

cedoor commented 7 months ago

Describe the improvement you're thinking about

Currently, functions such as deriveSecretScalar, derivePublicKey and signMessage accept different types of private keys. BigNumberish can be: bigint, number, string, hexadecimal (string) or Buffer. Although supporting and converting many types reduces the dev's effort in certain contexts, it may also lead to ambiguity if a type is not correctly identified. For example, a hexadecimal string without '0x' is identified as plain text or as a bigint (if it contains only digits) depending on its value. And inconsistency remains between the length of an accepted arbitrary text (no limit at the moment) and the value of a bigint or a number, which have different size limits.

In general, the right choice for determining how many and which types to support depends on the case. Prioritizing simplicity and accepting only one type (e.g. Buffer) may be the right choice if you want to make your code more efficient and eliminate ambiguity in the interpretation of types. Making the code more flexible and adaptable may be a better choice in the case of prioritizing the user experience.

This is an implementation of an algorithm where performance matters, but at the same time the aim is to give devs an optimal experience. An intermediate solution might be to reduce the number of types to 1 or 2, and provide utilities to convert any values before passing them to library functions.

Additional context

Originated from discussion in https://github.com/privacy-scaling-explorations/zk-kit/pull/178#discussion_r1509782449.

cedoor commented 7 months ago

@artwyman After all, 1 reasonable compromise for the private key could be to allow devs to pass: Buffer, Uint8Array and text, i.e. all types that can be easily converted with Buffer.from(value), which is similar to what circomlibjs does.

That should eliminate ambiguity on input length. We could add more doc to explain to devs to convert their inputs if they are bigints or hexadecimals.

artwyman commented 7 months ago

@artwyman After all, 1 reasonable compromise for the private key could be to allow devs to pass: Buffer, Uint8Array and text, i.e. all types that can be easily converted with Buffer.from(value), which is similar to what circomlibjs does.

Buffer is a subclass of Uint8Array so I think those two are mostly equivalent. Including string text seems fine, but does raise some ambiguity as to whether the string is being treated directly as byte (probably UTF-8) or some sort of encoded hex or decimal number.

cedoor commented 7 months ago

Including string text seems fine, but does raise some ambiguity as to whether the string is being treated directly as byte (probably UTF-8) or some sort of encoded hex or decimal number.

Yes true, but I think it could be a good trade-off. Let's say the private key will be optional (if users don't pass it it'll be generated randomly), which is a nice feature we should implement. In that case, I think most users will pass text (i.e. passwords) rather than bytes, so might be worth supporting that type as well.