enforce config.keys in TS and log an error when no keys are provided

[austinfrey]

reference: #59

[mixmix]

I like this. I would hazard you should probably throw instead of printing an error.

I'd also like to see a test around this - you may find you never see this log because toSodium could error loudly if things go wrong internally to that? (haven't looked at it)

[austinfrey]

@mixmix appreciate the review. the change has been made to throw rather than log. trying to think through the testing changes needed and could use some additional feedback.

this is what I've come up with so far to add a test case using the shs plugin directly, as catching the error when creating a new server catches the missing keys error but throws again when core.js goes to load and the shs transform is missing. Would the following suffice?

tape('throw error when no keys are supplied', function (t) {
  var shsPlugin = require('../lib/plugins/shs')
  var api = {}
  var config = {}

  // can't use create() here as catching the error for missing
  // keys causes another error downstream in ../lib/core where
  // the shs plugin is missing 'secret-stack needs at least 1 
  // transform protocol'
  try {
    shsPlugin.init(api, config)
  } catch (err) {
    t.ok(err, err.message)
  } 
  t.end()
})

Should this PR be enhanced to throw only when both keys and a seed are missing? That would save us from updating all the tests.

cc @staltz

[mixmix]

I am not familiar enough with this code but expect that both keys need to be there... Or at least the public key.

Your test looks really scoped and sufficient to me. BTW you have essentially recreated tapes t.throws

You could write it like this

t.throws(
  () => plugin.init(ssb, config), 
  /shs plugin requires valid config.keys/
  "throws without keys"
) 

Seconds arg is a regex which will be used to test error.message on the error which is thrown

[austinfrey]

This has been updated to include 2 new tests. One which throws an error when neither keys nor a seed are passed into with the config. A second test is included that shows the publicKey is being correctly populated when passing just a seed.

This is usually accomplished in multiserver where it would use the keys and expose the public key. Unfortunately we can't wait for this to make it down to multiserver when throwing for missing keys at this level, since the publicKey is needed right away.

This seems like a good idea to preserve some backward compatibility regarding the use of seeds, but open to being told otherwise :). This also introduces another dependency, secret-handshake, to use the toKeys method to convert the seed to a keypair but maybe using chloride directly would make more sense.

[mixmix]

That latest seed commit is interesting, and I'd recommend pulling it out of this branch and putting it in another pr. This is because it's different feature, and so putting it in here forces us to wait til two features are discussed and reviewed. If they were separate we can review and merge faster (they're also not dependent).

Reviewing that seed change I would ask :

1. is this needed? (what's the problem you're experiencing that motivates this)
2. What happens when keys and seed are present?

[austinfrey]

@mixmix Looking at this again, I think I was getting some wires crossed. It appeared to me that passing in a seed would not populate an id or publicKey at the secret-stack level, but it clearly does. I've backed out the changes where the seed overwrites the config.keys value but I did keep a check to make sure one or the other is present.

I tried to update the Config type to require either a set of keys OR a seed, I'm not confident it's the right solution as I'm pretty green when it comes to typescript.

[mixmix]

I'm green too. I think it would be good to get @staltz to help complete this. I would also defer to his opinions if they conflict with anything I've said as he knows this code better than me.

Appreciate you putting energy into this
🍉🦋

[austinfrey]

@staltz I am attempting to create a Config type that requires one OR the other of a property. This makes it's way through the typescript compiler without issue, but wondering if there is a better solution.