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/native/feature requests passphrase #13121

Merged
merged 8 commits into from
Jul 10, 2024

Conversation

juriczech
Copy link
Contributor

@juriczech juriczech commented Jul 2, 2024

Description

This PR enables device communication during locked & remembered passphrase device. In that case, device doesn't allow communication until device is unlocked via pin + passphrase. With how the code looked previous to these changes, navigation and cancelling authorization flow was quite problematic.

Modules for passphrase and device connection were merged into one: module-authorize-device. This was done so that these authorization screens are together within one stack so that we can go back to previous stack, without having to define it, once the flow is finished, be it success or fail.

Commits:

  • 9cc69c9 - this PR refactors previous TrezorConnect.on() listeners into a redux state logic as it's more flexible and allows for better cooperation with already existing pin logic, which was previously refactored into redux as well
  • 9cc69c9 - move module-passphrase into module-connect-device and merge stacks into one
  • 5a00405 - create new isCreatingNewPassphraseWallet flag in redux state so it allows us to treat these flows differently - specifically for exiting them
  • e489dfa - rename newly merged module-connect-device to module-authorize-device
  • dfc602d - rename stack inside module-authorize-device to fit package name (separated commits for better code review readability)
  • 1ab6262 - implementing passphrase form screen for feature authorization of passphrase with improved navigation logic
  • 86cb15d - since module-passphrase has been removed, I also removed package passphrase. Without this, we would have circular dependencies in future, so the logic was moved to device-authorization package.

Related Issue

Resolve #12132
Resolve #12689

Screenshots:

Zaznam.obrazovky.2024-07-04.v.12.48.20.mov

Error state visible here (copy and styles to be changed in a follow up PR):

Zaznam.obrazovky.2024-07-04.v.13.27.38.mov

@juriczech juriczech added the mobile Suite Lite issues and PRs label Jul 2, 2024
@juriczech juriczech force-pushed the feat/native/feature-requests-passphrase branch 8 times, most recently from dacadda to 1ab6262 Compare July 4, 2024 11:13
@juriczech juriczech marked this pull request as ready for review July 8, 2024 08:17
@juriczech juriczech requested a review from a team as a code owner July 8, 2024 08:17
@juriczech juriczech force-pushed the feat/native/feature-requests-passphrase branch 2 times, most recently from 52b323e to 2dab07f Compare July 9, 2024 06:14
Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

FIXED

I spotted one bug:

  1. Discover standard wallet
  2. make it view-only
  3. open new passphrase
  4. make it view-only
  5. disconnect Trezor
  6. reconnect Trezor
  7. go to verify Receive address
  8. click red Cancel button in app (see screenshot)
    Passphrase wallet is rejected completely and I'm navigated to dashboard of standard wallet. I would expect to go back to receive screen instead.
image

Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

FIXED

And one inconsistency:

  • If I click on 'X' while entering passhprase in app, it navigate somewhere else than the same button while entering passhprase on Trezor

Passhprase in app navigate back to top level (dashboard/My assets/Receive)
image

Passhprase on device just closing passphrase form and stay on Receive page
image

@juriczech
Copy link
Contributor Author

I spotted one bug:

  1. Discover standard wallet
  2. make it view-only
  3. open new passphrase
  4. make it view-only
  5. disconnect Trezor
  6. reconnect Trezor
  7. go to verify Receive address
  8. click red Cancel button in app (see screenshot)
    Passphrase wallet is rejected completely and I'm navigated to dashboard of standard wallet. I would expect to go back to receive screen instead.
image

Fix here: 73fa108

@juriczech
Copy link
Contributor Author

And one inconsistency:

  • If I click on 'X' while entering passhprase in app, it navigate somewhere else than the same button while entering passhprase on Trezor

Passhprase in app navigate back to top level (dashboard/My assets/Receive) image

Passhprase on device just closing passphrase form and stay on Receive page image

I believe this should fix that. Could you please try to retest it? 🙏
b58ba58

@juriczech juriczech force-pushed the feat/native/feature-requests-passphrase branch from b58ba58 to 6a65d5b Compare July 10, 2024 11:46
@juriczech juriczech force-pushed the feat/native/feature-requests-passphrase branch from 6a65d5b to 7cd6fb8 Compare July 10, 2024 12:37
@juriczech juriczech force-pushed the feat/native/feature-requests-passphrase branch from 7cd6fb8 to 94f10c4 Compare July 10, 2024 13:13
Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

It works great now! 🎉 👏

@juriczech
Copy link
Contributor Author

/rebase

Copy link

@juriczech juriczech merged commit d435821 into develop Jul 10, 2024
76 of 86 checks passed
@juriczech juriczech deleted the feat/native/feature-requests-passphrase branch July 10, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Suite Lite issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passphrase on add new account Passphrase on receive
2 participants