Breaking changes for the next major release

[axic]

(Taking the discussion from #112 to here)

Proposed changes:

• [x] Make .modn() return a BN (#112)
• [x] Renaming .strip() to ._strip() is a breaking change too (#105)
• [x] Maybe reviewing the constructor method(s) could lead to such a change (see #90 & #91)
• [ ] Rename .andln() to .andn() (see the README). Perhaps fold the functionality into toArrayLike and make .andn() safe in terms of working up to 53 bits
• [ ] Maybe at that stage two's complement could be made more internal (part of constructor and toString) (see #73)
• [ ] Split out the extended functions (saving destroyed bits) off iusrhn() into a specific shift right version, because it is only used in MPrime.split() and is complex.
• [ ] Remove internal functions from the BN context (good candidates are: _countBits, _zeroBits, etc. Others could be moved too with passing a self variable instead of using this, however that might have a speed penalty or increase?)
• [ ] Perhaps rethink naming/functionality of the following bitwise methods: setn (should be renamed isetn as it is in-place), testn, maskn, bincn

[axic]

I also propose to create a v5.x branch to merge these changes in.

[axic]

Perhaps also the following could be discussed as part of this:

• #79
• #107

[fanatid]

Any plans regarding https://github.com/indutny/bn.js/issues/35 as part of next major release ?

[fanatid]

@indutny just would like to know your opinion about Buffer (node/browser) instead Array here

[indutny]

@fanatid for what?

[fanatid]

out of curiosity... as I understand you used Array instead Buffer for zero dependencies, why not Uint8Array for example? if there were no question about dependencies, would you use Buffer instead Array?

[indutny]

@fanatid both Buffer and Uint8Array were slower for the purposes of bn.js . I've experimented with it several times, and at each try it was worse.

[indutny]

@fanatid additionally, allocation is a huge problem.

[fanatid]

oh, okay, so the main reason is performance? How long ago are you tested? Array allocation work better? Maybe I'm wrong, but as I remember, Buffer allocate more then requested? (or pre-allocated buffers works in such way, at least in node)

[indutny]

I've tested it about 6 months ago, but feel free to give it another try.

Array allocation is fast as hell, while Buffer/Uint8Array generally takes more time to create and additional time to manage (GC). Pre-allocating buffers works, but it doesn't currently fit into the scheme of things, and considering that they were slower anyway - I didn't see a reason to change APIs.

[fanatid]

Thank you for explanation. Can you publish your tests for Array/Buffer/Uint8Array comparison?

[indutny]

Something like this, I guess: https://gist.github.com/indutny/f37c466b4765e686b766b0b32557557c . Btw, just recalled that Buffers can't be used for it, only Uint32Array

[indutny]

Updated gist to make results more fair. Should be started either as node bench.js u or just node bench.js.

[fanatid]

I think #90 #91 #141 #151 should be added to list

[indutny]

Should we do it after January 1st? cc @fanatid @dcousens

[dcousens]

ACK, sounds good to me. I'll happily put the time in bumping some of the crypto-browserify packages up to the new changes.

[dcousens]

Maybe at that stage two's complement could be made more internal (part of constructor and toString) (see #73)

Did we want to elaborate more on what this change might entail? (Aka, summarise #73)

[dcousens]

@axic how do you use toTwos/fromTwos to parse a DER integer?

[axic]

@dcousens luckily I am not dealing with ASN.1. toTwos/fromTwos is used within https://github.com/ethereumjs.

[indutny]

Didn't mean to close this. @dcousens what do you think about the rest of the list? Is it something that we want to address in this major bump?

[axic]

@indutny I'd pretty much like modn to return a BN but take a Number if possible

[dcousens]

@indutny if you don't intend to change modn to return a BN, I'd remove it. Changing it is additionally dangerous for existing users who blindly upgrade, so removing is the better option until maybe later some minor bump that re-adds it.

andln

s/andln/andrn/g? What was l?

Is this function intentionally public?

I think the rest of the list is good, but, I don't have the time to do it, and assuming you don't, I think the changes in the last 24 hours have been fantastic.

Non-strict hex has been a liability from the get-go for me.

[indutny]

Yeah, I don't want to change it for exactly these reasons. Good call! Is the benefit of more sound API outweighs breaking changes? It would be better to deprecate it with a warning, but there is no code to do this yet.

Oh, perhaps l was for returning number... I guess we should rename it to modln then. Sincerely, I can't recall...

[indutny]

I don't mind naming it modln instead of modrn, and mentioning it in the docs. Would you care to open a PR?

[dcousens]

I don't mind naming it modln instead of modrn, and mentioning it in the docs. Would you care to open a PR?

I suppose, but what did ln mean? rn is more intuitive from your explanation of return number.

[dcousens]

Additional breaking change request:

• [ ] assert(str.length % 2 === 0) for base16/hex

[indutny]

I have no idea. Should we do same renaming for andln => andrn then?

-1 for hex padding. This is going to be hell of a breaking change.

[dcousens]

@indutny yes, but, is andln useful? It only works on the first word?

-1 for hex padding. This is going to be hell of a breaking change.

I know... how about require('bn.js/strict') which enforces that? Unexpected padding errors have caused countless problems for libraries.

[indutny]

I think it is useful.

Unexpected padding errors have caused countless problems for libraries.

Maybe we can make it throw for just hex, and accept without padding for base-16?

[dcousens]

Maybe we can make it throw for just hex, and accept without padding for base-16?

That sounds reasonable.

[axic]

Here was another potential thing: Rename toJSON, see https://github.com/indutny/bn.js/pull/164#issuecomment-303332320

[fanatid]

@axic how you think, what toJSON actually should return?

[axic]

Never mind, I've only realised now that toJSON is potentially used by JSON.stringify and it should have no other use beyond that. Perhaps documenting this would be enough. See this.

[axic]

Reviewing the release, it seems that #112 could have been more clear. Perhaps for the 6.x release then 😉