Page MenuHomePhabricator

[herald] Parse XEC send txs
ClosedPublic

Authored by bytesofman on Apr 25 2023, 23:02.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCac3f0a95ef44: [herald] Parse XEC send txs
Summary

T2972

Parse XEC send txs and include a summary of them in tg msgs

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
herald-parse-xec-sends
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23463
Build 46545: Build Diffecash-herald-tests
Build 46544: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Apr 26 2023, 13:05
Fabien added a subscriber: Fabien.

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
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
bytesofman marked 4 inline comments as done.

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.

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
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?

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.

bytesofman marked 5 inline comments as done.

Using maps and sets

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

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

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

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

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
bytesofman marked an inline comment as done.

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.

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.

This revision now requires changes to proceed.Apr 28 2023, 13:33

Remove OP_RETURN outputs before rendering addresses

This revision is now accepted and ready to land.Apr 28 2023, 15:37
This revision was automatically updated to reflect the committed changes.