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 payjoin option to hot wallet setup #2450

Merged

Conversation

dennisreimann
Copy link
Member

@dennisreimann dennisreimann commented Apr 8, 2021

Follow-up to the discussion in #2406.

Enables payjoin by default when creating a hot wallet and offers the user an opt-out.

If we enable it by default I'd recommend to show it immediately and not hide it in the advanced settings, though it feels like it would naturaly belong there.

Wording for the accompanying text tbd, the help icon links to the PayJoin docs.

This is currently only available when generating a new hot wallet. It could potentially also be offered on the seed import page, but there we have a toggle for first choosing it to be a hot wallet. I think it might be confusing to have the separate PayJoin toggle there as well, because having the hot wallet enabled is a prerequisite for offering the PayJoin toggle.

hotwallet-payjoin

Let's continue the discussion around that here.

@dennisreimann dennisreimann added Enhancement Improvements to an existing feature UI / UX Front-end issues, for front-end designers labels Apr 8, 2021
@dstrukt
Copy link
Member

dstrukt commented Apr 8, 2021

Initial thoughts.

  1. I'm also a bit conflicted on, should we tuck this behind "advanced settings" or not, as it does seem like that type of option ... but considering it's on by default, it seems like something that should be visible.

  2. Not entirely sure how we should proceed on the seed import issue raised, we could hide/show depending on if the Hot Wallet option is enabled/disabled, and then have it be toggled on by default .. which leads me to ..

after playing around with it, I have a functional request around the enable/disable switch + save button, that we should address when tackling the "Settings" page integration, but noting for the design meeting, as it's out of scope here (I think).

..and alternatively, I think breaking the recovery seed flow into multiple steps seems a bit overkill...

Will keep thinking on this one.

@Zaxounette
Copy link
Contributor

Zaxounette commented Apr 9, 2021

As stated in the previous discussion:
1°) I'd like a "recommended" added on the right of the "enabled/disabled" button for two reasons:

  • Because we're actually recommending it by enabling it by default, so let's make it clear visually.
  • Because some user types (me included) disable options that aren't recommended if they don't know what they are.

2°) The wording you put is good, it explains briefly what the gain is of using that feature, and links to the docs for more informations. There's really nothing more to add there IMHO.

3°) Regarding the seed import and this default behavior:
Couldn't a trick be used to make the payjoin enabled option appear only if the hot wallet option is ticked, and disappear if unticked ?

@Zaxounette
Copy link
Contributor

@dstrukt

I have a functional request around the enable/disable switch + save button, that we should address when tackling the "Settings" page integration, but noting for the design meeting, as it's out of scope here (I think).

If it's even remotely related to design, I feel like it is in scope of the meetings. IMO you can bring it up next time.

@NicolasDorier
Copy link
Member

I think from UX point of view this is confusing. Because now we have Enable payjoin at store level and at wallet level.

This checkbox would be better placed in the Receive page of the wallet and/or in the wallet settings rather than in the setup wizard.

@Kixunil
Copy link
Contributor

Kixunil commented Apr 9, 2021

@NicolasDorier would it make sense to make it per-wallet only? Per-store seems to be bloated.

@Kukks
Copy link
Member

Kukks commented Apr 9, 2021

@NicolasDorier would it make sense to make it per-wallet only? Per-store seems to be bloated.

Maybe, it is an onchain-wallet specific setting after all.

Maybe we should move it from the Store Blob to the payment method blob

@NicolasDorier
Copy link
Member

@Kukks @Kixunil I don't think that make sense.
Most of the properties at the store level are about setting the default settings when creating a new invoices. Payjoin is not different.

@MaiHistoryCompanyLimited

This comment was marked as spam.

@Kixunil
Copy link
Contributor

Kixunil commented Apr 11, 2021

In that case maybe adding the checkbox into the store wallet setup flow could work?

@Kukks
Copy link
Member

Kukks commented Apr 16, 2021

@Kukks @Kixunil I don't think that make sense.
Most of the properties at the store level are about setting the default settings when creating a new invoices. Payjoin is not different.

Payjoin is now usable without an invoice (using wallet receive). Does that change your opinion on this @NicolasDorier?

@NicolasDorier
Copy link
Member

NicolasDorier commented Apr 22, 2021

@Kukks I think the following:

  • The enable payjoin checkbox in the update store page should be enable payjoin on every invoices by default
  • Activating payjoin for the wallet, should not be done as a checkbox like this, but as a checkbox in the receive wallet page. (eventually saved in preference for the user in his cookie)

@dennisreimann
Copy link
Member Author

We discussed this in yesterdays design meeting, here are the main points that were brought up:

  • Consens is we want to make Payjoin the default option for hot wallets
  • Thus we need to make the user aware and also offer a good way to opt-out
    • The user needs to understand what using Payjoin means in terms of …
    • Transaction handling: It might be confusing to see the usage of UTXOs in a way you would not expect
    • Regulatory/legal compliance (might be hypothetical, but I think we cannot simply write it off)
  • Mentioning this feature in a prominent and clear step of the wallet creation thus seems intuitive
  • Moving the Payjoin toggle to the Wallet Receive tab seems like we would bury the option
    • Users might not even know of the option, less think to disable it if they wish if it's placed in that tab.
  • The Payjoin option is only useful for hot wallets, so it makes sense to have it in the hot wallet setup

Other than that two more points came up:

  • Right now the Payjoin toggle is also offered for non-hot wallets, which does not make sense.
  • For future reference the clarification of whether Payjoin is a store or wallet concept would make sense. As wallets are tight to a store right now this blends and we aren't distinguishing there, but it would help to better communicate this feature.

@dennisreimann
Copy link
Member Author

@Kukks @NicolasDorier What are your thoughts regarding the comments that came up in the design meeting?

@Kukks
Copy link
Member

Kukks commented Jun 1, 2021

We discussed this in yesterdays design meeting, here are the main points that were brought up:

  • Consens is we want to make Payjoin the default option for hot wallets

ACK

  • Thus we need to make the user aware and also offer a good way to opt-out

    • The user needs to understand what using Payjoin means in terms of …
    • Transaction handling: It might be confusing to see the usage of UTXOs in a way you would not expect
    • Regulatory/legal compliance (might be hypothetical, but I think we cannot simply write it off)

We should look to existing external docs instead

  • Mentioning this feature in a prominent and clear step of the wallet creation thus seems intuitive

ACK BUT the payjoin option is currently on a store wide level. We need to discuss to perhaps move the payjoin option to a wallet level then (eg. if payjoin is on right now it is on for both BTC and LTC if it is configured).

  • Moving the Payjoin toggle to the Wallet Receive tab seems like we would bury the option

I disagree with putting the payjoin toggle there.

  • Users might not even know of the option, less think to disable it if they wish if it's placed in that tab.
  • The Payjoin option is only useful for hot wallets, so it makes sense to have it in the hot wallet setup

Other than that two more points came up:

  • Right now the Payjoin toggle is also offered for non-hot wallets, which does not make sense.

Correct, if we move the payjoin option to a wallet level, we can handle this properly.

  • For future reference the clarification of whether Payjoin is a store or wallet concept would make sense. As wallets are tight to a store right now this blends and we aren't distinguishing there, but it would help to better communicate this feature.

IMO Payjoin = wallet feature.

@dennisreimann dennisreimann force-pushed the wallet-setup-payjoin-option branch 2 times, most recently from e7535c3 to 5eb1caf Compare June 4, 2021 06:33
@dennisreimann dennisreimann marked this pull request as ready for review June 4, 2021 08:39
@dstrukt
Copy link
Member

dstrukt commented Jun 4, 2021

🎉

I'm excited to see we're all on the same page now, and this is getting pushed forward after all the discussion, etc..

Will take a look at the branch as well.

@dennisreimann
Copy link
Member Author

Integrated @Kukks' feedback and made it so that the PayJoin enabling option is only show if one of the networks supports PayJoin and has a hot wallet associated.

@NicolasDorier Can you take a look again, let me know what you think and how to proceed here? Thanks!

@NicolasDorier NicolasDorier merged commit 3c80621 into btcpayserver:master Jun 18, 2021
@NicolasDorier
Copy link
Member

@dennisreimann @dstrukt rather than removing the option in update store if the wallet isn't hot. What about make the box disabled with a tooltip explaining it is only for hotwallet?

@dstrukt
Copy link
Member

dstrukt commented Jun 18, 2021

Hmm... is the intention simply to communicate that this functionality is possible with a hot wallet?

If you feel it's really important to communicate this possibility, that solution is fine with me, but otherwise, I'd opt to remove it to reduce noise in the UI, as it will be enabled and visible by default if the wallet is hot.

..but perhaps I'm missing some additional context? @dennisreimann

@dennisreimann
Copy link
Member Author

dennisreimann commented Jun 18, 2021

I'd second what @dstrukt just said. I think there'd be better places to educate the user about that functionality and presenting it as proposed isn't really actionable. The user would have to replace the existing wallet and I think that's rather unlikely.

Maybe we should point out earlier in the setup flow that this feature exists and which route the user should take to enable it. Featuring it in the first (import vs. create) and second (hot wallet vs. watch-only) branching pages would be a better option imho.

@dennisreimann dennisreimann deleted the wallet-setup-payjoin-option branch June 28, 2021 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvements to an existing feature UI / UX Front-end issues, for front-end designers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants