ruimarinho / bitcoin-core

A modern Bitcoin Core REST and RPC client.
483 stars 188 forks source link

Add support for bignumbers #17

Closed pedrobranco closed 7 years ago

pedrobranco commented 8 years ago

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

Fixes the issue https://github.com/seegno/bitcoin-core/issues/16.

ruimarinho commented 8 years ago

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

pedrobranco commented 8 years ago

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 commented 8 years ago

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

pedrobranco commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@pedrobranco can you rebase please?

ruimarinho commented 7 years ago

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

pedrobranco commented 7 years ago

@ruimarinho Rebased.

ruimarinho commented 7 years ago

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 commented 7 years ago

+1 for using just string.