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

Add verify address button to Receive modal #83

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Conversation

brusherru
Copy link
Member

It is requested by Ledger company to add a "verify" button on "Receive" modal, that asks User to verify the address on his Ledger device. So now it is implemented.

It may require design improvement or rephrasing.

image image image

And there is also possible another case with red text, saying: "The address is incorrect, probably you are using another Ledger device." :)

@brusherru brusherru self-assigned this Sep 10, 2024
Copy link

You can preview the changes at : https://e1239a33.smapp-lite-prod.pages.dev

Copy link
Collaborator

@monikasmolarek monikasmolarek left a comment

Choose a reason for hiding this comment

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

Once the address check is rejected on the ledger, it gets automatically the modal claiming (incorrectly) the ledger is disconnected.

also the error messages were a bit messy🤔

Screen.Recording.2024-09-10.at.16.22.51.mov

src/components/Receive.tsx Outdated Show resolved Hide resolved
Copy link

You can preview the changes at : https://df41b117.smapp-lite-prod.pages.dev

Copy link

You can preview the changes at : https://0fcd607e.smapp-lite-prod.pages.dev

@brusherru
Copy link
Member Author

@monikasmolarek thanks!
Fixed. There are two new messages for the cases:

  1. When the address is rejected on the device, it does not require to reconnect anymore, but displays:
    image

  2. When the device is locked or not connected there will be another message (in case User closes the "connect device" popup). Example:
    image
    image

Copy link
Collaborator

@monikasmolarek monikasmolarek left a comment

Choose a reason for hiding this comment

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

@brusherru This is indeed fixed, thanks !
but please double-check the case when you start the address verification, you don't do anything on the ledger, so the request there is waiting for your approve/reject, but in the Wallet you click OK or close the modal etc.
When the tx/action is pending on the device, the Wallet gets really confused about the device connection.

Screen.Recording.2024-09-12.at.00.55.34.mov

My steps in this rec:

  1. opened receive, clicked verify -> checked the device but didn’t approve or reject, left pending
  2. clicked OK on the modal
  3. opened receive, clicked verify again-> the reconnect modal appeared
  4. took device, approved the request -> it got approved "in the background" below the reconnect modal
  5. the wallet got confused about the connection

@brusherru
Copy link
Member Author

When the tx/action is pending on the device, the Wallet gets really confused about the device connection.

The problem here that we cannot cancel the operation on the ledger device if you close the window.
And at the same time, Ledger works in a synchronous way, which means if you run another operation while the previous one was not finished — it causes transport error, which also causes disconnecting from the device.

The only thing I can do here is to disable closing the popup once "Verify" button was clicked.
So I tweaked it, and hopefully it will work well :D

image

However, if you reload the page and click "Verify" once again — we don't have any chance to handle it properly in the app.

Copy link

You can preview the changes at : https://96c11141.smapp-lite-prod.pages.dev

@brusherru
Copy link
Member Author

Ok, I'm pretty sure it is fine, so I'm merging it.
If we find out any problems — I'll fix it :)

@brusherru brusherru merged commit 6671e49 into main Sep 17, 2024
1 check passed
@brusherru brusherru deleted the feat-verify-address branch September 17, 2024 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants