Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Cryptographically Secure RNG #108

Closed
wants to merge 4 commits into from
Closed

Add Cryptographically Secure RNG #108

wants to merge 4 commits into from

Conversation

benjaminBrownlee
Copy link

As a follow up to issue #97, I have modified this library to accommodate a cryptographically secure RNG.

@benjaminBrownlee
Copy link
Author

It appears BigInteger.min.js failed some checks. Did I make a mistake while appending the minifed code?

@peterolson
Copy link
Owner

peterolson commented Sep 12, 2017

Hi, I'm looking at the commit and it looks like it only supports running in the browser, using window.crypto. BigInteger.js is used on both the browser and on Node, so we need to support both environments. Also, perhaps it should fallback to randBetween when there is no crypto library present.

The failing build says something about not being able to parse something, I'm not sure exactly what's going on.

Also, there are no unit tests for this function. These should be added so that we can be more confident that it works correctly.

We will also need to add TypeScript definitions for any new functions that are added. I can do this later if you have trouble with it.

@benjaminBrownlee
Copy link
Author

I believe I have an edit so that this will work in a Node environment as well as a majority of browsers, and I also threw in your suggested fallback feature if users are using an environment without crypto tools. If you are curious, see my most recent gist edits.

I tried to append my minified scripts into the original minified file, which must have gone wrong. I will re-minify the forked regular file to avoid these errors next time.

However, as mentioned before, I have no idea where to turn to do "unit tests" on my function. Is there a preferred or formal process or host to do those?

Also, to be completely honest, I have not used TypeScript before. I actually have used a converter tool to convert your entire library to PHP, but TypeScript is a superset of JavaScript so tools can't convert in that direction.

@peterolson
Copy link
Owner

peterolson commented Sep 13, 2017

The unit tests are located in the spec/spec.js file. You can run them locally by opening the spec/SpecRunner.html file or doing npm test on node. You can look through the spec file and follow the example of the existing tests that specify what kinds of outputs we expect from the functions. You'll probably be most interested in the tests for the randBetween function.

The TypeScript definitions are in the BigInteger.d.ts file, which defines types and makes it more convenient for people using BigInteger.js in their TypeScript projects. The tsDefinitions.ts verifies that the library actually conforms to these type definitions.

As I said before, don't worry about the TypeScript definitions if you have trouble figuring it out, but it is important to have unit tests for every function in the library.

@peterolson peterolson closed this Aug 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants