Update the herald to recognize the authentication txs.
Details
Details
- Reviewers
bytesofman Fabien - Group Reviewers
Restricted Project - Commits
- rABC0ce298120039: [ecash-herald] Update parseOpReturn to recognize authentication txs
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- parseAuthTxs
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 29938 Build 59411: Build Diff ecash-herald-tests Build 59410: arc lint + arc unit
Event Timeline
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 672 |
This is a blockchain and public information already, what is your privacy concern here ? | |
| 672 | At least indicate what it's about, because Authentication tx is something only you can understand. | |
| 675 | See comment in D16623 | |
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 671 ↗ | (On Diff #49150) | This is still not helpful, how is the reader suppose to guess what protocol this is ? |
Comment Actions
Updated rendered messages
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 673 ↗ | (On Diff #49150) | Also just realized the comments on the previous revision didn't get posted. The privacy risk is more about the herald basically telling everyone in real time when certain addresses log in on their mobile devices (i.e. they're online), which is strictly an opt-in permission across most social platforms. People can always monitor addresses themselves but this automates it in real time if you start showing addresses with this parsed action. I also wanted to keep this app neutral hence the reference to the op return guideline but fair point on the user not seeing this context, so I've updated the msg. |
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 673 ↗ | (On Diff #49204) | If you don't do anything with that info it has little benefit. Either provide a count like CACHET does (my favorite solution) or link to the tx or something. |
| apps/ecash-herald/test/mocks/appTxSamples.js | ||
| 598 ↗ | (On Diff #49204) | You're missing the third case |
Comment Actions
Updated with on-spec but empty identifier condition
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 673 ↗ | (On Diff #49204) | This parseOpReturn function only takes in the opreturn hex. Unlike CACHET token txs, the send quantity nor txid is not part of the opreturn payload, hence not possible to render a count (it's a dust tx so it's always 5.5 tx) nor an explorer link in this function. Is it worth hardcoding the 5.5 tx dust amount to this message? Otherwise the current message is still better than seeing random hex. |
Comment Actions
Let's update the message with better usability in another diff, that's already an improvement