Page MenuHomePhabricator

[Cashtab] Parse eCash Chat prefixed messages
ClosedPublic

Authored by emack on Mar 30 2024, 10:11.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABCa1c959a43f10: [Cashtab] Parse eCash Chat prefixed messages
Summary

Updating Cashtab to recognize eCash Chat messages and XEC tipping transactions.

Test Plan

npm test
npm start
Visually check inbound Cashtab, ElectrumABC and eCash chat messages (use dev link from email or ping me for a wallet with these txs)

image.png (884×445 px, 68 KB)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ecashChatPrefix
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 28209
Build 55963: Build Diffcashtab-tests
Build 55962: arc lint + arc unit

Event Timeline

emack requested review of this revision.Mar 30 2024, 10:11
bytesofman added inline comments.
cashtab/src/chronik/fixtures/mocks.js
7282 ↗(On Diff #46622)

I hate to hold this up based on existing tech debt in our tx parsing function, but we really can't extend the bad API of "we have the opreturn message and also a flag for every parsed type"

This wasn't so bad when a tx could only be a cashtab msg or not. But it's pretty bad when we support a few protocols. We want to be able to support dozens or hundreds of protocols here.

Some possible approaches

  • Return the lokad ID in parsed (need to return some kind of constant if we do not have one) -- then in tx.js, conditionally render a label based on the lokad ID
  • stop returning opReturnMessage and instead just return opReturn as the raw hex. Add conditional rendering to parse this in Tx.js

parseTx needs a lot of refactoring now that we are using in-node chronik and will support many more known tx types. Since we have good tests for conditional rendering now, it is prob best to the absolute minimum parsing required in parseTx and then manage the data we have in Tx.js

...actually it's kind of a bigger job and this doesn't really make the tech debt worse than it is now. so, no blocker. but fyi I will be rejigging this soon.

cashtab/src/components/Home/Tx.js
837 ↗(On Diff #46622)

this is also not a great API. we should have a switch to handle all the various things that can now be rendered.

but, this diff isn't really increasing tech debt so much as reminding me that it needs to be fixed.

This revision is now accepted and ready to land.Mar 30 2024, 14:19

version bump and rebase

This revision was automatically updated to reflect the committed changes.