Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in parseBase for numbers containing a dot ('0.0079') #90

Closed
axic opened this issue Dec 19, 2015 · 8 comments
Closed

Bug in parseBase for numbers containing a dot ('0.0079') #90

axic opened this issue Dec 19, 2015 · 8 comments

Comments

@axic
Copy link
Contributor

axic commented Dec 19, 2015

new BN('0.0079', 10) // <BN: ffffb22f>
new BN('0.02', 10) // <BN: ffffff3a>

_init, parseBase or _parseBase will not care if a dot is included in a base-10 input, will process it as ('.' - 48) & 0xf.

I think it should just reject such input?

@axic axic changed the title Bug in parseBase Bug in parseBase for numbers containing a dot ('0.0079') Dec 19, 2015
@axic
Copy link
Contributor Author

axic commented Dec 19, 2015

The same applies to parseHex (as basically it is the same code as parseBase, with one exception: range is limited to f/F).

new BN('1234', 16) // <BN: 1234>
new BN('12.34', 16) // <BN: 12e34>

@indutny
Copy link
Owner

indutny commented Dec 19, 2015

I know that it should throw, but it doesn't right now. The thing is that this is an big integer library, so you should not really pass data like this to it.

@axic
Copy link
Contributor Author

axic commented Dec 19, 2015

I do know that and I don't pass those kind of numbers. However, users of my library are. I can either do pre-processing there which is quite big of an overhead. I would prefer bn.js to throw, I have the PR ready, will submit it soon - it is quite small.

@axic
Copy link
Contributor Author

axic commented Dec 19, 2015

A note on the integer side, the library's name is short for big number and doesn't specifically mentions anywhere it is integers and not decimals. I know that, but it is not clear from the first sight at the README, maybe it should be updated.

@indutny
Copy link
Owner

indutny commented Dec 19, 2015

Yeah, I totally agree. It should be updated. Thanks!

@fanatid
Copy link
Collaborator

fanatid commented Mar 28, 2016

new BN(4.2) return <BN: 4>
also should be assert here

@dcousens
Copy link
Contributor

dcousens commented May 23, 2017

A note was added to the README at some point

Note: decimals are not supported in this library.

@indutny
Copy link
Owner

indutny commented Nov 29, 2017

It throws now.

@indutny indutny closed this as completed Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants