Skip to content

Conversation

@norwnd
Copy link
Contributor

@norwnd norwnd commented Apr 1, 2023

Resolves #2254.

Rate fits reasonably well with the longest canceled/settling status and max rate step we currently have = 6

image

@norwnd
Copy link
Contributor Author

norwnd commented Apr 1, 2023

Note, fourSigFigs results in non-precise rate shown in "Your Orders" section (both in body and header now, should be 1.000001, not 1):

image

Is it something we want to fix (assuming it's possible to do without using external decimal-arithmetic libs) ? Reading through this discussion I've got a feeling that we don't care about this when displaying spot price (or similar). For order rate however user might be somewhat uneasy to see such rounding imo, but then ... we might run out of space to display rate without rounding.

@chappjc chappjc added this to the 0.6.1 milestone Apr 4, 2023
@chappjc chappjc requested a review from buck54321 April 4, 2023 02:10
@buck54321
Copy link
Member

I draw a distinction between numerical "display" values and "data" values. Display values, such as the market statistics shown at the top of the markets view, can employ rounding or truncation and should use significant figures to make values "pretty". Data values, such as order data, should not employ rounding, and should use e.g. UnitInfo.Conventional.ConversionFactor to decide how many digits/decimals to display.

@norwnd
Copy link
Contributor Author

norwnd commented Apr 8, 2023

To resolve the issue mentioned in #2281 (comment) I'll need to apply similar fix implemented in #2278 (comment), for rate included in "header" in this PR.

@chappjc
Copy link
Member

chappjc commented Apr 17, 2023

Can we just get this merged with the current reasonable solution (perhaps just forcing at least one decimal)? #2278 (comment) may contain the resolution, but is a can of worms and will preclude this small change from landing in 0.6.1

@chappjc
Copy link
Member

chappjc commented Apr 19, 2023

1.000001 is a rate that isn't likely to be practical (too much precision), so I'm inclined to just merge this as-is if there's not a quick resolution tomorrow.
Or for the purposes of this PR (without #2278 (comment) and strict rate step observation), we could use Doc.formatFiveSigFigs(rate) instead of of fourSigFigs(rate).

@norwnd
Copy link
Contributor Author

norwnd commented Apr 19, 2023

1.000001 is a rate that isn't likely to be practical (too much precision), so I'm inclined to just merge this as-is if there's not a quick resolution tomorrow.

I'd rather not try to come up with yet another way to handle this specific case (and I don't see anything simple we can do here), so I'd wait until #2278 (comment) is ready,

but merging as is is also fine, I believe, because we show this trimmed rate in "details" anyway, so showing in header as well doesn't really break anything.

@chappjc
Copy link
Member

chappjc commented Apr 19, 2023

Right it's no deviation from existing convention. I'm asking for @buck54321 call though regarding asap merge resolution.

@chappjc chappjc merged commit 4ff1cee into decred:master Apr 19, 2023
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: display rate for the collapsed view of orders in the 'your orders' section

3 participants