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

Remove prefix from QR code scans #121

Merged
merged 2 commits into from
Apr 4, 2024
Merged

Remove prefix from QR code scans #121

merged 2 commits into from
Apr 4, 2024

Conversation

takenagain
Copy link
Collaborator

@takenagain takenagain commented Mar 11, 2024

Fixes #120 by removing the prefix from the address when it is scanned in.

Before:
image

After:
image

@takenagain takenagain self-assigned this Mar 11, 2024
@takenagain takenagain marked this pull request as ready for review March 11, 2024 09:24
@CharlVS CharlVS added the QA Ready for QA testing label Mar 14, 2024
CharlVS
CharlVS previously approved these changes Mar 14, 2024
@smk762
Copy link
Contributor

smk762 commented Mar 15, 2024

Thanks!
I've tested QR codes for addresses with prefix for BLK, LTC and BCH, and can confirm the prefix is removed allowing a successful spend in most situations.
Confirmed the bch prefix from coins file being removed has no negative impact ( https://github.com/KomodoPlatform/coins/blob/master/coins#L1372)

I did encounter one issue though, using the QR code example from below as provided by a community member via https://coincards.com/ca/all-gift-cards/
image

Scanning this code also adds a suffix indicating the amount to be paid as shown in the image below
image

As this is related to this PR, but outside the original scope, I can create a new issue for this unless you prefer to continue in this PR.

For the suffix, redaction could be the simplest option, but it would be a nice UX enhancement to have the ?amount= value also populate the Amount field in the app send form when present. I'm not sure if there is an industry standard with respect to the suffixes, though I'd assume most would follow the obvious convention (? to indicate beginning of suffix, amount to indicate value payable). To cover all bases we could do

if suffix_params:
    if 'amount' in suffix_params:
        populate_amount_field(suffix_params['amount'])
    else:
        redact_suffix()          

@CharlVS
Copy link
Member

CharlVS commented Mar 15, 2024

@smk762 We could pivot this to be regex-based, which would then cover all current and future issues instead of having to add a handle for each new one.

Can you provide a regex that's safe to use for address sanitation?

@smk762
Copy link
Contributor

smk762 commented Mar 16, 2024

Can you provide a regex that's safe to use for address sanitation?

For Address: (?<=:)(.*?)(?=\?)

For Amount: (?<=amount=)(.*?)(?=&|$)

You can use the second one for any other anticipated query param like fields for memo etc. if needed - just change amount to the desired key.

Did not modify PaymentUriInfo to avoid issues with segwit variants
@takenagain
Copy link
Collaborator Author

Thanks for the regex, @smk762. The Amount field should now be populated if there is an ?amount= parameter.

I noticed the error below when scanning bitcoin:1M5m1DuGw...4oREzpCX from BTC-Segwit and testing potential changes to PaymentUriInfo. Should an exception be made for Segwit pairs when scanning QR codes @smk762, @CharlVS?

image

Copy link
Contributor

@smk762 smk762 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that address and amount are parsed and populate the send form fields as expected. I did not encounter any address validation errors with LTC/LTC-segwit scanning the same QR code, and where amount was greater than balance, the red box error showed as expected.
I was however able to scan a BLK qr code when attempting to send LTC. It populated the address field, then showed error when attempting to send. This is expected behaviour, I think validation taking place here rather than at scan is ok.

LGTM, thanks!

@CharlVS CharlVS merged commit 3dffc55 into dev Apr 4, 2024
1 check passed
@CharlVS CharlVS deleted the fix/remove-qr-prefix branch April 4, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA Ready for QA testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants