feat: new grpc call for subscribring alerts such as low balance (#864)

rsercano commented 3 years ago

attempts to resolve #864

This's a draft PR, I didn't want to waste time if I'm not in a right direction, so can you please check this one @sangaman?

In short; I created a copy of streamorders (ensureConnection could be in utils but reinitializing client is challenging since it's recursive and client should stay immutable if I take it as a function arg.) Main idea is to trigger alert events from anywhere (for now only from Lnd and Connext) and catch and merge them to pipeline just like in the streamorders

sangaman commented 3 years ago

Looks like the right approach, next step is actually firing the alerts and making sure Service is properly listening to and handling them.

rsercano commented 3 years ago

kilrau commented 3 years ago

To answer your question about this @raladev : an unbalanced event should be fired if e.g. 10% or less of a channel's total balance are on the local or remote side (with a side field saying if the low side is local or remote).

raladev commented 3 years ago

Depends on this I suggest several changes:

[x] change 0 alert to bound alert as @kilrau described:
user opened channel for 1 BTC but without remote balance -> 10% alert for remote part
user sold 0.99 BTC -> 10% alert for local part
[x] alert message should be parse-able json, it is information not for human, no one will monitor stream manually (check speed of new messages appearing in streamorders call), alert should not be user-friendly plaintext. Something like that: { total_balance:1 BTC # total balance of the channel side:"remote" bound:"10" side_balance:0.01 BTC # balance on side that leads to alert channel_point: "abc" # required if user have multiple btc channels }

WDYT? @rsercano @kilrau

kilrau commented 3 years ago

Thanks for the proposal! One small nit: I don't think channelpoint is very informative, so I'd rather go with xud peer_alias

{
total_balance:1 BTC # total balance of the channel
side:"remote"
bound:"10" # alert threshold in % of toal balance
side_balance:0.01 BTC # balance on side that leads to alert
peer_alias: "NiftyNei" # xud alias associated to channel
}

raladev commented 3 years ago

Thanks for the proposal! One small nit: I don't think channelpoint is very informative, so I'd rather go with xud peer_alias
{
total_balance:1 BTC # total balance of the channel
side:"remote"
bound:"10" # alert threshold in % of toal balance
side_balance:0.01 BTC # balance on side that leads to alert
peer_alias: "NiftyNei" # xud alias associated to channel
}

Alias will not be informative if user has several channels with one peer;
it is not trivial to get channel info by alias, I even cant suggest any solution for that, but channel point can be used to find channel using lndbtc directly.

rsercano commented 3 years ago

But if we're implementing something generic below json doesn't seem generic at all:

{
total_balance:1 BTC # total balance of the channel
side:"remote"
bound:"10" # alert threshold in % of toal balance
side_balance:0.01 BTC # balance on side that leads to alert
peer_alias: "NiftyNei" # xud alias associated to channel
}

In general I'm populating an alert with type and message. Don't know how to make it more generic. On the other hand as a UI developer it's much more simpler to read two fields from json and print message to screen and show an icon regarding to type, wdyt?

For: user opened channel for 1 BTC but without remote balance -> 10% alert for remote part

This's about wallet and channel balance I guess, so in this case we should check if my wallet balance is lowered %10 of channel balance then we fire alert? @raladev

raladev commented 3 years ago

On the other hand as a UI developer it's much more simpler to read two fields from json and print message to screen and show an icon regarding to type, wdyt?

if we agree that this call if only for xud-ui, im ok with this approach (json - with type and text message)

{
type:"channel_balance",
message:"your remote balance of BTC channel bla bla bla"
}

but if this call is not only for xud ui it would be better to change structure to :

type:"channel_balance"
message: {data_structure_for certain_alert_type} (https://github.com/ExchangeUnion/xud/pull/1984#issuecomment-724665817)

This's about wallet and channel balance I guess, so in this case we should check if my wallet balance is lowered %10 of channel balance then we fire alert? @raladev

nope, it is only about channel balance, you dont need to check wallet balance at all.

user opened 2 BTC channel (1 BTC local, 1 BTC remote) -> no alert (and no matter what wallet balance he has)
user sold 0.95 BTC -> 0.05 BTC local (2.5 %), 1.95 BTC remote (97.5%) -> 10% alert for local channel balance (and no matter what wallet balance he has)

rsercano commented 3 years ago

On the other hand as a UI developer it's much more simpler to read two fields from json and print message to screen and show an icon regarding to type, wdyt?

if we agree that this call if only for xud-ui, im ok with this approach (json - with type and text message)
{
type:"channel_balance",
message:"your remote balance of BTC channel bla bla bla"
}
but if this call is not only for xud ui it would be better to change structure to :
type:"channel_balance"
message: {data_structure_for certain_alert_type} (https://github.com/ExchangeUnion/xud/pull/1984#issuecomment-724665817)

What do you think @kilrau and @sangaman ?

kilrau commented 3 years ago

if we agree that this call is only for xud-ui, im ok with this approach (json - with type and text message)

In short: I don't agree. Albeit only xud-ui using these errors for display in a human-readable format right now, I expect tools like an automated channel rebalancer to use these alerts (and data in it!) to actually do things, like balancing channels back.

For that it should be able to extract minimum total_balance, side, side_balance from the message and I vouch for this option:

type:"channel_imbalanced"
message: {data_structure_for certain_alert_type}

For simplicity we can just go for the channelpoint, agreed on that @raladev . And let's not forget about connext vector here, it has to work for both. I just asked the connext team how channelpoints in vector look like.


{
total_balance:1 BTC # total balance of the channel
side:"remote"
bound:"10" # alert threshold in % of toal balance
side_balance:0.01 BTC # balance on side that leads to alert
channel_point: "abc" # required if user have multiple btc/ltc/eth channels
}

kilrau commented 3 years ago

Update: connext channel's are represented by their unique multisig address, I'd had to try a bit to find the endpoint delivering this from the connext client though. @sangaman @erkarl ?

sangaman commented 3 years ago

I was thinking just an enum type and string message made the most sense for alerts - and xud-ui would just display the message to the user - but if others think that's insufficient for xud-ui's purposes then we can have a structured message. We can use grpc's oneof so that the message takes on one of several different formats, so like:

oneof payload {
  string message = 1 [json_name = "message"];
  ChannelBalanceAlert balance_alert = 2 [json_name = "balance_alert"];
}

message ChannelBalanceAlert {
  uint64 capacity = 1;
  ...
}

That way if we have new alerts that we want structured formats in the future, we can just add to the oneof.

As for Connext channel identifiers, it seems to me like it'd be sufficient just to specify the token, right? Since we should only have one channel per token?

kilrau commented 3 years ago

if others think that's insufficient for xud-ui's purposes then we can have a structured message

Sufficient for xud-ui, but not for future purposes, see https://github.com/ExchangeUnion/xud/pull/1984#issuecomment-724868341.

We can use grpc's oneof so that the message takes on one of several different formats

That's nice, let's use this! Give it a shot and you can use the string message only for now for the channel_imbalanced_alert type @rsercano

As for Connext channel identifiers, it seems to me like it'd be sufficient just to specify the token, right? Since we should only have one channel per token?

Correct as of now, but that will definitely change, hence we want to add a channel identifier here.

rsercano commented 3 years ago

Currently %10 is hardcoded, we can take it to config if you wish, otherwise can you please have a look at this? @sangaman @raladev @kilrau

kilrau commented 3 years ago

I'm more concerned that currently it seems we'd get an alert every single time we update our balance (which should happen every few seconds) and that would be excessive, should we put in some minimum time limit between repeating alerts for the same currency?

Hmm, does it really matter if e.g. the UI subscribing to it can control when/how to show it to the user?

I'm also thinking that maybe we wouldn't want channel-level alerts but rather overall balance. It's not such a big deal if I have low balance in a single channel if I have plenty of capacity in other channels. This is particularly concerning because right now if I had a node with like 50 balanced channels, and 1 channel that was unbalanced (maybe one that I'm not even using for trading), I'd get this alert. Thoughts @kilrau?

We really should see this channel-level unbalanced alerts as one first alert type only. And I think we want both, channel-level as well as overall unbalanced alerts. Channel-level is useful because I expect this information to be used by a rebalancer tool and other tools, and an overall unbalanced alert is more useful for the user directly, especially with MPP enabled.

So if you don't mind could you additionally add the overall unbalanced alert type just like @sangaman described? @rsercano Think that makes a nice PR demoing two different unbalanced alert types.

kilrau commented 3 years ago

cc @krrprr

rsercano commented 3 years ago

I read all the reviews, and comments thanks for your time, will take care of them asap as much as I can and additionally yes I can add overall balance alert too, no problem @kilrau

rsercano commented 3 years ago

What's left for this PR so far:

Integration/rxjs testing
Overall balance alert

To continue to overall balance alert I'll be waiting your reviews, for tests I'm not really sure.

ghost commented 3 years ago

Integration/rxjs testing for tests I'm not really sure

Please create a follow-up issue to address this if not addressed within this PR.

I'm happy to screen-share pair with you in case you need help with constructing these tests.

rsercano commented 3 years ago

Added unit tests for checkLowBalance, still need to add marble tests for rxjs tho. On the other hand unit tests are in lnd client tests since it's a protected method and doesn't need to test it twice for connext/lnd. Let me know if I'm mistaken.

ghost commented 3 years ago

Something seems to have gone wrong. There are many unnecessary files commited.

rsercano commented 3 years ago

yes @erkarl fixed it, sorry, thanks for checking!!

krrprr commented 3 years ago

Hmm, does it really matter if e.g. the UI subscribing to it can control when/how to show it to the user?

I don't see a problem there as long as the message type is provided besides just the message. We would not want to show a message every time the balance changes (neither do we want to do it every minute I suppose), so we can decide on the UI side, based on the message type, what to do with that kind of messages that arrive very often. But that's a different discussion.

We can use grpc's oneof

I love the idea! This way the UI wouldn't have to contain that much business logic and can just show the provided message.

raladev commented 3 years ago

[x] does not work for me at all (maybe because of connext error in the logs) Screenshot from 2020-11-19 23-51-40 Screenshot from 2020-11-19 23-51-42
also i think that we dont need to check inbound balance for connext currencies because they are autoextendable and autoremovable (not sure about autoremovable, but i think that node can take some amount from collateral), but i dont know how to check outbound balance without inbound for connext. :(

Any ideas how we should handle 10% connext channel check? @kilrau @sangaman

rsercano commented 3 years ago

@raladev thanks for checking, I fixed the issue also found out there's a json issue and fixed it too, on the other hand I realized we update capacities for each currency for LND, therefore this function is working twice if you have LTC & BTC, and you'll see same message twice on every update capacity.

I'm open to suggestions at this point. @raladev @erkarl @kilrau @sangaman

rsercano commented 3 years ago

Added 2 simple marble tests, thanks to @erkarl, what's left is to add a balance alert apart from channel balance, but could you please check channel balance alert in the meantime @raladev

ghost commented 3 years ago

Added 2 simple marble tests, thanks to @erkarl

The test cases look good. Nice work!

rsercano commented 3 years ago

Added alert for total balance for LND, but I'm unsure how it would be for connext since there're no multi channels there and there's no total capacity. Also what's left for this PR from my side is this: https://github.com/ExchangeUnion/xud/pull/1984#issuecomment-731544580

ghost commented 3 years ago

I'm unsure how it would be for connext since there're no multi channels there and there's no total capacity

The total capacity for connext would be the capacity of the 1 channel that it has.

rsercano commented 3 years ago

I'm unsure how it would be for connext since there're no multi channels there and there's no total capacity

The total capacity for connext would be the capacity of the 1 channel that it has.

uhm, so what's the difference between LOW_CHANNEL_BALANCE and LOW_BALANCE alerts for connext?

kilrau commented 3 years ago

uhm, so what's the difference between LOW_CHANNEL_BALANCE and LOW_BALANCE alerts for connext?

Practically none now, but there will be in the near future where connext can have multiple channels too.

rsercano commented 3 years ago

then I'm leaving it as is for now, can @raladev test, and can @sangaman do a code review please?

rsercano commented 3 years ago

change subscribealerts to streamalerts (to keep naming policy - streamorders)

Changed the name in terms of grpc command, on the other hand in terms of code it's still subscribe because streamorders have same naming.

change time of alert throwing to 30s (for now they appear too often) , in future we can add the parameter to the call.

To achieve this, I defined a new timer which works every 30s.

change satoshi to user readable format in text message

I had to add a util import to Service layer for this, but it makes sense to me since message is the most important for this command due to xud-ui.

IDK what means 0 and 1 in text messages but it looks like incorrect side balance calculation

I fixed the 0,1 stuff. But what do you mean by incorrect calculation? @raladev

also it seems u use different values for LowChannelBalance and LowBalance alerts. (see total balance values on previous screen)

It's because LowChannelBalance shows channel's total balance, hence LowBalance shows the total balance @raladev

raladev commented 3 years ago

It's because LowChannelBalance shows channel's total balance, hence LowBalance shows the total balance @raladev

checked it. LowChannelBalance contains reserved amount, but LowTradingBalance does not.

LowChannelBalance: Remote channel balance (0.1) of one of the channels is lower than 10% of channel capacity (1.999638)
LowTradingBalance: Remote trading balance (0.08) is lower than 10% of trading capacity (1.959638)

[x] add currency to text message (0.1 BTC), because it is hard to understand which ln-channel I need to find

sangaman commented 3 years ago

We really should see this channel-level unbalanced alerts as one first alert type only. And I think we want both, channel-level as well as overall unbalanced alerts. Channel-level is useful because I expect this information to be used by a rebalancer tool and other tools, and an overall unbalanced alert is more useful for the user directly, especially with MPP enabled.

I am second guessing this, mainly because it seems outside the scope of xud. There are channel rebalancer tools out there, and I think generally speaking xud should not be too concerned with the workings of the swap clients it's using.

I'd be fine with only alerts for total balance levels - e.g. when we run low on balance (or inbound capacity) of a certain currency overall (relative to our total capacity - although that concept doesn't apply to connext - or to some configured threshold).

kilrau commented 3 years ago

I am second guessing this, mainly because it seems outside the scope of xud. There are channel rebalancer tools out there, and I think generally speaking xud should not be too concerned with the workings of the swap clients it's using.

Fair. To clarify: it will definitely be an external/separate tool doing the re-balancing, not xud itself.

I'd be fine with only alerts for total balance levels - e.g. when we run low on balance (or inbound capacity) of a certain currency overall (relative to our total capacity)

Thinking about this, I tend to agree. Especially with MPP on the horizon.

although that concept doesn't apply to connext - or to some configured threshold

Connext will eventually behave the same with vector: multiple channels to multiple channel partners, so I'd want to treat it equally from now on.

I am thinking it would probably make sense to split this into two PRs.
1. A framework for alerts via rpc

2. Balance alerts
The scope is pretty large and there are multiple concerns to address. I can try to continue reviewing as 1 PR but I think it would be cleaner and easier as 2.

Agree on that. So shall we do that @rsercano ?

PR: pure rpc alert streaming call
PR: implementing overall unbalanced alerts per currency (not per channel)

rsercano commented 3 years ago

Yes, I'll create 2 seperate PRs and close this one by taking @sangaman 's reviews into consideration. On the other hand, if I understood correctly :

we won't have channel balance alerts anymore? @kilrau
We'll definetly change the behaviour of current alerts implementation and I guess we'll need to keep them in the database with last triggered time and create a new class for them @sangaman ?

kilrau commented 3 years ago

we won't have channel balance alerts anymore? @kilrau

Only of total balance across all channels per currency, not for single channels.

rsercano commented 3 years ago

Please continue with #2023

ExchangeUnion / xud

feat: new grpc call for subscribring alerts such as low balance (#864) #1984