Details

Reviewers

alcipir
majcosta
Fabien

Group Reviewers

Restricted Project

Commits

rABCce6b5329e9c0: [Cashtab] parse sats from scanned qr code amount param

Summary

Cashtab does not parse parameters from an address nor does it remove them if they are present. However, it does validate for cashAddr format.

In its current state, Cashtab will throw a validation error after scanning a QR code with parameters.

This diff (1) Removes any scanned parameters from the address when entered into the 'to' field and
(2) parses for an amount param, and passes it to the amount field (Send BCHA screen only).

Its designed to work with mercurymessenger.io, which uses the prefix ecash: and satoshis after the amount param.

Test Plan

Generate a QR code at mercurymessenger.io
npm start to run Cashtab locally
Attempt to scan your mercurymessenger.io QR code from the Send screen
Observe address is populated with bitcoincash: prefix and amount is populated with satoshis converted to BCHA
Scan normal QR codes with no params and observe no amount is input into the amount field
Scan the mercurymessenger.io QR code with param from the SendToken.js screen and observe no amount is input into the amount field

Diff Detail

Repository

rABC Bitcoin ABC

Branch

support-scanned-amts

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 14801
Build 29556: Build Diff	cashtab-tests
Build 29555: arc lint + arc unit

Event Timeline

bytesofman created this revision.Jan 13 2021, 21:01

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 13 2021, 21:01

bytesofman requested review of this revision.Jan 13 2021, 21:01

Harbormaster completed remote builds in B14753: Diff 26934.Jan 13 2021, 21:03

Fabien requested changes to this revision.Jan 14 2021, 08:33

Fabien added inline comments.

web/cashtab/src/components/Common/ScanQRCode.js
61 ↗	(On Diff #26934)	This is hacky. If you wrap this inside a factility such as `isCurrency()` or `isToken()` then you can have a single place to add new prefixes, and have it to work everywhere the same. For now this is causing different behavior when pasting the address or using the QR code scanner.
91 ↗	(On Diff #26934)	The same thing with this block, I don't see why it should only be a QR code thing.
96 ↗	(On Diff #26934)	Is the checksum re-computed ?

This revision now requires changes to proceed.Jan 14 2021, 08:33

Move address parser checks into functions, add checksum conversion for ecash prefix

Harbormaster completed remote builds in B14764: Diff 26950.Jan 14 2021, 12:26

Rebase

Harbormaster completed remote builds in B14777: Diff 26968.Jan 14 2021, 15:17

More a question, but why limiting this parsing to QR code only ? Couldn't this be done at the lower level so the copying ecash:xxxx?amount=NNN into the address input field would behave the same ?

web/cashtab/src/components/Common/Ticker.js
22 ↗	(On Diff #26950)	Looks like all these additions could benefit from unit testing.

This revision now requires changes to proceed.Jan 14 2021, 15:22

Yes, good point. When I put this together yesterday, in my head I thought the copy-paste parse would need to be a separate diff and it was worth getting the QR scanner out first.

Reflecting...yes, it is possible to implement them both differently...but I don't think it offers any advantages ¯\_(ツ)_/¯

Will amend accordingly.

Moving parsing logic to form level and not QR code to support copy pasted and scanned ecash addr and params.
Added validation to Send.js and SendToken.js address fields in line with new params and valid addresses.
Added warning notices to Send.js and SendToken.js on detection of address params.

Harbormaster completed remote builds in B14781: Diff 26976.Jan 14 2021, 17:32

Bump extension version. Will submit this to Chrome store so extension can work with mercurymessenger.io.

Harbormaster completed remote builds in B14783: Diff 26978.Jan 14 2021, 19:18

You have a rebase issue, this diff includes changes from D8905. Also the tests are still missing.

This revision now requires changes to proceed.Jan 14 2021, 19:36

Adding unit tests for new address methods

Add unit test for isToken()

Harbormaster completed remote builds in B14784: Diff 26980.Jan 14 2021, 21:16

Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Tail of the build log:

/work/web/cashtab /work/abc-ci-builds/cashtab-tests
npm notice 
npm notice New minor version of npm available! 7.3.0 -> 7.4.0
npm notice Changelog: <https://github.com/npm/cli/releases/tag/v7.4.0>
npm notice Run `npm install -g npm@7.4.0` to update!
npm notice 
npm ERR! code ETARGET
npm ERR! notarget No matching version found for pushdata-bitcoin@1.2.1.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2021-01-14T21_16_13_847Z-debug.log
Build cashtab-tests failed with exit code 1

Harbormaster completed remote builds in B14785: Diff 26981.Jan 14 2021, 21:18

Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Tail of the build log:

/work/web/cashtab /work/abc-ci-builds/cashtab-tests
npm notice 
npm notice New minor version of npm available! 7.3.0 -> 7.4.0
npm notice Changelog: <https://github.com/npm/cli/releases/tag/v7.4.0>
npm notice Run `npm install -g npm@7.4.0` to update!
npm notice 
npm ERR! code ETARGET
npm ERR! notarget No matching version found for pushdata-bitcoin@1.2.1.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2021-01-14T21_18_43_087Z-debug.log
Build cashtab-tests failed with exit code 1

New unit test & fix package-lock.json build

Harbormaster completed remote builds in B14786: Diff 26982.Jan 14 2021, 21:37

Fabien requested changes to this revision.Jan 15 2021, 10:13

Fabien added inline comments.

web/cashtab/src/components/Common/Ticker.js
18 ↗	(On Diff #26982)	Is that a thing yet ?
47 ↗	(On Diff #26982)	There is an opportunity to factorize the code a bit here.
60 ↗	(On Diff #26982)	I don't think this makes a lot of sense, what about: export function toLegacy(address) if (isCash(address)) { const { _, type, hash } = cashaddr.decode(address); return cashaddr.encode('bitcoincash', type, hash); } // Maybe return an error instead ? return address; This will let you update the prefixes as you will.
95 ↗	(On Diff #26982)	This is a very limited used as is, because it only handles the amount param if it is the only param in the URI. It's fine to ignore the fields that are not supported yet but it will make sense to loop over the params, so something like `ecash:xxxx?label=hello&amount=42` would be parsed correctly.
web/cashtab/src/components/Common/__tests__/Ticker.test.js
41 ↗	(On Diff #26982)	Some missing cases: isToken returns false ecashToCashAddr fails
web/cashtab/src/components/Send/Send.js
187 ↗	(On Diff #26982)	See other comment regarding the `ecashToCashAddr()` function
web/cashtab/src/hooks/__tests__/useBCH.test.js
6 ↗	(On Diff #26982)	This seems to be an unrelated change

This revision now requires changes to proceed.Jan 15 2021, 10:13

bytesofman added inline comments.Jan 15 2021, 12:15

web/cashtab/src/components/Common/Ticker.js
18 ↗	(On Diff #26982)	No. However, we know that `simpleledger:` will not be the standard on BCHA, so I think it's a good opportunity to support an array of possible prefixes. I think this PR is the appropriate place since it uses the same approach as the base layer prefix expansion.
60 ↗	(On Diff #26982)	Excellent point, thanks!
95 ↗	(On Diff #26982)	The `.has()` method of the `URLSearchParams(queryString)` object in JavaScript will recognize the `amount` param regardless of its order.