gui: Uppercase bech32 addresses in qr codes #15371

pull benthecarman wants to merge 1 commits into bitcoin:master from benthecarman:gui_bech_32_optimized_qr_codes changing 1 files +3 −1

benthecarman commented at 3:20 PM on February 8, 2019: contributor

Closes #12191
fanquake added the label GUI on Feb 8, 2019
MarcoFalke renamed this:
~~gui: Optimization for bech32 qr codes~~
gui: Uppercase bech32 addresses in qr codes
on Feb 8, 2019
sipa commented at 4:15 PM on February 8, 2019: member

Concept ACK
in src/qt/guiutil.cpp:178 in eab2dee2d5 outdated
```
 174 | @@ -175,7 +175,8 @@ bool parseBitcoinURI(QString uri, SendCoinsRecipient *out)
 175 |  
 176 |  QString formatBitcoinURI(const SendCoinsRecipient &info)
 177 |  {
 178 | -    QString ret = QString("bitcoin:%1").arg(info.address);
 179 | +    bool bech_32 = (info.address.startsWith("bc1") || info.address.startsWith("tb1"));
```
laanwj commented at 5:11 PM on February 8, 2019:

I don't like hardcoding this here—don't we have constants to use somewhere?

benthecarman commented at 5:21 PM on February 8, 2019:

I looked around and couldn't find any constants, I could add them unless someone else knows if some exist already.
laanwj commented at 5:26 PM on February 8, 2019:

there's nothing on ChainParams ? Edit: This returns the prefix (bc tb bcrt):

const std::string& Bech32HRP() const { return bech32_hrp; }
sipa commented at 6:12 PM on February 8, 2019:

Yeah, you can test whether the address starts with params.Bech32HRP() + "1".

benthecarman commented at 6:17 PM on February 8, 2019:

params isn't available in this scope

sipa commented at 6:18 PM on February 8, 2019:

You can get the global params object by calling Params().
laanwj commented at 5:12 PM on February 8, 2019: member

Concept ACK
benthecarman force-pushed on Feb 8, 2019
gmaxwell commented at 11:50 PM on February 8, 2019: contributor

utACK

in src/qt/guiutil.cpp:180 in e7661fcd56 outdated

 174 | @@ -175,7 +175,9 @@ bool parseBitcoinURI(QString uri, SendCoinsRecipient *out)
 175 |  
 176 |  QString formatBitcoinURI(const SendCoinsRecipient &info)
 177 |  {
 178 | -    QString ret = QString("bitcoin:%1").arg(info.address);
 179 | +    bool bech_32 = info.address.startsWith(QString::fromStdString(Params().Bech32HRP() + "1"));
 180 | +
 181 | +    QString ret = QString("bitcoin:%1").arg(bech_32? info.address.toUpper() : info.address);

jonasschnelli commented at 6:50 AM on February 9, 2019:

nit: space between bech32 and the ? operator.

jonasschnelli commented at 6:51 AM on February 9, 2019: contributor

Concept ACK. About this implementation: a bit odd that a GUI util function needs to parse addresses in order to differentiate between Bech32 and legacy addresses.
benthecarman commented at 6:32 PM on February 9, 2019: contributor

@jonasschnelli It could be done with a parameter but then ReceiveRequestDialog::update(), ReceiveCoinsDialog::copyURI(), ReceiveRequestDialog::update(), and ReceiveRequestDialog::on_btnCopyURI_clicked() would need a new parameter as well
gui: Uppercase bech32 addresses in qr codes 3407b446cc
benthecarman force-pushed on Feb 9, 2019
laanwj commented at 4:02 PM on February 10, 2019: member

About this implementation: a bit odd that a GUI util function needs to parse addresses in order to differentiate between Bech32 and legacy addresses.

Very good point Jonas. It seems something that should happen at another level before passing it to this function. E.g. a flag somewhere to generate the beck32 address as uppercase in the first place.
benthecarman commented at 7:11 AM on February 11, 2019: contributor

@laanwj Are you saying that bech32 addresses should always be presented in uppercase?
laanwj commented at 12:46 PM on February 11, 2019: member

No, that's not what I'm saying. But if you need one in uppercase it's probably best to do so closer to the source where it's generated.
fanquake commented at 2:23 PM on February 11, 2019: member

Regardless of how it's done, is the end goal here to also have the address inside the QR code uppercase? As it's currently implemented (3407b44), we only get an uppercase address in the URI, and not for the other two addresses displayed in the QR code dialog. i.e:
benthecarman commented at 7:12 PM on February 11, 2019: contributor

I feel like the current implementation makes the most sense. It would be confusing to have addresses shown primarily in lowercase but when generating a URI it will be shown in uppercase. Here it just has the address in the URI in uppercase for the optimization and the user never knows that the optimization occurred.
hebasto commented at 4:37 PM on February 15, 2019: member

~Concept ACK.~
Sjors commented at 1:15 PM on February 19, 2019: member

Concept ACK. I don't mind too much what the URI looks like, but it doesn't need the same optimization as the QR code, so my light preference is to keep that lower case.
gmaxwell commented at 5:42 AM on February 20, 2019: contributor

reACK
meshcollider commented at 2:38 AM on April 14, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/pull/15371/commits/3407b446cc5ec0725c0505e3b933f43d86286ad5
hebasto commented at 10:26 AM on April 22, 2019: member

NACK See: #12191 (comment)

As BIP 0021 provides usage of ?, = and & characters which do not belong to the alphanumeric set, ~it is impossible to optimize QR codes by making addresses uppercase, unfortunately.~
sipa commented at 4:02 PM on April 22, 2019: member

@hebasto As I commented there, I believe yoi are misreading the wikipedia summary.
hebasto commented at 7:18 PM on April 22, 2019: member
1. My previous statement regarding impossibility to optimize QR codes by making addresses uppercase is wrong (see qrencode manual).
2. It worth to mention BIP 0021 allows further capitalizing:
The scheme component ("bitcoin:") is case-insensitive...
1. What are benefits of this PR for users / developers / maintainers?
jonasschnelli commented at 7:04 AM on April 29, 2019: contributor

Re utACK 3407b446cc5ec0725c0505e3b933f43d86286ad5 Though I'm not super happy about the string parsing to detect Bech32,... but probably acceptable. Going to merge this.
jonasschnelli merged this on Apr 29, 2019
jonasschnelli closed this on Apr 29, 2019
jonasschnelli referenced this in commit 3a0d6da098 on Apr 29, 2019
benthecarman deleted the branch on Apr 30, 2019
sidhujag referenced this in commit e316caff5c on May 1, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#12191 bech32-optimized QR codes