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

feat(core): show ETH account info on model T #4471

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(core): show ETH account info on model T #4471

wants to merge 2 commits into from
+41 −20

Conversation

ibz
Copy link
Contributor

@ibz ibz commented Jan 6, 2025 •
edited
Loading

This PR is a follow-up of #4175 to show the account info of ETH transactions on model T, as it was previously only done on TS5.

Screenshots

image

Test command

trezorctl ethereum sign-tx -n "m/44'/60'/0'/0/0" -d 6162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f706162636465666768696a6b6c6d6e6f70 --max-gas-fee 100 --max-priority-fee 100 -e 2 -g 100 -G 100 -i 1 0x1d1c328764a41bda0492b66baa30c4a339ff85ef 11

@ibz ibz self-assigned this Jan 6, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 6, 2025 •
edited
Loading

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@ibz ibz marked this pull request as ready for review January 7, 2025 09:48
@ibz ibz requested review from matejcik and prusnak as code owners January 7, 2025 09:48
@ibz ibz requested review from obrusvit and removed request for prusnak and matejcik January 7, 2025 09:48
@ibz ibz requested a review from matejcik January 16, 2025 09:26
obrusvit
obrusvit approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@obrusvit obrusvit left a comment

Choose a reason for hiding this comment

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

utACK

However, I noticed that the input flow for model T (Bolt) test does not visit all the possible info pages. TS3 Caesar input flow does that automatically which adds the screenshot with newly added info. If you think it's worth, please add it.

core/embed/rust/src/ui/component/text/op.rs Outdated
@@ -16,7 +16,7 @@ use heapless::Vec;

// So that there is only one implementation, and not multiple generic ones
// as would be via `const N: usize` generics.
const MAX_OPS: usize = 20;
const MAX_OPS: usize = 23;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if the previous value 20 was determined somehow before, maybe @matejcik can comment.
Anyway, we might be able to reduce it again once we get rid of font command from FormattedText

Copy link
Contributor

Choose a reason for hiding this comment

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

99 % sure it was chosen so that some particular screen fits in the limit.

i'd strongly prefer getting rid of this completely but for now bumping to 23 is fine

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd strongly prefer getting rid of this completely but for now bumping to 23 is fine

As we plan to use it more often, do you have an alternative to the fixed-sized Vec?

Copy link
Contributor

Choose a reason for hiding this comment

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

impl OpsSource for T 🤷‍♀️

@ibz ibz force-pushed the ibz/account-info branch from 338be11 to 93e282a Compare January 21, 2025 09:36
@ibz ibz linked an issue Jan 21, 2025 that may be closed by this pull request
Information about account missing when sending ETH. #3536
Closed
@ibz
Copy link
Contributor Author

ibz commented Jan 21, 2025

TS3 Caesar input flow does that automatically

Actually it did not do it, it just visited the first 2 pages. I changed it so that they both visit all pages now. Please have a look at b9d222e

obrusvit
obrusvit requested changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@obrusvit obrusvit left a comment

Choose a reason for hiding this comment

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

Let's first solve the requirements from @Hannsek

@ibz ibz marked this pull request as draft January 21, 2025 15:58
@ibz ibz force-pushed the ibz/account-info branch 3 times, most recently from a1bbaf3 to 6e4befd Compare January 23, 2025 17:20
@ibz ibz marked this pull request as ready for review January 23, 2025 17:41
@ibz ibz changed the title feat(core): show ETH account info on all models feat(core): show ETH account info on model T Jan 24, 2025
@ibz ibz force-pushed the ibz/account-info branch from 055646a to 97ab8fc Compare January 24, 2025 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obrusvit obrusvit obrusvit requested changes

@matejcik matejcik Awaiting requested review from matejcik matejcik is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@ibz ibz

Labels
None yet
Projects
Firmware
Status: 🏃‍♀️ In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Information about account missing when sending ETH.
3 participants
@ibz @matejcik @obrusvit