Closed AverageHelper closed 1 year ago
Considering that a port to TypeScript involves moving the source code into a subfolder, I don't know a good way to break this PR down for easier reviewing. Any incremental change from the original to here would almost certainly involve changes that look massive in a git diff.
I'm still learning, though. Any advice or critique here would be much appreciated!
I've opened a PR on your fork with improvements I'd like to suggest**.
** -- I am not affiliated with the maintainers.
Thanks for the interest - this is great! We have been writing our other Nth Party libraries in TypeScript for a while now, so it's good to bring OPAQUE up to the times. Thanks for being patient while I reviewed.
I stepped though the protocol once more and can confirm all steps remain the same. I don't think we need the !(x instanceof Uint8Array)
before sodium.is_zero
(because crypto_core_ristretto255_from_hash
always returns a Uint8Array
), but everything looks good enough to merge.
Style-wise (in TypeScript and for readability), we should rename most math variables (_H1_x_
vs H1_x
and x
vs X
) as issue #3 suggests, which I will do sometime after this merge, unless someone beats me to it.
@tvquizphd I'm going to merge what's on @AverageHelper's fork. I'd like to merge your changes as well, so please (re)open a PR. Thanks both of you!
Some exported types would be handy for TypeScript projects to more easily consume this library. There doesn't seem to be one in DefinitelyTyped yet. I started to create some types, but ended up porting the library to TypeScript anyway in order to learn the I/O enough to write that interface.
In the interest of stability, I think a TypeScript port makes sense anyway, so here's a PR.
My goal was to change the runtime logic as little as possible. The tests still pass, and coverage is virtually identical. Some exceptions:
==
, preferring instead stricter comparisons (like===
) or clearer falsy checks using!
.instanceof
check before passing a value tosodium.is_zero
. The source for that method would have thrown aTypeError
at runtime, so TypeScript's assessment seemed reasonable enough to warrant the additional check here.sodium
parameter. TheOPRF
constructor installs its own anyway, and did not accept the parameter we were passing there.async
. It returned aPromise
before, whose initializer received anasync
function anyway, so removing thatPromise
constructor and bubbling up theasync
declaration seemed a reasonable improvement.I'm no mathematician. If I somehow messed up the algorithm here, please tell me.