Uppercase ‘bitcoin:’ in QR code URI #410

pull benthecarman wants to merge 1 commits into bitcoin-core:master from benthecarman:uppercase-uri changing 1 files +5 −1
  1. benthecarman commented at 9:01 pm on August 23, 2021: contributor

    The folks at BTCPayServer did research on this earlier this year and it seems we have reached enough wallets supporting completely uppercase URIs for the QR code

    https://github.com/btcpayserver/btcpayserver/issues/2110

  2. hebasto renamed this:
    gui: Uppercase 'bitcoin:' in QR code URI
    Uppercase 'bitcoin:' in QR code URI
    on Aug 23, 2021
  3. hebasto added the label Feature on Aug 23, 2021
  4. ben-kaufman approved
  5. ben-kaufman commented at 9:30 pm on August 23, 2021: contributor
    ACK ae7bc4dc4014ffca0d68f53cdc9b27ed0aae52f0
  6. kristapsk commented at 10:02 am on August 24, 2021: contributor
    It only makes sense to do that if final paramCount is 0, as BIP21 parameters are case sensitive. And, apart for QR code encoding, BIP173 says that lower case is preferred. See https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/972, how I propose to do it for JoinMarket GUI.
  7. benthecarman force-pushed on Aug 24, 2021
  8. gui: Uppercase 'bitcoin:' in QR code URI b6704cf7e1
  9. benthecarman force-pushed on Aug 24, 2021
  10. benthecarman commented at 2:47 pm on August 24, 2021: contributor
    Used @kristapsk suggestion
  11. kristapsk approved
  12. kristapsk commented at 3:39 pm on August 24, 2021: contributor
    ACK b6704cf7e1b005d4122c032485c9b5bba1359da8
  13. hebasto requested review from meshcollider on Aug 24, 2021
  14. hebasto requested review from achow101 on Aug 26, 2021
  15. meshcollider commented at 11:34 pm on August 26, 2021: contributor

    I don’t know much about BIP21 but this LGTM.

    utACK b6704cf7e1b005d4122c032485c9b5bba1359da8

  16. shaavan commented at 3:27 pm on August 30, 2021: contributor

    Tested on Ubuntu 20.04.

    This PR allows the capitalization of the schema of a “bitcoin:” based URI. This is done only when all the parameters of URI are not prespecified. The motivation of this PR is that QR codes for all uppercase URIs are much simpler and hence easier to scan, as discussed here.

    Tested this PR by creating a payment receiving request and copying the URI of that request. The URI with no specified info works as expected. The schema is non-capitalized on master while it is capitalized on PR.

    Copying a URI with no prespecified info:

    Master PR
    bitcoin:TB1Q7A0G8SFXRFKCGY77PPDTS964ZHAHP7KMRFF9VH BITCOIN:TB1Q7A0G8SFXRFKCGY77PPDTS964ZHAHP7KMRFF9VH

    While tested okay with URI of no-predefined info. I observed some weird behavior while copying a URI with a specified label.

    Copying a URI with a “test_label”:

    Master PR
    bitcoin:TB1QFT0PFF87246F607S895CUSYEHJNGWUZLN69A3F?label=test_label bitcoin:tb1qft0pff87246f607s895cusyehjngwuzln69a3f?label=test_label

    When I specified the request label, the PR lowercased the address part of the URI. I don’t know if this is a deliberate behavior by the PR or a bug. If this is indeed an intentional behavior, then the PR description should address and explain this. Otherwise, OP should look into this issue.

  17. benthecarman commented at 3:32 pm on August 30, 2021: contributor

    When I specified the request label, the PR lowercased the address part of the URI. I don’t know if this is a deliberate behavior by the PR or a bug. If this is indeed an intentional behavior, then the PR description should address and explain this. Otherwise, OP should look into this issue.

    This is intentional and shouldn’t have an effect on users. Since the address is case insensitive it doesn’t change anything.

  18. shaavan commented at 4:46 pm on August 30, 2021: contributor

    This is intentional and shouldn’t have an effect on users. Since the address is case insensitive it doesn’t change anything.

    Makes sense. But I would personally prefer to avoid adding any unnecessary changes, that are changing the behavior with no actual benefits.

  19. benthecarman commented at 8:12 pm on August 30, 2021: contributor
    There are benefits, the code is much cleaner this way and the URI makes more sense to have a lowercase bech32 addr when we aren’t going full caps since this is the default defined by the BIP
  20. shaavan commented at 8:30 pm on August 30, 2021: contributor

    Now you do mention it, I think these benefits are worth the change.

    tACK b6704cf7e1b005d4122c032485c9b5bba1359da8

  21. hebasto added this to the milestone 23.0 on Sep 3, 2021
  22. hebasto added the label Needs release notes on Sep 3, 2021
  23. hebasto added the label Wallet on Sep 3, 2021
  24. hebasto commented at 11:22 am on September 3, 2021: member

    Tested b6704cf7e1b005d4122c032485c9b5bba1359da8.

    • tb1q4dcd7mdjdlfgdhjhz8rg5gw2834udp2u0n9jjs:
    master PR
    Screenshot from 2021-09-03 14-00-06 Screenshot from 2021-09-03 13-53-28

    I see no benefits for encoding: both QR codes have the same 29x29 size.

    • tb1qnle64twzzjae9mrryy02z30hqaca5747rsanxt + amount=0.42000000:
    master PR
    Screenshot from 2021-09-03 14-00-28 Screenshot from 2021-09-03 13-54-30

    Both QR codes have the same 33x33 size. Is this behavior change justified (at least, it is not mentioned in the PR description)?


    @benthecarman

    There are benefits, the code is much cleaner this way…

    I guess, this statement is wrong.

  25. benthecarman commented at 3:54 pm on September 3, 2021: contributor

    I see no benefits for encoding: both QR codes have the same 29x29 size.

    Huh, does qrencode not support mixed modes? I don’t see anything about it on the repo https://github.com/fukuchi/libqrencode

  26. benthecarman commented at 3:56 pm on September 3, 2021: contributor

    There are benefits, the code is much cleaner this way… I guess, this statement is wrong.

    This was inrespect to @ShaMan239’s comment about keeping the address uppercase even when everything else is lowercase, but still doing all uppercase when possible

  27. hebasto commented at 4:01 pm on September 3, 2021: member

    There are benefits, the code is much cleaner this way… I guess, this statement is wrong.

    This was inrespect to @ShaMan239’s comment about keeping the address uppercase even when everything else is lowercase, but still doing all uppercase when possible

    I beg pardon for misunderstanding on my side. I was thinking about QR code, not the C++ code. Sorry again.

  28. hebasto commented at 4:49 pm on September 3, 2021: member

    I see no benefits for encoding: both QR codes have the same 29x29 size.

    Huh, does qrencode not support mixed modes? I don’t see anything about it on the repo https://github.com/fukuchi/libqrencode

    It does. If an URI were 47 or less symbols length, this PR makes the GUI generate a QR code version 2 (25x25).

    But the shortest URI with bech32 address is 50 characters long.

  29. benthecarman commented at 10:00 pm on September 4, 2021: contributor

    It does. If an URI were 47 or less symbols length, this PR makes the GUI generate a QR code version 2 (25x25). But the shortest URI with bech32 address is 50 characters long.

    hmm so is it even worth it to do caps at all?

  30. hebasto commented at 7:46 pm on September 7, 2021: member

    It does. If an URI were 47 or less symbols length, this PR makes the GUI generate a QR code version 2 (25x25). But the shortest URI with bech32 address is 50 characters long.

    hmm so is it even worth it to do caps at all?

    For the one address type—P2WPKH—no, not worth it. Other address types need some research work.

  31. hebasto removed this from the milestone 23.0 on Nov 21, 2021
  32. RandyMcMillan commented at 8:45 pm on November 22, 2021: contributor

    tested b6704cf7e1b005d4122c032485c9b5bba1359da8

    on MacOS 10.15.7

    Screen Shot 2021-11-22 at 3 39 49 PM

  33. achow101 commented at 5:07 am on December 7, 2021: member
    I don’t see the benefit. With regtest taproot addresses (so the longest addresses we may make), the QR codes have identical 29x29 dimensions: Screenshot_20211206_235940 Screenshot_20211207_000612
  34. hebasto commented at 8:17 am on April 3, 2022: member
    Closing this for now. Feel free to re-open if it seems required.
  35. hebasto closed this on Apr 3, 2022

  36. bitcoin-core locked this on Apr 3, 2023
  37. hebasto removed the label Needs release notes on May 16, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 02:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me