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 version 2 (reduce QR-Code Density) #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaelWuensch
Copy link

This PR is a draft and open for discussion.

The main goal is to reduce the density of the lndconnect QR-Code, which causes problems on some scanning devices and leads to bad UX in general.

In the proposed solution there are two notable changes:

  • the introduction of a versioning system
  • optimizations to reduce density

Until now, no versioning system exists.
That's why for version 2 I changed the Uri from lndconnect:// to lndconn://
By doing this we will not break any existing implementations.
In the new version a query param "v" is added which is used for versioning to allow future updates.
In the current proposal the new version (2) is the default and you have to explicitly use --version=1 to generate an old connect string.

Density optimizations:

  • sha256 of the certificate instead of the full certificate
  • shorter names (lndconnect > lndconn, cert > c; macaroon > m)
  • QR-Code error correction reduced to low for both terminal and image QR-Code.

The error correction reduction for the QR-Code is also done for version 1 in this PR as it is a non breaking change.

Comparison images:
v1 - old:
v1-old

v1 - new:
v1-new

v2:
v2

I successfully tested certificate validation by hash on a custom build of Zap Android to proof that v2 is actually working.

This PR does not include an update for the documentation. I can add that later when we come to an consensus on what to do :)

Alternative solution:
Instead of a version 2 we could also just add a --certificateHash flag and reduce the error correction levels to low. The density reduction from shortening names is marginal and can be omitted.
It is a simple change but that would mean wallets that already support lndconnect would receive certificate hashes instead of full certificates without knowing it and fail although they claim to support it. (As long as they don't support the new specs)
For backwards compatibility wallets would have to implement a check of the certificate length. If it matches 32 byte (sha256 length) it is clear that the certificate is hashed, if not, it should be treated as a full certificate.

Additional note:
It might be possible to further reduce density by encoding with bech32 and use the upper case version, but that seems like it makes implementation of lndconnect unnecessarily complex for little benefit. With the optimizations from above the code is already easy to scan again.

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.

1 participant