search

altangent / lnd-async

Lightning Network Daemon gRPC async client
MIT License
21 stars 12 forks source link

The long numbers are as String type - is very bad :( #18

Closed LNBIG-COM closed 5 years ago

LNBIG-COM commented 5 years ago

https://github.com/altangent/lnd-async/blob/8daf5be2c6539e064943e73bb06aceb82c2133b5/lib/lnd-rpc.js#L69

Hello!

Was it done on purpose? There are problems with this setting - all numbers turn into String() type and any '+' operation causes a concatation strings but not numbers. I had problems already in my project and many people can have same problems. For example listChannels:

{ pending_htlcs: [], active: true, remote_pubkey: '03abf6f44c355dec0d5aa155bdbdd6e0c8fefe318eff402de65c6eb2e1be55dc3e', channel_point: '6f6cb287b23a596b5c9aff396c49fa57c00a5393131985c4bf5562f88dd30f8a:0', chan_id: '604819356264628224', capacity: '16777215', local_balance: '14929891', remote_balance: '1843676', commit_fee: '3648', commit_weight: '724', fee_per_kw: '5038', unsettled_balance: '0', total_satoshis_sent: '3028539', total_satoshis_received: '1184862', num_updates: '1569', csv_delay: 2016, private: false }

If i do item.total_satoshis_sent + item.total_satoshis_received i will have "30285391184862" not 4213401 (3028539+1184862)

bmancini55 commented 5 years ago

Hi thanks, for the issue. There is some room for improvement on the handling of this.

Longs as strings is by design. JavaScript's Number type stores values as 64-bit IEEE 754 floating point numbers. As a result, the max integer value is 2^53. So a 64-bit integer has the potential to overflow and result in precision loss.

IMO, the most portable method for handling this is passing int64 integers as strings and then either parsing them into Numbers via parseInt (which has the potential for precision loss) or passing them through a big number library such as BN.js, BigNumber.js, int64, long.js etc.

grpc-node mentions this in the compatibility section

If a proper way to work with 64 bit values (uint64, int64 etc.) is required, just install long.js alongside this library. All 64 bit numbers will then be returned as a Long instance instead of a possibly unsafe JavaScript number (see).

Because crypto projects use a variety of methods for big numbers, I prefer to keep them as strings and let the user control how they want to handle the number. An improvement would be adding an option into lnd-async that controls long treatment. If the user wants them to be Number and/or automatically convert to long.js Long type they can specify that in the options.

That said, I do think the default of a string is preferable to a standand JS Number type just because precisions loss is a lot more dangerous than incorrect typing. Either way, this option and behavior should be documented in the README.

bmancini55 commented 5 years ago

I added a longsAsNumbers flag to the connect method. This is now available in version 1.7.0.

LNBIG-COM commented 5 years ago

Hello!

Thanks, sorry i didn't know about this. I googled now this and you are right...