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

Jump to bottom

refactor(eddsa-poseidon)!: restrict message types #318

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(eddsa-poseidon)!: restrict message types #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 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,7 +9,7 @@ import {
subOrder,
unpackPoint
} from "@zk-kit/baby-jubjub"
import type { BigNumberish } from "@zk-kit/utils"
import type { BigNumber, BigNumberish } from "@zk-kit/utils"
import { crypto, requireBuffer } from "@zk-kit/utils"
import { bigNumberishToBigInt, leBigIntToBuffer, leBufferToBigInt } from "@zk-kit/utils/conversions"
import { requireBigNumberish } from "@zk-kit/utils/error-handlers"
Expand Down Expand Up @@ -87,7 +87,7 @@ export function derivePublicKey(privateKey: Buffer | Uint8Array | string): Point
* @param message The message to be signed.
* @returns The signature object, containing properties relevant to EdDSA signatures, such as 'R8' and 'S' values.
*/
export function signMessage(privateKey: Buffer | Uint8Array | string, message: BigNumberish): Signature<bigint> {
export function signMessage(privateKey: Buffer | Uint8Array | string, message: BigNumber): Signature<bigint> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the intent here is to change not only the types but the handling and checking of the inputs here. A BigNumber is limited to bigint or a stringified bigint (not an arbitrary string, not a buffer). The checkMessage function is the place where you should be checking that and converting to bigint appropriately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also per @cedoor's comment on #230 there should be some documentation above about the fact that the message is a single BabyJub field element, represented as a bigint. Best-practice for users with other types of input is probably to hash it (e.g. with a poseidon hash), or encode their inputs bytes directly if small enough (e.g. using bufferToBigInt()).

// Convert the private key to buffer.
privateKey = checkPrivateKey(privateKey)

Expand Down Expand Up @@ -121,7 +121,7 @@ export function signMessage(privateKey: Buffer | Uint8Array | string, message: B
* @param publicKey The public key associated with the private key used to sign the message.
* @returns Returns true if the signature is valid and corresponds to the message and public key, false otherwise.
*/
export function verifySignature(message: BigNumberish, signature: Signature, publicKey: Point): boolean {
export function verifySignature(message: BigNumber, signature: Signature, publicKey: Point): boolean {
if (
!isPoint(publicKey) ||
!isSignature(signature) ||
Expand Down Expand Up @@ -281,7 +281,7 @@ export class EdDSAPoseidon {
* @param message The message to be signed.
* @returns The signature of the message.
*/
signMessage(message: BigNumberish): Signature<bigint> {
signMessage(message: BigNumber): Signature<bigint> {
return signMessage(this.privateKey, message)
}

Expand All @@ -291,7 +291,7 @@ export class EdDSAPoseidon {
* @param signature The signature to be verified.
* @returns True if the signature is valid for the message and public key, false otherwise.
*/
verifySignature(message: BigNumberish, signature: Signature): boolean {
verifySignature(message: BigNumber, signature: Signature): boolean {
return verifySignature(message, signature, this.publicKey)
}
}
2 changes: 1 addition & 1 deletion packages/eddsa-poseidon/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Point } from "@zk-kit/baby-jubjub"
import type { BigNumberish } from "@zk-kit/utils"
import { type BigNumberish } from "@zk-kit/utils"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I honestly don't know what this type keyword is for. The build works fine for me locally if I simply remove it entirely, so I suggest doing that if it passes the GitHub checks.

Copy link
Member

Choose a reason for hiding this comment

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

Imports could be values or types. TS can infer what kind of import that is but the type keyword here makes it more explicit.

import { bigNumberishToBigInt, bufferToBigInt } from "@zk-kit/utils/conversions"
import { requireTypes } from "@zk-kit/utils/error-handlers"
import { isArray, isBigNumber, isBigNumberish, isObject } from "@zk-kit/utils/type-checks"
Expand Down
17 changes: 12 additions & 5 deletions packages/eddsa-poseidon/tests/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { babyjub, eddsa } from "circomlibjs"
import { Buffer } from "buffer"
import { crypto } from "@zk-kit/utils"
import { bufferToBigInt, crypto } from "@zk-kit/utils"
import { utils } from "ffjavascript"
import { r, packPoint, Point } from "@zk-kit/baby-jubjub"
import {
Expand Down Expand Up @@ -84,7 +84,7 @@
it("Should sign a message (number)", async () => {
const message = 22

const signature = signMessage(privateKey, message)
const signature = signMessage(privateKey, BigInt(message))

const circomlibSignature = eddsa.signPoseidon(privateKey, BigInt(message))

Expand All @@ -96,7 +96,7 @@
it("Should sign a message (hexadecimal)", async () => {
const message = "0x12"

const signature = signMessage(privateKey, message)
const signature = signMessage(privateKey, BigInt(message))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a stringified bigint, so should be accepted as a BigNumber without explicit conversion.

I also suggest adding another test case with a decimal string like "123" which should also work. Anything parsable by BigInt(s) should work.


const circomlibSignature = eddsa.signPoseidon(privateKey, BigInt(message))

Expand All @@ -108,7 +108,7 @@
it("Should sign a message (buffer)", async () => {
const message = Buffer.from("message")

const signature = signMessage(privateKey, message)
const signature = signMessage(privateKey, bufferToBigInt(message))

const circomlibSignature = eddsa.signPoseidon(privateKey, BigInt(`0x${message.toString("hex")}`))

Expand All @@ -117,7 +117,7 @@
expect(signature.S).toBe(circomlibSignature.S)
})

it("Should sign a message (string)", async () => {
it("Should sign a message if less than 32 bytes (string)", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this case shouldn't be supported without an explicit conversion to bigint

const message = "message"

const signature = signMessage(privateKey, message)
Expand All @@ -129,6 +129,13 @@
expect(signature.S).toBe(circomlibSignature.S)
})

it("Should fail if message is larger than 32 bytes (string)", async () => {
const message = bufferToBigInt(Buffer.from(crypto.getRandomValues(34)))

const fun = () => signMessage(privateKey, message)
expect(fun).toThrow("Size 32 is too small, need at least 33 bytes")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is failing on GitHub

})

it("Should throw an error if the message type is not supported", async () => {
const message = true

Expand Down Expand Up @@ -346,7 +353,7 @@
expect(fun).toThrow("Invalid packed signature point")
})

it("Should still unpack a packed signature with scalar malformed", async () => {

Check warning on line 356 in packages/eddsa-poseidon/tests/index.test.ts

View workflow job for this annotation

GitHub Actions / style 

Test has no assertions
const signature = signMessage(privateKey, message)

const packedSignature = packSignature(signature)
Expand Down
Loading