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.

[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 #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]

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