Page MenuHomePhabricator

[herald] Handle messages longer than Telegram max
ClosedPublic

Authored by bytesofman on May 1 2023, 23:47.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC79df74ee1653: [herald] Handle messages longer than Telegram max
Summary

T2972

Telegram will not broadcast a message longer than 4096 characters. These will throw an error.

These need to be handled somehow. This diff makes sure the telegram bot does not try to broadcast messages longer than 4096 characters.

  • If the block summary msg is less than 4096 characters, it is sent as normal
  • If the block summary msgs is greater than 4096 characters, it is divided into multiple msgs, and each one is sent replying the the previous msg
Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
herald-handle-overflow-msgs
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23597
Build 46808: Build Diffecash-herald-tests
Build 46807: arc lint + arc unit

Event Timeline

Although the new functions are "tested" by going through the mocks...they should also have their own unit tests

sendBlockSummary
splitOverflowTgMsg

Add unit tests and mocks for new functions

Fabien requested changes to this revision.May 2 2023, 08:56
Fabien added a subscriber: Fabien.
Fabien added inline comments.
apps/ecash-herald/src/telegram.js
34 ↗(On Diff #40138)

If this is guaranteed by design, don't assume but assert

48–51 ↗(On Diff #40138)

This is convoluted for no reason, and completely wrong, \n is a single char. You're not writing \n in the messages.

54 ↗(On Diff #40138)

Note that there is an off-by-one error here because the join() method doesn't add an \n at the end of each line.
E.g. there is a single \n in the output here:

process.stdout.write(['Two', 'lines'].join('\n'));
68 ↗(On Diff #40138)

This is an horrible anti-pattern, just do this after the loop.
This can also potentially send an empty message, not sure what's the outcome of that is.

76 ↗(On Diff #40138)
85 ↗(On Diff #40138)

That doesn't tell what the output of this function is

99 ↗(On Diff #40138)

Can msgSuccess be false ? Is it guaranteed that the bot raises an exception if the message failed to send ?
The Telegram bot API doesn't work like this but I couldn't find the node driver API documentation

118 ↗(On Diff #40138)

That's brittle, if the branch is false for whatever reason you don't return anything.
Right now this shouldn't happen, but you don't know who will edit this function in the future.

2 solutions:

  • have a default return value
  • assert the length is what is expected
apps/ecash-herald/test/mocks/telegramMsgs.js
4 ↗(On Diff #40138)

has this been generated ? If yes an @generated marker would be great

This revision now requires changes to proceed.May 2 2023, 08:56
bytesofman marked 6 inline comments as done.

responding to feedback

bytesofman added inline comments.
apps/ecash-herald/src/telegram.js
48–51 ↗(On Diff #40138)

While \n may be a single character, Telegram counts it as 2 out of 4096

The HTML msgs sent by the Telegram bot do contain \n

99 ↗(On Diff #40138)

Available documentation suggests the sent message object is always returned if there is no error

https://github.com/yagop/node-telegram-bot-api/blob/master/doc/api.md#TelegramBot+sendMessage
https://core.telegram.org/bots/api#sendmessage

Fabien requested changes to this revision.May 2 2023, 16:10

Nits only

apps/ecash-herald/src/telegram.js
31

As per the code, it's <=
It's also a good use case for console.assert()

45

Please comment that it's a telegram bot undocumented behavior that causes the \n to be accounted as 2 chars

115

Another good use for console.assert()

99 ↗(On Diff #40138)

I was thinking the opposite, if an error occurs it doesn't raise but return an error messsage

This revision now requires changes to proceed.May 2 2023, 16:10
bytesofman marked 2 inline comments as done.

adding console.assert and comments explaining undocumented telegram api behavior

bytesofman added inline comments.
apps/ecash-herald/src/telegram.js
99 ↗(On Diff #40138)

I don't really have hard evidence this never happens. Documentation suggests that the msg object is only returned if there is no error.

console.assert added to fish for this.

Fabien requested changes to this revision.May 2 2023, 18:54
Fabien added inline comments.
apps/ecash-herald/src/telegram.js
126 ↗(On Diff #40150)

This is duplicated, either assert and return msgSuccessArray unconditionally or log unconditionally then return false

This revision now requires changes to proceed.May 2 2023, 18:54
bytesofman marked an inline comment as done.

Don't duplicate asserted case

This revision is now accepted and ready to land.May 3 2023, 07:45
bytesofman marked an inline comment as done.

squashing commit