Page MenuHomePhabricator

[Cashtab] Upgrade tx parsing
ClosedPublic

Authored by bytesofman on Sun, Apr 7, 23:54.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABCc8744f31cac0: [Cashtab] Upgrade tx parsing
Summary

T2207 T3405

Upgrade Cashtab tx parsing to support all known OP_RETURN lokads and token tx types

This is another tech debt fix. Tx parsing grew incrementally and was never rebuilt to accomodate many different tx types. Instead, each new type was shimmed in.

Now that we have react testing library, we can handle and test most of the rendering logic in the actual component render. Imo this is the better place to do it. At a certain point, human readable "parsed" keys are pretty silly to be storing in the wallet state.

So, parseTx is refactored to return only some calculated values from a Tx_InNode. We could lose the function entirely, but it is useful to preserve the existing unit tests so we know we are getting the same behavior (well, we can tell the behavior is changing as we expect, where we expect).

All of the rendering logic is handled in Tx/index.js, which is tested according to what the tx's look like to the user and not a large collection of JSON values as previous.

We stop using a shimmed antd Collapse which saves us another ~75ish kb on the bundle size.

Icons from https://thenounproject.com/ paid account.

Feature changes

  • Every tx is parsed as an XEC tx on the first line
  • Every OP_RETURN gets an App Action, a row under the XEC tx info. EMPP is not yet supported, but, when it is supported, we can support multiple App Actions. The code is already set up to handle these as an array.
  • Every token action gets its own row. We do not test this (yet) with large ALP txs, as this diff is big enough already. But, same logic -- any tx can have several different token actions. The previous approach of "guess which seems to be the main thing, and then just render that" -- has clear shortcomings. This is a better way to present information to normal users and power users alike. Similar to what you see at etherscan.
  • Most actions get their own icons.
  • All txs are "to" or "from" an address or contact. We assume txs are from inputs[0].outputScript. If a tx is "to" multiple addresses -- for now we just assume it is to the first outputScript that is not an input of the tx. This can be optimized later (mb we pick the one who received the "most"), but will always need to make some kind of assumption unless you just render everything that is happening.
  • We now add a "to" address to contacts
  • Remove "copy" feature from Cashtab msg. Can be added back in if demand, but should have its own copy icon, not a conditionally rendered one in the expand panel.
  • Staking rewards have own render
Test Plan

npm test

this diff is live at https://cashtab-local-dev.netlify.app/

image.png (558×491 px, 66 KB)

image.png (295×489 px, 33 KB)

image.png (440×497 px, 53 KB)

expandable panel for each tx

image.png (117×497 px, 9 KB)

burning a token with a long name

image.png (246×478 px, 19 KB)

image.png (132×473 px, 17 KB)

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

parsing and tests for alias registrations and etoken receive txs

Tail of the build log:

Installing mock-chronik-client dependencies...
/work/modules/mock-chronik-client /work/abc-ci-builds/cashtab-tests

added 236 packages, and audited 237 packages in 2s

35 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
/work/cashtab /work/modules/mock-chronik-client /work/abc-ci-builds/cashtab-tests

added 1718 packages, and audited 1719 packages in 31s

262 packages are looking for funding
  run `npm fund` for details

1 moderate severity vulnerability

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> cashtab@2.24.2 build
> node scripts/build.js

Creating an optimized production build...
Failed to compile.

[eslint] 
src/chronik/index.js
  Line 9:10:  'toXec' is defined but never used  no-unused-vars

Search for the keywords to learn more about each error.


Build cashtab-tests failed with exit code 1

more tests, icons, match all functionality except for hiding msgs from unknown sender

version bump, better rendering of paybutton txs

Remove ability to use deprecated setting

use svg instead of png, update test accessors, update order of icons

stub support for incoming etoken txs

get notifications to work for tokens

bytesofman edited the test plan for this revision. (Show Details)

better stack of unknown msg, remove todo

remove todo comments that are done

bytesofman added inline comments.
cashtab/src/chronik/fixtures/mocks.js
7267 ↗(On Diff #46980)

extensive changes to mocks here to fit new shape and add some more cached tokens to test other rendered txs.

not necessary to review in detail. the txs are straight from chronik.

cashtab/src/chronik/index.js
200 ↗(On Diff #46980)

before this diff, this function handled all of the rendering. Because we could test this function and (at the time) were not testing rendering, parsed txs used many human readable flags to show how a tx should be rendered.

That logic is now all in an integration-tested component, which tests how the txs should render.

So, this function is stripped down to a few repeated calculations based on the output of chronik.tx.

cashtab/src/components/App/fixtures/mocks.js
1011 ↗(On Diff #46980)

updating a mock with an in-node chronik output

cashtab/src/components/Configure/Configure.js
184 ↗(On Diff #46977)

This setting was added in response to a scam being run a couple years back. imo the response was an over reaction.

complicated feature. hard to explain even on the switch.

results in lots of cashtab screenshots saying "THIS MIGHT BE A SCAM"

good riddance. will lose the setting field from localforage when we add something there and need to migrate.

cashtab/src/components/Home/Tx/__tests__/index.test.js
1 ↗(On Diff #46980)

a lot to review here.

key points:

  • we render all of the previous vectors of parseChronikTx
  • we test that we see expected strings and content in each tx

Tricky to review it all now but will be easy to review future upgrades of isolated tx types.

cashtab/src/components/Home/Tx/index.js
154 ↗(On Diff #46980)

not in this diff tho

cashtab/src/components/Home/TxHistory.js
16 ↗(On Diff #46980)

pass user locale for better rendering

20 ↗(On Diff #46980)

data was always a bad name for this prop

cashtab/src/utils/cashMethods.js
40 ↗(On Diff #46980)

we change the name of this to getHashes and move it to wallet, where it belongs

cashtab/src/utils/formatting.js
107 ↗(On Diff #46980)

generalize this function. useful function, should not only be used to render wallet balances.

cashtab/src/wallet/useWallet.js
673 ↗(On Diff #46980)

this logic is a placeholder.

imo this diff is big enough and we do not need to optimize here yet

task is up to put this logic into a function which can be unit tested. the notification will (probably) always be a little different from the rendered tx history component and require its own special handling.

It's still important tho as we want these notifications to be absolutely instant and with as much info as possible.

694 ↗(On Diff #46980)

in another diff

emack requested changes to this revision.Wed, Apr 10, 10:57
emack added a subscriber: emack.
emack added inline comments.
cashtab/src/chronik/index.js
207 ↗(On Diff #46982)

It would make more sense for this and 1 other link to point to the actual multi send tx in explorer so the user can the full recipients list. It is currently pointing to the history of the first output address which isn't helpful

image.png (97×523 px, 8 KB)

cashtab/src/components/Home/Tx/index.js
199–203 ↗(On Diff #46982)

I don't think displaying the static alias registration payment address here adds any value. Suggest removing to give more room for longer alias names.

image.png (241×526 px, 29 KB)

412–431 ↗(On Diff #46982)

Need to adjust the text-wrap css param here for messages with unbroken strings of 41 chars or more.

image.png (306×837 px, 59 KB)

424–427 ↗(On Diff #46982)

Replying to a message gives you this error when you click on the Cashtab Msg toggle in sendXec

image.png (334×861 px, 41 KB)

Also the message input should be open by default when its routed from this shortcut.

788 ↗(On Diff #46982)

This dropped off from this diff and imo is important to retain. Apply to both send and receive txs.

814 ↗(On Diff #46982)

Add "Tx ID" to the start of this message so its obvious to non-technical users what they just copied.

image.png (103×330 px, 13 KB)

827 ↗(On Diff #46982)

For whatever reason, if you're viewing this tx from the receiving wallet, this link takes you to blockchair with a BTC address displayed with BTC transactions. Not sure if it's a blockchair bug or something here. I can share seed via tg if needed.

image.png (1×570 px, 289 KB)

This revision now requires changes to proceed.Wed, Apr 10, 10:57

Also the existing horizontal lines separating each tx should be retained, otherwise each grouping of info is not immediately obvious.

Also the existing horizontal lines separating each tx should be retained, otherwise each grouping of info is not immediately obvious.

cashtab/src/chronik/index.js
207 ↗(On Diff #46982)

good point. Updated so it is 2 links.

Thought about just showing the other address if it is 2 addresses. However, this would take some presentation rework in "which contact is the user adding" -- so, for now, left at "and 1 other(s)"

cashtab/src/components/Home/Tx/index.js
199–203 ↗(On Diff #46982)

It is a bit of a half measure here, but imo worth it as a placeholder to help design the NFT-based alias tx rendering.

i.e. we should link to the address if we show it, but since this render will probably only be seen by beta testers not worth adding.

I think it's important to keep though because there is no easy way to tell the address if you open the tx in the explorer. It's encoded in a 21-byte string of type and hash that is not really human readable but is easy to parse in this kind of rendering function.

412–431 ↗(On Diff #46982)

good catch, patched

424–427 ↗(On Diff #46982)

good catch, patched

827 ↗(On Diff #46982)

Is the url still rendered correctly?

If I go to https://blockchair.com/ecash/transaction/20e7ed30631bf07fadcb294176c8a47c14e946437150deeed3939e81efa5067d.pdf I just get the receipt.

If the link is rendered correctly, not much we can do except deprecate or tolerate. imo the feature is kind of cool but I don't think it's really a value add.

bytesofman marked 3 inline comments as done.

improved styles per feedback

Note: investigating this CI failure, which is also happening on other diffs

image.png (74×237 px, 6 KB)

./contrib/teamcity/build-configurations.py cashtab-tests is working locally

Note: investigating this CI failure, which is also happening on other diffs

image.png (74×237 px, 6 KB)

./contrib/teamcity/build-configurations.py cashtab-tests is working locally

Looks like an issue with one of the CI servers:
https://reviews.bitcoinabc.org/harbormaster/build/56421/

HTTP 60

cashtab/src/chronik/fixtures/mocks.js
9788 ↗(On Diff #46982)

todo

bytesofman marked an inline comment as done.

remove missed todo

Minor non-blocking nit:
Use the existing shading in prod for the horizonal separator between txs. It's obvious enough to be a separator but not too obvious that it starts adding to the busy-ness of the screen.
Prod

image.png (29×496 px, 625 B)

This diff

image.png (33×522 px, 1 KB)

Re: the blockchair thing, it's working now which means it's a blockchair issue and not related to this diff. The link is rendered correctly.

This revision is now accepted and ready to land.Wed, Apr 10, 23:43

Minor non-blocking nit:
Use the existing shading in prod for the horizonal separator between txs. It's obvious enough to be a separator but not too obvious that it starts adding to the busy-ness of the screen.
Prod

image.png (29×496 px, 625 B)

This diff

image.png (33×522 px, 1 KB)

Re: the blockchair thing, it's working now which means it's a blockchair issue and not related to this diff. The link is rendered correctly.

updated separator color and added to theme

This revision was automatically updated to reflect the committed changes.