Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch financial information to be stored as integers #416

Open
BerglundDaniel opened this issue Jan 5, 2024 · 9 comments
Open

Switch financial information to be stored as integers #416

BerglundDaniel opened this issue Jan 5, 2024 · 9 comments
Labels
admin:shop Shop part in the admin interface

Comments

@BerglundDaniel
Copy link
Member

BerglundDaniel commented Jan 5, 2024

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.

@BerglundDaniel BerglundDaniel added the admin:shop Shop part in the admin interface label Jan 5, 2024
@HalfVoxel
Copy link
Contributor

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

@HalfVoxel
Copy link
Contributor

HalfVoxel commented Jan 5, 2024

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
Copy link
Contributor

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

@HalfVoxel
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

I experimented some and the db is using floats for me

@emanuelen5
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

BerglundDaniel commented Jan 13, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admin:shop Shop part in the admin interface
Projects
None yet
Development

No branches or pull requests

3 participants