T2972
Parse XEC send txs and include a summary of them in tg msgs
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABCac3f0a95ef44: [herald] Parse XEC send txs
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- herald-parse-xec-sends
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 23551 Build 46720: Build Diff ecash-herald-tests Build 46719: arc lint + arc unit
Event Timeline
Comment Actions
There is no test for the new parsing feature
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 57 ↗ | (On Diff #39948) | 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 ↗ | (On Diff #39948) | You don't need an iterator index here: for (var thisInput in inputs) { (or use Array.foreach()) |
| 81 ↗ | (On Diff #39948) | You need some container type that ensures unicity |
| 86 ↗ | (On Diff #39948) | dito |
| 88–90 ↗ | (On Diff #39948) | You can parse only once (not sure about the syntax here, but you get the idea) |
| 94 ↗ | (On Diff #39948) | misplaced comment ? |
| 95 ↗ | (On Diff #39948) | also what happens if my transaction burns some XEC in an OP_RETURN tx? |
| 100 ↗ | (On Diff #39948) | Same here, you don't want duplicates. |
| 117 ↗ | (On Diff #39948) | That's just plain wrong. you will display wrong data this is a self send tx with >=2 outputs |
| 322 ↗ | (On Diff #39948) | why ? |
| 325 ↗ | (On Diff #39948) | You'd better check there is a single sender, no ? |
Comment Actions
responding to feedback
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 79 ↗ | (On Diff #39948) | 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 ↗ | (On Diff #39948) | 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 ↗ | (On Diff #39948) | 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 ↗ | (On Diff #39948) | 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 ↗ | (On Diff #39948) | 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. |
| 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 ↗ | (On Diff #39948) | OK |
| 95 ↗ | (On Diff #39948) | I don't think you can do better than checking for OP_RETURN as the first opcode |
| 322 ↗ | (On Diff #39948) | 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. |
| 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. |
Comment Actions
The JSON.stringify(blocksMock, null, 2) does not preserve Map or Set types and requires modification
Comment Actions
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
Comment Actions
Update mocks. Looked at dropping unused key param from replace, revive functions, can't do it
| 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 |
Comment Actions
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. |
| 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. |