Details

Reviewers

PiRK
Fabien

Group Reviewers

Restricted Project

Commits

rABC26c4d516f9d2: [Cashtab] Support bip21 param from URL

Summary

Support new bip21 param in url at Cashtab's send screen. Parse it as a bip21 query string.

Background
Cashtab supports tx requests from webapps by parsing tx information from URL params.

Before this diff, only address and value are supported. PayButton and cashtab-components use this approach.

After this diff, we continue to support address and value params in a backwards compatible way.

Future
We should migrate to only supporting bip21 param. All new webapp features should be added through this param, which is flexible and has a well-defined spec. There is no spec for the legacy Cashtab approach. We should maintain support for the legacy approach until it is not required by PayButton or cashtab-components.

Reviewer notes
The various cases to handle in this diff are unfortunately somewhat complex (bip21 or legacy? if legacy, do you have only value and not address? is legacy valid? are legacy params duplicated?). However, automated integration tests for how specific inputs are rendered are straightforward to add for this feature. Simplification should be achieved by deprecating support for legacy params when feasible.

Test Plan

npm test

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bytesofman created this revision.Jan 24 2024, 04:57

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 24 2024, 04:57

Harbormaster completed remote builds in B26606: Diff 44516.Jan 24 2024, 05:00

ignore legacy params if duplicated

remove test that is covered & improved in another file

Harbormaster completed remote builds in B26632: Diff 44561.Jan 24 2024, 22:55

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

Failed tests logs:

====== CashTab Unit Tests: <SendXec /> Address field enabled + empty and amount field enabled + empty if legacy url params include value but not address ======
Error: expect(element).toHaveValue(ecash:qp33mh3a7qq7p8yulhnvwty2uq5ynukqcvuxmvzfhm)

Expected the element to have value:
  ecash:qp33mh3a7qq7p8yulhnvwty2uq5ynukqcvuxmvzfhm
Received:

    at Object.toHaveValue (/work/cashtab/src/components/Send/__tests__/SendXec.test.js:124:32)
    at Promise.then.completed (/work/cashtab/node_modules/jest-circus/build/utils.js:298:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/work/cashtab/node_modules/jest-circus/build/utils.js:231:10)
    at _callCircusTest (/work/cashtab/node_modules/jest-circus/build/run.js:316:40)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at _runTest (/work/cashtab/node_modules/jest-circus/build/run.js:252:3)
    at _runTestsForDescribeBlock (/work/cashtab/node_modules/jest-circus/build/run.js:126:9)
    at _runTestsForDescribeBlock (/work/cashtab/node_modules/jest-circus/build/run.js:121:9)
    at run (/work/cashtab/node_modules/jest-circus/build/run.js:71:3)
    at runAndTransformResultsToJestFormat (/work/cashtab/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
    at jestAdapter (/work/cashtab/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
    at runTestInternal (/work/cashtab/node_modules/jest-runner/build/runTest.js:367:16)
    at runTest (/work/cashtab/node_modules/jest-runner/build/runTest.js:444:34)
    at Object.worker (/work/cashtab/node_modules/jest-runner/build/testWorker.js:106:12)

Each failure log is accessible here:
CashTab Unit Tests: <SendXec /> Address field enabled + empty and amount field enabled + empty if legacy url params include value but not address

Harbormaster completed remote builds in B26633: Diff 44562.Jan 24 2024, 22:57

bytesofman edited the summary of this revision. (Show Details)Jan 24 2024, 23:00

bytesofman added inline comments.

cashtab/src/components/Send/__tests__/SendXec.test.js
105 ↗	(On Diff #44562)	moved to new file that only handles the param testing, `SendByUrlParams.test.js`
879 ↗	(On Diff #44562)	Confirm this is NOT disabled if we are parsing a bip21 string that is not set by the URL

add test confirming we ignore unsupported legacy params

Harbormaster completed remote builds in B26634: Diff 44563.Jan 24 2024, 23:06

bytesofman published this revision for review.Jan 24 2024, 23:08

bytesofman edited the summary of this revision. (Show Details)

with a comment nit

cashtab/src/components/Send/__tests__/SendByUrlParams.test.js
442 ↗	(On Diff #44563)

This revision is now accepted and ready to land.Jan 25 2024, 08:18

Fabien added a subscriber: Fabien.Jan 25 2024, 08:27

Fabien added inline comments.

cashtab/src/components/Send/SendXec.js
327 ↗	(On Diff #44563)	What happens if there is no `?` ? indexOf will return index -1 so we end up at the very beginning of hashRoute. Is that the expected behavior ?

bytesofman marked an inline comment as done.Jan 25 2024, 13:51

bytesofman added inline comments.

cashtab/src/components/Send/SendXec.js
327 ↗	(On Diff #44563)	If there is no `?`, we `return` above per line 320, as in this case `window.location.hash === '#/send'` If the user adds a param after `send` with no question mark, then the Cashtab 404 is rendered, as we don't recognize something like `#/sendbip21=...` as a route in the app. if ( !window.location \|\| !window.location.hash \|\| window.location.hash === '#/send' ) { return; } I added unit tests to cover the `no params` and `params added without a ? mark` cases.

unit tests for no params and no ? in params cases

Harbormaster completed remote builds in B26650: Diff 44582.Jan 25 2024, 13:55

remove test for bad route

In D15262#343464, @bytesofman wrote:

remove test for bad route

i need a new test page for the routes. for now, this one is manual -- cashtab will throw a 404 if the route is not #/send (or other supported routes from App.js)