Page MenuHomePhabricator

[herald] parse etoken send txs

Authored by bytesofman on May 3 2023, 18:21.


Group Reviewers
Restricted Project
rABCd4473c383031: [herald] parse etoken send txs


Parse etoken send txs

Test Plan

npm test

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

need to update mockedChronik for unit tests that call handleBlockConnected

Tail of the build log:

<a href="">no app:</a> 1629498127|85
<a href="">no app:</a> 1629500089|91
<a href="">no app:</a> 1629497915|79
<a href="">no app:</a> 1629500647|79
<a href="">no app:</a> 1629499023|76
<a href="">no app:</a> 1629497534|78
<a href="">no app:</a> 1629498535|87
<a href="">no app:</a> 1629500798|79
<a href="">no app:</a> 1629497457|77
<a href="">no app:</a> 1629498288|72
<a href="">no app:</a> 1629499274|64
<a href="">no app:</a> 1629500162|80
<a href="">no app:</a> 1629500720|82
<a href="">no app:</a> 1629499774|94
<a href="">no app:</a> 1629497610|79
<a href="">no app:</a> 1629499360|84
<a href="">no app:</a> 1629498460|71
<a href="">no app:</a> 1629500318|76
<a href="">no app:</a> 1629497132|78
<a href="">no app:</a> 1629498060|88
<a href="">no app:</a> 1629499897|105
<a href="">no app:</a> 1629497763|75
<a href="">no app:</a> 1629499571|93
<a href="">no app:</a> 1629497054|74
<a href="">no app:</a> 1629499185|75
<a href="">no app:</a> 1629498375|70
<a href="">no app:</a> 1629498610|74
<a href="">no app:</a> 1629497293|68
<a href="">no app:</a> 1629497209|76) for msg 1 of 4 Error: Error: message failed to send
    at MockTelegramBot.self.sendMessage (/work/apps/ecash-herald/test/mocks/telegramBotMock.js:22:23)
    at sendBlockSummary (/work/apps/ecash-herald/src/telegram.js:47:540)
    at handleBlockConnected (/work/apps/ecash-herald/src/events.js:22:38)
    at async Context.<anonymous> (/work/apps/ecash-herald/test/eventsTests.js:189:28)
File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                  
All files             |   88.59 |     75.6 |   96.29 |   88.21 |                                    
 ecash-herald         |     100 |      100 |     100 |     100 |                                    
  config.js           |     100 |      100 |     100 |     100 |                                    
 ecash-herald/src     |   88.56 |     75.6 |   96.29 |   88.18 |                                    
  chronik.js          |     100 |      100 |     100 |     100 |                                    
  chronikWsHandler.js |   93.75 |      100 |   66.66 |   93.75 | 19                                 
  events.js           |   83.33 |       80 |     100 |   83.33 | 31,52-56,84                        
  main.js             |   81.81 |       80 |     100 |   81.81 | 43-49                              
  parse.js            |   84.03 |    69.56 |     100 |   83.41 | ...278,416-505,525,532-538,618-629 
  telegram.js         |      95 |     62.5 |     100 |   94.73 | 128-133                            
  utils.js            |     100 |      100 |     100 |     100 |                                    

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='334']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='377']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='124']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='164']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='26']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='27']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='322']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='365']
##teamcity[blockClosed name='Code Coverage Summary']
Build ecash-herald-tests failed with exit code 7
67 ↗(On Diff #40179)

bugfix uncovered with longer mocks

this would be nice to fix on its own, but it changes all the mocks, which require special preparation. The mocks need to be updated for this feature anyway.

add reviver/replacer methods for BigNumber, add required mocks for unit tests

91 ↗(On Diff #40184)

I don't think it's worth saving mocks of "coingecko fails but chronik doesn't" and "chronik fails but coingecko doesn't"

If we test "everything works --> good" and "nothing works --> stuff we expect to not be there is not there" ... imo good enough for a telegram bot

opportunity to refactor and make this type of unit testing more straightforward by incrementally building the telegram msg, e.g. "getBlockSummaryPriceLines", "getBlockSummaryTokenSendLines" ... but imo that makes everything way more complicated, esp when you need to deal with splitting overflow msgs

25 ↗(On Diff #40184)

no previous unit tests used this call

Fabien requested changes to this revision.May 5 2023, 11:13
Fabien added a subscriber: Fabien.
Fabien added inline comments.
73 ↗(On Diff #40185)

Move the comment on its own line above

126 ↗(On Diff #40185)

Does that make sense to account for the XEC tx input when this is already accounted for in the token parsing section ?

66 ↗(On Diff #40185)

I don't understand this change. It also seems unrelated to the diff?

146 ↗(On Diff #40185)

just use Map.size here

157 ↗(On Diff #40185)

Map has a foreach() method that can make the code a bit easier to read

180 ↗(On Diff #40185)

Why isn't this method in chronik.js ?

44 ↗(On Diff #40185)

I think it's the test for the bug fix above ? It should be it's own diff. Also it's unclear how it differs from the previous case

92 ↗(On Diff #40185)

Your assert below will fail already, this log is certainly a debug leftover

171 ↗(On Diff #40185)

Same, should be its own diff

199 ↗(On Diff #40185)

Only this part is relevant to this diff, and even so it could be split if necessary

This revision now requires changes to proceed.May 5 2023, 11:13
bytesofman marked 2 inline comments as done.

responding to feedback

126 ↗(On Diff #40185)

tokenSendingOutputScripts may not contain all of the sendingOutputScripts of xecSendingOutputScripts

We could make xecSendingOutputScripts a map and add a "sendsToken" param to it if it also sends a token, but imo this is harder to work with and more confusing than just having a new Set of only tokenSendingOutputScripts

66 ↗(On Diff #40185)


Unrelated to the diff except that the bug was discovered by the changes to the msgs in this diff

146 ↗(On Diff #40185)

In this function value.value is still not a map, it's an Array stored as part of an object with a key dataType === 'Map'

So, map.size is not available

157 ↗(On Diff #40185)

In this function, we are "reviving" a map that was stored as a special object in JSON. It's still an array.

180 ↗(On Diff #40185)

hm. I put it there the first time I made this feature. Since, I've started putting these "promise returner" methods in utils. It's not really doing any logic with chronik, it's just providing a promise.

Some of these promise-returning functions were only used in unit tests, so I considered them a util

it's a bit ambiguous either way.

44 ↗(On Diff #40185)

This test was added for the specific message that was causing the unfixed function to error out. This msg is only available in this diff, since it is the token send information that created a message with the right combination of line lengths to throw the bug.

92 ↗(On Diff #40185)

lol indeed

171 ↗(On Diff #40185)

will just get rid of this test. I added it when I hadn't figured out that BigNumber was causing the problem. the test proved that this wasn't the issue.

199 ↗(On Diff #40185)

Could be split, but the functionality is directly required by the specific data types introduced in this diff (Maps that include BigNumber types in the values). It's a pretty customized mod. We might need more similar mods in the future that would also be related to specific data types introduced at some stage of this app.

I think less confusing to any future diff peruser if it's kept here.

Fabien requested changes to this revision.May 5 2023, 17:19
Fabien added inline comments.
92 ↗(On Diff #40214)

I see, you need this log because you don't use the built-in feature of the assert module.
You should rewrite your assert below instead:

    blockSummaryTgMsgs[j].length <= TG_MSG_MAX_LENGTH,
    'Message is too long: ${blockSummaryTgMsgs[j].length} > ${TG_MSG_MAX_LENGTH}',
This revision now requires changes to proceed.May 5 2023, 17:19

logging within assert for telegram test

This revision is now accepted and ready to land.May 5 2023, 18:10
This revision was automatically updated to reflect the committed changes.