refactor(eddsa-poseidon)!: restrict message types #318
Conversation
BREAKING CHANGE: message type re privacy-scaling-explorations#230
There was a problem hiding this comment.
I think this change needs to include functional code not just types and tests. In particular the checkMessage function is where the input needs to be validated and converted appropriately.
Also welcome to the team and thanks for picking up this PR. I'm a thorough and detail-oriented reviewer. Don't take that as being unwelcoming. :)
| @@ -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" | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Imports could be values or types. TS can infer what kind of import that is but the type keyword here makes it more explicit.
| @@ -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> { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()).
| @@ -96,7 +96,7 @@ describe("EdDSAPoseidon", () => { | |||
| it("Should sign a message (hexadecimal)", async () => { | |||
| const message = "0x12" | |||
|
|
|||
| const signature = signMessage(privateKey, message) | |||
| const signature = signMessage(privateKey, BigInt(message)) | |||
There was a problem hiding this comment.
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.
| @@ -117,7 +117,7 @@ describe("EdDSAPoseidon", () => { | |||
| 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 () => { | |||
There was a problem hiding this comment.
I think this case shouldn't be supported without an explicit conversion to bigint
| 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") |
There was a problem hiding this comment.
This test is failing on GitHub
|
awesome!! thanks @artwyman for your feedback. I am going to work through your comments right away. |
|
Hi @f3r10, are you still going to work on this? |
|
My apologies, @cedoor. I started a new job and have been really busy. I hope to contribute once I settle into it. |
Description
The type supported for the EdDSA message has been reduced to BigNumber (either bigint or string which must still be a stringified bigint))
Related Issue(s)
Closes #230
Checklist
yarn stylewithout getting any errors