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

Allow leading zeros when parsing hex strings. #23

Merged
merged 1 commit into from
Jan 12, 2017
Merged

Allow leading zeros when parsing hex strings. #23

merged 1 commit into from
Jan 12, 2017

Conversation

vethan
Copy link

@vethan vethan commented Nov 29, 2016

Currently Numeric.decodeQuanitity will fail if called with a hex value with a leading zero, such as "0x01". This doesn't make sense, leading zeroes are valid for hex numbers, and it's fairly common for a hex number to be padded. Check remove, and tests changed

@codecov-io
Copy link

codecov-io commented Nov 29, 2016

Current coverage is 58.04% (diff: 100%)

Merging #23 into master will decrease coverage by 0.02%

@@             master        #23   diff @@
==========================================
  Files           148        148          
  Lines          3515       3513     -2   
  Methods           0          0          
  Messages          0          0          
  Branches        596        595     -1   
==========================================
- Hits           2041       2039     -2   
  Misses         1200       1200          
  Partials        274        274          

Powered by Codecov. Last update fcc69e3...c8554f0

@conor10
Copy link
Contributor

conor10 commented Nov 29, 2016

Thanks for the submission. My issue with supporting zero padded quantity values is that it goes against the JSON-RPC specification. What's the specific use case you need this functionality for?

@vethan
Copy link
Author

vethan commented Nov 30, 2016

Ah. Interesting, this means that testrpc isn't obeying the specification. Even so, I wouldn't expect an exception on parsing, I'd expect an error but for execution to continue

@vethan
Copy link
Author

vethan commented Nov 30, 2016

Also, thanks for the link to the specification I'd been looking for something like that to figure out which of testrpc and web3j was off spec. I'll update my testrpc pull request.

@conor10
Copy link
Contributor

conor10 commented Dec 1, 2016

I'd rather fail if an invalid value is provided in the payload.

Would you be able to reference your testrpc pull request here please? I'd like to keep this change out of web3j. However, if it seems like it may take a while for the pull request to be accepted, happy to reconsider.

@vethan
Copy link
Author

vethan commented Dec 2, 2016

So, gonna have to re-do my testrpc PR. What seemed like a quick and easy change over there is having unforseen consequences because they don't differentiate between treating buffers as addresses and treating them as values >_>

Give me the weekend to investigate this in more detail

@vethan
Copy link
Author

vethan commented Dec 7, 2016

@conor10 So, I can't link my PR because my work so far still isn't a 100% fix, it's gonna take me a little while to get to grips with their codebase enough to do the fix properly. Opened issue trufflesuite/ganache-cli-archive#220

@conor10
Copy link
Contributor

conor10 commented Jan 11, 2017

Hey @vethan are you still looking into submitting a pull request to testrpc or have you put it on hold?

@vethan
Copy link
Author

vethan commented Jan 12, 2017

@conor10 unfortunately I've had to put it on hold for now, not sure when I'll have time to get around to it properly

@conor10 conor10 merged commit b6f0a68 into hyperledger:master Jan 12, 2017
@conor10
Copy link
Contributor

conor10 commented Jan 12, 2017

Merged - makes sense to have this working with testrpc.

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

Successfully merging this pull request may close these issues.

None yet

4 participants