Telegram support #341

2021-02-23T15:36:01-08:00

dazito commented

2021-02-23 15:36:01 -08:00

(Migrated from github.com)

No description provided.

lilianchiassai (Migrated from github.com) reviewed

2021-02-23 15:36:01 -08:00

dazito commented

2021-02-23 15:36:46 -08:00

(Migrated from github.com)

Before merging we need to create this entry in the wiki page: https://github.com/agonyforge/arbitrader/pull/341/files#diff-9ed7902517cecde48e205a8428a5e450aa6485ba19e23a6779edf26a2febefc7R187

scionaltera (Migrated from github.com) requested changes

2021-02-23 19:03:22 -08:00

scionaltera (Migrated from github.com) left a comment

Just a couple of small tweaks. Looks good overall.

Update a comment in the example config file to include a little more info.
We can easily handle the 'g' prefix in the group ID silently and avoid people asking why it doesn't work when they didn't read the instructions.

Just a couple of small tweaks. Looks good overall. * Update a comment in the example config file to include a little more info. * We can easily handle the 'g' prefix in the group ID silently and avoid people asking why it doesn't work when they didn't read the instructions.

application.example.yaml

					
				@ -51,6 +51,14 @@ notifications:

				    from: x@x.x

Should mention the 'g' thing in this comment here, and include a link to the wiki even though nobody reads it. :P

src/main/java/com/r307/arbitrader/service/NotificationServiceImpl.java

					
				@ -125,0 +100,4 @@

				    public void sendExitTradeNotification(Spread spread, BigDecimal longVolume, BigDecimal longLimitPrice, BigDecimal shortVolume,

				                                          BigDecimal shortLimitPrice, BigDecimal entryBalance, BigDecimal updatedBalance, BigDecimal exitTarget,

				                                          boolean isForceExitPosition, boolean isActivePositionExpired) {

I like that this is getting more generic. Nice job. 👍

src/main/java/com/r307/arbitrader/service/NotificationServiceImpl.java

					
				@ -147,12 +127,83 @@ public class NotificationServiceImpl implements NotificationService {

So if you copy 12345678 then below we have to send -12345678 right? But if you miss this instruction and copy g12345678 it isn't going to work. Could we just accept either g12345678 or 12345678 and make it a little more user friendly? We could just take the 'g' off in the getter in the configuration class if it's there, or in the setter if you prefer.

So if you copy `12345678` then below we have to send `-12345678` right? But if you miss this instruction and copy `g12345678` it isn't going to work. Could we just accept either `g12345678` or `12345678` and make it a little more user friendly? We could just take the 'g' off in the getter in the configuration class if it's there, or in the setter if you prefer.

src/main/java/com/r307/arbitrader/service/NotificationServiceImpl.java

This is interesting. We have Slack and Discord and we push all log messages to those platforms, but here we're treating Telegram more like email and only pushing entry/exit notifications. Is there a technical reason not to just ship all logs to Telegram like Discord and Slack? Is it a "channel" vs. "direct message" thing?

Along the same lines (not in this PR though)... it seems like we should expand on this sendInstantMessage() idea to add DMs via Slack and Discord just for entry/exit notifications? We can make new tickets for that, but it seems like a natural thing to expand the notification system and make it more powerful.

This is interesting. We have Slack and Discord and we push all log messages to those platforms, but here we're treating Telegram more like email and only pushing entry/exit notifications. Is there a technical reason not to just ship all logs to Telegram like Discord and Slack? Is it a "channel" vs. "direct message" thing? Along the same lines (not in this PR though)... it seems like we should expand on this `sendInstantMessage()` idea to add DMs via Slack and Discord just for entry/exit notifications? We can make new tickets for that, but it seems like a natural thing to expand the notification system and make it more powerful.

scionaltera commented

2021-02-23 19:04:09 -08:00

(Migrated from github.com)

Before merging we need to create this entry in the wiki page: https://github.com/agonyforge/arbitrader/pull/341/files#diff-9ed7902517cecde48e205a8428a5e450aa6485ba19e23a6779edf26a2febefc7R187

I think you guys have access to edit the wiki now. If not, let me know and I can do it.

> Before merging we need to create this entry in the wiki page: https://github.com/agonyforge/arbitrader/pull/341/files#diff-9ed7902517cecde48e205a8428a5e450aa6485ba19e23a6779edf26a2febefc7R187 I think you guys have access to edit the wiki now. If not, let me know and I can do it.

dazito (Migrated from github.com) reviewed

2021-02-24 15:11:11 -08:00

src/main/java/com/r307/arbitrader/service/NotificationServiceImpl.java

The main reasons why I set Telegram like email notifications are:

to avoid being rate limited (when sending all logs) and to be sure you will always get the the entry and exit trade information. Currently with discord (the one I am using) I am sometimes rate limited and I miss some lines in an entry/exit trade
to not have my phone frequently ringing. I can mute the group chat but then I get into the next issue:
to have instant notification when a trade occurs and not forcing me to pro actively open discord/email client/slack to check if the bot happen to have entered/exited a trade
I am not always interested in the logs, mostly while at work or away from the computer I am only interested in checking whether the bot did a trade or not

With this in mind I thought that Telegram could be a good candidate for only important notifications.

It is possible to send DM on Telegram but the setup is more cumbersome. It evolves the bot having to be able to read incoming messages from the user because (AFAIK) for DM the user has to first send a message to the bot. The bot reads the message and retrieves a messageId and replies back to the user with a value for messageId = messageId + 1 and we must keep track of this messageId for future DM.
In my opinion, using the group way to enable the bot to send messages to the user is more user friendly and much more practical from the code point of view.

The main reasons why I set Telegram like email notifications are: 1. to avoid being rate limited (when sending all logs) and to be sure you will always get the the entry and exit trade information. Currently with discord (the one I am using) I am sometimes rate limited and I miss some lines in an entry/exit trade 2. to not have my phone frequently ringing. I can mute the group chat but then I get into the next issue: 3. to have instant notification when a trade occurs and not forcing me to pro actively open discord/email client/slack to check if the bot happen to have entered/exited a trade 4. I am not always interested in the logs, mostly while at work or away from the computer I am only interested in checking whether the bot did a trade or not With this in mind I thought that Telegram could be a good candidate for only important notifications. It is possible to send DM on Telegram but the setup is more cumbersome. It evolves the bot having to be able to read incoming messages from the user because (AFAIK) for DM the user has to first send a message to the bot. The bot reads the message and retrieves a `messageId` and replies back to the user with a value for `messageId = messageId + 1` and we must keep track of this messageId for future DM. In my opinion, using the group way to enable the bot to send messages to the user is more user friendly and much more practical from the code point of view.

dazito (Migrated from github.com) reviewed

2021-02-24 15:13:00 -08:00

application.example.yaml

					
				@ -51,6 +51,14 @@ notifications:

				    from: x@x.x

I will add it. I thought of only adding the wiki entry when we are about to merge this branch otherwise I am afraid someone may see it in the wiki and try to configure it before this branch is merged. Even though I think very few people check the wiki page 😅

I will add it. I thought of only adding the wiki entry when we are about to merge this branch otherwise I am afraid someone may see it in the wiki and try to configure it before this branch is merged. Even though I think very few people check the wiki page :sweat_smile:

dazito (Migrated from github.com) reviewed

2021-02-24 15:14:12 -08:00

src/main/java/com/r307/arbitrader/service/NotificationServiceImpl.java

					
				@ -125,0 +100,4 @@

				    public void sendExitTradeNotification(Spread spread, BigDecimal longVolume, BigDecimal longLimitPrice, BigDecimal shortVolume,

				                                          BigDecimal shortLimitPrice, BigDecimal entryBalance, BigDecimal updatedBalance, BigDecimal exitTarget,

				                                          boolean isForceExitPosition, boolean isActivePositionExpired) {

Yeah, looks much better now :)

dazito (Migrated from github.com) reviewed

2021-02-24 15:39:44 -08:00

src/main/java/com/r307/arbitrader/service/NotificationServiceImpl.java

					
				@ -147,12 +127,83 @@ public class NotificationServiceImpl implements NotificationService {

I implemented it. Replacing the 'g' by a '-'

dazito (Migrated from github.com) reviewed

2021-02-25 12:21:06 -08:00

src/main/java/com/r307/arbitrader/service/NotificationServiceImpl.java

I also thought in the long term to be able to send commands to the bot via Telegram. Commands like:

force entry
force exit
check account balance
current trades
etc

I also thought in the long term to be able to send commands to the bot via Telegram. Commands like: - force entry - force exit - check account balance - current trades - etc