midonet / tssrp6a

This library is a dependency free TypeScript implementation of Secure Remote Password SRP6a.
MIT License
36 stars 8 forks source link

Alt de/serialization of steps and test #78

Closed jbis9051 closed 3 years ago

jbis9051 commented 3 years ago

Alternative approach for #77

Fixes #71

Closes #77

jbis9051 commented 3 years ago

@bgrosse-midokura Here's an alternative approach. It requires exporting all the step classes. I can change it to have 1 fromState method, but this way is more explicit.

bgrosse-midokura commented 3 years ago

Nice alternative. I think the tradeoff is now clear: either we add the code into the step classes, or we do some hacky almost-property-guessing (my PR).

I think your PR is less surprising and easier to discover for the user. We already have the "routines" separated from the step classes anyway, so we're not mixing too many concerns in the step classes.

I would like to see if we can avoid exporting/exposing those "state" types, I don't think it provides any actual type-safety.

bgrosse-midokura commented 3 years ago

So I would be happy to merge this instead of #77, but this is worthy a small note in the readme, similar to https://github.com/midonet/tssrp6a/pull/77/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R120

jbis9051 commented 3 years ago

So I would be happy to merge this instead of #77, but this is worthy a small note in the readme, similar to https://github.com/midonet/tssrp6a/pull/77/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R120

I've added a similar note and expanded on some things

bgrosse-midokura commented 3 years ago

So I would be happy to merge this instead of #77, but this is worthy a small note in the readme, similar to https://github.com/midonet/tssrp6a/pull/77/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R120

I've added a similar note and expanded on some things

Amazing!

bgrosse-midokura commented 3 years ago

@jbis9051 is it ready to be merged?

jbis9051 commented 3 years ago

@bgrosse-midokura I think so.