Add support for bignumbers #17

pedrobranco · 2016-08-24T10:01:22Z

This PR adds support for parsing correctly values with more than 15 significant digits (as string).

Fixes the issue #16.

ruimarinho · 2016-08-25T01:03:42Z

@pedrobranco can you add a test for this? Also, have you considered simply using a reviver function instead, as supported by JSON.parse?

pedrobranco · 2016-08-25T12:04:30Z

can you add a test for this

Yes.

Also, have you considered simply using a reviver function instead, as supported by JSON.parse?

Already tried the reviver function, but already returns the value without the correct precision.

> JSON.parse('{ "value": 150000000000000.00000000000000001 }', (k,v) => { console.log(k,v)})
value 150000000000000
 {}

ruimarinho · 2016-08-25T16:44:59Z

What's the difference between this and https://www.npmjs.com/package/json-bigint or https://github.com/josdejong/lossless-json?

pedrobranco · 2016-08-25T17:49:41Z

IMO json-bigint-string is less intrusive by parsing only the unsupported values as string.

const LosslessJSON = require('lossless-json');

let text = '{"normal":2.3,"long":123456789012345678901,"big":2.3e+500}';
let json = LosslessJSON.parse(text);
/*
{ normal: { [Number: 2.3] value: '2.3', type: 'LosslessNumber', isLosslessNumber: true },
  long:
   r {
     value: '123456789012345678901',
     type: 'LosslessNumber',
     isLosslessNumber: true },
  big:
   r {
     value: '2.3e+500',
     type: 'LosslessNumber',
     isLosslessNumber: true } }
*/

json.normal === 2.3 // false

let originalJson = JSON.parse(text); // { normal: 2.3, long: 123456789012345680000, big: Infinity }
originalJson.normal === 2.3 // true

WDYT?

ruimarinho · 2016-09-19T11:01:15Z

After digging a bit, I think we can use json-bigint with { storeAsString: true } options. Additionally, I would add strict checking as duplicate keys is valid on JSON but the native JSON.parse will only hold the last value. The end result could be something like:

const JSONBigInt = require('json-bigint')({ storeAsString: true, strict: true });

This qualifies for a breaking change so this would need to come in a form of a major version.

ruimarinho · 2016-09-19T11:02:25Z

package.json

@@ -39,6 +39,7 @@
  },
  "dependencies": {
    "bluebird": "^3.1.1",
+    "json-bigint-string": "^1.0.0",


Switch to json-bigint instead.

ruimarinho

Please remove dist/ files, as they are generated during the release phase.

ruimarinho · 2016-09-19T11:03:08Z

src/index.js

@@ -118,7 +118,8 @@ class Client {

      return this.request.postAsync({
        auth: _.pickBy(this.auth, _.identity),
-        body,
+        body: JSON.stringify(body),


We should make it clear that if users of the client want to keep precision, this should send those values as strings.

@pedrobranco ^

ruimarinho · 2016-09-19T11:03:33Z

test/index_test.js

@@ -199,7 +199,7 @@ describe('Client', () => {
      it('should return the proof-of-work difficulty', async () => {
        const difficulty = await new Client(config.bitcoind).getDifficulty();

-        difficulty.should.be.a.Number();
+        difficulty.should.be.a.String();


Can you rebase as this has been fixed already?

It should be unrelated to the changes on this PR.

Since we are parsing big numbers as String this method will now return the result as a String.

Is difficulty a big number?

ruimarinho · 2016-10-03T22:13:19Z

@pedrobranco can you rebase please?

ruimarinho · 2017-02-04T17:57:04Z

I would like to release this on the next major @pedrobranco.

ruimarinho · 2017-02-08T00:45:13Z

docker-compose.yml

@@ -10,7 +10,7 @@ sut:
    - bitcoind-username-only

 bitcoind:
-  image: seegno/bitcoind:0.13-alpine
+  image: seegno/bitcoind:0.13.0-alpine


ruimarinho · 2017-02-08T00:45:58Z

src/parser.js

@@ -5,6 +5,7 @@

 import RpcError from './errors/rpc-error';
 import _ from 'lodash';
+import jsonBigInt from 'json-bigint';


JSONBigInt?

Eslint rule triggers http://eslint.org/docs/rules/new-cap. Should we ignore it?

ruimarinho · 2017-02-08T00:48:23Z

src/parser.js

@@ -52,6 +53,9 @@ export default class Parser {
      throw new RpcError(response.statusCode);
    }

+    // Parsing the body with custom parser to support BigNumbers.
+    body = jsonBigInt({ storeAsString: true }).parse(body);


Can you address the other comments? (e.g. strict?)

pedrobranco · 2017-02-08T15:03:54Z

@ruimarinho Rebased.

ruimarinho · 2017-02-08T19:43:07Z

README.md

@@ -91,6 +91,10 @@ client.getInfo().then(([body, headers]) => console.log(body, headers));
 const [body, headers] = await client.getInfo();
 ```

+### Floating point number precision in JavaScript
+
+Due to [Javascript floating point precision](http://floating-point-gui.de/), to prevent precision loss when using `bitcoin-core` we should pass all big numbers (numbers with more than 15 significant digits) as string instead of using `Number` data type.


JavaScript's

Due to JavaScript's limited floating point precision, all big numbers (numbers with more than 15 significant digits) are returned as strings to prevent precision loss.

ruimarinho · 2017-02-08T19:56:47Z

I'm thinking that returning as a big number (instead of a string) could be an interesting option of the client. /cc @fixe @nunofgs?

fixe · 2017-02-09T16:19:06Z

+1 for using just string.

pedrobranco force-pushed the feature/support-bignumbers branch from 37adb4c to 64b4182 Compare August 24, 2016 10:49

ruimarinho reviewed Sep 19, 2016

View reviewed changes

pedrobranco force-pushed the feature/support-bignumbers branch from 64b4182 to 2540d58 Compare November 3, 2016 12:28

pedrobranco force-pushed the feature/support-bignumbers branch from 2540d58 to 0b1c732 Compare February 7, 2017 15:42

ruimarinho reviewed Feb 8, 2017

View reviewed changes

pedrobranco force-pushed the feature/support-bignumbers branch 2 times, most recently from bba077c to 0382821 Compare February 8, 2017 12:26

ruimarinho reviewed Feb 8, 2017

View reviewed changes

pedrobranco force-pushed the feature/support-bignumbers branch from 0382821 to 24f8649 Compare February 9, 2017 12:18

pedrobranco force-pushed the feature/support-bignumbers branch 2 times, most recently from 824e5d8 to 3ee0621 Compare February 10, 2017 15:22

Add support for big numbers

9ca5caf

pedrobranco force-pushed the feature/support-bignumbers branch from 3ee0621 to 9ca5caf Compare February 10, 2017 15:38

ruimarinho merged commit dc42ebe into ruimarinho:master Feb 12, 2017

ruimarinho mentioned this pull request Feb 12, 2017

Support Bitcoin's maximum precision #16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for bignumbers #17

Add support for bignumbers #17

pedrobranco commented Aug 24, 2016

ruimarinho commented Aug 25, 2016

pedrobranco commented Aug 25, 2016 •

edited

Loading

ruimarinho commented Aug 25, 2016

pedrobranco commented Aug 25, 2016 •

edited

Loading

ruimarinho commented Sep 19, 2016

ruimarinho Sep 19, 2016

ruimarinho left a comment

ruimarinho Sep 19, 2016

ruimarinho Feb 8, 2017

ruimarinho Sep 19, 2016 •

edited

Loading

ruimarinho Feb 8, 2017

pedrobranco Feb 8, 2017

ruimarinho Feb 8, 2017

pedrobranco Feb 10, 2017

ruimarinho commented Oct 3, 2016

ruimarinho commented Feb 4, 2017

ruimarinho Feb 8, 2017

ruimarinho Feb 8, 2017

pedrobranco Feb 8, 2017 •

edited

Loading

ruimarinho Feb 8, 2017

pedrobranco commented Feb 8, 2017

ruimarinho Feb 8, 2017

ruimarinho Feb 8, 2017

ruimarinho commented Feb 8, 2017

fixe commented Feb 9, 2017

Add support for bignumbers #17

Add support for bignumbers #17

Conversation

pedrobranco commented Aug 24, 2016

ruimarinho commented Aug 25, 2016

pedrobranco commented Aug 25, 2016 • edited Loading

ruimarinho commented Aug 25, 2016

pedrobranco commented Aug 25, 2016 • edited Loading

ruimarinho commented Sep 19, 2016

Choose a reason for hiding this comment

ruimarinho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruimarinho Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruimarinho commented Oct 3, 2016

ruimarinho commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobranco Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobranco commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruimarinho commented Feb 8, 2017

fixe commented Feb 9, 2017

pedrobranco commented Aug 25, 2016 •

edited

Loading

pedrobranco commented Aug 25, 2016 •

edited

Loading

ruimarinho Sep 19, 2016 •

edited

Loading

pedrobranco Feb 8, 2017 •

edited

Loading