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

Fix the rawDecode to utils toBuffer #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VoR0220
Copy link

@VoR0220 VoR0220 commented Oct 11, 2017

This caused me a heap of problems decoding an output for Parity, and I imagine it's likely to help others down the road.

This caused me a heap of problems decoding an output for Parity, and I imagine it's likely to help others down the road.
@coveralls
Copy link

Coverage Status

Coverage increased (+2.2%) to 85.97% when pulling e6f3243 on VoR0220:patch-1 into ee6ded6 on ethereumjs:master.

@axic
Copy link
Member

axic commented Dec 6, 2017

@VoR0220 what is the problem this is solving?

@VoR0220
Copy link
Author

VoR0220 commented Dec 6, 2017

Well for me, this wasn't compiling previously before I popped this in. Might be an OS X issue, might be an NPM issue, I'm not certain. I believe it had something to do with the way that it assumed the input and didn't convert it prior to. Either way, this converts any inputs into a buffer beforehand and is therefore a better way of going about it from my perspective.

@axic
Copy link
Member

axic commented Dec 6, 2017

I think the main motivation was not require strict inputs and not introduce the mess other ethereumjs libraries have with the toBuffer function, which just hides a bunch of different type conversions and makes users lazy.

To be honest I think it shouldn't even expect a hex string, rather a Buffer only and that leaves "bufferization" to the caller.

@se3000
Copy link

se3000 commented Mar 29, 2018

Is there anything preventing this from being merged? We were running into this error when decoding bytes, and can confirm that it indeed fixes the error.

@se3000
Copy link

se3000 commented Mar 29, 2018

fixes:

  2) Contract: Args #add has the type:
     Error: Number can only safely store up to 53 bits
      at assert (node_modules/bn.js/lib/bn.js:6:21)
      at BN.toNumber (node_modules/bn.js/lib/bn.js:506:7)
      at decodeSingle (node_modules/ethereumjs-abi/lib/index.js:234:52)
      at Function.ABI.rawDecode (node_modules/ethereumjs-abi/lib/index.js:367:19)
      at Context.it (test/Args_test.js:30:24)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:160:7)

And we're running into this on OSX.

@holgerd77
Copy link
Member

@se3000 What do you take as input so that this error emerges?

@holgerd77
Copy link
Member

@axic Can't really judge from the discussion if this is a valid PR or not.

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

Successfully merging this pull request may close these issues.

None yet

5 participants