T2972
Parse etoken send txs
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABCd4473c383031: [herald] parse etoken send txs
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- herald-parse-etoken-txs
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 23626 Build 46866: Build Diff ecash-herald-tests Build 46865: arc lint + arc unit
Event Timeline
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (ecash-herald-tests) failed.
Tail of the build log:
<a href="https://explorer.e.cash/tx/0473d97d997b61c5018205b27316b6ae660a9b7835a46166fa87e0b1b26de2dd">no app:</a> 1629498127|85
<a href="https://explorer.e.cash/tx/0d0a722a21aeca90ebb3d0954475ccb67f18c02945bc138c1f2ae6d507e3feb7">no app:</a> 1629500089|91
<a href="https://explorer.e.cash/tx/0d9a82afc6b2605b25f8dab8b398579c3d408dc4c25919f6827a1afa5a0f6e5a">no app:</a> 1629497915|79
<a href="https://explorer.e.cash/tx/1205ec2b6105716eccb95f5b26c5d65d81a390ac8bacc6ee1f20aa1757015143">no app:</a> 1629500647|79
<a href="https://explorer.e.cash/tx/134b0feae8567aa52d73975746376b785564cbc907f8ce7dfc44f90edd869145">no app:</a> 1629499023|76
<a href="https://explorer.e.cash/tx/136742fdb231e1342f790a5123f46414c3957f7d199b80ea729ecba274e3b787">no app:</a> 1629497534|78
<a href="https://explorer.e.cash/tx/1478f35e98cff2227a826bc93463d2813b5161929267806d49ec994088747bfa">no app:</a> 1629498535|87
<a href="https://explorer.e.cash/tx/2061d46821889fe8767c6fb747b87e37e3961eab46e8a7dc9098719d170fca52">no app:</a> 1629500798|79
<a href="https://explorer.e.cash/tx/26df82bc6624d8814fe23073ba1b1b8b1ddff68de955ba01fd8dbb5e2db34eb6">no app:</a> 1629497457|77
<a href="https://explorer.e.cash/tx/28bfff0be82734dbfa346cda5d45fb8deeaacce6edc817bd9d6f2c6c82c203ea">no app:</a> 1629498288|72
<a href="https://explorer.e.cash/tx/29e4bcf352a9524856099ae43fa25b2c67f661e0486875a35a3dc5e02466c4b5">no app:</a> 1629499274|64
<a href="https://explorer.e.cash/tx/2fddd13d532ec44c43ee4fa68b587f15d575e73d566e7d30f6bc495a61074e42">no app:</a> 1629500162|80
<a href="https://explorer.e.cash/tx/30cfe0f7b05197b371e050eb06642e969d037754f456f76272e98890b8ed2581">no app:</a> 1629500720|82
<a href="https://explorer.e.cash/tx/32f7ca6768bedb81603dfd5618263f84c7cb42fa4bae4eeb2dda8a4eac0cdd4d">no app:</a> 1629499774|94
<a href="https://explorer.e.cash/tx/3411daaf624965c7731bc169e7831d9e56075986a1639cb1dc74e1b8d9c797b9">no app:</a> 1629497610|79
<a href="https://explorer.e.cash/tx/4cf484655aa1948cfc3cd291a119806c8b2b5e0d233e44866dc0c9015b24ce1e">no app:</a> 1629499360|84
<a href="https://explorer.e.cash/tx/4d46bd9ba22889a496cf4d37e5f0307216c8be93885ba82fcc0d3965c63693c3">no app:</a> 1629498460|71
<a href="https://explorer.e.cash/tx/4f55182147356e5ccbf6c06225e817ac405a50fbe04c0f6eb5a4eb04462c7b12">no app:</a> 1629500318|76
<a href="https://explorer.e.cash/tx/53c43d805bbbb9618e48cde71f5ff659fea02689f825cde823984b30443f0b30">no app:</a> 1629497132|78
<a href="https://explorer.e.cash/tx/56bc3c81bb81bc92ba25acc407602207a0fdada4261f7f205d141ab34b616ce9">no app:</a> 1629498060|88
<a href="https://explorer.e.cash/tx/592f4435d3ef8e2e2f0108cffc7b727798f359bad8521a084ca668bad55512c3">no app:</a> 1629499897|105
<a href="https://explorer.e.cash/tx/5d4f5668277ac87f170711461f0bef8f716556b6433c39729a4d0f22a1f1a9ae">no app:</a> 1629497763|75
<a href="https://explorer.e.cash/tx/63ee98065e0c2358423ccc2ceae21a00ff8ed5e132d460a463334f1368ae3936">no app:</a> 1629499571|93
<a href="https://explorer.e.cash/tx/64d204d6dd894e2b93ec2a9a518fb6c9fb9313098a06859b605e440884372c60">no app:</a> 1629497054|74
<a href="https://explorer.e.cash/tx/67b05c5f3cc1d1d2415aae8232254bc790fe8d1965e9b529fc3b7bae4acf818d">no app:</a> 1629499185|75
<a href="https://explorer.e.cash/tx/6fb44256ab3b7ecdb4dd4955d94dd1f6dc1bdeee8a523651fd71e699c524af01">no app:</a> 1629498375|70
<a href="https://explorer.e.cash/tx/707051559904c61d0873824b9a215b93c90452724be49342554438215ba392d0">no app:</a> 1629498610|74
<a href="https://explorer.e.cash/tx/7168c1feb93bba72b68c5ac833a9f428dcb88a9e199f53db1613bcc07a70dfec">no app:</a> 1629497293|68
<a href="https://explorer.e.cash/tx/7d85c406e5a0cd75fb92388f8d875e3e7eded9584d01414f18f57793063b1e69">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| apps/ecash-herald/src/telegram.js | ||
|---|---|---|
| 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. |
| apps/ecash-herald/src/events.js | ||
|---|---|---|
| 91 | 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 | |
| apps/ecash-herald/test/mocks/chronikMock.js | ||
| 25 | no previous unit tests used this call | |
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 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 ? |
| apps/ecash-herald/src/telegram.js | ||
| 66 ↗ | (On Diff #40185) | I don't understand this change. It also seems unrelated to the diff? |
| apps/ecash-herald/src/utils.js | ||
| 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 ? |
| apps/ecash-herald/test/telegramTests.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 |
| apps/ecash-herald/test/utilsTests.js | ||
| 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 |
Comment Actions
responding to feedback
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 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 |
| apps/ecash-herald/src/telegram.js | ||
| 66 ↗ | (On Diff #40185) | Unrelated to the diff except that the bug was discovered by the changes to the msgs in this diff |
| apps/ecash-herald/src/utils.js | ||
| 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. |
| apps/ecash-herald/test/telegramTests.js | ||
| 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 |
| apps/ecash-herald/test/utilsTests.js | ||
| 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. |
| apps/ecash-herald/test/telegramTests.js | ||
|---|---|---|
| 92 ↗ | (On Diff #40214) | I see, you need this log because you don't use the built-in feature of the assert module. assert.strictEqual(
blockSummaryTgMsgs[j].length <= TG_MSG_MAX_LENGTH,
true,
'Message is too long: ${blockSummaryTgMsgs[j].length} > ${TG_MSG_MAX_LENGTH}',
); |