-
Notifications
You must be signed in to change notification settings - Fork 53
Add bLIP 55: Webhook Registration (LSPS5) #55
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
Conversation
68e565d
to
4b9484d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, a reference implementation section would be useful.
Hmm, good point in this case, as there is no implementation of this protocol as of today. We plan to add support to |
4b9484d
to
fb3434d
Compare
fb3434d
to
629294d
Compare
Rebased after #55 landed. |
Needs a rebase and this should be good to go? |
Ah, I though we'd wanted to wait with this until a reference implementation is available? |
Good point, if there is one that is being worked on then it's better to wait! |
Yes, there is work-in-progress, will give an update here once that has been merged! |
hey @t-bast @tnull , just pushed a reference implementation for this PR. lightningdevkit/rust-lightning#3662 |
"app_names": ["My LSPS-Compliant Lightning Wallet", "Another Wallet With The Same Signing Device"], | ||
"max_webhooks": 42 | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason why this doesn't also include the webhook URLs? Including them might make it more ergonomic to work with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on the spec call, I have no strong opinion on whether to include the URLs here or not. I think there might have been some privacy concerns if apps include some identifiers in the URLs, as these would leak to other apps registering webhooks. Then again, if you have the apps already share a Lightning wallet, it's probably fine for them to learn about each other, too?
I now updated this PR to: a) make it clear HTTPS is mandatory I also want to note that the reference implementation has been merged on the LDK side (see lightningdevkit/rust-lightning#3662), so we should be able to proceed with this PR (cc @t-bast). |
Why don't you include a link to the implementation as a reference implementation? That could be quite useful for people implementing such a proposal from scratch to be able to look at actual code to clarify details. |
Ah, good point. Now added a corresponding section and squashed the fixup commits while I was at it: diff --git a/blip-0055.md b/blip-0055.md
index facc1f1..f1dc372 100644
--- a/blip-0055.md
+++ b/blip-0055.md
@@ -48,4 +48,11 @@ signs and can send to the mobile OS developer server.
This bLIP is licensed under the MIT license.
+## Reference Implementation
+
+A reference implementation of this protocol can be found as part of the
+[`lightning-liquidity`][] crate.
+
+[`lightning-liquidity`]: https://github.com/lightningdevkit/rust-lightning/tree/main/lightning-liquidity
+
## Protocol |
In typical mobile environments, a program that is not currently being focused on by the user will be suspended, with all its TCP connections dropped. This specification provides a way for a mobile client to register some specific webhook by which the LSP can signal a push notification to the application developer server, which will in turn convert the push notification to one that it itself signs and can send to the mobile OS developer server. The given protocol is based on LSPS0/bLIP 50 and has been previously stabilized by the LSP Spec group. As previously discussed on multiple occasions, the LSP Spec group is however moving to a bLIP-centric process, which is why we 'upstream' these previously-stabilized specifications here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM: note that the README at https://github.com/lightningdevkit/rust-lightning/tree/main/lightning-liquidity doesn't mention that LSPS5 is implemented, you may want to update that?
Ah, thanks for pointing that out. (cc @martinsaposnic, mind including that in one of the follow-ups?) |
sounds good, will update that today |
Based on #52.
In typical mobile environments, a program that is not currently being focused on by the user will be suspended, with all its TCP connections dropped.
This specification provides a way for a mobile client to register some specific webhook by which the LSP can signal a push notification to the application developer server, which will in turn convert the push notification to one that it itself signs and can send to the mobile OS developer server.
The given protocol is based on LSPS0/bLIP 50 and has been previously stabilized by the LSP Spec group.
As previously discussed on multiple occasions, the LSP Spec group is however moving to a bLIP-centric process, which is why we 'upstream' these previously-stabilized specifications here.
(cc the original author @ZmnSCPxj-jr)