Fix finance USD conversion mechanism #1177
Conversation
| // Apart from always considering the USD decimals (2), | ||
| // if there's the strange case that the above is negative, | ||
| // we take it as a carry as we know we already shifted to far, | ||
| // and will compensate by shifting the token amount by this much | ||
| const carryAmount = | ||
| decimalsToShift.toNumber() < 0 | ||
| ? decimalsToShift.add(USD_DECIMALS) | ||
| : USD_DECIMALS |
There was a problem hiding this comment.
I'll admit I haven't found practical examples, but I'm just making sure! 😃
There was a problem hiding this comment.
Ouch I completely missed that!
I was wondering if a good strategy could be to do:
- Scale the conversion ratio up to the same number of decimals than the given token.
- Use
divideRoundBigInt()to get the result in USD and scale the amount down to 2 decimals. - Use
formatTokenAmount(usdAmount, 2)to display it.
Let me know what you think!
|
This sounds good! Wondering on how would we scale up the conversion ratio up to the proper number of decimals without potentially running into overflow issues though? It's really why there's a lot of string manipulation here. |
| // Return it to its original precision | ||
| // Note that we don't have to subtract the "extra carry" | ||
| // as it's undone during the division | ||
| .mul(precisionTarget) |
There was a problem hiding this comment.
Not sure if we need to consider any USD decimals here, as our "USD" converted amount is still in the original token's decimals after the conversion.
Wondering if we can simplify everything down to:
const [whole = '', dec = ''] = convertRate.split('.')
const parsedDec = dec.replace(/0*$/, '')
const carryAmount = parsedDec.length
const rate = `${whole}${parsedDec}`.replace(/^0*/, '')
return amount
.mul(new BN('10').pow(carryAmount))
.div(new BN(rate))
As we don't need to use the decimals at all.
There was a problem hiding this comment.
I think we could completely cut the string manipulations by multiplying the two values and relying on formatTokenAmount(). What do you guys think?
PR: #1180
There was a problem hiding this comment.
My only worry with the above is overflow issues on many tokens due to the ceiling for safe numbers in Javascript (Number.MAX_SAFE_INTEGER) is around 9*10^15, having 3 decimals less than most tokens, which have 18. This is really the only reason why I considered doing this string manipulation, rather than just shifting the decimals by normal math.
There was a problem hiding this comment.
Yes, the loss of precision with Number is exactly why are using BN.js, and the proposed solution is taking care of the token decimals… but I realize now that rate itself, once scaled up, could also easily become too large :-(
I am wondering if it would be easier to implement TokenAmount.convert() directly? We would use a mix of string splitting for the rate, and BigInt calculations otherwise.
There was a problem hiding this comment.
could also easily become too large :-(
Yeah, and the solutions are not pretty :( In general these conversions are a headache when maintaining them in BigInts.
I am wondering if it would be easier to implement TokenAmount.convert() directly? We would use a mix of string splitting for the rate, and BigInt calculations otherwise.
I think it could be good! it's also pretty high leverage as doing these conversions is both hard to get 100% right, and at the same time extremely common to do. Should we wait for .convert() to be implemented, or should we merge and then upgrade once it's done? Asking this mainly because it's breaking Aave's finance app.
There was a problem hiding this comment.
I was thinking that we could add it to TokenAmount directly and use it from here, but we could merge this first to make sure it gets fixed quick yes.
I added the commit with the Jest configuration.
What would you think of:
- Perhaps simplify a bit the process, following @sohkai’s comment.
- Add some tests to make sure everything works fine.
- Merge it until
TokenAmount.format()is ready?
There was a problem hiding this comment.
Sounds good to me! Let's do that.
* Fix finance USD conversion mechanism * Add Jest * Simplify conversion function * Add some tests for the conversion utility Co-authored-by: Pierre Bertet <[email protected]>
* Agent: replicate rate conversion strategy from aragon#1177 * Remove unused parameter * Remove redundant return on map :)
Fixes a problem that arose with the move to using bn.js everywhere, where the conversion from Token Amount to USD was being done incorrectly, as bn.js itself can't handle decimals, and they were being passed onto the function. This adds a few things:
toDecimalandfromDecimalexist, but required a bit of hacking to make them work for this, so I implemented another version)conversion-utils.jsto remove bloat from theBalances.jsfile.Few screenshots from the AAVE Org (where the problem arose due to WBTC breaking the previous implementation):
