Move optional arguments after mandatory arguments

[davegomez]

These changes switch the position of the obj and hmac_key arguments to move the optional argument to the end of the list.

To accomplish this I created a parameter validation function to assert the order of the arguments keeping backward compatibility, the function also displays a deprecation warning that can be removed when we consider appropriate.

Closes issue #67.

cc/ @christianbundy

[staltz]

This is a decently written PR @davegomez, but I'm still worried about these changes that I mentioned here. Not only would we change the semantics of ssb-keys, we would also have to consider making breaking changes to ssb-validate which has similar function arguments. Because verifyObj might be in the hot path, it might be important for performance, so an additional validateParameters might have a negative performance impact.

So a breaking change plus a possible performance tax are negatives that in my opinion are not enough to be greater than the positives (minor developer experience improvement). Not a net positive in my view, unfortunately.

[davegomez]

Well, I'm not sure about the performance because validateParameters is doing exactly the same this line is doing if (hmac_key) b = hmac(b, u.toBuffer(hmac_key)); so it's not like we are including additional conditions, maybe just the OR (isBuffer(obj) || isString(obj) || obj === null)) which shouldn't be a huge load.

This changes are backwards compatible, would that be breaking?
Even when you can still use the old syntax without any issue even in ssb-validate?

[staltz]

Oh, now I see that validateParameters not only validates but also swaps the arguments. Hmm, let me think about this a bit more.

Sorry for the misunderstanding.

[staltz]

Okay, I think this is acceptable then, if backwards compatibility is kept. I just want to make sure this new change still supports the weird corner cases, I'll mention a few inline.

[davegomez]

That's right.

These changes in fact are not doing anything different than the current code, but just improving the validation of the arguments and the swapping.

I also added test to guarantee old code will keep working as well as new code with the new order.

[staltz]

One thing I noticed is that the old API supported verifyObj(keys, undefined, obj) but if you're checking for a null, then the undefined case will not go inside this if, and that would be a breaking change. So I think it's safer to just check if the second argument is falsy.

By the way, it would be easier to understand this function's implementation if the arguments were called first and second.

[staltz]

Also, to be sure, it would be good to have tests for all these weird cases. (In fact, good to have tests for these weird cases before this PR #80 is merged, to ensure backwards compat)

[davegomez]

Ha! I had undefined in my initial code and I didn't keep it because I couldn't find a usage example with undefined.

I'll tidy up the tests and update the PR.

[staltz]

Good @davegomez, also note it would be useful to have a separate PR for tests that should be true for the current main branch codebase.

[davegomez]

By the way, it would be easier to understand this function's implementation if the arguments were called first and second.

While doing this I thought that is important for the function to differentiate what we are doing here. Because this is not a generalistic function used to swap two parameters but is instead of checking for very specific conditions for each of the parameters.

I added a comment and docs to clarify what is happening instead.

[davegomez]

@staltz I updated the PR with the changes discussed here.

[davegomez]

@staltz WAIT! Don't merge yet. I forgot a couple of changes.

[davegomez]

@staltz The PR is good to go.

[staltz]

@davegomez Tests aren't passing in CI.

[davegomez]

@davegomez Tests aren't passing in CI.

This is a very interesting case.

The following lines are using a Buffer created with Buffer.alloc.
https://github.com/ssb-js/ssb-keys/pull/80/files#diff-910eb6f57886ca16c136101fb1699231R113-R114
https://github.com/ssb-js/ssb-keys/pull/80/files#diff-910eb6f57886ca16c136101fb1699231R175-R176

These buffers are always filled with 0s, creating the same value over and over again, which is not safe to use as a symmetric key for HMAC authentication.

We should discourage this method of HMAC creation informing about the possible issue in the documentation.

[davegomez]

PR Update

• I resolved the conflicts cause by the merge of the PR add tests for sign and verify when hmac_key is falsy #83.
• I added new tests for falsy hmac_key values using the new arguments' order.
• I fixed the commit list to make it easier to navigate.

[staltz]

I double appreciate internal documentation with JSDoc 👍

[staltz]

@davegomez It looks ready in my opinion, I'm going to ask for someone else to review as well.

cc @christianbundy @mixmix @soapdog

[staltz]

Hmm, I realize this support for both ways will complicate ssb-neon-keys' compatibility with ssb-keys, and also overall it might be very confusing that both argument orders are magically supported. I'm still somewhat on the fence with this change, in my opinion it doesn't make the code overall better (this doesn't mean I will block it from happening, just sharing my feelings about it).

[davegomez]

Isn't ssb-neon-keys already supporting both way as the current version of ssb-keys does?

Remember the value swapping is not new, is just not this detailed and robust, but the first line of both functions is swapping the values in the case the hmac key is not present.

[staltz]

Isn't ssb-neon-keys already supporting both way as the current version of ssb-keys does?

Yeah, the thing is I realized that ssb-neon-keys doesn't do the swapping. One obstacle I have is that I don't know how to (or maybe we can't) check for falsy values within Rust / Neon.

[davegomez]

I faced the same issue with Swift and decided not to support the current order, but go with using the hmac key as the last argument, providing support only to "new implementations."

[mixmix]

You can ask for rust help @staltz

I've looked over this and think it's well tested and documented. It's the middle of the night here so I would like another person to check closely (could be me tmrw!), as you don't crypto after midnight.

[mixmix]

Ok more thinking :

• I'm fairly strongly against magical argument fixing in such important low level apis.
• let's do a breaking change!

Why? I would hazard a guess that these methods are used less than a dozen times across all repos and those instances would be easy to find.

We could surface them very quickly with a loud throw for incorrect usage.

[staltz]

Why? I would hazard a guess that these methods are used less than a dozen times across all repos and those instances would be easy to find.

Ok, I think we should not publish this breaking change version before we do the investigative work to highlight all usage cases that we can identify in the wild. And then we should rather atomically update all usages.

[mixmix]

Let's start a collaborative checklist of modules we want to check:

• recursively search ssb-server
• recursively search patchwork
• ask some maintainers:
  • cel
  • arj
  • christian

feel free to edit this message

[soapdog]

I'm afraid of breaking changes like this in such a low-level package. I understand and appreciate the desire of the PR to move optional arguments after mandatory ones but I'd rather have the code that figures out if the module is being called in legacy mode and fix the call than to introduce a breaking change.

Still, I don't want to block this if others feel like this is the way forward. I just wanted to voice that I'm OK with the magical code in the name of keeping it working for the current users. If it is documented well enough and maintainers are encouraged to refactor to the new order, then once they refactor we can introduce the breaking change. Basically for me it needs to happen backwards:

1 - introduce magical code (aka merge this PR).
2 - ask developers to refactor to the new order.
3 - after a verifiable number of projects are working with the new order including the major clients and servers, introduce the breaking change.

[staltz]

@soapdog Sounds like a good plan, we can minimize the time this magic is used in production.

[mixmix]

I'm good with that idea. If like to decide

• to console.warn when old usage
• to decide a date we would like to aim to remove the magical code

(how about a throw which triggers after a certain unix time?! I've always been fascinated with the idea of code which expires. Actually maybe the magical code just doesn't run after a certain date, that would be fun)