Cardano UI simplification for flows initiated by Suite #4256 #4284

obrusvit · 2024-10-25T10:58:09Z

This PR aims to simplify Cardano flows initiated by Trezor Suite. Namely:

send transaction with no additional features - this should behave roughly like sending Bitcoin, i.e. confirming address i, recipient i and Summary at the end. No "Choose level of details" screen.
staking transaction with no additional features - this should behave almost exactly the same except without the initial screen asking a user to "Choose level of details"

This is achieved by:

modifying the ORDINARY_SIGNER to categorize the transaction further
based on the category, modify the layouts shown

TODO:

I need to verify that the categorization of a transaction to either SIMPLE_SEND, SIMPLE_STAKE_DELEGATE, SIMPLE_STAKE_WITHDRAW or NOT_SUITE_TX is generally valid
- testing with Suite, I hopefully got it right
fix discrepancies for total_amount calculation for the summary - e.g. change output should not be added to the final amount?
- 0dc6427
fix Recipient i showing
- 9f71794
tests - I don't think new tests are needed (there are many for ADA) but:
- checking the existing UI diff - especially cardano/sign_tx.json with "simple transaction" in the name are affected
  - there seems to be like 3 cases where a slightly more complicated transaction triggers should_show_more = True, shouldn't be a big deal.
- enable other UI tests with "Show more details" for mercury
mercury summary layout has a wrong button text and title

github-actions · 2024-10-25T11:28:14Z

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)	2724
T3T1 Safe 5	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
All	main(screens)

core/src/apps/cardano/sign_tx/signer.py

mmilata

Looking like a massive UI improvement!

There's a Safe 3 testcase where hold-to-confirm is introduced as a second-to-last action which probably isn't right:

Then again, T3T1 also has some weirdness which is not new?:

mmilata · 2024-10-28T20:57:23Z

core/embed/rust/src/ui/model_mercury/flow/confirm_with_info.rs

+pub enum ConfirmWithInfo {
+    Main,
+    Menu,
+}


Wondering what's the main difference from other two-screen flows we already have 🤔 Menu items?

This flow has different option in the menu "Show all" and it returns INFO on click of that button. It seemed the best way for now is to just create a new flow.
I'm thinking about a more universal constructor of these SwipeFlows which would be supplied with menu options and what they do... No clear idea at the moment.

To answer the top comment:

There's a Safe 3 testcase where hold-to-confirm is introduced as a second-to-last action which probably isn't right:
Then again, T3T1 also has some weirdness which is not new?

These screens show the derivation paths and they are shown after the transaction is signed and only if should_show_more is true, i.e. when the user opted for "Show all". IIUC, Cardano TX is not initiated with derivation paths of the inputs, they are optionally provided by the host afterwards. The UX is quite strange ("Hold to confirm", then simple "Confirm") but I didn't want to change the flows not coming from Suite.
I was also perplexed at first.

I'm thinking about a more universal constructor of these SwipeFlows which would be supplied with menu options and what they do

I think main problem is making the parameters of this constructor available to the logic in handle_swipe/handle_event, which is currently not possible without abusing RwLock/AtomicU.*

core/src/apps/cardano/sign_tx/signer.py

common/tests/fixtures/cardano/sign_tx.show_details.json

obrusvit · 2024-10-28T23:56:43Z

Tested with Suite and found out that the categorization must allow witness_request_count higher than 1 as this is equal to the number of inputs which can be higher even for simple transaction. The logic is similar to bitcoin -> more inputs might be needed in order to to make the TX.

fixed here: 8b5e087

mmilata

clippy is failing
needs regenerated fixtures incl. translations

otherwise LGTM

obrusvit · 2024-10-29T22:44:01Z

ignoring changelog for python job failing as I only corrected a param
lots of UI diff generated for T3B1 also for non-Cardano stuff because of adding a previously missing option to cancel confirm_properties

- remove the choise of detail level ("Show simple" vs "Show all") when signing simple transactions - these are the transactions typically enabled by TrezorSuite.

This prompt was unintuitive as the menu button served as the "show more" button. This commit implements a small SwipeFlow which hides the option to the context menu.

- change the button text based on `hold` - make it abortable

Also show Recipient {i} for simple tx

Rationale: the removed test cases are considered "simple" as of the recent changes and they do not prompt a user for "level of details".

github-actions · 2024-10-30T00:54:02Z

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

bosomt · 2024-11-01T12:18:39Z

QA OK

checked on TS5 device

Info:

Suite version: web 24.11.0 (ff5fe34e9ecaee40808f50c235c08cf8e588b0c8)
Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/130.0.0.0 Safari/537.36
OS: MacIntel
Screen: 5120x1440
Device: Trezor T3T1 2.8.4 regular (revision 31d228a)
Transport: WebUsbTransport

obrusvit self-assigned this Oct 25, 2024

obrusvit requested review from matejcik and prusnak as code owners October 25, 2024 10:58

obrusvit requested review from mmilata and removed request for prusnak October 25, 2024 10:58

obrusvit added altcoin Any non-Bitcoin coins core Trezor Core firmware. Runs on Trezor Model T and T2B1. labels Oct 25, 2024

obrusvit mentioned this pull request Oct 25, 2024

Cardano UI simplification for flows initiated by Suite #4256

Closed

obrusvit commented Oct 25, 2024

View reviewed changes

core/src/apps/cardano/sign_tx/signer.py Outdated Show resolved Hide resolved

mmilata reviewed Oct 28, 2024

View reviewed changes

mmilata approved these changes Oct 29, 2024

View reviewed changes

obrusvit force-pushed the obrusvit/cardano-trezor-suite-flows branch from dc3e71a to a4d1846 Compare October 29, 2024 21:46

obrusvit added 4 commits October 30, 2024 00:52

feat(cardano): ask detail lvl only on complex tx

a9da691

- remove the choise of detail level ("Show simple" vs "Show all") when signing simple transactions - these are the transactions typically enabled by TrezorSuite.

fix(cli/cardano): chunkify click option

0b58977

fix(ui/mercury): fix should_show_more prompt

a65455a

This prompt was unintuitive as the menu button served as the "show more" button. This commit implements a small SwipeFlow which hides the option to the context menu.

fix(ui/model_r): adjust model_r confirm_properties

bd9aaef

- change the button text based on `hold` - make it abortable

obrusvit force-pushed the obrusvit/cardano-trezor-suite-flows branch from a4d1846 to 1b3f4c1 Compare October 29, 2024 23:54

obrusvit added the translations Put this label on a PR to run tests in all languages label Oct 29, 2024

obrusvit added 5 commits October 30, 2024 01:52

feat(cardano): use nicer summary for send tx

cf65219

Also show Recipient {i} for simple tx

fix(ui/model_r): add title for altcoin_tx_summmary

ce6af6f

test(cardano): allow tests with details on mercury

dc90e38

test(cardano): reduce testset sign_tx_show_details

a459bde

Rationale: the removed test cases are considered "simple" as of the recent changes and they do not prompt a user for "level of details".

chore: update fixtures

a390f0c

obrusvit force-pushed the obrusvit/cardano-trezor-suite-flows branch from 1b3f4c1 to a390f0c Compare October 30, 2024 00:53

obrusvit merged commit 2e0e8eb into main Oct 30, 2024
135 of 138 checks passed

obrusvit deleted the obrusvit/cardano-trezor-suite-flows branch October 30, 2024 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cardano UI simplification for flows initiated by Suite #4256 #4284

Cardano UI simplification for flows initiated by Suite #4256 #4284

obrusvit commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Oct 25, 2024 •

edited

Loading

mmilata left a comment •

edited

Loading

mmilata Oct 28, 2024

obrusvit Oct 28, 2024

mmilata Oct 29, 2024

obrusvit commented Oct 28, 2024

mmilata left a comment •

edited

Loading

obrusvit commented Oct 29, 2024

github-actions bot commented Oct 30, 2024

bosomt commented Nov 1, 2024

Cardano UI simplification for flows initiated by Suite #4256 #4284

Cardano UI simplification for flows initiated by Suite #4256 #4284

Conversation

obrusvit commented Oct 25, 2024 • edited Loading

github-actions bot commented Oct 25, 2024 • edited Loading

mmilata left a comment • edited Loading

Choose a reason for hiding this comment

mmilata Oct 28, 2024

Choose a reason for hiding this comment

obrusvit Oct 28, 2024

Choose a reason for hiding this comment

mmilata Oct 29, 2024

Choose a reason for hiding this comment

obrusvit commented Oct 28, 2024

mmilata left a comment • edited Loading

Choose a reason for hiding this comment

obrusvit commented Oct 29, 2024

github-actions bot commented Oct 30, 2024

bosomt commented Nov 1, 2024

obrusvit commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Oct 25, 2024 •

edited

Loading

mmilata left a comment •

edited

Loading

mmilata left a comment •

edited

Loading