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

client/core,app: Fix rate/volume decimal precision #2208

Merged
merged 2 commits into from Mar 15, 2023

Conversation

martonp
Copy link
Contributor

@martonp martonp commented Mar 7, 2023

Closes #2174

client/core/core.go Outdated Show resolved Hide resolved
Comment on lines 746 to 745
tmpl.price.textContent = fourSigFigs(convRate)
tmpl.price.textContent = withNumDecimals(convRate, mkt.convRateNumDecimals)
Copy link
Member

@buck54321 buck54321 Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point is to lop off extra zeros in the decimals. In many cases, this new scheme will force the showing of decimal places we don't care about. For example, if the current rate is 1,284.0, and the rate step is 1e4, this will show 1,284.0000, which is clearly not what we want. Sig figs is the way to go. Nobody should care about seeing more than 3 or 4. We just need to clean up the max number of decimals too.

Edit: Actually, with the integer handling, a better example would be a price of 98.0 and a rate step of 1e2, resulting in 98.000000 displayed. The point is that we don't need or want exact values to the decimal place in these displays. Sig figs is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, no need for exact values.

Copy link
Member

@chappjc chappjc Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the intent then to be using maximumFractionDigits, not maximumSignificantDigits (for the big numbers)?

Also, regardless of value, I feel like we'd want at least one decimal e.g. 1,284.0 is Buck's example.

Copy link
Member

@chappjc chappjc Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance:

const FourSigFigsSmall = new Intl.NumberFormat('en-US', {
  minimumFractionDigits: 1,
  maximumSignificantDigits: 5
})

const FourSigFigsBig = new Intl.NumberFormat('en-US', {
  minimumFractionDigits: 1,
  maximumFractionDigits: 1
})

function fourSigFigs (v) {
  if (v<1000) return FourSigFigsSmall.format(v)
  return FourSigFigsBig.format(v)
}

console.log(fourSigFigs(123456.789))
console.log(fourSigFigs(123456))
console.log(fourSigFigs(1234.789))
console.log(fourSigFigs(123.789))
console.log(fourSigFigs(0.01357))
console.log(fourSigFigs(0.0121357))
console.log(fourSigFigs(0.0001357))

Output:

> "123,456.8"
> "123,456.0"
> "1,234.8"
> "123.79"
> "0.01357"
> "0.012136"
> "0.0001357"

(excuse the use of 5 there however)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The minimumFractionDigit is ignored in FourSigFigsSmall because it is overridden by the default minimumSignificantDigits of 1. You can't really mix those.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/NumberFormat#roundingpriority

I've changed it so that it always adds a decimal though.

@chappjc chappjc added this to the 0.6 milestone Mar 13, 2023
@chappjc chappjc merged commit c6e94a4 into decred:master Mar 15, 2023
5 checks passed
@martonp martonp deleted the rateVolDecimals branch January 20, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: trim Rate precision to Market rate step
4 participants