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

feat(core): improve ETH send flow. #4095

Merged
merged 5 commits into from
Aug 27, 2024
Merged

feat(core): improve ETH send flow. #4095

merged 5 commits into from
Aug 27, 2024

Conversation

ibz
Copy link
Contributor

@ibz ibz commented Aug 8, 2024

Refs #3858

@ibz ibz self-assigned this Aug 8, 2024
@ibz ibz requested a review from obrusvit August 8, 2024 13:17
Copy link

github-actions bot commented Aug 8, 2024

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

Copy link

github-actions bot commented Aug 13, 2024

legacy UI changes device test(screens) main(screens)

@ibz ibz force-pushed the ibz/20240808-eth-tx branch 5 times, most recently from 78bec4a to 857c659 Compare August 14, 2024 09:11
@ibz
Copy link
Contributor Author

ibz commented Aug 14, 2024

Here are screenshots from the screens of the flow...

image

Note: Account info screen missing for now, since we don't have the account # and wallet info...

image

image

image

image

image

image

@ibz ibz force-pushed the ibz/20240808-eth-tx branch 6 times, most recently from 67b5872 to 86dd94c Compare August 16, 2024 08:07
@ibz ibz marked this pull request as ready for review August 16, 2024 08:26
@ibz ibz added the translations Put this label on a PR to run tests in all languages label Aug 16, 2024
@Hannsek Hannsek linked an issue Aug 17, 2024 that may be closed by this pull request
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.

Some points to discuss regarding the flow.

UI part in rust seems good 👍

core/translations/en.json Show resolved Hide resolved
tests/device_tests/ethereum/test_signtx.py Outdated Show resolved Hide resolved
@obrusvit
Copy link
Contributor

This PR also adds Success screen with "Transaction signed" and "Continue in the app" for all models.
emu00000000

But I don't think it's desirable UX-wise e.g. when you Stake.
I think the success screens are designed for TS5 only and the idea for the future is to have them timeout after a few seconds while TrezorSuite already gets the result (Success). This is not yet possible, tracked here #3666

So I would remove the Success screen for now.

@ibz
Copy link
Contributor Author

ibz commented Aug 19, 2024

So I would remove the Success screen for now.

OK, I'll remove the success screen from all models then, as discussed with you (re #3666).

What about the progress screen - does it even make sense given it is not a "real" progress and the signing is pretty fast? @Hannsek

@Hannsek
Copy link
Contributor

Hannsek commented Aug 19, 2024

Progress screen "signing transaction" is quick for small transaction, for bigger transaction it is not that quick. Let's keep it.

@ibz ibz force-pushed the ibz/20240808-eth-tx branch 2 times, most recently from 1eecdc0 to acb1cde Compare August 20, 2024 14:34
@ibz
Copy link
Contributor Author

ibz commented Aug 20, 2024

So I would remove the Success screen for now.

#4095 (comment)

@ibz ibz force-pushed the ibz/20240808-eth-tx branch 2 times, most recently from c5ac68b to e155de8 Compare August 22, 2024 07:31
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.

Sending ETH works nicely 👍

Apart from the individual comments, I'd like to discuss a few more points:

  • the new_confirm_summary is very very long, perhaps we can break it down into smaller pieces or even flows
  • ETH confirm stake/unstake/claim flows are broken now - they don't show the info according to Figma. The flow in python function was not changed but we changed the confirm_summary.rs which now disrupts it. We should check whether it can be mapped to the new confirm_summary but I'm afraid we'd have to write a new flow_. I can help.
  • I didn't check other operations on ETH, e.g. with data or unknown contract etc.

@obrusvit
Copy link
Contributor

There's a limitation in the ButtonRequest behavior of the confirm_output as it does not send the BR everytime if the user goes back and forth. We ignore it for now because Suite does not handle them repeatedly anyway.

We realized that sending BR from rust on every Event::Attach is not equivalent to sending BR in a while loop with from uPy. A layout started from uPy has two pages, it sends only one BR
A usage of .repeated_button_request from rust sends it twice for every page.

@ibz ibz force-pushed the ibz/20240808-eth-tx branch 2 times, most recently from 5f06900 to f25f7e1 Compare August 27, 2024 11:45
@ibz ibz merged commit 0a0c100 into main Aug 27, 2024
119 of 120 checks passed
@ibz ibz deleted the ibz/20240808-eth-tx branch August 27, 2024 13:21
@Hannsek
Copy link
Contributor

Hannsek commented Aug 28, 2024

@ibz next time please ping @lapohoda to check it. :)

@bosomt
Copy link

bosomt commented Sep 4, 2024

QA OK

  • Device: Trezor T3T1 2.8.2 regular (revision d7e1f0f)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
translations Put this label on a PR to run tests in all languages
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

Send ETH (EVM)
4 participants