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

Qt: advise users not to switch wallets when opening a BIP70 URI. #16858

Merged
merged 1 commit into from Sep 15, 2019

Conversation

@jameshilliard
Copy link
Contributor

commented Sep 12, 2019

It would probably be a good idea to have something like this before #15584 is merged.

@jameshilliard jameshilliard referenced this pull request Sep 12, 2019
1 of 3 tasks complete
@fanquake fanquake added the GUI label Sep 12, 2019
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Thanks! (hadn't noticed that this was a PR, not an issue)

@laanwj laanwj added this to the 0.19.0 milestone Sep 12, 2019
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Thanks for adding this.
utACK 1153caf
Without further objectsions, this gets merged on the 15th of Sept.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

String-only ACK.

@@ -328,7 +328,9 @@ void PaymentServer::handleURIOrFile(const QString& s)
#ifndef ENABLE_BIP70
if (uri.hasQueryItem("r")) { // payment request
Q_EMIT message(tr("URI handling"),
tr("Cannot process payment request because BIP70 support was not compiled in."),
tr("Cannot process payment request because BIP70 support was not compiled in.")+
tr("Due to widespread security flaws in BIP70 it's strongly recommended that any merchant instructions to switch wallets be ignored.")+

This comment has been minimized.

Copy link
@achow101

achow101 Sep 12, 2019

Member

nit: Spaces at beginning of each sentence, or maybe a <br>. Currently the notification puts these as one line and it kind of runs on. It's a giant block of text.

This comment has been minimized.

Copy link
@jameshilliard

jameshilliard Sep 14, 2019

Author Contributor

I'll probably do a follow up commit to clean up formatting once I'm no longer traveling.

@achow101

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Screenshot:

Screenshot_20190912_160205

Copy link
Member

left a comment

ACK 1153caf

Whilst I agree that some more spacing would be nice, if this commit doesn't get updated it can just be merged as is. Did bare-minimum testing on macOS.

message

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

With so much text, it might make sense to turn this into an a popup instead of a notification. No one reads so much text in a notification so quickly.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

Maybe drop the last sentence or just rephrase to "Cannot process payment request - BIP70 has been disabled by default due to widespread security flaws. Ignore any recommendations to change wallets to use BIP 70".

@jameshilliard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

@TheBlueMatt I think it's important to have an explicit recommendation on what's needed to fix the error. The instructions for users to contact merchants to provide a BIP21 compatible URI should help discourage merchants from forcing BIP70 by increasing support requests if they refuse to provide BIP21 compatible URI's(reduced support costs are the main reason BIP70 is still being forced by processors).

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

Hmm, but its ultimately not up to the user - asking for BIP21 from the merchant isn't so different from asking for "not BIP70".

@jameshilliard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

Well it's up to the merchant ultimately to support it, but if a bunch of users submit support requests to the merchant asking for BIP21 compatible URI's then it's much more likely the merchant will add support for BIP21 due to customer demand as opposed to potentially some other protocol.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

@jameshilliard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

Well you would want to specify BIP21 so that the merchant doesn't just suggest a different incompatible BIP70-like payment method.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

@jameshilliard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

Does literally anyone implement that?

Bitpay does apparently as an alternative to BIP70.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

@jameshilliard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

Well I think the point is to ensure that customers are not just requesting a BIP70 alternative but BIP21 specifically as Bitpay is likely to suggest using jsonPaymentProtocol if the request is not explicitly for BIP21 support. I wonder if it would make sense to link to a tool like https://github.com/achow101/payment-proto-interface in the error message for decoding bitpay requests.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

I mean I'm not sure how you capture that in a way that (a) users understand and (b) is actually short enough that any user will read it (cause the current text no one is gonna read).

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Going to merge this.
Turing into a popup, if that makes sense, can be done later.

jonasschnelli added a commit that referenced this pull request Sep 15, 2019
…IP70 URI.

1153caf Qt: advise users not to switch wallets when opening a BIP70 URI. (James Hilliard)

Pull request description:

  It would probably be a good idea to have something like this before #15584 is merged.

ACKs for top commit:
  jonasschnelli:
    utACK 1153caf
  fanquake:
    ACK 1153caf

Tree-SHA512: 6e682dd280c44eaafb1206c32439df42a20173c33297bf93dd607f0a7a2faec8e2d17fff83c85027083ebd11a71795b443e707992251574370dd1d46b7bff060
@jonasschnelli jonasschnelli merged commit 1153caf into bitcoin:master Sep 15, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 16, 2019
…ing a BIP70 URI.

1153caf Qt: advise users not to switch wallets when opening a BIP70 URI. (James Hilliard)

Pull request description:

  It would probably be a good idea to have something like this before bitcoin#15584 is merged.

ACKs for top commit:
  jonasschnelli:
    utACK 1153caf
  fanquake:
    ACK 1153caf

Tree-SHA512: 6e682dd280c44eaafb1206c32439df42a20173c33297bf93dd607f0a7a2faec8e2d17fff83c85027083ebd11a71795b443e707992251574370dd1d46b7bff060
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.