Page MenuHomePhabricator
Differential D13523

[alias-server] Check if chaintip is finalized by avalanche before adding new aliases
AbandonedPublic
Actions

Authored by bytesofman on Mar 31 2023, 16:06.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Summary

T3060

Depends on D13522

Get the blockheight and hash of the chaintip. Then find out if it is confirmed by avalanche.

If it is not, wait 5s then try again.

If it still is not, exit the loop. The app will get a new chaintip and check again the next time a block is found.

Note: A subsequent diff will use this blockheight to confirm that only avalanche confirmed txs are added to valid alias registrations.

Test Plan

npm test
node index.js, most likely you will see Chaintip ${tipHeight}:${tipHash} is finalized by avalanche. in the log
Edit src/websocket.js by adding the line isAvalancheFinalized = false at line 73
node index.js, confirm the app waits 5s then checks again and gets confirmation of avalanche finalization

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bytesofman added a child revision: D13524: [alias-server] Only include avalanche confirmed txs in getValidAliasRegistrations function.Mar 31 2023, 16:44
Fabien requested changes to this revision.Mar 31 2023, 17:38
Fabien added a subscriber: Fabien.
Fabien added inline comments.
web/alias-server/src/websocket.js
53 ↗(On Diff #39108)

you already have the block hash in wsMsg.blockHash, why do you need this ?

79 ↗(On Diff #39108)

I get the idea but you should wait a be cautious with that:

  1. How do you deal with reentrency ?
  2. This is unecessary and slow during IBD
  3. You want to make sure avalanche is enabled or this will never work
  4. In real world you want to retry this a few times, 5s is on the edge for finalization. It usually takes ~3s on my machines so you don't want this to randomly fail because your timeout is too short
This revision now requires changes to proceed.Mar 31 2023, 17:38
bytesofman added inline comments.Mar 31 2023, 18:50
web/alias-server/src/websocket.js
53 ↗(On Diff #39108)

This function is called on app startup. In this case, we don't have wsMsg.blockHash.

Even if we do have the blockhash though, we do not have the blockheight. What we need to parse valid aliases is the blockheight of the most latest avalanche finalized block.

79 ↗(On Diff #39108)
  1. How do you deal with reentrency ?

Hm. Reviewing, this looks like a good option: https://www.npmjs.com/package/async-lock

Of course...it is yet another dependency.

The test app that is currently running (the abandoned previous chain) deals with this through database mgmt. The validAliases database is write only, with unique txid keys. There is also a stored serverState that tracks what the db represents, i.e. you have some validAliases in the database and you also have a processedConfirmedBlockheight that corresponds to those aliases. If two blocks are found quickly and the loop is entered twice, you could get some kind of a database error on one of the loops. But the database is keyed so it cannot accept multiple valid txs with the same txid. Also, each loop starts based on what is in the database. So you can only iteratively add.

  1. This is unecessary and slow during IBD

Yes. In this case, however, the server is almost never doing IBD. The odds of running npm start with an empty database and hitting a just-found block that isn't avalanche confirmed are quite small. Even if it does happen, meh, wait 5s one time. Not the end of the world.

This indexer doesn't go one block at a time. It's using onBlock as a trigger for checking the transaction history of the address. When it starts up, one loop is enough to get all of the valid aliases.

  1. You want to make sure avalanche is enabled or this will never work

Yes, can add this chekc.

  1. In real world you want to retry this a few times, 5s is on the edge for finalization. It usually takes ~3s on my machines so you don't want this to randomly fail because your timeout is too short

OK. I'll think about this. Mb best to start the loop 10s after a block is found every time. There is not imo a high demand for "incredibly urgent alias validation." Best solution here would be chronik websocket gives msgs when blocks are avalanche confirmed, not found. I've pinged Tobias about this. But I think it can be handled with current tech for now.

bytesofman planned changes to this revision.Mar 31 2023, 18:50
Fabien added inline comments.Mar 31 2023, 19:03
web/alias-server/src/websocket.js
79 ↗(On Diff #39108)
  1. Ok but what about the same txid but different block height ? That is what could happen when a block is orphaned
  2. Because chronik doesn't trigger the callbacks during IBD ?
  3. Yeah increasing the timeout a bit might be enough, at least good enough for a first iteration
bytesofman added inline comments.Mar 31 2023, 21:38
web/alias-server/src/websocket.js
79 ↗(On Diff #39108)
  1. Ok but what about the same txid but different block height ? That is what could happen when a block is orphaned

The assumption I'm making with this approach is that the loop is never entered unless we have a known blockheight that is finalized by avalanche. There is an implicit assumption that chronik's tx history call will not return tx history from a block with the same height as the finalized block (but is a different block).

...this assumption is (maybe) okay if we are using the blockhash from the chronik websocket, since that was the block received by chronik, and if the block received by chronik is finalized, then calling tx history on the same chronik server and only accepting txs through that blockheight should be okay.

Still, a lot of 'should.' Probably best to

  • use the blockhash chronik pushed from the websocket (unless you are running the app for the first time)
  • also call isfinaltransaction on every tx
  1. Because chronik doesn't trigger the callbacks during IBD ?

If the node itself is in IBD, I don't know what chronik would return for getting the chaintip. I don't think any of the chronik calls necessary for alias-server to work would function. The way the app is designed here now tho, it will only process txs through a block that is confirmed by avalanche.

For starting up the alias-server app on an empty database -- going through the 'on block' loop once will add all valid aliases through the highest known valid blockheight (alias-server does not iterate over every block in the blockchain, it is only checking the tx history of one address). So it will only wait 5 seconds one time in that process, and only if you happen to start up the app at the exact moment a block has been found but is not yet finalized.

  1. Yeah increasing the timeout a bit might be enough, at least good enough for a first iteration

I'll do this for now and see how it works over a few days

bytesofman added a comment.EditedMar 31 2023, 21:42

Feedback actions on this diff

  • Wait 10s after a block is found before starting the "block found" loop, goal being to make it likely that the block found is avalanche confirmed when you check
  • Use the blockhash pushed by the websocket unless you are running on app startup
  • Create task to implement isFinalTransaction to doublecheck transactions before adding them (I need to rebuild the incremental tx processing logic before doing this)
  • Solution for concurrency / re-entry to "block found" loop --> I've done some testing on this module, https://www.npmjs.com/package/async-lock, seems like it works well and I can add a script to test it on the loop we want. I've added this as an item in T3060. It needs to be implemented but imo is not critical to the logic of this diff
bytesofman updated this revision to Diff 39114.Mar 31 2023, 22:22

Improving loop logic and comments per review

Harbormaster completed remote builds in B22880: Diff 39114.Mar 31 2023, 22:23
bytesofman updated this revision to Diff 39115.Mar 31 2023, 22:32

Don't exit loop if undefined variable is undefined

Harbormaster completed remote builds in B22881: Diff 39115.Mar 31 2023, 22:33
bytesofman updated this revision to Diff 39116.Mar 31 2023, 23:35

Improve tip height validation

Harbormaster completed remote builds in B22882: Diff 39116.Mar 31 2023, 23:36
Fabien requested changes to this revision.Apr 1 2023, 08:14

unrelated notes: after a quick look at all the utils.js methods, it seems they all belong to another module and there should be no utils.js at all

web/alias-server/src/websocket.js
33 ↗(On Diff #39116)

Apart from the fact that this is not a loop, this is a big monolithic block with no test.
That should be split into parts, each part having its unit tests.

There are several design decisions here that I either don't understand or are trivially flawed, see below.

This whole piece of code needs a good refactor.

38 ↗(On Diff #39116)

This is non sense, you have the switch just below.

123 ↗(On Diff #39116)

The sole purpose of this branch is to set tipHash and tipHeight, this could just be done in their respective switch case and you get rid of the startup special case entirely.

The only exception is the 10s delay which is simply not at the right place and should happen before the isfinal call.

142 ↗(On Diff #39116)

This makes no sense. You app can fail to load randomly depending on when the last block was mined.

190 ↗(On Diff #39116)

Are you really loading ALL the aliases from the db only to get the last processed block ?
So if there are 1M aliases you will load 1M entries to retrieve 4 bytes ?

This revision now requires changes to proceed.Apr 1 2023, 08:14
bytesofman added a comment.Apr 1 2023, 13:29

unrelated notes: after a quick look at all the utils.js methods, it seems they all belong to another module and there should be no utils.js at all

See comments -- let me know if you want me to put up a diff that wipes everything that's not being used at the moment.

Utils overview
Functions that will be scrapped:
getConfirmedTxsToBeAddedToDb, removeUnconfirmedTxsFromTxHistory - this will be totally removed, the app does not need to save all confirmed txs, this was an option explored and found dumb while buildling up the app
getValidAliasTxsToBeAddedToDb - this is comparing sorted arrays to make sure a db error is not thrown trying to add something that's already in the db. it should be deprecated with improvements to the db function.

Function that could be in a library
outputScriptToAddress - this could go in ecashaddrjs. it's not there now tho so need it here.

Helpful alias-server utils
generateReservedAliasTxArray - this is a useful helper method to take an array of strings of reserved aliases and format them the same way as valid alias txs are parsed
getAliasFromHex, getHexFromAlias, getAliasBytecount - these are useful helper methods for the app
isValidAliasString this is important in keeping to the spec

web/alias-server/src/websocket.js
33 ↗(On Diff #39116)

The current state of this repo is an arbitrary snapshot of the previous stack, which was used mostly as a way to save my work during a greenfield construction phase of this app

One option is to just delete all the stuff in the app and set it as a template that only does what we've covered so far in this stack. That could be easier to review?

Otherwise my plan was to follow the outline in T3060 to incrementally replace stuff.

38 ↗(On Diff #39116)

Not sure what you mean.

the switch statement relies on type and not bool variables. It's just a code readability helper to have isAppStartup later instead of always type === 'start'

123 ↗(On Diff #39116)

I'm not sure what you mean here.

startup case is different from block found case in that it does not start with a known blockhash. But other than that, its logic is all the same (needs to update the database to the most recent state of valid aliases).

so startup needs to be handled somewhere. It could be handled before this switch case starts, so that it is entered with a blockhash same as the websocket.

142 ↗(On Diff #39116)

can be handled by adding a helper function to get the most recently confirmed avalanche block (get the highest block, if no check next, if no check next, etc)

seems like that should be its own diff though, an improvement to the base case.

For an initially small change, this diff is already becoming quite large.

190 ↗(On Diff #39116)

Pretty much all of the logic that comes after this avalanche stuff is bad. It represents an arbitrary step from the earlier stack that is due to be totally reconstructed in this stack (and was totally reconstructed in the last abandoned stack).

I think we are getting ahead of ourselves in reviewing it now. Some ways to handle the situation:

  1. I could put up a diff that removes it all right now
  2. I can keep following the outline in T3060, incrementally replacing things.

Are you really loading ALL the aliases from the db only to get the last processed block ?

e.g. this step specifically will be totally thrown out. For one, it's wrong...the last processed block is not the highest blockheight of registered aliases; it's the highest blockheight of confirmed txs seen by the app.

However, we do need to know the alias history in order to know whether or not new txs are valid. So getting all the aliases is necessary to evaluate whether or not new aliases are valid (they might pay the right fee and do everything write but conflict with an earlier registration).

Fabien added a comment.Apr 1 2023, 15:44

OK from your comments I get a better sense of what's going one. The issue is that this code managed to be landed while being in a very bad shape and now we have to live with it.

I think you should start by cleaning up the existing: remove the dead code, discard the utils, make sure the names are accurate, add TODO comments to make it clear where rework is expected to happen, split the logic into pieces with unit tests etc.
I'm sure that would reveal a lot of simplifications and this diff will end up being much shorter and simpler to review. For now this diff is adding an important piece to an already flawed logic surrounded by code that you know needs to be reworked and still comes with no test: it's basically impossible to review properly.

web/alias-server/src/websocket.js
35–147 ↗(On Diff #39116)
38 ↗(On Diff #39116)

I mean there is no reason for testing type === 'startup' here when you can set the flag in the below switch case that does the same comparison

123 ↗(On Diff #39116)

See suggestion to get a sense of what I mean

156 ↗(On Diff #39116)

That's not very helpful as a log, you might as well not print it. Or at least gate it with an IBD flag to avoid bloating your logs

bytesofman updated this revision to Diff 39125.Apr 1 2023, 18:39

Removing isAppStartup param with better fallthrough handling

Harbormaster completed remote builds in B22888: Diff 39125.Apr 1 2023, 18:40
bytesofman added a comment.Apr 1 2023, 18:41
In D13523#300727, @Fabien wrote:

OK from your comments I get a better sense of what's going one. The issue is that this code managed to be landed while being in a very bad shape and now we have to live with it.

I think you should start by cleaning up the existing: remove the dead code, discard the utils, make sure the names are accurate, add TODO comments to make it clear where rework is expected to happen, split the logic into pieces with unit tests etc.
I'm sure that would reveal a lot of simplifications and this diff will end up being much shorter and simpler to review. For now this diff is adding an important piece to an already flawed logic surrounded by code that you know needs to be reworked and still comes with no test: it's basically impossible to review properly.

Okay. This stack I think is a good implementation of avalanche rpc and I would like to preserve it. Can this come after landing this stack?

imo easier to deal with a "dead code removal and unit tests pending" stack separately from this feature

web/alias-server/src/websocket.js
35–147 ↗(On Diff #39116)

oh i get it. ok pushed an update here.

Fabien requested changes to this revision.Apr 3 2023, 08:27

I'm OK with doing the cleanups after this diff, but not OK with adding code with no test. This should at least come with unit tests for the newly added feature, otherwise you have no confidence that this works, and no way to check you don't introduce any regression with the cleanups.

web/alias-server/src/websocket.js
41 ↗(On Diff #39125)

chaintipInfo only lives in the startup branch, and doesn't need to be declared at that scope

103 ↗(On Diff #39125)

this should be gated as well, if you already gave tipHeight you don't need all of this. Unless you don't want to set tipHeight in the startup case and always use that method ? This will be even slightly more robust because you have error handling if tipHeight is not a number, which is not the case with the startup case.

This revision now requires changes to proceed.Apr 3 2023, 08:27
bytesofman updated this revision to Diff 39157.Apr 3 2023, 13:42

rebase

Harbormaster completed remote builds in B22911: Diff 39157.Apr 3 2023, 13:49
bytesofman updated this revision to Diff 39163.Apr 3 2023, 14:33
bytesofman marked an inline comment as done.

variable definition in case, gating blockdetails chronik call for blockconnected case

bytesofman planned changes to this revision.Apr 3 2023, 14:34

Need unit tests for parseWebsocketMessage

web/alias-server/src/websocket.js
41 ↗(On Diff #39125)

This leads to a lint error. eslint does not want you to define variables in case loops

https://eslint.org/docs/latest/rules/no-case-declarations

The "solution" is to put brackets around the startup: case, but this would prevent desired fallthrough of tipHash and tipHeight to the BlockConnected case.

Added comment to prevent lint error.

103 ↗(On Diff #39125)

gated. the validation error will be thrown for both startup and blockconnected cases.

Harbormaster completed remote builds in B22917: Diff 39163.Apr 3 2023, 14:37
bytesofman updated this revision to Diff 39177.Apr 3 2023, 17:35

Adding unit tests for new logic of parseWebsocketMessage

bytesofman added a comment.Apr 3 2023, 17:36

Unit tests added.

The telegramBot param had to be moved to index.js and passed as a param to avoid parseWebsocketMessage refusing to exit the process after unit tests completed.

Harbormaster completed remote builds in B22923: Diff 39177.Apr 3 2023, 17:57
teamcity added a comment.Apr 3 2023, 17:57
Build Bitcoin ABC Diffs / Diff Testing (alias-server-tests) failed.

Tail of the build log:

    at Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Object.<anonymous> (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:64:4)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Module.require (node:internal/modules/cjs/loader:996:19)
    at require (node:internal/modules/cjs/helpers:92:18)
    at Object.<anonymous> (/work/web/alias-server/src/websocket.js:2:621)
    at Module._compile (node:internal/modules/cjs/loader:1092:14)
    at Module.replacementCompile (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:60:13)
    at Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Object.<anonymous> (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:64:4)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Module.require (node:internal/modules/cjs/loader:996:19)
    at require (node:internal/modules/cjs/helpers:92:18)
    at Object.<anonymous> (/work/web/alias-server/test/websocketTests.js:3:35)
    at Module._compile (node:internal/modules/cjs/loader:1092:14)
    at Module.replacementCompile (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:60:13)
    at Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Object.<anonymous> (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:64:4)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:201:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:154:23)
    at async Loader.import (node:internal/modules/esm/loader:177:24)
    at async importModuleDynamicallyWrapper (node:internal/vm/module:437:15)
    at async formattedImport (/work/web/alias-server/node_modules/mocha/lib/nodejs/esm-utils.js:9:14)
    at async Object.exports.requireOrImport (/work/web/alias-server/node_modules/mocha/lib/nodejs/esm-utils.js:42:28)
    at async Object.exports.loadFilesAsync (/work/web/alias-server/node_modules/mocha/lib/nodejs/esm-utils.js:100:20)
    at async singleRun (/work/web/alias-server/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at async Object.exports.handler (/work/web/alias-server/node_modules/mocha/lib/cli/run.js:370:5)
--------------------|---------|----------|---------|---------|-------------------
File                | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
--------------------|---------|----------|---------|---------|-------------------
All files           |     7.1 |        0 |       0 |    7.33 |                   
 alias-server       |     100 |      100 |     100 |     100 |                   
  config.js         |     100 |      100 |     100 |     100 |                   
  secrets.sample.js |     100 |      100 |     100 |     100 |                   
 alias-server/src   |    6.57 |        0 |       0 |    6.78 |                   
  alias.js          |    6.34 |        0 |       0 |    6.66 | 13-183            
  chronik.js        |    6.32 |        0 |       0 |    6.66 | 11-205            
  log.js            |      20 |        0 |       0 |      20 | 9-13              
  rpc.js            |   17.64 |        0 |       0 |   17.64 | 7-57              
  telegram.js       |   14.28 |      100 |       0 |   14.28 | 3-17              
  utils.js          |       6 |        0 |       0 |    6.25 | 12-134            
  websocket.js      |    4.65 |        0 |       0 |    4.72 | 13-443            
--------------------|---------|----------|---------|---------|-------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='25']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='352']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='0']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='117']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='0']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='34']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='25']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='341']
##teamcity[blockClosed name='Code Coverage Summary']
mv: cannot stat 'test_results/alias-server-junit.xml': No such file or directory
Build alias-server-tests failed with exit code 1
bytesofman added a comment.EditedApr 3 2023, 18:04

Build Bitcoin ABC Diffs / Diff Testing (alias-server-tests) failed.

Not sure what's going on here, something to do with the CI implementation. I'm not able to recreate it locally with npm test or npm run junit

update: may be related to unused websocketMocks file; removed

bytesofman updated this revision to Diff 39180.Apr 3 2023, 18:08

Removing unused websocketMocks file

web/alias-server/src/telegram.js
7 ↗(On Diff #39177)

This line was being called when websocket.js imported telegram.js

This would cause unit tests to keep polling with a live telegram bot and thus never end.

Refactor required to test functions that use the telegram bot without having a live telegram bot

web/alias-server/src/websocket.js
41 ↗(On Diff #39177)

I'm not able to mock chronik responses in JS. So, isUnitTest and unitTestMocks are used to manually pass mocks for unit test conditions (and return specific points of the loop logic)

Harbormaster completed remote builds in B22924: Diff 39180.Apr 3 2023, 18:15
teamcity added a comment.Apr 3 2023, 18:15
Build Bitcoin ABC Diffs / Diff Testing (alias-server-tests) failed.

Tail of the build log:

    at Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Object.<anonymous> (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:64:4)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Module.require (node:internal/modules/cjs/loader:996:19)
    at require (node:internal/modules/cjs/helpers:92:18)
    at Object.<anonymous> (/work/web/alias-server/src/websocket.js:2:621)
    at Module._compile (node:internal/modules/cjs/loader:1092:14)
    at Module.replacementCompile (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:60:13)
    at Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Object.<anonymous> (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:64:4)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Module.require (node:internal/modules/cjs/loader:996:19)
    at require (node:internal/modules/cjs/helpers:92:18)
    at Object.<anonymous> (/work/web/alias-server/test/websocketTests.js:3:35)
    at Module._compile (node:internal/modules/cjs/loader:1092:14)
    at Module.replacementCompile (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:60:13)
    at Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Object.<anonymous> (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:64:4)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:201:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:154:23)
    at async Loader.import (node:internal/modules/esm/loader:177:24)
    at async importModuleDynamicallyWrapper (node:internal/vm/module:437:15)
    at async formattedImport (/work/web/alias-server/node_modules/mocha/lib/nodejs/esm-utils.js:9:14)
    at async Object.exports.requireOrImport (/work/web/alias-server/node_modules/mocha/lib/nodejs/esm-utils.js:42:28)
    at async Object.exports.loadFilesAsync (/work/web/alias-server/node_modules/mocha/lib/nodejs/esm-utils.js:100:20)
    at async singleRun (/work/web/alias-server/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at async Object.exports.handler (/work/web/alias-server/node_modules/mocha/lib/cli/run.js:370:5)
--------------------|---------|----------|---------|---------|-------------------
File                | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
--------------------|---------|----------|---------|---------|-------------------
All files           |     7.1 |        0 |       0 |    7.33 |                   
 alias-server       |     100 |      100 |     100 |     100 |                   
  config.js         |     100 |      100 |     100 |     100 |                   
  secrets.sample.js |     100 |      100 |     100 |     100 |                   
 alias-server/src   |    6.57 |        0 |       0 |    6.78 |                   
  alias.js          |    6.34 |        0 |       0 |    6.66 | 13-183            
  chronik.js        |    6.32 |        0 |       0 |    6.66 | 11-205            
  log.js            |      20 |        0 |       0 |      20 | 9-13              
  rpc.js            |   17.64 |        0 |       0 |   17.64 | 7-57              
  telegram.js       |   14.28 |      100 |       0 |   14.28 | 3-17              
  utils.js          |       6 |        0 |       0 |    6.25 | 12-134            
  websocket.js      |    4.65 |        0 |       0 |    4.72 | 13-443            
--------------------|---------|----------|---------|---------|-------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='25']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='352']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='0']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='117']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='0']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='34']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='25']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='341']
##teamcity[blockClosed name='Code Coverage Summary']
mv: cannot stat 'test_results/alias-server-junit.xml': No such file or directory
Build alias-server-tests failed with exit code 1
bytesofman updated this revision to Diff 39183.Apr 3 2023, 21:28

refactoring to remove 'secrets' dependency from unit tests

Harbormaster completed remote builds in B22926: Diff 39183.Apr 3 2023, 21:28
bytesofman updated this revision to Diff 39184.Apr 3 2023, 21:29

linting

Harbormaster completed remote builds in B22927: Diff 39184.Apr 3 2023, 21:30
bytesofman added a comment.Apr 3 2023, 21:32

teamcity issue was related to secrets.js being imported by telegram.js, refactored to correct

Fabien mentioned this in D13522: [alias-server] Better handling of startup case.Apr 4 2023, 07:25
Fabien added inline comments.Apr 4 2023, 08:34
web/alias-server/index.js
13

How is that related to this diff ?

bytesofman marked an inline comment as done.Apr 4 2023, 09:51
bytesofman added inline comments.
web/alias-server/index.js
13

This needs to be initialized in index.js and passed down to all functions that use it in order to support the unit tests.

Before this change, telegramBot was initialized in src/telegram.js, which was required by websocket.js. In this case, importing any function from websocket.js would also fire up the Telegram bot.

This behavior was okay for running the app (telegram bot should always be on and always be polling while app is running). However this behavior caused the unit tests to hang indefinitely, as the unit tests would open up a polling telegram bot without shutting it down.

Initializing the bot on app startup and then passing it as a param is better code organization in general (plus will allow passing mock telegram bot for other unit tests). It had to happen in this diff to allow unit testing of websocket.js

Fabien added inline comments.Apr 4 2023, 10:09
web/alias-server/index.js
13

Got it, you should do this refactor in its own diff

bytesofman marked an inline comment as done.Apr 4 2023, 10:16
bytesofman added inline comments.
web/alias-server/index.js
13

Agreed this would be much better code organization. At this point, considering D13522, probably best to

Land D13521
Abandon D13522, D13523, D13524

New diff to migrate telegram app initialization
Rebuild D13523 and D13524 on top of that

bytesofman abandoned this revision.Apr 4 2023, 15:12

Will be rebuilt on top of D13560

Revision Contents
Changeset List

PathSize
web/
alias-server/
22 lines
src/
20 lines
13 lines
158 lines
test/
200 lines

Diff 39184

View Options

web/alias-server/index.js

Loading...
View Options

web/alias-server/src/chronik.js

Loading...
View Options

web/alias-server/src/telegram.js

Loading...
View Options

web/alias-server/src/websocket.js

Loading...
View Options

web/alias-server/test/websocketTests.js

Loading...