Page MenuHomePhabricator

[alias-server] Enable telegram announcements of alias registrations
ClosedPublic

Authored by bytesofman on Jul 12 2023, 18:19.

Details

Summary

Send a summary Telegram message as new aliases are registered

Required more helper functions than originally planned. Unit tests added. Also tested by wiping the db and node index.js. It runs into rate limit errors (as expected) trying to send too many msgs, but these are handled elegantly without crashing the app.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
tg-bot-live-alias
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 24477
Build 48554: Build Diffalias-server-tests
Build 48553: arc lint + arc unit

Event Timeline

should add a unit test for rate limiting the telegram API

logs from running node index.js on an empty database:

image.png (272×772 px, 41 KB)

should add a unit test for rate limiting the telegram API

logs from running node index.js on an empty database:

image.png (272×772 px, 41 KB)

There's not really a good way to test this because we don't want to Promise.all(aliasAnnouncementPromises); in handleBlockConnected. It's easy enough to verify that thes promises cannot be rejected, only resolved, and review logs from above to confirm telegram errors don't crash the app

patch block explorer link error

patch unit tests to expect correct urls

Tail of the build log:

npm WARN deprecated har-validator@5.1.5: this library is no longer supported
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 request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142

added 621 packages, and audited 622 packages in 8s

98 packages are looking for funding
  run `npm fund` for details

14 vulnerabilities (5 low, 8 moderate, 1 high)

To address issues that do not require attention, run:
  npm audit fix

Some issues need review, and may require choosing
a different dependency.

Run `npm audit` for details.

> alias-server@1.0.0 test
> mocha --reporter mocha-junit-reporter --reporter-options mochaFile=test_results/alias-server-junit.xml --reporter-options testsuitesTitle=Alias Server Unit Tests --reporter-options rootSuiteTitle=Alias Server

Connected successfully to MongoDB server
Initialized serverState on app startup
Configured connection to database ecashAliases
------------------------|---------|----------|---------|---------|-------------------------------
File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s             
------------------------|---------|----------|---------|---------|-------------------------------
All files               |   88.79 |    87.13 |   88.23 |   88.98 |                               
 alias-server           |     100 |      100 |     100 |     100 |                               
  config.js             |     100 |      100 |     100 |     100 |                               
  secrets.sample.js     |     100 |      100 |     100 |     100 |                               
 alias-server/constants |     100 |      100 |     100 |     100 |                               
  alias.js              |     100 |      100 |     100 |     100 |                               
  op_return.js          |     100 |      100 |     100 |     100 |                               
 alias-server/src       |   88.69 |    87.13 |   88.23 |   88.88 |                               
  alias.js              |   97.46 |    97.36 |     100 |   97.36 | 166,180                       
  app.js                |   97.56 |    61.11 |     100 |   97.56 | 32                            
  chronik.js            |   64.17 |    83.33 |   71.42 |   65.07 | 19,44,72,113-118,151-196      
  chronikWsHandler.js   |   76.19 |       20 |   66.66 |   76.19 | 23,74-86                      
  db.js                 |    88.4 |    95.45 |     100 |    88.4 | 70-71,112-113,125-126,157-161 
  events.js             |   85.48 |    93.75 |      60 |   85.24 | 35-40,96-101,117,192,220-229  
  main.js               |    90.9 |     62.5 |     100 |    90.9 | 32                            
  rpc.js                |   93.75 |    83.33 |     100 |   93.75 | 36                            
  script.js             |     100 |      100 |     100 |     100 |                               
  telegram.js           |     100 |      100 |     100 |     100 |                               
  utils.js              |     100 |       95 |     100 |     100 | 66                            
------------------------|---------|----------|---------|---------|-------------------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='404']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='455']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='149']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='171']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='45']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='51']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='396']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='445']
##teamcity[blockClosed name='Code Coverage Summary']
Build alias-server-tests failed with exit code 2
PiRK added inline comments.
apps/alias-server/src/telegram.js
28 ↗(On Diff #41424)

This comment is probably detailing/duplicating the code too much. If you ever want to change the displayed number of characters you will have to maintain the comment as well. The comment about the prefix is useful, but could probably be moved to the function documentation for the aliasObject argument.

30–31 ↗(On Diff #41424)

If the magic values were replaced by some constant (prefix_length, prefix_length + 3) it would make the comment unnecessary.

Fabien requested changes to this revision.Jul 13 2023, 16:26
Fabien added a subscriber: Fabien.

The elephant in the room: the tests are broken

This revision now requires changes to proceed.Jul 13 2023, 16:26

The elephant in the room: the tests are broken

I accidentally pushed up a format-improvement change without editing the expected results, then immediately updated them

for whatever reason, teamcity emailed the test failure as the most recent thing to happen, even though the tests did run again and pass when I pushed up the fix

Fabien requested changes to this revision.Jul 14 2023, 09:09
Fabien added inline comments.
apps/alias-server/src/utils.js
33 ↗(On Diff #41440)

Double doc ?

This revision now requires changes to proceed.Jul 14 2023, 09:09

remove legacy/redundant function doc

Fabien requested changes to this revision.Jul 17 2023, 08:17

You're missing an integration test, checking the telegram bot sends a message if there is a block containing an alias registration

apps/alias-server/package.json
25 ↗(On Diff #41449)

Why do you need to bump the version ?

apps/alias-server/src/events.js
234 ↗(On Diff #41449)

This should be its own method so you can test it with unit tests.
Also please make sure telegrambot is nullable so it's not a requirement for the app to have a telegram channel

apps/alias-server/src/utils.js
71 ↗(On Diff #41449)

This is a limited duplicate of the one from the herald, it's time to think about the global architecture

This revision now requires changes to proceed.Jul 17 2023, 08:17
bytesofman marked an inline comment as done.

Move msg broadcasting to own function, do not require a telegramBot for the app to function, unit tests for this, revert library upgrades

Change array building to avoid lint error

apps/alias-server/package.json
25 ↗(On Diff #41449)

I didn't realize this was already installed. will revert.

apps/alias-server/src/utils.js
71 ↗(On Diff #41449)

Could be part of coin-select or mb it's own thing -- I think we could use some kind of a standardized "large numbers" library -- that picks the best / most-lightweight bignumber library and offers methods for dealing with calculating and rendering eCash and token values. Then this sort of formatting helper function could fit there.

apps/alias-server/src/utils.js
71 ↗(On Diff #41449)

It doesn't need to be a lib. Isn't it possible to have a common source file ?

apps/alias-server/src/utils.js
71 ↗(On Diff #41449)

Yes. I'll implement this with mockedChronik (implemented now in alias-server, ecash-herald, and examples) first by adding it to a lib folder in the apps/ top level directory.

For this particular case, the formatting needs of each app are slightly different. While this function looks a lot like the ecash-herald one, it needs to be unique here.

This revision is now accepted and ready to land.Jul 19 2023, 11:40