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

Notifications #76

Closed
wants to merge 14 commits into from
Closed

Notifications #76

wants to merge 14 commits into from

Conversation

JssDWt
Copy link
Collaborator

@JssDWt JssDWt commented Apr 14, 2023

Add notifications when the peer is offline.

  • grpc server for registering a notification url for your pubkey.
  • Sending a notification if the peer is offline and a registration was present.
  • Hold a htlc a (configurable) little while when a notification was sent, waiting for the peer to come online.

This is a very basic implementation of #63. It does not include the following features yet:

  • Removing a webhook subscription after expiry or because it's not functioning or because the client wants to removee the webhook subscription.
  • Log of published webhooks
  • Retry mechanism for temporary failure to deliver the webhook call.
  • Initial subscribed notification
  • Chain notifications
  • Solid mechanisms for not publishing duplicate notifications after restarts or replay of htlcs.

Note that the time period for holding the htlc might get tricky when you get close to the cltv delta. So the holding period should generally be short.

@JssDWt JssDWt requested review from roeierez and yaslama April 14, 2023 18:55
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

@JssDWt I think this is great! I dropped some comments to discuss.

lnd/client.go Outdated Show resolved Hide resolved
lnd/client.go Outdated Show resolved Hide resolved
cln/cln_interceptor.go Show resolved Hide resolved
interceptor/intercept.go Show resolved Hide resolved
interceptor/intercept.go Outdated Show resolved Hide resolved
interceptor/intercept.go Show resolved Hide resolved
@JssDWt JssDWt force-pushed the notifications branch 3 times, most recently from aba6f14 to ddd0f24 Compare April 17, 2023 11:54
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

LGTM

@JssDWt
Copy link
Collaborator Author

JssDWt commented Apr 20, 2023

  • added another integration test
  • fixed a bunch of bugs related to checking whether the peer is online and looking up the peer based on the scid
  • renamed updated_at to refreshed_at
  • also wait for the channel to become active after the peer comes online if the htlc is a regular forward

More efficient peer online and scid lookups are out of scope for this PR and is tracked by this issue: #77

@JssDWt JssDWt requested review from yaslama and roeierez April 21, 2023 07:40
@JssDWt
Copy link
Collaborator Author

JssDWt commented May 4, 2023

Merged into this PR more efficient lookups of peer id based on scid and more efficient waiting for the peer to come online (that was blocking this PR to be merged) #78

@JssDWt JssDWt mentioned this pull request Jun 16, 2023
@JssDWt
Copy link
Collaborator Author

JssDWt commented Jun 16, 2023

Closing this in favor of #88

@JssDWt JssDWt closed this Jun 16, 2023
@JssDWt JssDWt deleted the notifications branch July 28, 2023 21:19
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.

3 participants