Add formatTokenAmount() and formatNumber() #728
Conversation
|
Thanks for opening this pull request! Someone will review it soon 🔍 |
ECWireless
changed the title
|
Wondering if there are any other changes I need to make? |
There was a problem hiding this comment.
Thank you for the PR @ECWireless!
I left one comment about supporting big integers, and I also wanted to ask if you could add some tests to the PR? You can find an example here: https://github.com/aragon/aragon-ui/blob/master/src/utils/color.test.js
Co-Authored-By: Brett Sun <[email protected]>
…ss/aragon-ui into add-number-formatting-utilities
|
Congrats on merging your first pull request! Aragon is proud of you 🦅 |
| return [...integer].reduceRight( | ||
| (result, digit, index, { length }) => { | ||
| const position = length - index - 1 | ||
| if (position > 0 && position % 3 === 0) { |
There was a problem hiding this comment.
Ah jeez, I thought reduceRight() would change the index so that it started "backwards" from 0. That was one of the main simplifications I had wanted with it :).
What do you think instead of doing this with [...integer].reverse().reduce()? That way the position is the index :).
| amount, | ||
| decimals = 18, | ||
| roundToDecimals = 2, | ||
| { symbol = '', displaySign = false } = {} |
There was a problem hiding this comment.
Ah, I missed this in the first pass around... but I wonder if it would be worth making decimals and roundToDecimals part of the options object?
There was a problem hiding this comment.
Done in https://github.com/aragon/aragon-ui/pull/760/commits/7ed7f5109073ba0cbca45c83ba279cca569a9325
- Removed the default
18fromdecimalsto make it a required parameter. roundToDecimalshas moved intooptions.digits.
| }) | ||
|
|
||
| test('should handle large numbers without decimals', () => { | ||
| expect(formatTokenAmount(BigInt('2839000000000000000000'), 18, 8)).toEqual( |
There was a problem hiding this comment.
Might be worthwhile adding a test like formatTokenAmount(BigInt('2839000000010000000000'), 18, 8) or something, just to see the decimal truncation after the last significant digit.
There was a problem hiding this comment.
Done and good find! It made visible an issue with the way leading & trailing zeros were handled: https://github.com/aragon/aragon-ui/pull/760/commits/6435c36c47a6df33b7e6ac1abd0c0a976f00e3a2
|
|
||
| const amountConverted = divideRoundBigInt( | ||
| amount, | ||
| JSBI.exponentiate(_10, JSBI.subtract(decimals, roundToDecimals)) |
There was a problem hiding this comment.
Just wondering what happens if this goes negative. Should we guard against this and min it at 0?
There was a problem hiding this comment.
I think we could throw in that case, what do you think? https://github.com/aragon/aragon-ui/pull/760/commits/12b1e72804b30d1ecde51f0940e1bec53b167277
| * @param {BigInt|string|number} divisor Divisor | ||
| * @returns {string} | ||
| */ | ||
| function divideRoundBigInt(dividend, diviser) { |
There was a problem hiding this comment.
Ahh, we forgot to update the parameter name here :)
There was a problem hiding this comment.
bpierre
changed the title
Adding a
formatNumber()andformatTokenAmount()utility for issue #719