Change cryptography module to TypeScript - Discussion for #417

[shuse2]

Discussion for #417

[tobiaslins]

I think there was no conclusion about which type system LiskHQ wants to use.

We should discuss first in #417 and after that we should implement it in the chosen technology!

[willclarktech]

@tobiaslins This PR is one of two which were created to aid that discussion, and was not intended to be merged as is. See also #556

Feel free to review the PR if you have comments to make!

[blotzu]

Although the change looks quite benign it can introduce some fake safety nets.
Some of the methods are used by in other places. All callers should be updated in order to avoid calling a method that requires a string with something else.
Example:
getPrivateAndPublicKeyFromPassphrase is called by signRawTransaction without any argument type checks

Should enforce argument types in the signRawTransaction() method as well or add manual type checks since Flow/TypeScript will not complain about this partial change.

A more safe approach would be to add unit tests to all changed methods and their callers first in another PR. This would ensure that no non-obvious issues are introduced.
The existing test code would need some refactoring in order to make it compatible with the Flow or TypeScript types.
In order to ensure that the change can be done gradually and without introducing fake safety nets (like in the above example), the changes should start with the modules that have the least amount of internal callers. In this case signRawTransaction() would have to be refactored before getPrivateAndPublicKeyFromPassphrase().

Overall using automated type checking is a good idea, however it can lead to potentially dangerous situations when the type checks are only left to the transpiler.
There is no way to be sure that the original caller passed the correct type of object as long as these types are not enforced by the language or by having explicit checks in the code.

Both Flow and TypeScript require some processing before the code can be executed as JavaScript.
This adds extra dependencies to the project an extra step to the build process.
For Flow, an easy workaround would be to comment out all annotations. The main downside is that this would make the code harder to read.
This would mean however that the existing code could still be run in a browser without any other modifications and the normal development flow would remain unchanged.

Switching to Flow or TypeScript will not solve all problems.
Any project that includes this package might still send the wrong argument types to all exported modules.
A possible solution would be to use flow-runtime/ts-runtime in order to check the values at runtime and avoid these kind of issues altogether.
This might degrade the performance though.

[shuse2]

Thank you for the comment! @blotzu

I agree that adding/swithching flow or typescript will not solve all the problem.
I think main reason to use flow or typescript is to reduce error within the module.
However, additionally, it will help Typescript/flow compatible module to use the definitions.

Currently all of the module have babel compilation, therefore, there will not be big difference in terms of build process.

No matter how it is supportive these tools are, all of the function should be / will be covered by unit test.
Therefore, the behavior of each function should be assure by that, too.