search

bitcoinjs / bolt11

A library for encoding and decoding lightning network payment requests as defined in BOLT #11.
MIT License
92 stars 64 forks source link

Use bn.js instead bignumber.js #2

Closed dcousens closed 6 years ago

dcousens commented 6 years ago

Great work @fanatid :1st_place_medal:

dcousens commented 6 years ago

@junderw it might be useful to make an isolate bolt-utils module that handles the unit conversions?

Or maybe we make a general-purpose module for going between:

etc?

dcousens commented 6 years ago

I'll re-open this at a later date after some more review and tests

dabura667 commented 6 years ago

Yeah, we should separate it out to a different function.

However, this PR is making a huge misunderstanding about mSats.

The lightning payment request specifies milli, micro, nano, and pico as valid multipliers, BUT the lowest possible unit in ln payments is milliSatoshis. 1 milliSatoshi == 10 picoBitcoin

So any value less than 10p (10 picoBitcoin) is invalid.

dcousens commented 6 years ago

@dabura667 is there any reason to support the other units?

I'd think mSats and Sats are all that should matter.

fanatid commented 6 years ago

@dabura667 oh, sorry, I did not knew about this, probably I should read paper before 😆

dabura667 commented 6 years ago

BOLT11 doesn’t define a satoshi or millisatoshi multiplier.

It also says that you need to use the largest multiplier possible to eliminate trailing zeroes and keep the human readable part an integer.

dcousens commented 6 years ago

Aka, use the lowest common denominator? Aka, mSats unless Sats is an integer?

dabura667 commented 6 years ago

ie. 1 satoshi == 10n 1 mSat == 10p 1 bit == 1u 1 mBTC == 1m 0.1 BTC == 100m 54.321 BTC == 54321m

dcousens commented 6 years ago

@dabura667 I think we should be looking at it like:

1 millisatoshi = 1
1 satoshi = 1e3 millisatoshi
1 mBTC = 1e8 * 1e3 * satoshis
1 BTC = 1e8 satoshis

Aka, satoshi, unless millisatoshi is required if satoshi isn't an integer denomination.

dabura667 commented 6 years ago

Yea, I think having the amount be in milliSatoshis will confuse developers... so I think the amount should be handled in satoshis and allow for 3 decimal places by basically multiplying satoshis by 1000 from the get go and checking if the result is an integer, if not, throw an error.

dabura667 commented 6 years ago

satoshisToHrp And hrpToSatoshis

Would be good functions to add imo.

dcousens commented 6 years ago

will confuse developers... so I think the amount should be handled in satoshis and allow for 3 decimal places

Developers will learn. Everyone said satoshis was confusing, but it is the unit for development (until millisatoshi, anyway).

Any "wrappers" we make around it will only serve to confuse. You literally just outlined an error case, where there should be no Error.

satoshisToHrp Human-readable-prefix? Reads as if it is related to Bech32.

New users wonder why transaction.addOutput(..., 0.12) isn't 0.12 Bitcoin... its because the internals don't work with Bitcoin. The same here.

I think this is an important consideration in how we make our API's sane.