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 SEPA QR Code for buyer payment #7005

Merged
merged 1 commit into from Mar 27, 2024
Merged

Add SEPA QR Code for buyer payment #7005

merged 1 commit into from Mar 27, 2024

Conversation

ghost
Copy link

@ghost ghost commented Jan 16, 2024

Implements request #7001 Allowing to initiate Sepa Payment by scanning QR Code
The Show QRC setting is saved in preferences for subsequent use.

qrc-1

qrc-2

qrc-3

@hoschi-it
Copy link

Thanks for implementing!
I would suggest changing the text to "Show QR Code", as the shortage "QRC" is not widely known.

@ghost
Copy link
Author

ghost commented Jan 17, 2024

I would suggest changing the text to "Show QR Code", as the shortage "QRC" is not widely known.

Done: qrc-4

@ghost
Copy link
Author

ghost commented Jan 17, 2024

It would be good if someone could confirm that the QR code scans ok with an actual SEPA bank app.

@alvasw
Copy link
Contributor

alvasw commented Feb 2, 2024

Hi @jmacxx,
sorry for the delay! I found someone to test the QR Code with a SEPA bank app - still waiting to hear back from him.

Copy link
Contributor

@alvasw alvasw left a comment

Choose a reason for hiding this comment

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

Nit

The QR Code works! I received video recordings of users testing four SEPA banking apps.

Feedback

  • We can show the QR Code by default because most SEPA banking apps support QR Codes.
  • After scanning the QR Code users can't double check the data because it's hidden. I think we should show the BIC and IBAN while showing the QR Code (instead of hiding it).
  • Two of the tested banking apps sent the image to the banking servers to read the QR Code. In both images the text "[...] trade ID or other text like 'bitcoin', 'BTC', or 'Bisq. [...]" was visible. Banks that do OCR processing could flag users' accounts. We should add a softbreak to the text, so that it's never near the QR Code.

@ghost
Copy link
Author

ghost commented Feb 7, 2024

qrc-5

6bc79e5 Addressing your comments. Note there is a bug with the scroll bar which affects buyer forms that have many vertical elements. Which is why I prefer the toggle button to switch between right column fields and QRC, to limit the vertical size of the form.

@hoschi-it
Copy link

hoschi-it commented Feb 11, 2024

Two of the tested banking apps sent the image to the banking servers to read the QR Code. In both images the text "[...] trade ID or other text like 'bitcoin', 'BTC', or 'Bisq. [...]" was visible. Banks that do OCR processing could flag users' accounts. We should add a softbreak to the text, so that it's never near the QR Code.

Do you think, showing the QR code in full screen (on click or something) would be useful to mitigate that risk completely?

@ghost
Copy link
Author

ghost commented Feb 12, 2024

Do you think, showing the QR code in full screen (on click or something) would be useful to mitigate that risk completely?

👍

Per your suggestion, perhaps instead of showing the small QR code (which has issues with limited screen real estate), simply a button to show it like this in a popup, :

qrc-7

@alejandrogarcia83 alejandrogarcia83 added this to the v1.9.15 milestone Feb 13, 2024
@pazza83
Copy link

pazza83 commented Feb 15, 2024

Can we hide the text, and make the screen full size with just the QR in the centre to protect privacy

@ghost
Copy link
Author

ghost commented Feb 16, 2024

Can we hide the text, and make the screen full size with just the QR in the centre to protect privacy 👍

qrc-a1

qrc-a2

[Note: code & image updated after request from @pazza83 (below) to vertically center the QRC].

@pazza83
Copy link

pazza83 commented Feb 16, 2024

@jmacxx is it possible to add a few more line breaks at the top to give more 'padding / blank space' between the menu bar and the image. To make it easier for a user not to accidently include the Bisq menu bar in their screenshot

@ghost
Copy link
Author

ghost commented Feb 21, 2024

Done, see #7005 (comment) for update.

@alvasw
Copy link
Contributor

alvasw commented Feb 25, 2024

It looks a lot better! What do you think about switching to a QR code icon. I tried out two variations and prefer the one with the icon only. What do you think?

Icon with text:
qr_code_icon_and_text
Icon only:
qr_code_only_icon

@ghost ghost closed this Feb 26, 2024
@ghost
Copy link
Author

ghost commented Feb 26, 2024

@alvasw I think this looks good.

qrc-b2


Also added a one-time message explaining why the QR is shown so large.

qrc-b1

@ghost ghost reopened this Feb 26, 2024
Copy link
Contributor

@alejandrogarcia83 alejandrogarcia83 left a comment

Choose a reason for hiding this comment

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

utACK

@alejandrogarcia83 alejandrogarcia83 merged commit ee987be into bisq-network:master Mar 27, 2024
3 checks passed
@alejandrogarcia83 alejandrogarcia83 modified the milestones: v1.9.15, v1.9.16 Apr 9, 2024
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.

4 participants