Page MenuHomePhabricator
Differential D17448

[Cashtab] Implement exhaustive parsing of tx notifications
ClosedPublic
Actions

Authored by bytesofman on Wed, Jan 1, 14:34.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCa8765b3de5d5: [Cashtab] Implement exhaustive parsing of tx notifications
Summary

We recently upgraded how Cashtab parses notifications so that it can all be handled async. This also means we can do the same for incoming tx notifications.

Have been meaning to do this for some time. This means our notifications for incoming txs can be much better. It also fixes some existing bugs right now. For example, Agora Sales are parsed correctly in tx history, but not in incoming tx notifications.

Because there are so many cases, may detect some available improvements and optimizations in prod. I don't think it's practical to get this perfect right here. Having all of the tests for all of the cases is a good start.

Test Plan

npm test

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

Diff Detail

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

Event Timeline

bytesofman created this revision.Wed, Jan 1, 14:34
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Jan 1, 14:34
Harbormaster completed remote builds in B31867: Diff 51893.Wed, Jan 1, 14:34
teamcity added a comment.Wed, Jan 1, 14:39
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Tail of the build log:

Run `npm audit` for details.

> ecash-lib@1.2.0 build
> tsc && tsc -p ./tsconfig.build.json && cp -r ./src/ffi ./dist

/work/modules/ecash-agora /work/abc-ci-builds/cashtab-tests

added 364 packages, and audited 367 packages in 2s

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

2 vulnerabilities (1 moderate, 1 high)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> ecash-agora@0.2.0 build
> tsc && tsc -p ./tsconfig.build.json

/work/cashtab /work/abc-ci-builds/cashtab-tests
npm warn deprecated @humanwhocodes/config-array@0.11.14: Use @eslint/config-array instead
npm warn deprecated @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
npm warn deprecated eslint@8.56.0: This version is no longer supported. Please see https://eslint.org/version-support for other options.

added 1489 packages, and audited 3310 packages in 25s

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

8 vulnerabilities (6 moderate, 2 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.

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

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

TS2366: Function lacks ending return statement and return type does not include 'undefined'.
    1029 |     selectedFiatTicker: string,
    1030 |     genesisInfo?: GenesisInfo,
  > 1031 | ): string => {
         |    ^^^^^^
    1032 |     const {
    1033 |         parsedTokenEntries,
    1034 |         satoshisSent,


Build cashtab-tests failed with exit code 1
bytesofman updated this revision to Diff 51898.Wed, Jan 1, 23:25

first pass finish tests and implement

Harbormaster completed remote builds in B31870: Diff 51898.Wed, Jan 1, 23:30
teamcity added a comment.Wed, Jan 1, 23:30
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Tail of the build log:

Run `npm audit` for details.

> ecash-lib@1.2.0 build
> tsc && tsc -p ./tsconfig.build.json && cp -r ./src/ffi ./dist

/work/modules/ecash-agora /work/abc-ci-builds/cashtab-tests

added 364 packages, and audited 367 packages in 2s

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

2 vulnerabilities (1 moderate, 1 high)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> ecash-agora@0.2.0 build
> tsc && tsc -p ./tsconfig.build.json

/work/cashtab /work/abc-ci-builds/cashtab-tests
npm warn deprecated @humanwhocodes/config-array@0.11.14: Use @eslint/config-array instead
npm warn deprecated @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
npm warn deprecated eslint@8.56.0: This version is no longer supported. Please see https://eslint.org/version-support for other options.

added 1489 packages, and audited 3310 packages in 25s

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

8 vulnerabilities (6 moderate, 2 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.

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

Creating an optimized production build...

Treating warnings as errors because process.env.CI = true.
Most CI servers set it automatically.

Failed to compile.

[eslint] 
src/wallet/useWallet.ts
  Line 41:5:   'toXec' is defined but never used           @typescript-eslint/no-unused-vars
  Line 50:10:  'toFormattedXec' is defined but never used  @typescript-eslint/no-unused-vars


Build cashtab-tests failed with exit code 1
bytesofman updated this revision to Diff 51899.Thu, Jan 2, 03:03

lint

Harbormaster completed remote builds in B31871: Diff 51899.Thu, Jan 2, 03:09
bytesofman updated this revision to Diff 51900.Thu, Jan 2, 03:10

send xec notification OR token notification

Harbormaster completed remote builds in B31872: Diff 51900.Thu, Jan 2, 03:16
bytesofman updated this revision to Diff 51905.Thu, Jan 2, 13:15

do not return a notification for cases where Cashtab already creates one from UI

bytesofman edited the summary of this revision. (Show Details)Thu, Jan 2, 13:17
bytesofman edited the test plan for this revision. (Show Details)
Harbormaster completed remote builds in B31877: Diff 51905.Thu, Jan 2, 13:20
bytesofman published this revision for review.Thu, Jan 2, 13:24

image.png (366×682 px, 508 KB)

Fabien requested changes to this revision.Thu, Jan 2, 14:22
Fabien added a subscriber: Fabien.
Fabien added inline comments.
cashtab/src/chronik/index.ts
1088 ↗(On Diff #51905)

You should move the processing of the recipient apart just like you did with the renderedAmount. The reason is that you want to show the contact name if this address belongs to your contact list (for a follow-up)

This revision now requires changes to proceed.Thu, Jan 2, 14:22
bytesofman updated this revision to Diff 51926.Thu, Jan 2, 16:35

split out the addr parse and preview logic, set up for easier rendering of contacts in notifications

Harbormaster completed remote builds in B31891: Diff 51926.Thu, Jan 2, 16:40
bytesofman updated this revision to Diff 51927.Thu, Jan 2, 16:41

less confusing variable names

bytesofman marked an inline comment as done.Thu, Jan 2, 16:43
Harbormaster completed remote builds in B31892: Diff 51927.Thu, Jan 2, 16:47
Fabien accepted this revision.Thu, Jan 2, 17:41
This revision is now accepted and ready to land.Thu, Jan 2, 17:41
Closed by commit rABCa8765b3de5d5: [Cashtab] Implement exhaustive parsing of tx notifications (authored by bytesofman). · Explain WhyThu, Jan 2, 17:48
This revision was automatically updated to reflect the committed changes.
bytesofman added a commit: rABCa8765b3de5d5: [Cashtab] Implement exhaustive parsing of tx notifications.

Revision Contents
Changeset List

PathSize
cashtab/
extension/
public/
2 lines
4 lines
2 lines
src/
chronik/
__tests__/
26 lines
fixtures/
443 lines
264 lines
wallet/
83 lines

Diff 51930

View Options

cashtab/extension/public/manifest.json

Loading...
View Options

cashtab/package-lock.json

Loading...
View Options

cashtab/package.json

Loading...
View Options

cashtab/src/chronik/__tests__/index.test.js

Loading...
View Options

cashtab/src/chronik/fixtures/vectors.js

Loading...
View Options

cashtab/src/chronik/index.ts

Loading...
View Options

cashtab/src/wallet/useWallet.ts

Loading...