-
Notifications
You must be signed in to change notification settings - Fork 127
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
Add formatTokenAmount() and formatNumber() #728
Add formatTokenAmount() and formatNumber() #728
Conversation
Thanks for opening this pull request! Someone will review it soon 🔍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we didn't do it on the format.js
file, but it'd be great to add a bit of JSDoc to it :).
Wondering if there are any other changes I need to make? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boom! Lookin' fine 🔥. Just some comments to improve this further!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌🙌🙌I can't wait to use this everywhere!! No more of my BN.js hacks running around!
Co-Authored-By: Brett Sun <[email protected]>
…ss/aragon-ui into add-number-formatting-utilities
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in https://github.com/aragon/aragon-ui/pull/760/commits/7ed7f5109073ba0cbca45c83ba279cca569a9325
- Removed the default
18
fromdecimals
to make it a required parameter. roundToDecimals
has moved intooptions.digits
.
}) | ||
|
||
test('should handle large numbers without decimals', () => { | ||
expect(formatTokenAmount(BigInt('2839000000000000000000'), 18, 8)).toEqual( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, we forgot to update the parameter name here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a
formatNumber()
andformatTokenAmount()
utility for issue #719