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.

Sign up for GitHub

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

Jump to bottom

BN parses invalid characters #151

Closed

kumavis opened this issue Nov 7, 2016 · 7 comments

Contributor

kumavis commented Nov 7, 2016

bn = new BN('xxx', 16)
<BN: 888>

danfinlay commented Nov 7, 2016

Yeah, I just ran into this, took a long time to debug, because I assumed hex-prefixed base 16 would be digested gracefully.

> var a = new BN('0x01', 16)
undefined
> a
<BN: 801>

Hex prefixes should be removed automatically, and otherwise invalid characters should throw a descriptive error.

danfinlay mentioned this issue

Some transactions have unusually high gas cost MetaMask/metamask-extension#787

Closed

danfinlay added a commit to MetaMask/metamask-extension that referenced this issue


          Fix gas price buffering

Our gas price buffering logic had a bug, because bn.js has inconsistent behavior when using hex-prefixed output.  The issue has been opened with them here:
indutny/bn.js#151

We've corrected our usage in the mean time.

danfinlay mentioned this issue

i787 High gas bug MetaMask/metamask-extension#793

Merged

Collaborator

fanatid commented Nov 7, 2016

related with #141
unfortunately that PR don't fix this issue

danfinlay commented Nov 7, 2016

If it throws an error for the hex prefix, I'd consider that mostly solving the issue. Wouldn't the x trigger an error in that PR?

Collaborator

fanatid commented Nov 7, 2016

@FlySwatter I already checked and looks like that PR don't throw error
as I understand it is happens because #141 fix _parseBase, but when base equal 16 _parseHex is used..

Contributor

axic commented Nov 7, 2016

Also: #90 & #91 (#91 actually implements the fix)

fanatid mentioned this issue

Breaking changes for the next major release #129

Open

8 tasks

Contributor Author

kumavis commented Jan 24, 2017

Just ran in to this again omg fml

Contributor

dcousens commented Jan 25, 2017

@kumavis wrap the BN constructor in something that is stricter so as to avoid this issue.

indutny closed this as completed in

e924509

fanatid mentioned this issue

Semver-major release: 5.0.0 #219

Closed

meck93 mentioned this issue

[Snyk] Upgrade bn.js from 5.0.0 to 5.1.1 alexscheitlin/master-project-evoting#147

Closed

chancehudson mentioned this issue

Server api zkopru-network/zkopru#164

Merged

luu-alex mentioned this issue

5072 - updating bn 1.x web3/web3.js#5074

Merged

16 tasks

luu-alex mentioned this issue

OZ tests investigation web3/web3.js#5609

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment