feat: new grpc call for subscribing alerts and low balance alert (#864) #2023

rsercano · 2020-12-05T08:54:02Z

Resolves #864

This adds a new middleware between alerts that are being thrown during xud flows and grpc level as we discussed @sangaman But I haven't created two separate PRs, it doesn't make sense since we add a single alert instead of two as in the previous PR (#1984)

Created a new middleware Alerts class.
I added time to alert message let me know if want me to remove it or put it to json output.
Minimum balance alert threshold to re-throw is 10 seconds, let me know if you want to change.
I added a TODO to Alerts class, it's because we need a clear mechanism there for the map.

This requires a full re-test sorry @raladev

ghost · 2020-12-07T08:58:14Z

Thanks for the PR.

Created a new middleware Alerts class.

Why do we need a a new class Alerts? Currently, the class has no public methods and it's just a wrapper event emitter for swap client. Would it make sense to move this to SwapClientManager? Thoughts @sangaman @rsercano?

rsercano · 2020-12-07T09:01:24Z

Thanks for the PR.

Created a new middleware Alerts class.

Why do we need a a new class Alerts? Currently, the class has no public methods and it's just a wrapper event emitter for swap client. Would it make sense to move this to SwapClientManager? Thoughts @sangaman @rsercano?

Since alerts are not specific to swaps/swapclients actually it was @sangaman's idea to create a new class, which I think makes more sense because in a further time we can add more alerts to xud flow's different places.

raladev · 2020-12-07T13:44:32Z

@rsercano can u please rebase it ? (need to get connext changes for vector from master).

…at/subscribealerts-rework � Conflicts: � test/simulation/xudrpc/xudrpc.pb.go

rsercano · 2020-12-07T20:47:53Z

@rsercano can u please rebase it ? (need to get connext changes for vector from master).

rebased @raladev

raladev

u dont need to throw alert for remote balance of connext currencies because this value is not user controllable, and depends on collateralization
(Tue Dec 08 2020 11:20:58 GMT+0000) LowTradingBalance: Remote trading balance (0 ETH) is lower than 10% of trading capacity (15 ETH)
we need to decide how it will look for connext or we just should support this alert only for lnd (looks correct for me) @rsercano @sangaman @kilrau
text message contains time, but json does not contain timestamp, would be good to add.

(Tue Dec 08 2020 11:21:22 GMT+0000) LowTradingBalance: Remote trading balance (0 ETH) is lower than 10% of trading capacity (15 ETH)
^Csimnet > streamalerts -j
Successfully connected, streaming
{
  "type": "LowTradingBalance",
  "payload": {
    "totalBalance": 1500000000,
    "side": "Remote",
    "sideBalance": 0,
    "bound": 10,
    "currency": "ETH"
  }
}

kilrau · 2020-12-10T13:17:44Z

* we need to decide how it will look for connext or we just should support this alert only for lnd (looks correct for me) @rsercano @sangaman @kilrau

The user will be able to influence this. At the moment the requestCollateral is hidden from the user, but once users need to pay for collateral we will have to show it to the user. We can keep this alert imo.

…at/subscribealerts-rework � Conflicts: � lib/Logger.ts � lib/connextclient/ConnextClient.ts � lib/service/Service.ts � test/jest/Service.spec.ts

rsercano · 2020-12-12T07:20:26Z

@raladev added date to json output as Unix time and formatted in formatted output :)

kilrau · 2020-12-14T10:58:08Z

Let's give this another round of review @sangaman @raladev

sangaman · 2020-12-22T07:17:35Z

I pushed a commit to do some refactoring and to preserve type checking. As a general rule I really think we ought to preserve types whenever possible, which are lost with declaring emit: Function.

My last concern is in regards to the proto file field naming & comments/description for purposes of API documentation. See my comments on the unresolved issues above.

kilrau · 2020-12-22T10:26:03Z

also keep an eye on the failing builds

sangaman · 2020-12-22T14:17:50Z

also keep an eye on the failing builds

Thanks, just a linting issue. Fixed.

rsercano · 2020-12-29T06:23:09Z

I guess @sangaman already pushed a commit for this review: #2023 (comment) Is there anything to do for me here? @raladev @sangaman

raladev · 2020-12-29T14:03:52Z

I guess @sangaman already pushed a commit for this review: #2023 (comment) Is there anything to do for me here? @raladev @sangaman

Resolve conflicts
Check tests, they contain JS errors

…at/subscribealerts-rework � Conflicts: � lib/swaps/SwapClient.ts � test/simulation/xudrpc/xudrpc.pb.go

sangaman

The proto file still needs work unfortunately. The bound field is not in the proto definition currently, but it's being used in the xud code. I also still have some open questions for the naming and commenting of the balance alert fields in the proto file, we need to make sure they are clear.

Once that's done we can squash and merge everything. I'm taking care of the merge conflicts, but we need another commit to address the proto file concerns. See if my questions make sense to you @kilrau, I can also take care of the final changes once it's clear to me which exactly which fields we want on the balance alert message.

sangaman · 2021-01-05T07:07:28Z

proto/xudrpc.proto

+  // The bound of the low balance in percentage.
+  uint32 bound = 3 [json_name = "bound"];
+  // The current side balance.
+  uint64 side_balance = 4 [json_name = "side_balance"];


I think we still need an answer here and I'm not sure what it is.

sangaman · 2021-01-05T07:08:40Z

lib/grpc/GrpcService.ts

+      if (serviceAlert.type === AlertType.LowTradingBalance) {
+        const balanceServiceAlert = serviceAlert as Alert;
+        const balanceAlert = new xudrpc.BalanceAlert();
+        balanceAlert.setBound(balanceServiceAlert.bound);


Bound is not in the xudrpc.proto definition currently. Regenerating the protos results in compile errors currently. Do we want this field?

sangaman · 2021-01-05T07:09:52Z

proto/xudrpc.proto

+  Side side = 2 [json_name = "side"];
+  // The bound of the low balance in percentage.
+  uint32 bound = 3 [json_name = "bound"];
+  // The current side balance.


Can you change this or would you like me to?

…s-rework

…at/subscribealerts-rework

rsercano · 2021-01-08T08:46:12Z

#2023 (comment) @kilrau @raladev

kilrau · 2021-01-08T13:41:41Z

I synced with @michael1011 about this and we will not use this information for a channel rebalancer any time soon, the UI will rather use it to display a notification about a general imbalance on a certain asset (not even a specific channel). I think the fields in this PR deliver this, so if if passed your review with that in mind, we can merge as is @sangaman

rsercano requested review from a user, sangaman and raladev December 5, 2020 08:54

rsercano self-assigned this Dec 5, 2020

rsercano force-pushed the feat/subscribealerts-rework branch from e006009 to 8e794da Compare December 5, 2020 08:54

rsercano mentioned this pull request Dec 5, 2020

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

Closed

rsercano force-pushed the feat/subscribealerts-rework branch 2 times, most recently from a9fbc51 to 368167f Compare December 5, 2020 10:34

feat: new grpc call for subscribing alerts and low balance alert (#864)

b7a77f6

rsercano force-pushed the feat/subscribealerts-rework branch from 368167f to b7a77f6 Compare December 5, 2020 14:58

rsercano added 2 commits December 7, 2020 23:33

Merge branch 'master' of https://github.com/ExchangeUnion/xud into fe…

a58c1c6

…at/subscribealerts-rework � Conflicts: � test/simulation/xudrpc/xudrpc.pb.go

rebased

3a890e0

raladev requested changes Dec 8, 2020

View reviewed changes

rsercano added 9 commits December 12, 2020 09:14

Merge branch 'master' of https://github.com/ExchangeUnion/xud into fe…

0f4a1c5

…at/subscribealerts-rework � Conflicts: � lib/Logger.ts � lib/connextclient/ConnextClient.ts � lib/service/Service.ts � test/jest/Service.spec.ts

rebased and fixed import

032482f

fixed eslint issues

6458e81

fixed prettier formatting

947ab1a

fixed prettier formatting

d2cff5c

fixed prettier formatting

6f3ee1c

fixed prettier formatting

9ccda04

fixed prettier formatting

8dcf5cf

fixed review

f7b7aab

rsercano requested a review from raladev December 12, 2020 07:20

rsercano force-pushed the feat/subscribealerts-rework branch from 004c937 to 0f95df4 Compare December 19, 2020 06:18

rsercano requested review from sangaman, raladev and a user December 19, 2020 07:31

raladev previously approved these changes Dec 21, 2020

View reviewed changes

sangaman dismissed raladev’s stale review via da42e0a December 22, 2020 07:15

refactor: type checking for BalanceAlert event

3469df2

sangaman force-pushed the feat/subscribealerts-rework branch from da42e0a to 3469df2 Compare December 22, 2020 14:17

rsercano added 2 commits January 3, 2021 08:49

Merge branch 'master' of https://github.com/ExchangeUnion/xud into fe…

25b3954

…at/subscribealerts-rework � Conflicts: � lib/swaps/SwapClient.ts � test/simulation/xudrpc/xudrpc.pb.go

fixing tests

510ebd4

rsercano requested a review from raladev January 3, 2021 05:59

raladev previously approved these changes Jan 4, 2021

View reviewed changes

sangaman requested changes Jan 5, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into feat/subscribealert…

abc763f

…s-rework

sangaman dismissed raladev’s stale review via abc763f January 5, 2021 07:17

rsercano added 4 commits January 5, 2021 18:24

Merge branch 'master' of https://github.com/ExchangeUnion/xud into fe…

18286f6

…at/subscribealerts-rework

fixed proto

9705f1a

fixed comment on proto

1965731

fixed lint

9d380b9

kilrau requested review from sangaman and raladev January 8, 2021 13:41

raladev approved these changes Jan 8, 2021

View reviewed changes

sangaman removed their request for review October 5, 2022 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new grpc call for subscribing alerts and low balance alert (#864) #2023

feat: new grpc call for subscribing alerts and low balance alert (#864) #2023

rsercano commented Dec 5, 2020

ghost commented Dec 7, 2020 •

edited by ghost

Loading

rsercano commented Dec 7, 2020

raladev commented Dec 7, 2020

rsercano commented Dec 7, 2020

raladev left a comment •

edited by rsercano

Loading

kilrau commented Dec 10, 2020

rsercano commented Dec 12, 2020

kilrau commented Dec 14, 2020

sangaman commented Dec 22, 2020

kilrau commented Dec 22, 2020

sangaman commented Dec 22, 2020

rsercano commented Dec 29, 2020

raladev commented Dec 29, 2020 •

edited

Loading

sangaman left a comment

sangaman Jan 5, 2021

sangaman Jan 5, 2021

rsercano Jan 5, 2021

sangaman Jan 5, 2021

rsercano commented Jan 8, 2021

kilrau commented Jan 8, 2021

feat: new grpc call for subscribing alerts and low balance alert (#864) #2023

Are you sure you want to change the base?

feat: new grpc call for subscribing alerts and low balance alert (#864) #2023

Conversation

rsercano commented Dec 5, 2020

ghost commented Dec 7, 2020 • edited by ghost Loading

rsercano commented Dec 7, 2020

raladev commented Dec 7, 2020

rsercano commented Dec 7, 2020

raladev left a comment • edited by rsercano Loading

Choose a reason for hiding this comment

kilrau commented Dec 10, 2020

rsercano commented Dec 12, 2020

kilrau commented Dec 14, 2020

sangaman commented Dec 22, 2020

kilrau commented Dec 22, 2020

sangaman commented Dec 22, 2020

rsercano commented Dec 29, 2020

raladev commented Dec 29, 2020 • edited Loading

sangaman left a comment

Choose a reason for hiding this comment

sangaman Jan 5, 2021

Choose a reason for hiding this comment

sangaman Jan 5, 2021

Choose a reason for hiding this comment

rsercano Jan 5, 2021

Choose a reason for hiding this comment

sangaman Jan 5, 2021

Choose a reason for hiding this comment

rsercano commented Jan 8, 2021

kilrau commented Jan 8, 2021

ghost commented Dec 7, 2020 •

edited by ghost

Loading

raladev left a comment •

edited by rsercano

Loading

raladev commented Dec 29, 2020 •

edited

Loading