New utility modules for handling errors and checking types

[cedoor]

Description

This PR adds two utility modules to the @zk-kit/package: error-handlers and type-checks. It also renames some modules and the isStringifiedBigint function to isStringifiedBigInt, which can be considered as a breaking change.

Related Issue(s)

Closes #177

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have run yarn prettier and yarn lint without getting any errors
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[artwyman]

I reviewed the code, but not configuration or tests. I have a few small suggestions to resolve in this PR. My comments also raise some more general concerns about the way flexible inputs may lead to bugs, but that discussion can be taken outside of this PR if you prefer.

[artwyman]

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.

[artwyman]

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.

[artwyman]

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.

[cedoor]

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.

[cedoor]

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?

[cedoor]

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?

[artwyman]

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.

[artwyman]

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).

[cedoor]

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.

[cedoor]

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.

[artwyman]

As discussed, I think this PR can go ahead, with the understanding that some of the ongoing discussions will be continued in new Issues/PRs.