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

ETH send flow - unify button requests #3393

Merged
merged 6 commits into from
Dec 4, 2023
Merged

ETH send flow - unify button requests #3393

merged 6 commits into from
Dec 4, 2023

Conversation

grdddj
Copy link
Contributor

@grdddj grdddj commented Nov 7, 2023

Fixes #3392:

  • send the same button requests in ETH send flow for T2B1 as for T2T1

@grdddj grdddj requested review from mmilata and Hannsek and removed request for prusnak and matejcik November 7, 2023 09:04
@grdddj grdddj changed the title Grdddj/send eth button requests ETH send flow - unify button requests Nov 7, 2023
@grdddj grdddj self-assigned this Nov 7, 2023
@grdddj grdddj mentioned this pull request Nov 7, 2023
Closed
@Hannsek Hannsek mentioned this pull request Nov 16, 2023
Closed
mmilata
mmilata reviewed Nov 21, 2023
View reviewed changes
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Looking good, would like to see UI diffs before approving.

tools/check-bitcoin-only Outdated
@@ -10,7 +10,7 @@ EXCEPTIONS+=( "mnemonic" ) # has NEM in it
EXCEPTIONS+=( "workflow" "overflow" ) # has Flo in it
EXCEPTIONS+=( "SyntaxError" ) # has Axe in it
EXCEPTIONS+=( "DKDNEM" ) # has NEM in it, some sort of weird coincidence
EXCEPTIONS+=( "confirm_ethereum_tx" ) # is model-specific, so is in layout/__init__.py instead of ethereum/layout.py
EXCEPTIONS+=( "confirm_ethereum_tx ethereum_tx_summary" ) # is model-specific, so is in layout/__init__.py instead of ethereum/layout.py
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EXCEPTIONS+=( "confirm_ethereum_tx ethereum_tx_summary" ) # is model-specific, so is in layout/__init__.py instead of ethereum/layout.py
EXCEPTIONS+=( "confirm_ethereum_tx" "ethereum_tx_summary" ) # is model-specific, so is in layout/__init__.py instead of ethereum/layout.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a5d3e2f

core/src/trezor/ui/layouts/tr/__init__.py
ButtonRequestType.ConfirmOutput,
modify_layout.page_count(),
)
result = await ctx_wait(modify_layout)
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid you'll have to call .request_complete_repaint() on the two layouts otherwise they'll paint over each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this situation different in TR than in TT? This is the same exact code as in confirm_modify_output's code in core/src/trezor/ui/layouts/tt/__init__.py

Copy link
Member

Choose a reason for hiding this comment

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

Seems to be the same & in both cases there is garbage on screen when you go back from the MODIFY AMOUNT screen in test_p2wpkh_in_p2sh_fee_bump_from_external.

TBH this should be handled automagically somewhere in class Layout, not sure if it's worth spending time on fixing it or rewriting to rust ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I did the quick fix in 4970a6b --- requesting repaint in all while True: cases where we define the layout in front of the loop

@grdddj grdddj force-pushed the grdddj/send_eth_button_requests branch from c51f5f2 to a5d3e2f Compare November 22, 2023 08:19
@grdddj
Copy link
Contributor Author

grdddj commented Nov 22, 2023

Looking good, would like to see UI diffs before approving.

Rebased on main so we see the correct UI diff

@Hannsek
Copy link
Contributor

Hannsek commented Nov 22, 2023 •
edited
Loading

vertical alignment has changed for some reason:
image

diff on TT is not available.

@grdddj
Copy link
Contributor Author

grdddj commented Nov 22, 2023

vertical alignment has changed for some reason: image

diff on TT is not available.

Yes, the alignment might have changes a little, because we are using a different layout for this screen. Hopefully, it is not an issue.

I have pushed new commits, so the TT diff should be visible in a moment

@Hannsek
Copy link
Contributor

Hannsek commented Nov 22, 2023

Yes, the alignment might have changes a little, because we are using a different layout for this screen. Hopefully, it is not an issue.

Can we somehow easily fix it?

@grdddj
Copy link
Contributor Author

grdddj commented Nov 22, 2023

Yes, the alignment might have changes a little, because we are using a different layout for this screen. Hopefully, it is not an issue.

Can we somehow easily fix it?

Easily - no

@Hannsek
Copy link
Contributor

Hannsek commented Nov 22, 2023

Then leave it like that for now. I'd like to give higher prio for the whole centering task in the near future.

@matejcik
Copy link
Contributor

is this ready to merge (after fixing the conflict)?

@grdddj
Copy link
Contributor Author

grdddj commented Nov 30, 2023

is this ready to merge (after fixing the conflict)?

From my side yes. Feel free to take over the PR

@grdddj
fix(core): unify sent button requests in ETH send flow
43e9d95 
Makes sure T2B1 will send the same ButtonRequests as T2T1.
Does it by splitting the Rust layout into two separate dialogs.

[no changelog]
@grdddj
chore(core): decrease code-size a little bit
7a933cc 
[no changelog]
@grdddj
fix(core): unify button requests in modify amount flow
c8138a3 
[no changelog]
@grdddj
chore(tools): add new Ethereum layout name into exceptions for altcoi…
8c4773d 
…n strings

[no changelog]
@grdddj
fix(core): completely repaint layouts before showing them in a loop
d24a8a7 
[no changelog]
@grdddj
chore(tests): regenerate fixtures
05df37c 
[no changelog]
@grdddj grdddj force-pushed the grdddj/send_eth_button_requests branch from 9b5e4ec to 05df37c Compare December 4, 2023 11:48
@matejcik matejcik merged commit 26232c8 into main Dec 4, 2023
57 checks passed
@matejcik matejcik deleted the grdddj/send_eth_button_requests branch December 4, 2023 12:15
@bosomt
Copy link

bosomt commented Dec 28, 2023

QA OK

already released via 2.6.4 firmware version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmilata mmilata mmilata left review comments

@matejcik matejcik matejcik approved these changes

@Hannsek Hannsek Awaiting requested review from Hannsek

Assignees

@grdddj grdddj

Labels
None yet
Projects
Firmware
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Align send flow button request on T2B1 with TT
5 participants
@grdddj @Hannsek @matejcik @bosomt @mmilata