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

[rsercano]

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]

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

[rsercano]

[kilrau]

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]

Depends on this I suggest several changes:

• 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

• 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]

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]

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
}

1. Alias will not be informative if user has several channels with one peer;
2. 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]

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]

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.

0. user opened 2 BTC channel (1 BTC local, 1 BTC remote) -> no alert (and no matter what wallet balance he has)
1. 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]

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]

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]

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]

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]

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 #1984 (comment).

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.

[ghost]

Added 2 simple marble tests, thanks to @erkarl

The test cases look good. Nice work!

[rsercano]

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: #1984 (comment)

[ghost]

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]

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]

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]

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

[raladev]

• change subscribealerts to streamalerts (to keep naming policy - streamorders)
• change time of alert throwing to 30s (for now they appear too often) , in future we can add the parameter to the call.
• change satoshi to user readable format in text message
• IDK what means 0 and 1 in text messages but it looks like incorrect side balance calculation
  
• change LowBalance to LowTradingBalance (because LowBalance looks like alert for on-chain balance)
• change text of assertion:

LowChannelBalance: 0 channel balance is lower than 990950 for BTC by 10%
LowBalance: 0 balance is lower than 980950 for BTC by 10%
LowChannelBalance: 1 channel balance is lower than 599963800 for LTC by 10%
LowBalance: 1 balance is lower than 587963800 for LTC by 

to

LowChannelBalance: (Remote/Local) channel balance (0.1 BTC) of one of the channels is lower than 10% of channel capacity (2 BTC).
LowBalance: (Remote/Local) trading balance (0.1 BTC) is lower than 10% of trading capacity (2 BTC).

OR maybe anyone can suggest something more readable. @kilrau @sangaman

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

[rsercano]

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]

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)

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

[sangaman]

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.

[sangaman]

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]

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 ?

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

[rsercano]

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]

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

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

[rsercano]

Please continue with #2023