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

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

Closed
cedoor opened this issue Mar 6, 2024 · 3 comments · Fixed by #215
Closed

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

cedoor opened this issue Mar 6, 2024 · 3 comments · Fixed by #215
Assignees
Labels
refactoring ♻️ A code change that neither fixes a bug nor adds a feature
Milestone

Comments

@cedoor
Copy link
Member

cedoor commented Mar 6, 2024

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 #178 (comment).

@cedoor cedoor added the refactoring ♻️ A code change that neither fixes a bug nor adds a feature label Mar 6, 2024
@cedoor cedoor added this to the Beta milestone Mar 6, 2024
@cedoor cedoor added this to ZK-Kit Mar 6, 2024
@cedoor cedoor moved this to 📋 Backlog in ZK-Kit Mar 6, 2024
@cedoor
Copy link
Member Author

cedoor commented Mar 6, 2024

@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
Copy link
Collaborator

artwyman commented Mar 6, 2024

@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
Copy link
Member Author

cedoor commented Mar 7, 2024

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.

@cedoor cedoor moved this from 📋 Backlog to ♻️ Grooming in ZK-Kit Mar 7, 2024
@cedoor cedoor moved this from ♻️ Grooming to 🗒 Tasks in ZK-Kit Mar 13, 2024
@cedoor cedoor self-assigned this Mar 13, 2024
@cedoor cedoor moved this from 🗒 Tasks to 🏗 In Progress in ZK-Kit Mar 18, 2024
cedoor added a commit that referenced this issue Mar 18, 2024
Tyes supported for the EdDSA private key has been reduced to 3: Buffer, Uint8Array and string, i.e.
buffers or text.

re #188
@cedoor cedoor moved this from 🏗 In Progress to 👀 In Review in ZK-Kit Mar 18, 2024
cedoor added a commit that referenced this issue Mar 18, 2024
Types supported for the EdDSA private key have been reduced to 3: Buffer, Uint8Array and string.

re #188
cedoor added a commit that referenced this issue Mar 18, 2024
cedoor added a commit that referenced this issue Mar 18, 2024
@github-project-automation github-project-automation bot moved this from 👀 In Review to ✔️ Done in ZK-Kit Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring ♻️ A code change that neither fixes a bug nor adds a feature
Projects
Status: ✔️ Done
Development

Successfully merging a pull request may close this issue.

2 participants