-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add Reverse Swap notification #953
Conversation
5166080
to
76b6cba
Compare
ca76cfd
to
e63dc2c
Compare
0108a93
to
68cec99
Compare
68cec99
to
c88c64e
Compare
c88c64e
to
0234597
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.
Looks good! dropped two comments.
} | ||
|
||
this.bitcoinAddress?.let {address -> |
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.
I think it would be nice to be able to recognize whether it is a swap or reverse swap without trying both.
If we pass a query param app_data in the webhook url it returns back in the notification payload.
See here where it is grapped from the query param: https://github.com/breez/notify/blob/main/http/router.go#L40
And here how it is populated into the payload: https://github.com/breez/notify/blob/main/breezsdk/init.go#L43
Perhaps we can use it here to identify if it is a swap or reverse swap?
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.
Yeah, I thought that too, but the redeemSwap
first checks there is a swap for that address so it's pretty inexpensive.
What if the application or NDS is already using the app_data
param in someway? We would override it and potentially break NDS/app logic.
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.
That's a good argument. I am ok then to leave this inexpensive check.
ec1266c
to
fb13b8f
Compare
@@ -194,20 +194,20 @@ class NotificationHelper { | |||
group = LNURL_PAY_WORKGROUP_ID | |||
} | |||
val swapTxConfirmedNotificationChannel = NotificationChannel( |
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.
Should we rename the notification channel & notification channel group variable as well?
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
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
This PR adds background processing of the reverse swap flow using webhooks:
process_reverse_swap
method to handle processing an individual reverse swap by addressFixes #924