⚙ D13782 [herald] Parse XEC send txs

bytesofman created this revision.Apr 25 2023, 23:02

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 25 2023, 23:02

bytesofman requested review of this revision.Apr 25 2023, 23:02

Harbormaster completed remote builds in B23463: Diff 39948.Apr 25 2023, 23:03

There is no test for the new parsing feature

apps/ecash-herald/src/parse.js
57	no need to declare it there, it's only used once next to the return statement. In fact you certainly don't need a var at all
79	You don't need an iterator index here: `for (var thisInput in inputs) {` (or use `Array.foreach()`)
81	You need some container type that ensures unicity
86	dito
88–90	You can parse only once (not sure about the syntax here, but you get the idea)
94	misplaced comment ?
95	also what happens if my transaction burns some XEC in an OP_RETURN tx?
100	Same here, you don't want duplicates. (example: https://explorer.e.cash/tx/101dd159b996d67c8ddca301f9fd2c0e1bf96ecf21a1ba5d2da3f9760f8d9bab)
117	That's just plain wrong. you will display wrong data this is a self send tx with >=2 outputs
322	why ?
325	You'd better check there is a single sender, no ?

This revision now requires changes to proceed.Apr 26 2023, 13:05

responding to feedback

apps/ecash-herald/src/parse.js
79	handled here. out of habit, i always write it with this syntax. going forward will need to get this as a rule in the linter to clean up other JS apps.
86	Yes, this suboptimal syntax is used throughout all of my JS apps. I've added a task to clear this up with a linter. But I think fixing this specific change is not related to the diff. T3146
95	good point, TIL This is how I check for OP_RETURN outputs, but the real way to check should be the outputScript? i.e. starting with `6a`? Is there a better way?
117	Modified this approach. We lose some information with the unicity of the input / output arrays, in that we can't say something like "this tx consolidated 18 utxos into 3" However it is probably still the right approach. I don't think we need utxo consolidation summaries if self-send txs are already flagged as such. Users who want to know more can investigate these txs.
322	It's an arbitrary judgment call. There are some editorial decisions involved in how much info goes into the msg, what kind of stuff helps or harms readability. Ultimately, the amounts will be rendered in USD, as I think this is more useful. At this stage, I think XEC decimal places just aren't that interesting in the Telegram msg, since, for now anyway, 0.53 XEC isn't that much economic activity.

Harbormaster completed remote builds in B23493: Diff 39988.Apr 26 2023, 16:28

Fabien requested changes to this revision.Apr 27 2023, 13:05

Fabien added inline comments.

apps/ecash-herald/src/parse.js
80 ↗	(On Diff #39988)	Use a `Set` instead
90 ↗	(On Diff #39988)	This one is not useful, it's used only once.
97 ↗	(On Diff #39988)	Dito, use a Map script => amount this time to avoid the quadratic behavior...
112 ↗	(On Diff #39988)	Then the whole block becomes: xecChangeOutputs.set(outputScript, (xecChangeOutputs.get(outputScript) ?? 0) + value);
142 ↗	(On Diff #39988)	Similar here
151 ↗	(On Diff #39988)	Is that intermediate var needed at all ?
79		OK
95		I don't think you can do better than checking for OP_RETURN as the first opcode
322		OK, but in this case keep the total count to the right value (including decimals) and strip at display time instead, so it's clearly an "editorial" decision and you don't carry a wrong value.

This revision now requires changes to proceed.Apr 27 2023, 13:05

bytesofman added inline comments.Apr 27 2023, 14:30

apps/ecash-herald/src/parse.js
80 ↗	(On Diff #39988)	Before I do this refactor -- was thinking there is some value in keeping repeated values. This carries information of how many inputs and outputs are in the tx, so we could in the future create messages like "This tx consolidated 73 utxos into 3 utxos", something like this Would this be an acceptable approach? Is it a waste of memory or some other reason we need no duplicate entries?

Fabien added inline comments.Apr 27 2023, 17:11

apps/ecash-herald/src/parse.js
80 ↗	(On Diff #39988)	Then you can map a counter of occurrence to the address. Because counting will require you to loop over your array to do the summing and that's very inefficient.

Using maps and sets

The JSON.stringify(blocksMock, null, 2) does not preserve Map or Set types and requires modification

Harbormaster completed remote builds in B23534: Diff 40050.Apr 28 2023, 00:20

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

Tail of the build log:

/work/apps/ecash-herald /work/abc-ci-builds/ecash-herald-tests
npm WARN deprecated request-promise@4.2.6: request-promise has been deprecated because it extends the now deprecated request package, see https://github.com/request/request/issues/3142
npm WARN deprecated uuid@3.4.0: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
npm WARN deprecated har-validator@5.1.5: this library is no longer supported
npm WARN deprecated request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142

added 435 packages, and audited 436 packages in 3s

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

4 moderate severity vulnerabilities

Some issues need review, and may require choosing
a different dependency.

Run `npm audit` for details.

> ecash-herald@1.0.0 test
> mocha --reporter mocha-junit-reporter --reporter-options mochaFile=test_results/ecash-herald-junit.xml --reporter-options testsuitesTitle=Ecash Herald Unit Tests --reporter-options rootSuiteTitle=Ecash Herald

----------------------|---------|----------|---------|---------|-------------------
File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------------------|---------|----------|---------|---------|-------------------
All files             |   96.13 |    85.32 |   93.75 |      96 |                   
 ecash-herald         |     100 |      100 |     100 |     100 |                   
  config.js           |     100 |      100 |     100 |     100 |                   
 ecash-herald/src     |   96.12 |    85.32 |   93.75 |   95.98 |                   
  chronikWsHandler.js |   93.75 |      100 |   66.66 |   93.75 | 19                
  events.js           |    82.6 |       80 |     100 |    82.6 | 29,50-54,71       
  main.js             |   81.81 |       80 |     100 |   81.81 | 43-49             
  parse.js            |   98.12 |     82.5 |     100 |   98.03 | 152,193-196       
  telegram.js         |     100 |      100 |     100 |     100 |                   
  utils.js            |     100 |      100 |     100 |     100 |                   
----------------------|---------|----------|---------|---------|-------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='249']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='259']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='93']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='109']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='15']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='16']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='240']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='250']
##teamcity[blockClosed name='Code Coverage Summary']
Build ecash-herald-tests failed with exit code 1

Adding helper functions to support JSON saving of Set and Map types

Harbormaster completed remote builds in B23535: Diff 40051.Apr 28 2023, 04:22

Update mocks. Looked at dropping unused key param from replace, revive functions, can't do it

bytesofman added inline comments.Apr 28 2023, 04:57

apps/ecash-herald/src/utils.js
103 ↗	(On Diff #40057)	`key` is not used here. However, the function passed to `JSON.stringify` as a replacer needs `key` and `value` params to work
118 ↗	(On Diff #40057)	ditto

Harbormaster completed remote builds in B23541: Diff 40057.Apr 28 2023, 04:57

Fabien requested changes to this revision.Apr 28 2023, 11:47

Fabien added inline comments.

apps/ecash-herald/src/parse.js
77 ↗	(On Diff #40057)	How does that work at all ???
98 ↗	(On Diff #40057)	My solution had 1 less lookup :) : xecChangeOutputs.set(thisOutput.outputScript, (xecChangeOutputs.get(thisOutput.outputScript) ?? 0) + value);
99 ↗	(On Diff #40057)	I don't think you need the if branch. An OP_RETURN tx cannot be send to self
101 ↗	(On Diff #40057)	Out of scope: why is that a config ? I doubt you can configure you app to work properly with another opcode
113 ↗	(On Diff #40057)	see suggestion above

This revision now requires changes to proceed.Apr 28 2023, 11:47

remove extra lookup in maps

apps/ecash-herald/src/parse.js
77 ↗	(On Diff #40057)	In this case, `i`, is an index, `0, 1, 2, 3...` etc. It's a javascript syntax thing.
98 ↗	(On Diff #40057)	Updated. There is no `??` operator in JS but I think this change does the same thing.
99 ↗	(On Diff #40057)	issue is that i don't want any outputScripts in `xecReceivingOutputs` that will cause `cashaddr.encodeOutputScript` to throw a validation error
101 ↗	(On Diff #40057)	It's not really a config so much as a constant. It's stored in `config` out of convenience, to avoid magic number. Best path fwd here is I think to make a `constants.js` file. This though I do think is out of scope for this diff. `6a` was already in `config.js` before this diff.

Harbormaster completed remote builds in B23550: Diff 40069.Apr 28 2023, 13:02

Improving syntax with ??

Harbormaster completed remote builds in B23551: Diff 40070.Apr 28 2023, 13:15

Fabien requested changes to this revision.Apr 28 2023, 13:33

Fabien added inline comments.

apps/ecash-herald/src/parse.js
99 ↗	(On Diff #40057)	I see. But cashaddr.encodeOutputScript isn't call in this function, right ? So this check is misplaced, you should run it where the conversion happens. This makes the logic easier to follow also: if there is no associated address, don't display an address.
101 ↗	(On Diff #40057)	Fair enough, though creating a constants.js might be overkill (and I generally don't like kitchen sink files like this) for a single constant. But maybe there are more to be added.