makerspace / makeradmin

Stockholm Makerspace member administration and store.
20 stars 14 forks source link

Switch financial information to be stored as integers #416

Open BerglundDaniel opened 8 months ago

BerglundDaniel commented 8 months ago

We currently use floating point numbers to store prices, transaction amounts etc. This can causes errors because of various floating point related drawbacks when it comes doing math with them. I suggest we switch to using only integers with cent/ören. I don't think we have any bugs caused by this atm but for reliability we should probably fix it in the future.

I think we could convert the existing info by multiplying it with 100 and rounding off any extra digits. In theory there shouldn't be any extra digits.

HalfVoxel commented 8 months ago

I think a large part of the python code already uses the Decimal type to handle amounts.

HalfVoxel commented 8 months ago

We also store things in the database using the Numeric(precision="15,3") type for products. Which is a decimal type with 3 digits of precision. We need 3 digits because some things can have a very low per-unit cost, but you buy a lot of them (e.g. grams of brass). For transactions, we use Numeric(precision="15,2").

HalfVoxel commented 8 months ago

The frontend is kinda lax on the precision, though. But the backend validates that the user was presented with the correct price.

HalfVoxel commented 8 months ago

At least, I think our db stores them as decimals. It's possible that our database doesn't have support for decimals, and sqlalchemy falls back to floats.

BerglundDaniel commented 8 months ago

Decimal works but can be a bit tricky too. The database should be in decimal if I understand the code correctly, but as you say it could also be that the engine doesn't support it.

I think integers would be easier to deal with than Decimal. But if we should change depends on on what others think. I also need to read up a bit more about it. The downside with integers would be that it would set a minimum price per unit of 1 öre or such. We for instance have acrylic with 100g unit instead of 1g so that can work too for high volume with low cost per volume items. Might be other downsides too

The downside with Decimal is has limitations which might not be obvious. Integers might be more clear with their limitations. At the same time most of the code likely doesn't need to deal with Decimals on that level

BerglundDaniel commented 8 months ago

I experimented some and the db is using floats for me

emanuelen5 commented 8 months ago

I honestly think this might be just a waste of time. Do we have any indication that the float precision would be too small and make any calculations become inaccurate?

BerglundDaniel commented 8 months ago

It's not the precision itself that is the problem. The problem is that floats can not be represented exactly in binary form. See for instance https://www.laac.dev/blog/float-vs-decimal-python/

BerglundDaniel commented 7 months ago

Sql says that the db is decimal

mysql> SELECT COLUMN_NAME, DATA_TYPE  FROM INFORMATION_SCHEMA.COLUMNS  WHERE TABLE_NAME = 'webshop_transactions';

+-------------+-----------+
| COLUMN_NAME | DATA_TYPE |
+-------------+-----------+
| amount      | decimal   |
| created_at  | datetime  |
| id          | int       |
| member_id   | int       |
| status      | enum      |
+-------------+-----------+
5 rows in set (0.00 sec)

But in Python sqlachlemy says it is float or int

test-test-1              | INFO     makeradmin:db.py:239 transaction: Transaction(id=None, amount=100.0, type=<class 'decimal.Decimal'>, status=completed, created_at=None)
test-test-1              | INFO     makeradmin:db.py:244 create_transaction after db commit***
test-test-1              | INFO     makeradmin:db.py:245 transaction: Transaction(id=1, amount=100, type=<class 'int'>, status=completed, created_at=2024-01-05 23:14:15)