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.

Sign up for GitHub

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

New utility modules for handling errors and checking types #178

Merged
merged 9 commits into from
Mar 6, 2024
Merged

New utility modules for handling errors and checking types #178

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
331ceec
feat(utils): add new utility modules for errors and types
cedoor Feb 21, 2024
0f2ee83
build(utils): remove declarations when using output.dir option
cedoor Feb 21, 2024
309510b
refactor(utils): re-use zk-kit utility functions
cedoor Feb 29, 2024
759f122
chore: update lock file
cedoor Feb 29, 2024
3d6c6df
style(eddsa-poseidon): fix lint error
cedoor Feb 29, 2024
30be5b8
refactor(utils): rename packing to proof-packing
cedoor Mar 2, 2024
6e81e2f
docs(utils): add throws js-doc tag
cedoor Mar 2, 2024
932b4c5
refactor(utils): update error messages
cedoor Mar 5, 2024
2dde56a
refactor(utils): rename errors re object checks
cedoor Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion jest.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ const projects: any = fs
rootDir: `packages/${name}`,
displayName: name,
moduleNameMapper: {
"@zk-kit/(.*)": "<rootDir>/../$1/src/index.ts" // Interdependency packages.
"@zk-kit/(.*)/(.*)": "<rootDir>/../$1/src/$2.ts",
"@zk-kit/(.*)": "<rootDir>/../$1/src/index.ts"
}
}))

Expand Down
49 changes: 19 additions & 30 deletions packages/eddsa-poseidon/src/eddsa-poseidon.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,15 @@ import {
subOrder,
unpackPoint
} from "@zk-kit/baby-jubjub"
import {
BigNumber,
BigNumberish,
F1Field,
isHexadecimal,
isStringifiedBigint,
leBigIntToBuffer,
leBufferToBigInt,
scalar
} from "@zk-kit/utils"
import type { BigNumberish } from "@zk-kit/utils"
import { bigNumberishToBigInt, leBigIntToBuffer, leBufferToBigInt } from "@zk-kit/utils/conversions"
import { requireBigNumberish } from "@zk-kit/utils/error-handlers"
import F1Field from "@zk-kit/utils/f1-field"
import * as scalar from "@zk-kit/utils/scalar"
import { poseidon5 } from "poseidon-lite/poseidon5"
import blake from "./blake"
import { Signature } from "./types"
import * as utils from "./utils"
import { checkMessage, checkPrivateKey, isPoint, isSignature, pruneBuffer } from "./utils"

/**
* Derives a secret scalar from a given EdDSA private key.
Expand All @@ -37,12 +32,12 @@ import * as utils from "./utils"
*/
export function deriveSecretScalar(privateKey: BigNumberish): string {
// Convert the private key to buffer.
privateKey = utils.checkPrivateKey(privateKey)
privateKey = checkPrivateKey(privateKey)

let hash = blake(privateKey)

hash = hash.slice(0, 32)
hash = utils.pruneBuffer(hash)
hash = pruneBuffer(hash)

return scalar.shiftRight(leBufferToBigInt(hash), BigInt(3)).toString()
}
Expand Down Expand Up @@ -74,14 +69,14 @@ export function derivePublicKey(privateKey: BigNumberish): Point<string> {
*/
export function signMessage(privateKey: BigNumberish, message: BigNumberish): Signature<string> {
// Convert the private key to buffer.
privateKey = utils.checkPrivateKey(privateKey)
privateKey = checkPrivateKey(privateKey)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not strictly related to this PR, and I'm not sure if it's actionable, but I wanted to share some feedback about my experience with this. I think the level of flexibility on inputs here has a downside that the caller may not always be sure how the input is being interpreted, which could have an impact for compatibility.

For example, in my tests using this library, I was using a test private key borrowed from here: https://github.com/iden3/circomlibjs/blob/4f094c5be05c1f0210924a3ab204d8fd8da69f49/test/eddsa.js#L103

That key is intended to be a hex string representing 32 bytes of key to be hashed by the initial blake hash below. That's how I need to interpret it to remain consistent with existing uses of EdDSA in Zupass. However the hex digits happen to all be in the range 0-9, so it was being interpreted as a stringified bigint. This worked but was treating my string as a different key than I intended. If any of my digits had been A-F, it would've instead treated my key as a raw string of 64 bytes, rather than 32 bytes, which again would've silently worked, but been treating it as a different key. What I should have done (and will now do) is convert my string into a buffer in advance using Buffer.from(pk, "hex"), meaning I'll be bypassing all the type flexibility here.

As I said, I'm not sure if there's a proposal here, but I suggest thinking about the tradeoff between accepting flexible input, vs. being stricter in order to make sure users are only getting the behavior they expect. Given that this function also accepts arbitrary strings (like passwords?), I think it might be best not to accept stringified bigints too, since it's impossible to know what the user's intent if the string happens to be made up of digits. This is the kind of behavior which might be difficult to maintain compatibly if the code ever changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's actually another related point which I think is a potential bug. If I put a "0x" before my private key, it's interpreted as hex, but ends up as a 31-byte buffer rather than a 32-byte buffer. The reason is that the hex string starts with 00, and the conversion from string -> bigint -> buffer ends up truncating the leading zeroes. I'm not sure what you think is "correct" here, but if you intend bigints to be converted into fixed-size buffers, you should pass a size parameter here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I discovered that the conversion issue I mentioned was only in my test code, not my intended production library, which was already calling Buffer.from(pk, "hex"). My point about the potential for confusion and mistakes remains, though it seems I don't have a major issue to fix on my side. I'm going to be writing more compatibility tests to rule out this kind of issue in future.

Copy link
Member Author

@cedoor cedoor Mar 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As I said, I'm not sure if there's a proposal here, but I suggest thinking about the tradeoff between accepting flexible input, vs. being stricter in order to make sure users are only getting the behavior they expect.

I totally agree and I'd propose the following types: arbitrary strings (yes like passwords, we should keep this as it seems the most common use-case to me), bigint, number, Buffer, or hex strings (must start with '0x'). It would be like now, but without bigint strings. Ofc we'd need to add more doc.

I realized message is a bit different here, it must be a 32-byte value, so we should check the arbitrary string length I guess.

This is something we should think about now, before making this lib stable IMO. Let me know what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason is that the hex string starts with 00, and the conversion from string -> bigint -> buffer ends up truncating the leading zeroes.

The problem here is the way bigNumberishToBuffer converts hexadecimal strings to buffers. It could convert it by using Buffer.from(n, "hex") if it's a hex string. "0x" should be removed before converting it ofc. IMO if the hex starts with zeroes it should consider them when converting to buffer. How does it sound?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, we could create separate issues for these conversations and close this PR. It looks like there are three:

  1. Restrict the number of supported types for the EdDSA privateKey parameter
  2. Add size parameter to the bigNumberish functions - ish
  3. Check the length of arbitrary strings for the EdDSA message parameter

Other issues could be included in these 3. Missing anything?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm good with breaking things out. Quick responses to your latest thoughts:

Size parameters seem like a lot of overhead, particularly if they're usually forced to be 32. Using a type like Buffer/Uint8Array/string which has its own size seems like the cleanest way to do that, but I think bigint is actually the clearest/simplest type for most of this library since it avoids byte order issues. As discussed elsewhere, this private key is distinct because it's an input of bytes, not of a field element.

I think the question of which input encoding (Buffer, hex string, etc) for private keys is fuzzy enough that it's going to be hard to do right for all users. In Zupass, I've standardized on hex strings, and we have toHexString/fromHexString helpers to convert from/to Buffers. That's mostly because that was the standard set by the EdDSATicketPCD, and I want simplicity and clarity. There are situations in which I might consider performance most important, where I might already have my bytes in a Buffer and resent the need to convert it to hex unnecessarily. But I come from lower-level languages than JavaScript, so I'm not sure how much that level of micro-optimization really matters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to add another wrinkle to string encodings: The Zupass/PCD standard is hex strings without the 0x prefix. I think that actually makes sense when you're encoding an arbitrary-length array of bytes, because 0x to me suggests a single number. But it is part of what lead to the ambiguity which started this conversation (because my test key happened to contain only the digits 0-9).

Copy link
Member Author

@cedoor cedoor Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are situations in which I might consider performance most important.

That's the point I think. In this case, we have an algorithm implementation and even if it is JS perhaps it is better to minimize conversions and type-checks in functions and prioritize performances, but also give devs an optimal experience.

Re: #188.

Copy link
Member Author

@cedoor cedoor Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think that actually makes sense when you're encoding an arbitrary-length array of bytes.

That makes sense. If we add new utility functions to convert hex to buffer or buffer to hex we shouldn't use 0x. In the other cases, it makes sense to use 0x as they are mostly conversions between bigints and hexadecimal.

Re: #189.


// Convert the message to big integer.
message = utils.checkMessage(message)
message = checkMessage(message)

const hash = blake(privateKey)

const sBuff = utils.pruneBuffer(hash.slice(0, 32))
const sBuff = pruneBuffer(hash.slice(0, 32))
const s = leBufferToBigInt(sBuff)
const A = mulPointEscalar(Base8, scalar.shiftRight(s, BigInt(3)))

Expand Down Expand Up @@ -112,8 +107,8 @@ export function signMessage(privateKey: BigNumberish, message: BigNumberish): Si
*/
export function verifySignature(message: BigNumberish, signature: Signature, publicKey: Point): boolean {
if (
!utils.isPoint(publicKey) ||
!utils.isSignature(signature) ||
!isPoint(publicKey) ||
!isSignature(signature) ||
!inCurve(signature.R8) ||
!inCurve(publicKey) ||
BigInt(signature.S) >= subOrder
Expand All @@ -122,7 +117,7 @@ export function verifySignature(message: BigNumberish, signature: Signature, pub
}

// Convert the message to big integer.
message = utils.checkMessage(message)
message = checkMessage(message)

// Convert the signature values to big integers for calculations.
const _signature: Signature<bigint> = {
Expand Down Expand Up @@ -150,7 +145,7 @@ export function verifySignature(message: BigNumberish, signature: Signature, pub
* @returns A string representation of the packed public key.
*/
export function packPublicKey(publicKey: Point): string {
if (!utils.isPoint(publicKey) || !inCurve(publicKey)) {
if (!isPoint(publicKey) || !inCurve(publicKey)) {
throw new Error("Invalid public key")
}

Expand All @@ -163,19 +158,13 @@ export function packPublicKey(publicKey: Point): string {
/**
* Unpacks a public key from its packed string representation back to its original point form on the Baby Jubjub curve.
* This function checks for the validity of the input format before attempting to unpack.
* @param publicKey The packed public key as a string or bigint.
* @param publicKey The packed public key as a bignumberish.
* @returns The unpacked public key as a point.
*/
export function unpackPublicKey(publicKey: BigNumber): Point<string> {
if (
typeof publicKey !== "bigint" &&
(typeof publicKey !== "string" || !isStringifiedBigint(publicKey)) &&
(typeof publicKey !== "string" || !isHexadecimal(publicKey))
) {
throw new TypeError("Invalid public key type")
}
export function unpackPublicKey(publicKey: BigNumberish): Point<string> {
requireBigNumberish(publicKey, "publicKey")

const unpackedPublicKey = unpackPoint(BigInt(publicKey))
const unpackedPublicKey = unpackPoint(bigNumberishToBigInt(publicKey))

if (unpackedPublicKey === null) {
throw new Error("Invalid public key")
Expand Down
33 changes: 12 additions & 21 deletions packages/eddsa-poseidon/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import { Point } from "@zk-kit/baby-jubjub"
import {
BigNumberish,
bigNumberishToBigInt,
bigNumberishToBuffer,
bufferToBigInt,
isBigNumberish,
isStringifiedBigint
} from "@zk-kit/utils"
import { isBigNumberish, requireTypes, type BigNumberish } from "@zk-kit/utils"
import { bigNumberishToBigInt, bigNumberishToBuffer, bufferToBigInt } from "@zk-kit/utils/conversions"
import { isArray, isObject, isStringifiedBigInt } from "@zk-kit/utils/type-checks"
import { Signature } from "./types"

/**
Expand All @@ -29,7 +24,7 @@ export function pruneBuffer(buff: Buffer): Buffer {
* @returns True if the object is a valid point, false otherwise.
*/
export function isPoint(point: Point): boolean {
return Array.isArray(point) && point.length === 2 && isStringifiedBigint(point[0]) && isStringifiedBigint(point[1])
return isArray(point) && point.length === 2 && isStringifiedBigInt(point[0]) && isStringifiedBigInt(point[1])
}

/**
Expand All @@ -39,11 +34,11 @@ export function isPoint(point: Point): boolean {
*/
export function isSignature(signature: Signature): boolean {
return (
typeof signature === "object" &&
isObject(signature) &&
Object.prototype.hasOwnProperty.call(signature, "R8") &&
Object.prototype.hasOwnProperty.call(signature, "S") &&
isPoint(signature.R8) &&
isStringifiedBigint(signature.S)
isStringifiedBigInt(signature.S)
)
}

Expand All @@ -53,15 +48,13 @@ export function isSignature(signature: Signature): boolean {
* @returns The private key as a Buffer.
*/
export function checkPrivateKey(privateKey: BigNumberish): Buffer {
requireTypes(privateKey, "privateKey", ["bignumberish", "string"])

if (isBigNumberish(privateKey)) {
return bigNumberishToBuffer(privateKey)
}

if (typeof privateKey !== "string") {
throw TypeError("Invalid private key type. Supported types: number, bigint, buffer, string.")
}

return Buffer.from(privateKey)
return Buffer.from(privateKey as string)
}

/**
Expand All @@ -70,13 +63,11 @@ export function checkPrivateKey(privateKey: BigNumberish): Buffer {
* @returns The message as a bigint.
*/
export function checkMessage(message: BigNumberish): bigint {
requireTypes(message, "message", ["bignumberish", "string"])

if (isBigNumberish(message)) {
return bigNumberishToBigInt(message)
}

if (typeof message !== "string") {
throw TypeError("Invalid message type. Supported types: number, bigint, buffer, string.")
}

return bufferToBigInt(Buffer.from(message))
return bufferToBigInt(Buffer.from(message as string))
}
6 changes: 3 additions & 3 deletions packages/eddsa-poseidon/tests/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ describe("EdDSAPoseidon", () => {

const fun = () => derivePublicKey(privateKey as any)

expect(fun).toThrow("Invalid private key type.")
expect(fun).toThrow(`Parameter 'privateKey' is none of the following types: bignumberish, string`)
})

it("Should sign a message (bigint)", async () => {
Expand Down Expand Up @@ -140,7 +140,7 @@ describe("EdDSAPoseidon", () => {

const fun = () => signMessage(privateKey, message as any)

expect(fun).toThrow("Invalid message type.")
expect(fun).toThrow(`Parameter 'message' is none of the following types: bignumberish, string`)
})

it("Should verify a signature", async () => {
Expand Down Expand Up @@ -241,7 +241,7 @@ describe("EdDSAPoseidon", () => {
it("Should not unpack a public key if the public key type is not supported", async () => {
const fun = () => unpackPublicKey("e")

expect(fun).toThrow("Invalid public key")
expect(fun).toThrow(`Parameter 'publicKey' is not a bignumber-ish`)
})

it("Should not unpack a public key if the public key does not correspond to a valid point on the curve", async () => {
Expand Down
9 changes: 9 additions & 0 deletions packages/utils/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,13 @@ or [JSDelivr](https://www.jsdelivr.com/):

## 📜 Usage

```typescript
// You can import modules from the main bundle.
import { errorHandlers, typeChecks } from "@zk-kit/utils"

// Or by using conditional exports.
import { requireNumber } from "@zk-kit/utils/error-handlers"
import { isNumber } from "@zk-kit/utils/type-checks"
```

For more information on the functions provided by `@zk-kit/utils`, please refer to the [documentation](https://zkkit.pse.dev/modules/_zk_kit_utils.html).
31 changes: 31 additions & 0 deletions packages/utils/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,36 @@
"types": "./dist/types/index.d.ts",
"require": "./dist/index.cjs",
"default": "./dist/index.js"
},
"./error-handlers": {
"types": "./dist/types/error-handlers.d.ts",
"require": "./dist/lib.commonjs/error-handlers.cjs",
"default": "./dist/lib.esm/error-handlers.js"
},
"./type-checks": {
"types": "./dist/types/type-checks.d.ts",
"require": "./dist/lib.commonjs/type-checks.cjs",
"default": "./dist/lib.esm/type-checks.js"
},
"./conversions": {
"types": "./dist/types/conversions.d.ts",
"require": "./dist/lib.commonjs/conversions.cjs",
"default": "./dist/lib.esm/conversions.js"
},
"./packing": {
"types": "./dist/types/packing.d.ts",
"require": "./dist/lib.commonjs/packing.cjs",
"default": "./dist/lib.esm/packing.js"
},
"./scalar": {
"types": "./dist/types/scalar.d.ts",
"require": "./dist/lib.commonjs/scalar.cjs",
"default": "./dist/lib.esm/scalar.js"
},
"./f1-field": {
"types": "./dist/types/f1-field.d.ts",
"require": "./dist/lib.commonjs/f1-field.cjs",
"default": "./dist/lib.esm/f1-field.js"
}
},
"files": [
Expand All @@ -38,6 +68,7 @@
"devDependencies": {
"@rollup/plugin-terser": "^0.4.4",
"@rollup/plugin-typescript": "^11.1.6",
"@types/snarkjs": "^0",
"rimraf": "^5.0.5",
"rollup": "^4.12.0",
"rollup-plugin-cleanup": "^3.2.1",
Expand Down
36 changes: 27 additions & 9 deletions packages/utils/rollup.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,30 @@ const banner = `/**
* @see [Github]{@link ${pkg.homepage}}
*/`

export default {
input: "src/index.ts",
output: [
{ file: pkg.exports["."].require, format: "cjs", banner },
{ file: pkg.exports["."].default, format: "es", banner }
],
external: [],
plugins: [typescript({ tsconfig: "./build.tsconfig.json" }), cleanup({ comments: "jsdoc" })]
}
export default [
{
input: "src/index.ts",
output: [
{ file: pkg.exports["."].require, format: "cjs", banner },
{ file: pkg.exports["."].default, format: "es", banner }
],
plugins: [typescript({ tsconfig: "./build.tsconfig.json" }), cleanup({ comments: "jsdoc" })]
},
{
input: "src/index.ts",
output: [
{
dir: "./dist/lib.commonjs",
format: "cjs",
banner,
preserveModules: true,
entryFileNames: "[name].cjs"
},
{ dir: "./dist/lib.esm", format: "es", banner, preserveModules: true }
],
plugins: [
typescript({ tsconfig: "./build.tsconfig.json", declaration: false, declarationDir: undefined }),
cleanup({ comments: "jsdoc" })
]
}
]
13 changes: 4 additions & 9 deletions packages/utils/src/conversions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
* the order of bytes is always big-endian.
*/

import { BigNumberish } from "./types"
import { isHexadecimal, isStringifiedBigint } from "./number-checks"
import { isBigInt, isHexadecimal, isNumber, isStringifiedBigInt } from "./type-checks"
import { BigNumber, BigNumberish } from "./types"

/**
* Converts a bigint to a hexadecimal string.
Expand Down Expand Up @@ -161,13 +161,8 @@ export function bigIntToBuffer(n: bigint): Buffer {
* @returns The bigint representation of the BigNumberish value.
*/
export function bigNumberishToBigInt(n: BigNumberish): bigint {
if (
typeof n === "number" ||
typeof n === "bigint" ||
(typeof n === "string" && isStringifiedBigint(n)) ||
(typeof n === "string" && isHexadecimal(n))
) {
return BigInt(n)
if (isNumber(n) || isBigInt(n) || isStringifiedBigInt(n) || isHexadecimal(n)) {
return BigInt(n as BigNumber | number)
}

return bufferToBigInt(n as Buffer)
cedoor marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading
Loading