Panic if the server does not use TBs #111
Conversation
|
Merge this after #108 is merged? |
Yes (https://github.com/coniks-sys/coniks-go/blob/master/keyserver/server.go#L112).
Yes, I think so. |
| @@ -80,6 +80,13 @@ func TestRegisterExistedUserWithTB(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestRegisterWithoutTB(t *testing.T) { | |||
| // workaround for #110 | |||
There was a problem hiding this comment.
I'm wondering how quickly these WithoutTB tests are going to rot?
There was a problem hiding this comment.
Sorry, I didn't really understand your question. These tests should panic and rot right after directory initialization. Should we remove these tests?
There was a problem hiding this comment.
There was a problem hiding this comment.
I mean that since we're panicking early, the rest of the code won't be executed and will likely accumulate bugs or otherwise be unuseful over time.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah, thanks for teaching me this jargon.
Sorry, I should be more careful with idioms.
From my side, I think we would never run a key server without signed promises, so do other developers. Thus, removing these tests and keeping useTBs until we address #89 seem fine to me.
I'm ok with this reasoning.
There was a problem hiding this comment.
Another option might be to truncate everything after where the test is expected to panic. We probably want to keep tests around that assert the panic so that doesn't regress.
There was a problem hiding this comment.
From my side, I think we would never run a key server without signed promises, so do other developers. Thus, removing these tests and keeping useTBs until we address #89 seem fine to me.
I agree, at least for now, we won't be running a key server that doesn't use TBs (and should recommend the same to developers), but I think this would mean removing useTBs instead.
Another option might be to truncate everything after where the test is expected to panic. We probably want to keep tests around that assert the panic so that doesn't regress.
I agree with this, as long as we have useTBs, we should keep the tests that are expected to panic, but we can truncate the rest of the tests.
| @@ -17,6 +17,12 @@ type ConiksDirectory struct { | |||
|
|
|||
| func NewDirectory(epDeadline merkletree.TimeStamp, vrfKey vrf.PrivateKey, | |||
| signKey sign.PrivateKey, dirSize uint64, useTBs bool) *ConiksDirectory { | |||
|
|
|||
| // Fix me: see #110 | |||
There was a problem hiding this comment.
Can you use the canonical FIXME? Makes it a bit easier to grep.
|
This might be a little to late, but wouldn't it be more consequent to make TBs non-optional (and completely remove |
|
Probably, but TBs were a good candidate to flesh out #89 |
|
LGTM |
|
@liamsi What do you think about this pull? |
|
LGTM with the truncated test. |
Workaround for #110