xud icon indicating copy to clipboard operation
xud copied to clipboard

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

Open rsercano opened this issue 3 years ago • 14 comments

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

rsercano avatar Dec 05 '20 08:12 rsercano

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?

erkarl avatar Dec 07 '20 08:12 erkarl

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.

rsercano avatar Dec 07 '20 09:12 rsercano

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

raladev avatar Dec 07 '20 13:12 raladev

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

rebased @raladev

rsercano avatar Dec 07 '20 20:12 rsercano

* 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.

kilrau avatar Dec 10 '20 13:12 kilrau

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

rsercano avatar Dec 12 '20 07:12 rsercano

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

kilrau avatar Dec 14 '20 10:12 kilrau

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.

sangaman avatar Dec 22 '20 07:12 sangaman

also keep an eye on the failing builds

kilrau avatar Dec 22 '20 10:12 kilrau

also keep an eye on the failing builds

Thanks, just a linting issue. Fixed.

sangaman avatar Dec 22 '20 14:12 sangaman

I guess @sangaman already pushed a commit for this review: https://github.com/ExchangeUnion/xud/pull/2023#discussion_r547105133 Is there anything to do for me here? @raladev @sangaman

rsercano avatar Dec 29 '20 06:12 rsercano

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

  • [x] Resolve conflicts
  • [x] Check tests, they contain JS errors

raladev avatar Dec 29 '20 14:12 raladev

https://github.com/ExchangeUnion/xud/pull/2023#discussion_r551751553 @kilrau @raladev

rsercano avatar Jan 08 '21 08:01 rsercano

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

kilrau avatar Jan 08 '21 13:01 kilrau