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

chore(core): hardcode ETH and Gwei units in ETH send summary #3414

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

grdddj
Copy link
Contributor

@grdddj grdddj commented Nov 22, 2023

Fixes #3246:

  • enforces usage of certain units in ETH summary - ETH for amount and total fee, Gwei ETH for the rest.

Not sure, however, that we want to force it, as there are these cases where a very small value gets displayed in a very big unit. Previously, the code was checking this --- either showing ETH if the value was bigger than 10 ^ 9 and wei ETH otherwise.

image

@grdddj grdddj requested a review from matejcik as a code owner November 22, 2023 15:35
@grdddj grdddj requested review from mmilata and Hannsek and removed request for matejcik November 22, 2023 15:35
@grdddj grdddj self-assigned this Nov 22, 2023
@grdddj grdddj mentioned this pull request Nov 22, 2023
@Hannsek
Copy link
Contributor

Hannsek commented Nov 22, 2023

Ahhh, ok, it cannot be like that :(
For Maximum fee and Amount please revert the changes.

Gwei ETH for the rest.

We should use only Gwei/Wei (without ETH) for gas limit a and priority fee.

@matejcik
Copy link
Contributor

i think we just need to touch the "10^9" number -- so that we get at most, say, 10 decimals?

@grdddj
Copy link
Contributor Author

grdddj commented Nov 27, 2023

Ahhh, ok, it cannot be like that :( For Maximum fee and Amount please revert the changes.

Gwei ETH for the rest.

We should use only Gwei/Wei (without ETH) for gas limit a and priority fee.

Reverted and changed in b428d53

@grdddj
Copy link
Contributor Author

grdddj commented Nov 27, 2023

i think we just need to touch the "10^9" number -- so that we get at most, say, 10 decimals?

Not sure what are the requirements ... but we have good unittests for this function, feel free to suggest some new behavior

https://github.com/trezor/trezor-firmware/blob/main/core/tests/test_apps.ethereum.layout.py

There are some edge-cases like this, where there will be much more than 10 valid digits:

text = format_ethereum_amount(1000000000000000001, None, ETH)
self.assertEqual(text, "1.000000000000000001 ETH")

@Hannsek - a general question, who is wanting these changes?

@Hannsek
Copy link
Contributor

Hannsek commented Nov 27, 2023

It's me. Our fee units are not standard.

It should look like this:
image

@grdddj
Copy link
Contributor Author

grdddj commented Nov 28, 2023

It's me. Our fee units are not standard.

It should look like this: image

Should be the case now. Priority fee is only shown in eip1559, which looks like we have test-screens only for TR

image

image

@Hannsek
Copy link
Contributor

Hannsek commented Nov 28, 2023

Why do we have max gas price on T2B1 and gas price on TT? We should have it the same on both devices. Please use only gas price.

@grdddj
Copy link
Contributor Author

grdddj commented Nov 28, 2023

Why do we have max gas price on T2B1 and gas price on TT? We should have it the same on both devices. Please use only gas price.

That is not the difference between models, but between 1559 and normal signing. All these texts are the same for both devices

@matejcik matejcik merged commit 01eb896 into main Dec 1, 2023
57 checks passed
@matejcik matejcik deleted the grdddj/eth_units branch December 1, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

ETH fees units
4 participants