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

gui: Uppercase bech32 addresses in qr codes #15371

Merged

Conversation

Projects
None yet
9 participants
@benthecarman
Copy link
Contributor

commented Feb 8, 2019

Closes #12191

@fanquake fanquake added the GUI label Feb 8, 2019

@MarcoFalke MarcoFalke changed the title gui: Optimization for bech32 qr codes gui: Uppercase bech32 addresses in qr codes Feb 8, 2019

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Concept ACK

Show resolved Hide resolved src/qt/guiutil.cpp Outdated
@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Concept ACK

@benthecarman benthecarman force-pushed the benthecarman:gui_bech_32_optimized_qr_codes branch from eab2dee to e7661fc Feb 8, 2019

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

utACK

Show resolved Hide resolved src/qt/guiutil.cpp Outdated
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 9, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

@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

@benthecarman benthecarman force-pushed the benthecarman:gui_bech_32_optimized_qr_codes branch from e7661fc to 3407b44 Feb 9, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

@laanwj Are you saying that bech32 addresses should always be presented in uppercase?

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

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:

qr code

@benthecarman

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Concept ACK.

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

reACK

@meshcollider

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

utACK 3407b44

@hebasto

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

@hebasto As I commented there, I believe yoi are misreading the wikipedia summary.

@hebasto

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

  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

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Re utACK 3407b44
Though I'm not super happy about the string parsing to detect Bech32,... but probably acceptable. Going to merge this.

@jonasschnelli jonasschnelli merged commit 3407b44 into bitcoin:master Apr 29, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Apr 29, 2019

Merge #15371: gui: Uppercase bech32 addresses in qr codes
3407b44 gui: Uppercase bech32 addresses in qr codes (Ben Carman)

Pull request description:

  Closes #12191

ACKs for commit 3407b4:
  meshcollider:
    utACK 3407b44
  jonasschnelli:
    Re utACK 3407b44

Tree-SHA512: d63ecf8e9805c46c9f554cc929661a37837bc3ba9b7b931331c2a5c2b81468742e1819c9add73966083011709cc15ae1870a454348af8591b3d75d3765dca568

@benthecarman benthecarman deleted the benthecarman:gui_bech_32_optimized_qr_codes branch Apr 30, 2019

sidhujag added a commit to syscoin/syscoin that referenced this pull request May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.