ExchangeUnion / xud

Exchange Union Daemon 🔁 ⚡️
https://exchangeunion.com
GNU Affero General Public License v3.0
115 stars 49 forks source link

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

Open rsercano opened 3 years ago

rsercano commented 3 years ago

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)

This requires a full re-test sorry @raladev

erkarl commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

rsercano commented 3 years ago

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

rebased @raladev

kilrau commented 3 years ago
* 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.

rsercano commented 3 years ago

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

kilrau commented 3 years ago

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

sangaman commented 3 years ago

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 commented 3 years ago

also keep an eye on the failing builds

sangaman commented 3 years ago

also keep an eye on the failing builds

Thanks, just a linting issue. Fixed.

rsercano commented 3 years ago

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

raladev commented 3 years ago

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

rsercano commented 3 years ago

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

kilrau commented 3 years ago

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