Open axic opened 8 years ago
I also propose to create a v5.x
branch to merge these changes in.
Perhaps also the following could be discussed as part of this:
Any plans regarding https://github.com/indutny/bn.js/issues/35 as part of next major release ?
@fanatid for what?
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?
@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.
@fanatid additionally, allocation is a huge problem.
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)
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.
Thank you for explanation. Can you publish your tests for Array/Buffer/Uint8Array comparison?
Something like this, I guess: https://gist.github.com/indutny/f37c466b4765e686b766b0b32557557c . Btw, just recalled that Buffer
s can't be used for it, only Uint32Array
Updated gist to make results more fair. Should be started either as node bench.js u
or just node bench.js
.
I think #90 #91 #141 #151 should be added to list
Should we do it after January 1st? cc @fanatid @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.
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)
@axic how do you use toTwos/fromTwos
to parse a DER integer?
@dcousens luckily I am not dealing with ASN.1. toTwos
/fromTwos
is used within https://github.com/ethereumjs.
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?
@indutny I'd pretty much like modn
to return a BN but take a Number if possible
@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.
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...
I don't mind naming it modln
instead of modrn
, and mentioning it in the docs. Would you care to open a PR?
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
.
Additional breaking change request:
assert(str.length % 2 === 0)
for base16/hexI 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.
@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.
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?
Maybe we can make it throw for just hex, and accept without padding for base-16?
That sounds reasonable.
Here was another potential thing: Rename toJSON
, see https://github.com/indutny/bn.js/pull/164#issuecomment-303332320
@axic how you think, what toJSON
actually should return?
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](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#toJSON()_behavior).
Reviewing the release, it seems that #112 could have been more clear. Perhaps for the 6.x release then 😉
(Taking the discussion from #112 to here)
Proposed changes:
.modn()
return a BN (#112).strip()
to._strip()
is a breaking change too (#105).andln()
to.andn()
(see the README). Perhaps fold the functionality intotoArrayLike
and make.andn()
safe in terms of working up to 53 bitsiusrhn()
into a specific shift right version, because it is only used inMPrime.split()
and is complex._countBits
,_zeroBits
, etc. Others could be moved too with passing aself
variable instead of using this, however that might have a speed penalty or increase?)setn
(should be renamedisetn
as it is in-place),testn
,maskn
,bincn