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

Change tftprice():u32 digit count #857

Open
A-Harby opened this issue Aug 31, 2023 · 5 comments
Open

Change tftprice():u32 digit count #857

A-Harby opened this issue Aug 31, 2023 · 5 comments
Labels
priority_minor type_bug Something isn't working
Milestone

Comments

@A-Harby
Copy link

A-Harby commented Aug 31, 2023

Can we change TFT price decimal value from only 3 digits to 7 digits, since most of the calculation is done based on tftprice():u32 value which doesn't give the accurate precision amount.

image

Even on the stellarchain the value is less than tfchain.
image

@sameh-farouk sameh-farouk added this to the later milestone Oct 31, 2023
@sameh-farouk
Copy link
Member

@A-Harby can you elaborate or include more context?

@A-Harby
Copy link
Author

A-Harby commented Oct 31, 2023

Yeah, what I meant was that TFchain is using a 3-digit representation with a TFT value, and when the TFT current value of 0.006x is fetched from the stellar chain, it is approximate, and when dealing with numbers with such a small value, it is very important not to discard the small digits, which will result in a wrong billing or conversion not being accurate, especially since our billing is calculated per hour with a small amount usually.

So my suggestion was to increase the TFT digit to have more accurate results, and based on Hanafy's recommendation, it was decided to be 7 digits instead of only 3.

@sameh-farouk sameh-farouk added priority_major type_bug Something isn't working and removed priority_major labels Oct 31, 2023
@sameh-farouk
Copy link
Member

okay we should look into this

@sameh-farouk
Copy link
Member

sameh-farouk commented Nov 1, 2023

TFchain is using a 3-digit representation with a TFT value

First, Let me correct that assumption.

TFChain tokenDecimals is configured in the chain spec of the chain as 7 decimals.

https://github.com/threefoldtech/tfchain/blob/38ea78797ca2bb2c1ee27efa43b607c86b322e6c/substrate-node/chainspecs/main/chainSpecRaw.json#L8-L10

You can call also use system_properties RPC call from polkadot.js UI to returns the on-chain properties and check the tokenDecimals and verify that.

Each balance amount is encoded in TFChain as a unsigned 64-bit integer. The balance amount unit seen by end-users is scaled down by a factor of ten million (10,000,000) .
For example, the integer amount value 25,123,456 equals 2.5123456 TFT. This scaling allows for seven decimal places of precision in human-friendly amount units.
This allows TFChain to store amount as low as 0.0000001 TFT (one ten-millionth) which stored as an integer value of one.

Stellar store asset amount in similar fashion and also allows for seven decimal places of precision but stored it as signed 64 bit integer so in Stellar the largest amount unit possible is significantly lesser than TFChain.

Now, how this stored integer encoded later and presented to end user is left to the clients and library. If there is an precision issue when query account balance you should check how the client encode the balance.

Now back to the value you referred to, the TFT price. If you complain about the price stored in chain is 7 which later encoded by clients as 0.007 (7/1000) while the price on stellar is 0.0065 then this is a different issue.

Here, the offchain worker retrieve the price for a hundred TFT in USDC from stellar (per the time of my testing the price is 0.6530164) and do some converting, calculation and rounding that cause the lose of precision in the result (7 ) as we scaled up the price by a factor of one thousand only (allows for only three decimal places of precision).

Some(((lowest) * 1000).round().to_num::<u32>())

let tft_usd = (U32F32::from_num(price) / U32F32::from_num(DST_AMOUNT))
.round()
.to_num::<u32>();

Before we modify anything, let's make sure that this is an bug and not that it was planned this way on purpose. can you confirm @LeeSmet @robvanmieghem @xmonader ?

@LeeSmet
Copy link
Contributor

LeeSmet commented Nov 2, 2023

The thing here is that token price is expressed in mUSD, miliUSD. This is used everywhere in both specs and documentation. As such, the current representation is in line with these specs. When it comes to regular currency, 3 is the most that is generally used (which is what we have).

The main problem is indeed the low token price. The issue with adjusting precision now for low price is that it is essentially a never ending street. Adding more precision now would be a temporary fix because there is no guarantee that the price does not drop to a level where the new precision is also insufficient to express a possible future low price beyond what the new price would be.

This also leads to problems with hyperinflation, as adjusting price now and using a more accurate representation would lead to deployment cost suddenly going up dramatically (by 50% currently).

There are likely other ramifications as well with changing this.

@xmonader xmonader added this to 3.14.x Nov 5, 2023
@xmonader xmonader removed this from 3.13.x Nov 5, 2023
@renauter renauter removed this from 3.14.x Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority_minor type_bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants