-178 = 10 * (-17) + 2, -178 = -10 * 17 + 8

[Yaffle]

-178 = 10 * (-17) + 2, -178 = -10 * 17 + 8

[dcousens]

@Yaffle care to add example code?

[Yaffle]

@dcousens ,

      var a = "-178";
      var n = "10";
      var q = new BN(a, 10).div(new BN(n, 10)).toString();
      var r = new BN(a, 10).mod(new BN(n, 10)).toString();
      console.log("${a} = ${n} * ${q} + ${r}".replace("${a}", a).replace("${n}", n).replace("${q}", q).replace("${r}", r));

[indutny]

Should be fixed by #54 . PTAL.

@dcousens I need to consult with you on this a bit. It is a breaking change, and I wonder if there might be any modules that will need to be updated to support this one. The main difference is that .mod() will now return negative number, when the input was negative itself.

There was few places in elliptic that needs to be fixed, but it wasn't that complicated (considering that I have pretty big test case).

What are your thoughts? Is it worth fixing, at the price of potentially breaking user code in un-obvious places?

[dcousens]

This could break a lot of existing code, especially since its an assumptions many users could be relying on.

In my case, I know for a fact I am relying on this relying on this behaviour, however, those cases are relying on a different BN library.
At best, this change is absolutely a major version bump IMHO.

@calvinmetcalf thoughts?

edit: Seems the only place this might affect crypto-browserify is in https://github.com/indutny/miller-rabin/blob/master/lib/mr.js, though it gets put in a red first, @indutny thoughts?

[indutny]

@dcousens I have a pending fix for elliptic library, but this will require major bump on it as well. I guess we can deal with it.

Still would like to hear @calvinmetcalf thoughts on this.

[jprichardson]

Doesn't it make sense to make the fix for the sake of correctness and predicability and to just bump the major? Maintaining a changelog would help to communicate these sorts of issues for anyone that wants to upgrade major versions.

[indutny]

@jprichardson the problem is that checking it might be not trivial. But I agree that it should be done.

[Yaffle]

Possibly, you might change umod too.

a	n	mod(master)	umod(fix/gh-47)	Euclidean division
4	3	1	1	1
4	-3	1	1	1
-4	3	2	2	2
-4	-3	1	-4	2

[indutny]

Ok, I think it should throw on negative reduction number in umod. Does it sound right?

[Yaffle]

I think, you might change umod to "euclidean division" if you so like non-negative results...

[indutny]

@Yaffle thanks, fixed.