BN.isBN() breaks code when minified

[axic]

In the PR #109 it was suggested to include n.constructor.name === 'BN' in the isBN() check. Unfortunately this breaks behaviour with minified code.

The following sample elliptic code runs properly browserified, but fails when minified:

var crypto = require('crypto')
var EC = require('elliptic').ec
var ec = new EC('secp256k1')

var privateKey = crypto.randomBytes(32)
console.log(ec.keyFromPrivate(privateKey).getPublic(true, true))

The log output is:

Uncaught TypeError: Cannot read property 'fromRed' of null
n.getX @ test.br.min.js:490
n._encode @ test.br.min.js:400
n.encode @ test.br.min.js:401
i.getPublic @ test.br.min.js:588
i.57.crypto @ test.br.min.js:1060
n @ test.br.min.js:6
e @ test.br.min.js:7
(anonymous function) @ test.br.min.js:8

The isBN() section in the BN constructor should be there to have a quick path, but should we assume the other code path should handle a BN input properly (by toString() and parsing that)?

Had no time to fully track down everything. Perhaps this is more of a bug in elliptic where it assumes passing through BN objects will keep the reduction context (which it does if isBN() deems it to be an instance, otherwise the reduction context is lost).

[axic]

Actually elliptic depends on the fact that the BN constructor allows the passthrough of BN instances. Removing that (by removing the shortcut in the constructor) the majority of tests fail in elliptic.

[adrai]

have this issue too
I'm using it via: https://github.com/crypto-browserify/browserify-sign

[jprichardson]

@axic @adrai presumably you're using UglifyJS2? If so, take a look at the --reserved flag: https://github.com/mishoo/UglifyJS2#mangler-options. I think that should solve your problem.

[axic]

@jprichardson I'm using minify (which could be using uglify in the back, don't know). I not entirely sure that is the right way to go.

Your solution must be mentioned on the pages of all dependencies to avoid possible issues. That might be too much required.

[calvinmetcalf]

so should we revert #109?

[axic]

I would say just revert to the original proposal of #109, e.g. remove the n.constructor.name === 'BN' part. It should behave exactly the same as before merging.

[bergr01]

This is causing issues for me as well at the moment - if this doesn't get fixed in BN.js itself I might have to find a way to exclude BN.js from minification....

[jprichardson]

@bergr01 does --keep-fnames UglifyJS2 flag fix this for you?

[felix]

It seems to work for me with latest bn.js, thanks.
Update: Perhaps not, had to revert to v4.10.5 :-(

[bergr01]

@jprichardson Didn't seem to help, still getting 'fromRed only works with numbers in reduction context' in minified build of bn.js when running my application in Safari 9.1.1. @indutny Fedor any chance of looking into this some more? @axic how did you proceed in your case?

[indutny]

How do you feel about:

diff --git a/lib/bn.js b/lib/bn.js
index cb1d597..0326dd8 100644
--- a/lib/bn.js
+++ b/lib/bn.js
@@ -55,8 +55,11 @@
   }

   BN.isBN = function isBN (num) {
+    if (num instanceof BN)
+      return true;
+
     return num !== null && typeof num === 'object' &&
-      num.constructor.name === 'BN' && Array.isArray(num.words);
+      num.constructor.wordSize === BN.wordSize && Array.isArray(num.words);
   };

   BN.max = function max (left, right) {

? This should fix it.

[bergr01]

Sorry for not responding sooner @indutny , Will try out the latest version and see how I go!