Argon2 bindings

#1

Hi,

I’ve just released my first bindings - bs-argon2.
If anyone is interested or just willing to take a look, I would appreciate any feedback on my attempt to create an idiomatic interface to the node-argon2 library.

Specifically, there are two design decisions that I’d appreciate feedback on:

  1. The argon2 library exposes two objects (defaults, and limits), and I ended up parsing them on load into their own respective record types. I’m wondering whether there’s a more efficient way of making such objects be available idiomatically to reason code (fields that are variants, etc.).
  2. The hashing function can take either a string or a buffer to hash and returns a promise that contains either a string or a buffer - depending on the ‘raw’ option passed to it. I decided to implement two different functions - one for each return type and have both take a variant as an input.

Thanks,
leeor.

#2

Argon2 exports a TypeScript definition file: https://github.com/ranisalt/node-argon2/blob/master/argon2.d.ts . Perhaps it would make sense to follow that closely?

Regarding more idiomatic typing like variants and such, I see you’ve defined some variants type but are actually converting them to polymorphic variants in some calls, I would say there’s no need for that–just expose the polymorphic variant types directly.

More generally, I see that you are using fixed versions in your package.json, I would advise using semantic version ranges like "bs-platform": "^5.0.4", to prevent users from going through version resolution conflict headaches.

#4

Thanks for taking the time to look at this module!

I have followed the typescript definition file to create the "external"s, but my intention was to not simply create a one-to-one instance of the API in BS, but to create an API that is more fitting for the BS ecosystem. The typescript API is not accurate btw, it’s missing the “associatedData” option - typescript definitions only go so far.

I’m converting to polymorphic variants because, to my understanding of “[@bs.unwrap]” it is only meaningful when the whole definition of the variant is inlined into the external declaration. So I can’t define the variant prior to the external. Did I get that wrong? I think I’ve even tried that when writing another set of bindings, and couldn’t get that to work.

As for the versions, you’re right, I forgot to go back to the package.json dependencies and update the semver strings.

Thanks,
Leeor.

#5

Gotcha. And, noted about the TypeScript definition being incomplete.

About the polymorphic variants, you don’t need to define the type beforehand. You can directly inline it into the external, as you said, and expose that definition to the library user. See e.g. https://bucklescript.github.io/docs/en/function#trick-2-polymorphic-variant-bsunwrap . The binding for the padLeft function is exposed to the user, who can directly call it with the polymorphic variant values. No need to define a new variant type which delegates to the polymorphic variant type.

#6

I think I wasn’t sure the polymorphic variant being declared inline over and over again would be passed correctly from my function to the external. It works well, and the only con I have for the use of polymorphic variants here is that writing:

Argon2.hash(Argon2.`String("password"))

Results in a syntax error on that backtick. A local open fixes it, though.

Thanks again for your input.

#7

You don’t need a local open. Polymorphic variant constructors are ‘universal’, i.e. they exist across all modules. You can do Argon2.hash(`String("password"))

#8

I missed that bit about polymorphic variant scopes. That nature of polymorphic variants is implicit in most texts I’ve read that cover them, except for the 2ality article which I just went over again. I wonder if it should be made explicit in the BuckleScript or Reason docs that describe their usage for writing bindings.

Eventually, I think that for libraries, the use of polymorphic variants should be avoided where possible. For this library, I was trying to use them to reduce code duplication, but I think the risk involved is not worth the benefit of saving some lines. The tradeoff is to give them longer, more unique names or just leave them out.

The compiled code, using a variant and four different external definitions, is still very concise as all of the boilerplate is gone:

  let hashPromise =
    switch (input) {
    | String(str) => hashString(str, hashOptions)
    | Buffer(buf) => hashBuffer(buf, hashOptions)
  };

  hashPromise;

is compiled into:

return Argon2.hash(input[0], hashOptions);

So the only impact is the array access.

#9

What is the risk?

#10

The compiler will try to infer the types they take by creating an upper bound from the different usages.

So if two libraries define the same polymorphic variant using the same name but each will use the value it holds differently, let’s say one uses it as a string, the other as an int, the compiler will infer that the variant requires a type that is both string AND int. See this for a detailed example from the 2ality article.
Even if one library defines explicitly the type the polymorphic variant takes then the breakage will just occur earlier. You can see this if you use the example in the link but annotate x in even1 and/or even2. Merely trying to define even will then break.

In any case, I think that such pollution of the global scope should be avoided in libraries, and using regular variants has no other downsides I can see, it is also more concise than defining polymorphic variants with longer unique names.

#11

That’s an interesting example, thanks for that. In any case these polymorphic variant constructors with payloads are usually used as type tags, e.g. foo(`string("password")) or bar(`int(0)). So I’d say there’s a low likelihood that you’ll see something like `int("hello"). But, totally understand wanting to avoid the possibility. Btw, it’s also possible to use an abstract type here with zero-overhead conversions from different types to the target type, thanks to the %identity built-in.