gRPC `SubscribeAddedOrders` + `SubscribeRemovedOrders` float issue

kilrau commented 5 years ago

------------ADDED------------
price: 0.0073
quantity: 23.978099999999998
pair_id: "LTC/BTC"
id: "37bfa860-f999-11e8-b029-671f6857cde7"
peer_pub_key: "03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0"
created_at: 1544165201801

should be 23.9781

kilrau commented 5 years ago

Can you check this one? @sangaman

sangaman commented 5 years ago

Sorry this slipped by me.

I think this is a shortcoming of using double for the proto field definition. There's definitely sufficient precision to round correctly, but the true value will always be something that's off by some tiny amount.

This is also partly a shortcoming of whatever tool was used to create that output, but if we're seeing it there it will probably pop up elsewhere too. It's written in python right?

I can think of two possible approaches:

Switch to uint64 for quantity and measure quantity in satoshis. Consumers of the API would then need to know to divide by 10^8 for display purposes. Where this gets a little tricker is with price which is also currently a double and would experience the same possible imprecisions, but I don't think it's as intuitive to measure price in satoshis. I'd be somewhat hesitant to implement this.
Keep the proto file the same, and provide some instruction to consumers of the API to round to 8 decimal places for display purposes.

Would be good to see if anyone else has other ideas or cleaner approaches. @moshababo @offerm

Being able to see the code for the tool that created that output might help too.

kilrau commented 5 years ago

uint64 for quantity sounds right. Switching to Satoshi's also fine with me, but could we check how lnd handle's this since they go even sub-satoshi as far as I know

kilrau commented 5 years ago

Getting the code of the application into our organization asap...

kilrau commented 5 years ago

Here it is: https://github.com/ExchangeUnion/xud-exchange-integration-example @sangaman , a full environment setup and ready to use in our glcoud xud-testing project

sangaman commented 5 years ago

Lnd is using satoshis everywhere amounts are concerned, sub-satoshis are mostly used internally. Let's discuss this on the call today since I don't think there's an obvious solution here.

kilrau commented 5 years ago

As discussed on the call we aim for integers as internal representation without decimals. As default, 9 digits form one "full" unit = we have a default precision of 8 decimals. Also this needs to be made crystal clear in the documentation (as part of this issue).

michael1011 commented 5 years ago

Just to be sure: this issue is about switching all internal quantities to satoshis and not just converting to and from satoshis in the GrpcService and Service classes? And do we have a decision on whether to denominate the price in satoshis? @sangaman @kilrau

kilrau commented 5 years ago

I'd say hold off on this. Price in Satohis is weird. Price in BTC and quantity in Satohis is weird too. We might just go for the decimal precision in the documentation and that's it. Any better ideas?

Decimal precision with sensible defaults (10^8) sounds good to me.

sangaman commented 5 years ago

Just to be sure: this issue is about switching all internal quantities to satoshis and not just converting to and from satoshis in the GrpcService and Service classes? And do we have a decision on whether to denominate the price in satoshis? @sangaman @kilrau

Let's discuss this on the call again since it's been a little while, although now I'm again questioning whether this is an issue worth dealing with at the moment. Changing all the internal types is a pretty big change, and it may make more sense to have clients round for display purposes. Rounding/formatting the number will have likely need to happen anyway, especially for prices. Python uses the same IEEE 754 double precision floating numbers that javascript does.

https://stackoverflow.com/questions/31824958/how-to-send-floating-point-numbers-over-network-using-google-protobuf

kilrau commented 5 years ago

@michael1011 as discussed in the call we stick with what we have, just add a line about this in the wiki.

kilrau commented 5 years ago

I'm very sorry to reopen this after we switched back and forth twice already, but I got a db-experienced friend over and the first thing he screamed when I showed him the xud order book: "OMG, are you using floats to store this?" "Never use floats with money."

And it seems he is right, floats are only a approximation of a decimal number but never precise because it's stored as binary. Currently it's not a big problem because we don't perform (many/any?) operations on prices or quantities but as soon as we do rounding errors become a real issue.

So I suggest to redefine the scope of this issue to:

replace all quantities and prices internally and on the gRPC with a bigint, using smallest unit of each currency: satoshis, litoshis, wei.
add documentation for this in wiki, api.md and other places I forgot
adjust the CLI to still display things how it currently does

Source: https://husobee.github.io/money/float/2016/09/23/never-use-floats-for-currency.html

sangaman commented 5 years ago

Wei won't work I'm pretty sure because it's too small a unit to represent with 64 bit numbers. I'd say if we go this route we should stick to using 10^-8 (satoshi size) for all base units internally. It's standard for exchanges to have minimum order size increments and I think similar logic applies.

Currently we track amounts of coins by integer satoshis, and order quantity/prices with floating point numbers. When we calculate amounts of satoshis, we do the multiplication/division, move the decimal point over, and round to the nearest integer.

I'm still not convinced that representing price as an integer is a good idea. It's not intuitive and it won't entirely eliminate the need for rounding for times when we have to divide a number by the price.

For quantities though I'm on board to switch to using satoshi integers as the base unit everywhere internally.

kilrau commented 5 years ago

Wei won't work I'm pretty sure because it's too small a unit to represent with 64 bit numbers

Quickly checked that, you are right, even bigint is not enough. Agree on 10^-8 precision for all. Should have check that before.

I'm still not convinced that representing price as an integer is a good idea. It's not intuitive and it won't entirely eliminate the need for rounding for times when we have to divide a number by the price.

Not intuitive, maybe. Doesn't eliminate the need for rounding: definitely not.

What it DOES though is forcing everyone to perform & think about rounding when storing a result of an arithmetic operation. With float numbers one might forget that very easily. And the result becomes more inaccurate the more arithmetic operations are performed on a float value which wasn't rounded corrected manually on each step. Check another example (here)[https://dzone.com/articles/never-use-float-and-double-for-monetary-calculatio]. I just think that might seem like a small problem now, but it will be a very weird "wrong price" bug in future and hard to debug.

An alternative could be BigDecimal instead of BigInt to keep the intuitivity of decimals for price. Got to pay attention how to use the constructor though as described in here. So for me there are 3 ways forward:

bigint for all prices. Internally and gRPC. Con: not intuitive
bigint for all prices. Internally (for all operations). gRPC layer uses float values)
BigDecimal for all prices. Internally and gRPC.

Actually 3. does sound like the cleanest.

sangaman commented 5 years ago

There is no native JS big integer/big decimal. We could use something like https://www.npmjs.com/package/big-js, but I believe this just does the rounding for us and I'm not familiar with this package and not sure it'd help enough to warrant another dependency. But gRPC does not have anything like big decimal, if we want a precise decimal number we'd have to use a string I'm pretty sure.

We already round the result of the arithmetic operations to calculate amount based on quantity & price, and I think in this case it would be making the amount a big decimal type that would enforce rounding, not the price. You can divide two decimal numbers and wind up with a floating number.

Here's what I propose:

Use integer satoshis for all quantities internally and on the gRPC layer. Quantity is only subject to addition/subtraction operations with other integer quantities, never division that may introduce sub-integer values.
Leave price as is. I suspect that we actually won't even need any client-side rounding for display purposes despite some of my earlier concerns, as seen in the bug report to start this issue.

kilrau commented 5 years ago

Ok. Please check if you agree with Daniel and if so go ahead with his approach. @michael1011

sangaman commented 5 years ago

Thanks. I think another key point here is that the price, once set, does not change. I suspect that the quantity may have been going off by microscopic amounts when we updated it.

I created a jsfiddle to recreate a similar error: https://jsfiddle.net/sangaman/rce4kx30/

const x = 1.3498;
const y = x - 1.1111;
const z = y - 0.2387;

It simulates an order with a quantity of 1.3498. It gets matched with an order with 1.1111 quantity, and then another order with 0.2387 quantity. You'd expect it to now have a quantity of 0, but z actually has a value of 1.3877787807814457e-16. By using integers for quantity we can be confident that seemingly simple addition/subtraction won't give us unexpected results. Removing the decimal points in the code above leaves us with zero.

kilrau commented 5 years ago

LGTM @michael1011

ExchangeUnion / xud

gRPC `SubscribeAddedOrders` + `SubscribeRemovedOrders` float issue #740