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

Thousand amount separator #2394

Closed
hynek-jina opened this issue Jul 20, 2022 · 6 comments · Fixed by #2425
Closed

Thousand amount separator #2394

hynek-jina opened this issue Jul 20, 2022 · 6 comments · Fixed by #2425
Assignees
Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature Product related issue visible for end user T1B1 legacy Trezor One

Comments

@hynek-jina
Copy link

hynek-jina commented Jul 20, 2022

With the use of sat units, there was a need to implement thousand separators for amount in Trezor Suite.
trezor/trezor-suite#5545

The user needs to see the amount in the Suite and on the Trezor device in the same format for control when sending a transaction.

Most required is en-us formatting (like i.e. 123,456.78) but more general solution would be even better for the future.

Acceptance criteria

Given I view my coins in sats
When I prepare a transaction for 4000 sats
And I press Review & Send
Then I should see 'Confirm sending 4,000 sat' on the Trezor

Model 1

Given I see 'Confirm sending 4,000 sat'
When I tap 'Confirm'
Then I should see 'Really send 4,153 sat'

Model T

Given I see 'Confirm sending 4,000 sat'
When I tap 'Confirm'
Then I should see 'Total amount: 4,153 sat'

@hynek-jina hynek-jina added the feature Product related issue visible for end user label Jul 20, 2022
@sime
Copy link
Contributor

sime commented Jul 21, 2022

Having a hardcoded locale scheme for a thousand separator when sats are used should be trivial.

We want to avoid locale's that support spaces. Then a single scheme can cover the majority of cases.

@matejcik We would like to avoid spaces for security POV?

@prusnak
Copy link
Member

prusnak commented Jul 21, 2022

We don't respect locale on Trezor firmware and always use . as a decimal separator. I think we should agree on a thousands separator ( or , or ') and use that statically. Suite should use the same separators as firmware, thus it should not change it according to locale.

@hynek-jina
Copy link
Author

Then we can go simply with , thousand separator which is currently in Suite as well.

@sime sime added core Trezor Core firmware. Runs on Trezor Model T and T2B1. T1B1 legacy Trezor One labels Jul 22, 2022
@sime sime moved this to 🔥 Priority in Firmware Jul 22, 2022
@hynek-jina hynek-jina moved this from 🔥 Priority to 🏃‍♀️ In progress in Firmware Aug 2, 2022
Repository owner moved this from 🏃‍♀️ In progress to 🤝 Needs QA in Firmware Aug 2, 2022
@bosomt
Copy link

bosomt commented Aug 5, 2022

QA OK

TT OK

Info:

  • Suite version: desktop 22.8.1 (503b472935dec8a9c07aac5eef5ba43f86780a38)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuite/22.8.1 Chrome/100.0.4896.160 Electron/18.3.5 Safari/537.36
  • OS: MacIntel
  • Screen: 1680x1050
  • Device: model T 2.5.2 Universal (revision 0d87b55)
  • Device: model 1 1.11.2 Universal (revision 0d87b55)
  • Transport: bridge 2.0.31

@hynek-jina hynek-jina moved this from 🤝 Needs QA to ✅ Approved in Firmware Aug 5, 2022
@bosomt
Copy link

bosomt commented Aug 5, 2022

Maybe we can consider to adding this separator to Trezor Suite too ?
It would look clearer.

image

image

@hynek-jina
Copy link
Author

@bosomt
It's almost there: trezor/trezor-suite#5545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature Product related issue visible for end user T1B1 legacy Trezor One
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants