Page MenuHomePhabricator

[Cashtab] parse sats from scanned qr code amount param
ClosedPublic

Authored by josephroyking on Wed, Jan 13, 21:01.

Details

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
  1. Generate a QR code at mercurymessenger.io
  2. npm start to run Cashtab locally
  3. Attempt to scan your mercurymessenger.io QR code from the Send screen
  4. Observe address is populated with bitcoincash: prefix and amount is populated with satoshis converted to BCHA
  5. Scan normal QR codes with no params and observe no amount is input into the amount field
  6. 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien requested changes to this revision.Thu, Jan 14, 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.Thu, Jan 14, 08:33

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

Fabien requested changes to this revision.Thu, Jan 14, 15:22

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.Thu, Jan 14, 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.

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

Fabien requested changes to this revision.Thu, Jan 14, 19:36

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

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

Adding unit tests for new address methods

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

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

Fabien requested changes to this revision.Fri, Jan 15, 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.Fri, Jan 15, 10:13
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.

Generalize ecash to bitcoincash conversion function, add tests, use startWith() string method

Do not attempt to send if address converted to error

Remove condition check from toLegacy() call in Send.js

Fabien added inline comments.
web/cashtab/src/components/Common/Ticker.js
95 ↗(On Diff #26982)

Right, I missed the paramCheck variable was the result of the separation on ? and not &

This revision is now accepted and ready to land.Fri, Jan 15, 13:47