Setup project integrate shamir #4
Conversation
There was a problem hiding this comment.
There seems to be a lot of copied code from secrets.js and secrets.js-grempe. There's also a newer version (all off the same base) secret-sharing.js.
I'm not sure what you changed. I see refactoring some names and adding a random number generator, plus some more tests which is great.
The reason you mentioned to rebuild these libraries from scratch was to use a better random number generator. All of those libraries have a setRNG that we could use and remove a lot of this code. Did you have other reasons that I'm not seeing?
Given that the logic seems near exact, I think the community would benefit the most from letting this project be a light wrapper around secret-sharing.js that adds the react-native random number generator with setRNG. Then we can add some test cases to the upstream one, rather than split implementations. It might also be valuable to make some of your refactoring changes on the upstream one as well.
lib/config.js
Outdated
| @@ -0,0 +1,46 @@ | |||
| /*eslint no-undef: "off"*/ | |||
There was a problem hiding this comment.
What's your reasoning for turning it off? Likely config is actually undefined. Something like
const config = { ... }
export default config;Would fix this error and have the benefits of letting config be a const rather than a var
|
|
||
| export class CharacteristicTwoGaloisField { | ||
| constructor(numElements) { | ||
| this.n = Math.log2(numElements); |
There was a problem hiding this comment.
What is n and what does it represent? From looking at the code you copied from, is it bits?
Should we make it more clear what it is?
lib/galoisField.js
Outdated
| } | ||
| getExponentOfNextDegree(expOfCurrentDegree) { | ||
| let primitivePolynomial = config.primitivepolynomials[this.n]; | ||
| var polynomial = expOfCurrentDegree.timesMonomialOfDegree(1); |
There was a problem hiding this comment.
I learned something new! eslint/recommended has no-var turned off. I wouldn't have expected that.
var seems to be used a lot, any reason why? While this one and probably the rest are fine, var can lead to a lot of subtle bugs so I tend to avoid them for let and const.
lib/galoisField.test.js
Outdated
| var expectedResult = Polynomial.toIntegerForm('1100'); //equivelent of x^3 + x^2 | ||
| var actualResult = new Polynomial(polynomialOne).plus(polynomialTwo); | ||
| expect(expectedResult).toBe(actualResult.getValue()); | ||
| expect(4).toBe(new Polynomial(1).plus(5).getValue()); |
There was a problem hiding this comment.
What, 1 + 5 = 4? 😛
Super nitty, it's easier to read things like expect(new Polynomial(1).plus(5).getValue()).toBe(4);. This is what's called a "Yoda Clause".
Mmmmm, 4 I expect a polynomial of 1 plus 5 to be
lib/galoisField.test.js
Outdated
| describe('create exponent and log tables for the field', () => { | ||
| it('should output the correct exp and log tables for GF(2^3)', () => { | ||
| var gf = new CharacteristicTwoGaloisField(Math.pow(2, 3)); | ||
| expect(gf.exps[0]).toBe(1); |
There was a problem hiding this comment.
Does .toEqual not work with arrays?
lib/utils.js
Outdated
| } | ||
|
|
||
| // return null so this result can be re-processed if the result is all 0's. | ||
| if ((str.match(/0/g) || []).length === str.length) { |
There was a problem hiding this comment.
Why not just use a regex for this whole expression? /^0+$/g
package.json
Outdated
| @@ -31,7 +31,8 @@ | |||
| ], | |||
| "homepage": "https://github.com/rh389/react-native-securerandom#readme", | |||
| "dependencies": { | |||
| "base64-js": "*" | |||
| "base64-js": "*", | |||
There was a problem hiding this comment.
We should probably lock this down to ^2.4.9 otherwise we could have breaks
There was a problem hiding this comment.
latest version I see is 1.3.0 - but point taken
There was a problem hiding this comment.
😅 Reading is hard, yes 1.3 is right
| @@ -50,7 +50,8 @@ export function bytesToBits(byteArray) { | |||
| } | |||
|
|
|||
| // return null so this result can be re-processed if the result is all 0's. | |||
| if ((str.match(/0/g) || []).length === str.length) { | |||
| const allZeroPattern = /^0+$/; | |||
There was a problem hiding this comment.
This is fine as is. For clarity on my statement about regex being expensive to compile, this regex being defined in the function will be newly declared, created, and compile every time the function is called.
To make sure that it's only compiled once, you can move the declaration outside of the function.
WIP of integrating shamir's but code amount has far exceeded amount appropriate for review. Still need to set up JSDoc.
So far - this is a rewrite of existing shamir javascript implementation (https://github.com/amper5and/secrets.js/ ) that uses react-native-securerandom for the random number generator, and focuses on emphasizing and explaining the concepts of Galois Fields of characteristic 2, the fields from which coefficients are randomly generated.