Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lnrpc: Add subscribe peer events rpc #3397

Merged

Conversation

carlaKC
Copy link
Collaborator

@carlaKC carlaKC commented Aug 12, 2019

This PR adds a subscribe peer events rpc stream, following on from #3354. The motivation for adding this stream would be to provide information for people who want to write their own autopilot logic external to LND.

Pull Request Checklist

  • All changes are Go version 1.12 compliant
  • Code is commented, has been formatted with go fmt and lines are wrapped at 80
  • Running make check, go vet andmake lint ok
  • All commits are logically structured, build properly and pass tests

@carlaKC carlaKC force-pushed the peernotifier-addsubscribepeerrpc branch from 897e1e0 to a7652dd Compare October 9, 2019 09:34
rpcserver.go Outdated Show resolved Hide resolved
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM! Question more becomes whether this RPC is useful to expose

rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. As far as the RPC, are there any plans to extend it with other notification types that could make it more useful to expose?

@carlaKC
Copy link
Collaborator Author

carlaKC commented Oct 10, 2019

Code LGTM! Question more becomes whether this RPC is useful to expose

I think that it's useful in the context of people building their own channel closing logic? If we're going to expose the final channel insights metrics (rather than time series), be it an all time value of with start/end params, I think that it makes sense to expose the individual elements over RPC so that people can easily build from scratch if they want to.

@carlaKC carlaKC force-pushed the peernotifier-addsubscribepeerrpc branch from a7652dd to b30250e Compare October 10, 2019 07:55
@carlaKC
Copy link
Collaborator Author

carlaKC commented Oct 10, 2019

LGTM. As far as the RPC, are there any plans to extend it with other notification types that could make it more useful to expose?

None at present, but I can look into it if online/offline aren't enough to justify adding a new endpoint?

@halseth
Copy link
Contributor

halseth commented Oct 10, 2019

Also useful for wallets that want to show the number of connected peers without having to poll listpeers I think :)

@carlaKC
Copy link
Collaborator Author

carlaKC commented Nov 5, 2019

This one has been sitting around for a while now. Should I rebase for merge or close because we don't want to add this rpc endpoint?

@halseth
Copy link
Contributor

halseth commented Nov 5, 2019

I think it is still useful, but we can let it sit around a bit longer until someone has a specific use case for it in mind.

@carlaKC
Copy link
Collaborator Author

carlaKC commented Nov 5, 2019

Ok cool 😀 didn’t want to leave it hanging around without following up.

@fusion44
Copy link

I have a usecase :)

In my Wallet, I display a list of currently connected peers. Wherever possible, I use subscriptions to make the app feel more responsive and dynamic. Subscribing to this means I don't have to implement pull to refresh and save some bytes of data transfered.

https://github.com/fusion44/sendmany/blob/master/docs/screenshots/screenshot_3.png

@carlaKC carlaKC force-pushed the peernotifier-addsubscribepeerrpc branch from b30250e to 64388fa Compare December 15, 2019 10:39
@carlaKC
Copy link
Collaborator Author

carlaKC commented Dec 15, 2019

I have a usecase :)

Awesome!

Rebased on master just in case we want to merge this :D

@halseth
Copy link
Contributor

halseth commented Dec 17, 2019

Needs another rebase! 😅

@carlaKC carlaKC force-pushed the peernotifier-addsubscribepeerrpc branch from 64388fa to 3c28507 Compare December 17, 2019 19:27
@carlaKC
Copy link
Collaborator Author

carlaKC commented Dec 17, 2019

rerebased 😂 going to add it to the 0.9 milestone so it gets some attention

@carlaKC carlaKC added the v0.9.0 label Dec 17, 2019
@joostjager joostjager added this to the 0.9.0 milestone Dec 17, 2019
@halseth halseth merged commit fd41538 into lightningnetwork:master Dec 17, 2019
@carlaKC carlaKC deleted the peernotifier-addsubscribepeerrpc branch April 3, 2020 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants