Amounts displaying refactoring #1907

shoom3301 · 2023-01-20T08:18:21Z

Summary

How does it work

Docs: https://github.com/cowprotocol/cowswap/blob/fix/139/src/cow-react/utils/amountFormat/README.md
Implementation: https://github.com/cowprotocol/cowswap/blob/fix/139/src/cow-react/utils/amountFormat/index.ts#L18
Test cases: https://github.com/cowprotocol/cowswap/blob/fix/139/src/cow-react/utils/amountFormat/index.test.ts

Testing

localStorage.setItem('amountsRefactoring', '1') - run this in the browser console and reload the page to highlight places with the refactored amounts

What changed

Removed usage of @cowprotocol/cow-js
formatSmartAmount, formatSmart, formatSmartLocaleAware, formatMax were replaced by <TokenAmount> component
Fiat amounts we replaced by <FiatAmount> component
Changed formatting for swap and Limit orders form inputs (formatAmountInput function)
Fixed a bug in limit orders: 1) Set up a trade (WETH -> COW) 2) Don't change the rate, just add a couple of digits at the end (4468.9031 -> 4468.903177) 3) Wait ~15 sec 4) AR: the rate resets to the previous ER: the rate doesn't change

Affected area

Swap page
Limit orders. Please, pay attention on the receipt modal (amounts accuracy). The component uses BigNumber.js and because of it we convert amounts using .toNumber() cast
ETH flow stepper
Swap fees information + tooltips
Wrap/unwrap
Account page balances
Account locked GNO vesting
Account vesting
Vesting claim (status, table, investment flow, etc.)
Activity details (tx, order)
Cow token balance
Subsidy modal
Tokens list search modal
Tokens page
Order signing pending modal

vercel · 2023-01-20T08:18:25Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
swap-dev	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback

🌃 Cosmos ↗︎

github-actions · 2023-01-20T08:18:35Z

CLA Assistant Lite All Contributors have signed the CLA.

elena-zh

Hey @shoom3301 , great job!

However, we should think what to display when users trade with tiny amounts now:
And it would be also great to apply these changes to the Price field onn the Limit orders page:
There are no changes to amounts on an order confirm screen. Is this OK?
I noticed that 4 decimals are added to an amount when go to the Limit orders page and back to the Swap page:
Receive/from tooltip has different formatting for amounts:

Thanks!

anxolin · 2023-01-24T10:23:22Z

Why 4 decimals?

The UI should show you exactly what you sign. No more, no less :)

This means:

For Sell orders: We show how much you receive given the current price. We use the receive token precision, and round down (so we make sure the trade is executable)
For buy orders: We show how much you need to pay given the current price. We use the sell token precision, and round up (so we make sure the trade is executable)

anxolin · 2023-01-24T10:26:33Z

And it would be also great to apply these changes to the Price field onn the Limit orders page

Agree, price should have good precision, at least if it is a small amount.
I think the smart formatting tries to take the size of the value into account. If you are handling trillions, maybe you don't need decimals, but if you are dealing with something smaller than 0.1 you will need some.

Maybe is a matter of having XX significant digits?

anxolin · 2023-01-24T10:27:17Z

Receive/from tooltip has different formatting for amounts:

Yes we should be consistent

src/cow-react/modules/trade/utils/tokenViewAmount.ts

… fix/139 � Conflicts: � src/cow-react/modules/trade/utils/tokenViewAmount.ts

shoom3301 · 2023-02-01T09:03:07Z

@elena-zh thank you!
All must be fixed

elena-zh · 2023-02-01T13:59:43Z

Hey @shoom3301 , great job!
Some issues I see:

I don't think that we should show all decimals on Receive/From fields
The same with the tooltip: I tried to explain in the thread that is would be great to leave it as it is now (using formatted amounts) instead of the current view:

Sorry if I mixed you up and explained no clear it
Seems that now the form is overloaded with numbers now

Is it possible to use 4-decimals rounding for amounts that >1, and use the current logic (up to 18 decimals) when amounts are less than 1?
Can't insist on this, but think that it would be a bit nicer from UI perspective.

It would be great to add a space between the number and a token name in the last sentence. And not to show the full amount (as we agreed to show it in input fields only)

Thanks!

… fix/139

shoom3301 · 2023-02-02T09:23:02Z

@elena-zh thanks, fixed!

Is it possible to use 4-decimals rounding for amounts that >1, and use the current logic (up to 18 decimals) when amounts are less than 1?

It could create other problems, so I would like to think about it in another task

elena-zh · 2023-02-02T15:38:43Z

Some edge cases that I see now:

Amounts in the To/From fields do not match with the tooltip:
When a value is huge, lots of 0 are displayed in its end:

Received amount is in imcorrect on the modal:
Price still can show only zeros in the field
Amount is not rounded to 6 decimals in the Review limit order modal

alfetopito

Just a few more comments. I'll eventually reach 0 :D

src/cow-react/common/pure/CurrencyInputPanel/CurrencyPreview.tsx

src/cow-react/modules/limitOrders/pure/ReceiptModal/FilledField.tsx

src/cow-react/utils/amountFormat/README.md

src/cow-react/utils/amountFormat/index.test.ts

src/custom/components/CowBalanceButton/index.tsx

elena-zh · 2023-02-07T17:08:05Z

Hey @shoom3301 , amazing job! All issues I reported are fixed.
Some of them, as I mentioned above, are reported in separate tasks.
However, 1 tiny issue is found with localization of amounts:
France: there should be comma instead of period to separate a decimal part

Germany: the same (there should be comma instead of period to separate a decimal part):

Rus: the same (there should be comma instead of period to separate a decimal part):

Could you please take a look into this issue?

shoom3301 · 2023-02-08T06:33:12Z

@elena-zh thanks! Fixed!
Anyway, I would discuss it with the team, because I don't like the format of RU localization :) I used to see dots instead of commas

elena-zh

Approved!!!

shoom3301 · 2023-02-08T11:04:44Z

@SocketSecurity ignore [email protected]
@SocketSecurity ignore [email protected]
@SocketSecurity ignore [email protected]

alfetopito

Code is approved, no more comments \o/

Now testing the app, a few comments :D

ru language:

Shouldn't the thousands separator apply also to the input numbers?

pt-br language

I haven't seen all comments but I assume Elena has mentioned this.
The decimal separator is different in the display fields from the input field.
To be fair, this has been like this since the start.
Are we keeping this behaviour?

ar language (arabic)

Hehe, this is funny.
Not sure we want to do something about this, but anyway.
With this language we can notice that the decimals part is not respecting the language locale.

elena-zh · 2023-02-08T11:58:55Z

Hey @alfetopito

I haven't seen all comments but I assume Elena has mentioned this.
The decimal separator is different in the display fields from the input field.
To be fair, this has been like this since the start.
Are we keeping this behaviour?

I think it was discussed in https://cowservices.slack.com/archives/C0361CDG8GP/p1675354573189409?thread_ts=1675337080.573789&cid=C0361CDG8GP : there is a separate task for this #213 , and it will be addressed (one day) in a separate PR.

shoom3301 · 2023-02-08T13:43:48Z

Thanks @alfetopito !
We definitely should add formatting for inputs, but it requires enough efforts, so I think we should address it in the next iteration

Remove formatting of trade amounts using formatSmartAmount()

f51235b

shoom3301 requested review from a team January 20, 2023 08:18

shoom3301 self-assigned this Jan 20, 2023

vercel bot deployed to Preview January 20, 2023 08:26 View deployment

elena-zh reviewed Jan 20, 2023

View reviewed changes

alfetopito reviewed Jan 24, 2023

View reviewed changes

src/cow-react/modules/trade/utils/tokenViewAmount.ts Outdated Show resolved Hide resolved

src/cow-react/modules/trade/utils/tokenViewAmount.ts Show resolved Hide resolved

shoom3301 added 2 commits January 31, 2023 19:37

Merge branch 'develop' of https://github.com/cowprotocol/cowswap into…

9730904

… fix/139 � Conflicts: � src/cow-react/modules/trade/utils/tokenViewAmount.ts

Fix tokenViewAmount

7e16a0e

vercel bot deployed to Preview February 1, 2023 07:53 View deployment

Fix tokenViewAmount

0d14cdf

vercel bot deployed to Preview February 1, 2023 09:05 View deployment

alfetopito approved these changes Feb 1, 2023

View reviewed changes

shoom3301 added 2 commits February 2, 2023 15:11

Merge branch 'develop' of https://github.com/cowprotocol/cowswap into…

3cdce9e

… fix/139

Fix formatSmartAmount usage

80a3c65

vercel bot deployed to Preview February 2, 2023 09:25 View deployment

shoom3301 added 2 commits February 2, 2023 19:01

Use toSignificant() strategy for tokenViewAmount()

a7d2c61

Fix e2e test

d446afc

vercel bot deployed to Preview February 2, 2023 13:06 View deployment

Fix e2e test

192d99b

vercel bot deployed to Preview February 2, 2023 13:38 View deployment

shoom3301 and others added 2 commits February 7, 2023 20:27

Rename FF

9f98ef1

Fix code style issues with Prettier

57177f4

vercel bot deployed to Preview February 7, 2023 14:35 View deployment

shoom3301 added 2 commits February 7, 2023 20:49

Fix issues

958b204

Merge remote-tracking branch 'origin/fix/139' into fix/139

94ae0a3

vercel bot deployed to Preview February 7, 2023 14:55 View deployment

Round balance amount

83382af

vercel bot deployed to Preview February 7, 2023 15:18 View deployment

alfetopito reviewed Feb 7, 2023

View reviewed changes

Fix issues

d8b1bec

vercel bot deployed to Preview February 7, 2023 15:32 View deployment

shoom3301 added 2 commits February 7, 2023 21:39

Show less than 0.0001 for tokens with 0 decimals

36c967d

Fix e2e test

8e5199f

vercel bot deployed to Preview February 7, 2023 15:46 View deployment

elena-zh self-requested a review February 7, 2023 15:53

shoom3301 added 2 commits February 8, 2023 12:27

Fix issues

97df229

Fix (v)COW balance displaying

149a6a4

vercel bot deployed to Preview February 8, 2023 06:37 View deployment

Fix zero trim regarding locale

be28eb0

vercel bot deployed to Preview February 8, 2023 06:50 View deployment

elena-zh approved these changes Feb 8, 2023

View reviewed changes

alfetopito approved these changes Feb 8, 2023

View reviewed changes

shoom3301 merged commit c2ed079 into develop Feb 8, 2023

github-actions bot locked and limited conversation to collaborators Feb 8, 2023

alfetopito deleted the fix/139 branch February 8, 2023 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amounts displaying refactoring #1907

Amounts displaying refactoring #1907

shoom3301 commented Jan 20, 2023 •

edited

Loading

vercel bot commented Jan 20, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Jan 20, 2023

elena-zh left a comment

anxolin commented Jan 24, 2023

anxolin commented Jan 24, 2023

anxolin commented Jan 24, 2023

shoom3301 commented Feb 1, 2023

elena-zh commented Feb 1, 2023 •

edited

Loading

shoom3301 commented Feb 2, 2023 •

edited

Loading

elena-zh commented Feb 2, 2023

alfetopito left a comment

elena-zh commented Feb 7, 2023 •

edited

Loading

shoom3301 commented Feb 8, 2023

elena-zh left a comment

shoom3301 commented Feb 8, 2023

alfetopito left a comment

elena-zh commented Feb 8, 2023

shoom3301 commented Feb 8, 2023

Amounts displaying refactoring #1907

Amounts displaying refactoring #1907

Conversation

shoom3301 commented Jan 20, 2023 • edited Loading

Summary

How does it work

Testing

What changed

Affected area

vercel bot commented Jan 20, 2023 • edited by github-actions bot Loading

github-actions bot commented Jan 20, 2023

elena-zh left a comment

Choose a reason for hiding this comment

anxolin commented Jan 24, 2023

anxolin commented Jan 24, 2023

anxolin commented Jan 24, 2023

shoom3301 commented Feb 1, 2023

elena-zh commented Feb 1, 2023 • edited Loading

shoom3301 commented Feb 2, 2023 • edited Loading

elena-zh commented Feb 2, 2023

alfetopito left a comment

Choose a reason for hiding this comment

elena-zh commented Feb 7, 2023 • edited Loading

shoom3301 commented Feb 8, 2023

elena-zh left a comment

Choose a reason for hiding this comment

shoom3301 commented Feb 8, 2023

alfetopito left a comment

Choose a reason for hiding this comment

elena-zh commented Feb 8, 2023

shoom3301 commented Feb 8, 2023

shoom3301 commented Jan 20, 2023 •

edited

Loading

vercel bot commented Jan 20, 2023 •

edited by github-actions bot

Loading

elena-zh commented Feb 1, 2023 •

edited

Loading

shoom3301 commented Feb 2, 2023 •

edited

Loading

elena-zh commented Feb 7, 2023 •

edited

Loading