leather-io / extension

Leather browser extension
https://leather.io
MIT License
293 stars 140 forks source link

Support increase fee for Ledger #2345

Closed kyranjamie closed 2 years ago

kyranjamie commented 2 years ago

When increasing the fee for a transaction, we need to go through the ledger connection flow. This hasn't yet been integrated.

cc/ @markmhx @andresgalante how important is this, a must for initial release?

andresgalante commented 2 years ago

This is not necessary for the initial release, we'll implement increasing fee with Ledger after.

landitus commented 2 years ago

Here's the flow for increasing transaction fees, updated with Ledger support.

Figma

User lifecycle flow - Increase transaction fee v2
landitus commented 2 years ago

Here's a prototype and short video demo of the flow

The biggest divergence from the normal Ledger TX flow is that the Ledger modal cannot overlay in the same way since the Increase fees is already a modal. Given this situation, I've mocked a transition between modals instead. Biggest UX question is what does the X close button on Ledger modal do?

Figma prototype

CleanShot 2022-04-29 at 16 33 29@2x

https://user-images.githubusercontent.com/239215/165965989-2e545d1d-f0fd-4eec-a09e-4182bfb1547e.mp4

markmhendrickson commented 2 years ago

Perhaps if we have sequential overlay modal states (e.g. here with "Increase fee" first then "Connect Ledger"), all but the first view should have a back arrow option in the top-left corner?

It could still have a close option in the top-right corner for all views, which will simply close the overlay modal in general (e.g. here taking the user back to the home screen activity). That way the user has a clear way to either go back in the process or simply abort the process entirely.

landitus commented 2 years ago

Perhaps if we have sequential overlay modal states (e.g. here with "Increase fee" first then "Connect Ledger"), all but the first view should have a back arrow option in the top-left corner?

It could still have a close option in the top-right corner for all views, which will simply close the overlay modal in general (e.g. here taking the user back to the home screen activity). That way the user has a clear way to either go back in the process or simply abort the process entirely.

Ok, that sounds good, so I've updated the prototype with this behavior. I think it's better now!

Figma prototype

fbwoolf commented 2 years ago

@landitus is this ready for development?

fbwoolf commented 2 years ago

@landitus I think this is done being designed so I'm going to get started on it, but let me know if I am wrong. It seems like in your last comment it is done?

fbwoolf commented 2 years ago

@markmhx I'm questioning needing the back button on every ledger modal step here after working on this. It seems like it might be enough to provide it on the first Connect step to go back to the modal to increase fee, but once the user initiates connecting to ledger maybe just using the X to cancel out completely is simpler? Also, there is some funky stuff happening right now using the X button we need to fix before releasing.

fbwoolf commented 2 years ago

I am going to put up a PR that just has the back button on the first ledger modal to connect to see how people feel testing it. I think it is enough bc backtracking through the ledger flows isn't something we want to do imo. Right now, even if a user clicks on the X icon, it doesn't cancel out connecting or signing the tx on the ledger. For example, you still have to approve or reject the tx on the ledger if you close the tx on the details modal step.

markmhendrickson commented 2 years ago

cc @landitus on the design question above re: the X 👆

landitus commented 2 years ago

sorry for the delay @fbwoolf . The last time we reviewed the increase fee flow with @kyranjamie we realized this flow (as seen in the Figma) will not work on the widget since the Ledger can't be connected/accessed from an extension widget. So the next idea was to force this flow into a new tab and then try to improve the UX later. Though it's not ideal, it would allow the Ledger to be connected to the browser. Open to any suggestions! Maybe I can take a fresh look at it again and see if another idea comes up.

fbwoolf commented 2 years ago

sorry for the delay @fbwoolf . The last time we reviewed the increase fee flow with @kyranjamie we realized this flow (as seen in the Figma) will not work on the widget since the Ledger can't be connected/accessed from an extension widget.

Yep, that is fine, I can force it to be full page but my question is also for full page view in the modal. I don't think we need the back button except on the first connect screen for ledger to go back to the increase fee form ...once the user has initiated trying to connect the ledger that sequence can be really fast and using the back button isn't ideal. I think just having them close the modal at that point is better?

fbwoolf commented 2 years ago

You can test it in full page view with the build in the linked PR.

fbwoolf commented 2 years ago

@landitus I think there is some weird behavior happening here even with the X button but it is the same with sending a tx in full page. If you try to cancel out of the modal the UI flashes and ultimately the ledger still asks you to approve or reject the tx. Maybe after the user clicks to Connect the ledger should handle things? Maybe the X shouldn't even be available on all steps?

fbwoolf commented 2 years ago

We should probably get @kyranjamie's feedback on this when he is back bc the X button has been left just as a route change ...it really doesn't perform any cancellation of the call to ledger, which might be intentional or not possible to do?

landitus commented 2 years ago

Maybe after the user clicks to Connect the ledger should handle things? Maybe the X shouldn't even be available on all steps?

You are probably onto something here @fbwoolf. While building the initial flows with @kyranjamie we've seen out-of-sync issues on the Ledger hardware if a transaction is not completed on the Wallet UI. I think your idea of letting the hardware take over fixes this and if doable we should go for it! Let's see what @kyranjamie thinks as well!

landitus commented 2 years ago

Maybe we can show some clarifying hover message while we remove the affordance of the close/back buttons and the hardware takes over? 👀

https://user-images.githubusercontent.com/239215/175377404-6f2a31e6-267a-4088-92cd-84909799d354.mp4

fbwoolf commented 2 years ago

Maybe we can show some cool hover message like this while we remove the affordance of the close/back and the hardware takes over? 👀

I love that idea! Looks really great. 🚀

landitus commented 2 years ago

Here's a revised flow where we now force the Increase Fee flow into a new tab so that the browser can interact with the Ledger device.

I've prototyped the Happy path and the Transaction denied path. Other states can be copied from the transaction flow. Also, the flows have been updated in Figjam with the branching logic "if it's a hardware wallet".

https://user-images.githubusercontent.com/239215/175534252-108df260-105f-4429-8d56-ce9b18b24199.mp4

fbwoolf commented 2 years ago

Thx @landitus I'll get these changes implemented today. 🙌

fbwoolf commented 2 years ago

Refactored this and tested it locally ...such an improvement imo.

landitus commented 2 years ago

I'm trying to test this build locally with my Ledger but I'm stuck here. I've updated everything (Ledger Live) reinstalled Stacks app (0.19) and still I get the out of date notice.

CleanShot 2022-06-24 at 22 11 25@2x
fbwoolf commented 2 years ago

I'm trying to test this build locally with my Ledger but I'm stuck here. I've updated everything (Ledger Live) reinstalled Stacks app (0.19) and still I get the out of date notice.

You likely aren't using the latest Stacks app from Zondax? You need to follow the test instructions here: https://github.com/hirosystems/stacks-wallet-web/discussions/2451

fbwoolf commented 2 years ago

The latest build is actually v0.22.7: https://github.com/Zondax/ledger-blockstack/releases

fbwoolf commented 2 years ago

I think it might be better to change the wait message to Ledger device in use bc during the connect process you really just have to wait for a response (success or failure), but with the tx signing you do have to approve or reject the tx on the device. So, looking for a more generic message that works for both?

Screen Shot 2022-06-25 at 11 55 30 AM

EDIT: Or, maybe Wait for Ledger device?

fbwoolf commented 2 years ago

This is all working great except one thing I'm seeing with routing I haven't been able to figure out yet, but still looking into it. When you go from the increase fee form > connect ledger ...the activity list disappears on the home page behind the modal. It must have something to do with the nested routes but not sure yet.

Screen Shot 2022-06-25 at 12 16 26 PM