Simplify verification of KeyFrag in WASM bindings

[fjarri]

wasm-bindgen does not support Option<&MyType> parameters, and Option<MyType> have undesired side effects on the JS side (see https://github.com/rustwasm/wasm-bindgen/issues/2370). So instead of one verify() with optional parameters we have to have four methods. Fix it when the blocker is fixed, or find a way to make it less cumbersome.

[vepkenez]

I guess the plan for this one is to call one of plain verify, verifyWithDelegatingKey, verifyWithReceivingKey, or verifyWithDelegatingAndReceivingKeys based on the arguments?

[vepkenez]

*The python version of this https://github.com/nucypher/pyUmbral/blob/18e868ceb2030a2db0ad89dcfeb9d47247cead6b/umbral/key_frag.py#L214

      def verify(self,
               verifying_pk: PublicKey,
               delegating_pk: Optional[PublicKey] = None,
               receiving_pk: Optional[PublicKey] = None,
               ) -> 'VerifiedKeyFrag':`

Has these optional kwargs... I think the only way to do this in JS is to accept an object like

KeyFrag.verify({verifying_pk: required_pk, delegating_pk: some_pk, receiving_pk: null})

does this seem ok?

[fjarri]

Is that a common pattern in JS to handle optional arguments? Another possible variant is to just accept null as a value, so you could write e.g.

kfrag.verify(verifying_pk, delegating_pk, null)

but I guess that is more error-prone.

[vepkenez]

Well the main difference with that would be that the arguments need to be positional in that case.

It is fairly normal to take an object as “**kwargs”. It will just need to be slightly different.

On Mon, Jul 19, 2021 at 9:35 PM Bogdan Opanchuk @.***> wrote:

Is that a common pattern in JS to handle optional arguments? Another possible variant is to just accept null as a value, so you could write e.g.

kfrag.verify(verifying_pk, delegating_pk, null)

but I guess that is more error-prone.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/nucypher/rust-umbral/issues/25#issuecomment-883045350, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABODDFFOED6HRPHEMDESFLTYT4I3ANCNFSM4UG3YTAQ .

[fjarri]

If it's the standard way to deal with kwargs, then that'll work. verifying_pk is not optional though, so it can be positional.

[vepkenez]

Yeah it could be (verifying_pk, {delegating_pk: null, receiving_pk: null}) maybe @piotr-roslaniec has an opinion about this?

[piotr-roslaniec]

I think using objects as named parameters is not considered "best practice" as it's more pricey than passing primitives. However, there are cases where we want to provide the caller with extra safety (a lot of positional/optional parameters, security concerns, hard to debug behaviors).

That being said, I think this is a completely acceptable solution in this case:

function verify((verifying_pk, {delegating_pk, receiving_pk} = {}) {}

If verify, verifyWithDelegatingKey, verifyWithReceivingKey, or verifyWithDelegatingAndReceivingKeys had fundamentally different behaviors (which I don't think they have) I would suggest having them as separate functions.

[vepkenez]

thanks @piotr-roslaniec @fjarri I think I can go forward with this consensus! I like this process. :)

[fjarri]

For now we're using a custom type (OptionPublicKey, declared as TS PublicKey | null) and a manual dynamic conversion using wasm-bindgen-derive. This removes the need in repeating the methods on TS side, and we only have a single method

verify(verifying_pk: PublicKey, delegating_pk: PublicKey | null, receiving_pk: PublicKey | null): VerifiedKeyFrag;

When wasm-bindgen fixes the underlying issue, the internals can be updated too.