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.

Sign up for GitHub

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

Jump to bottom

Amount units #1369

Merged
merged 4 commits into from
Jan 22, 2021
Merged

Amount units #1369

merged 4 commits into from
Jan 22, 2021

Conversation

prusnak
Copy link
Member

@prusnak prusnak commented Nov 25, 2020 •
edited
Loading

Fixes #324

  • add amount units to Protobuf
  • implement amount units in Core
  • implement amount units in Legacy
  • add UI tests

@prusnak prusnak added this to the 2021-01 milestone Nov 25, 2020
@tsusanka tsusanka modified the milestones: 2021-01, 2021-02 Dec 29, 2020
@prusnak prusnak force-pushed the amount-unit branch 3 times, most recently from bc0897c to 14fe400 Compare January 5, 2021 13:09
@prusnak prusnak force-pushed the amount-unit branch 2 times, most recently from b818e74 to 9b09cae Compare January 18, 2021 15:17
@prusnak prusnak marked this pull request as ready for review January 18, 2021 17:03
tsusanka
tsusanka approved these changes Jan 20, 2021
View reviewed changes
Copy link
Contributor

@tsusanka tsusanka left a comment

Choose a reason for hiding this comment

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

Core LGTM! Did not review Legacy.

core/src/apps/bitcoin/sign_tx/layout.py Outdated Show resolved Hide resolved
@prusnak
Copy link
Member Author

prusnak commented Jan 20, 2021 •
edited
Loading

There is still one elephant in the room we need to address:

If unit is set to satoshi, we just show sat on the display no matter what coin is selected making it impossible to determine which coin is being used. Should we use the sat BTC, sat TEST, sat LTC as suffix? If yes, then we'll probably won't have space to show this on the display of T1 :-/

But anyway, here's the change that does that: b4ba0de

@tsusanka
Copy link
Contributor

@prusnak how about we leave only "sat" for Bitcoin and "sat [coin]" for all the others? cc @matejzak

@prusnak
Copy link
Member Author

prusnak commented Jan 20, 2021 •
edited
Loading

TT is not an issue, we have plenty of space:

left = before | right = after

00000004 00000004

matejcik
matejcik requested changes Jan 21, 2021
View reviewed changes
common/protob/messages-bitcoin.proto Outdated Show resolved Hide resolved
common/protob/messages-bitcoin.proto Outdated Show resolved Hide resolved
core/src/apps/bitcoin/sign_tx/approvers.py Outdated Show resolved Hide resolved
core/src/apps/bitcoin/sign_tx/layout.py Outdated Show resolved Hide resolved
core/src/apps/bitcoin/authorize_coinjoin.py Outdated Show resolved Hide resolved
tests/device_tests/test_msg_signtx_amount_unit.py Outdated Show resolved Hide resolved
@mcudev
Copy link
Contributor

mcudev commented Jan 21, 2021

I read a justification for using "bits" and "bitcents" that made a lot of sense to me: https://twitter.com/adam3us/status/1335354727450746881

I understand this can be akin to a religious argument, and I don't want to get into that, but I figured that I'd mention it in the off chance that the terms had not crossed your radar.

@prusnak
Copy link
Member Author

prusnak commented Jan 21, 2021 •
edited
Loading

Found an issue - we should not be multiplying amounts, just decreasing the decimal numbers. Fixed that, but hit some issues with the fixup commits, so I rebased the branch completely on top of the master. Sorry about that :-/

@tsusanka tsusanka merged commit be2ca47 into master Jan 22, 2021
@tsusanka tsusanka deleted the amount-unit branch January 22, 2021 13:07
@prusnak prusnak mentioned this pull request Jan 22, 2021
Closed
4 tasks
@dahaca dahaca mentioned this pull request Jun 21, 2022
Merged
@hynek-jina hynek-jina mentioned this pull request Sep 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsusanka tsusanka tsusanka approved these changes

@matejcik matejcik matejcik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
21.02
Development

Successfully merging this pull request may close these issues.

Allow amounts denomination in Satoshi
4 participants
@prusnak @tsusanka @mcudev @matejcik