plugin: sendpay_success and sendpay_failure notifications#2945
Merged
rustyrussell merged 10 commits intoElementsProject:masterfrom Sep 11, 2019
Merged
plugin: sendpay_success and sendpay_failure notifications#2945rustyrussell merged 10 commits intoElementsProject:masterfrom
sendpay_success and sendpay_failure notifications#2945rustyrussell merged 10 commits intoElementsProject:masterfrom
Conversation
trueptolemy
force-pushed
the
sendpay-notifications
branch
2 times, most recently
from
8b34063 to
24db409
Compare
trueptolemy
force-pushed
the
sendpay-notifications
branch
2 times, most recently
from
89141be to
cc2f36a
Compare
rustyrussell
requested changes
Aug 28, 2019
Contributor
rustyrussell
left a comment
rustyrussell
left a comment
There was a problem hiding this comment.
I love a PR where my only complaint is the details of the testing.
This is awesome! Trivial inversion of the testing/plugin pattern, and I look forward to committing!
| check_result = check(fail, response) | ||
| return check_result | ||
| plugin.log("no record") | ||
| return False |
Contributor
There was a problem hiding this comment.
FWIW, I just undid similar logic in plugins/forward_payment_status.py.
Turns out it's better to have the plugin just save up the notifications and spew them back on command, then have the comparison logic in the test itself. That way, if something goes wrong, it's much easier to debug (and also to enhance the tests in future, since they only need to be changed in one place).
Contributor
Author
There was a problem hiding this comment.
Thank you :-)
I'll learn from plugins/forward_payment_status.py!
trueptolemy
force-pushed
the
sendpay-notifications
branch
7 times, most recently
from
cefc9b5 to
bb6d6bf
Compare
Contributor
Author
|
Modified the test and the test plugin. @rustyrussell What about now? |
rustyrussell
approved these changes
Sep 9, 2019
Contributor
rustyrussell
left a comment
rustyrussell
left a comment
There was a problem hiding this comment.
Ack bb6d6bf
Sorry for the delay!
But, of course, now it needs rebase. Since it's your code which changed, I'll leave that to you.
trueptolemy
force-pushed
the
sendpay-notifications
branch
2 times, most recently
from
e3e5513 to
beb51b8
Compare
Contributor
Author
|
Thank you! :-) |
`sendpay_success`
A notification for topic `sendpay_success` is sent every time a sendpay
success(with `complete` status). The json is same as the return value of
command `sendpay`/`waitsendpay` when these cammand succeeds.
```json
{
"sendpay_success": {
"id": 1,
"payment_hash": "5c85bf402b87d4860f4a728e2e58a2418bda92cd7aea0ce494f11670cfbfb206",
"destination": "035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d",
"msatoshi": 100000000,
"amount_msat": "100000000msat",
"msatoshi_sent": 100001001,
"amount_sent_msat": "100001001msat",
"created_at": 1561390572,
"status": "complete",
"payment_preimage": "9540d98095fd7f37687ebb7759e733934234d4f934e34433d4998a37de3733ee"
}
}
```
`sendpay` doesn't wait for the result of sendpay and `waitsendpay`
returns the result of sendpay in specified time or timeout, but
`sendpay_success` will always return the result anytime when sendpay
successes if is was subscribed.
We will also call this warped function in the json process of the 'sendpay_failure' notification.
…ails pPayment field includes the basic information of the payment, so the return valves of 'sendpay_success()' and 'sendpay_fail()' should include this field. Note "immediate_routing_failure" is before payment creation, and for this case, return won't include payment fields.
(The json when sendpay successes is too different when sendpay fails, so
divide the sendpay result into two notifications: `sendpay_success` and
`sendpay_failure`)
`sendpay_failure`
A notification for topic `sendpay_failure` is sent every time a sendpay
success(with `failed` status). The json is same as the return value of
command `sendpay`/`waitsendpay` when this cammand fails.
```json
{
"sendpay_failure": {
"code": 204,
"message": "failed: WIRE_UNKNOWN_NEXT_PEER (reply from remote)",
"data": {
"id": 2,
"payment_hash": "9036e3bdbd2515f1e653cb9f22f8e4c49b73aa2c36e937c926f43e33b8db8851",
"destination": "035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d",
"msatoshi": 100000000,
"amount_msat": "100000000msat",
"msatoshi_sent": 100001001,
"amount_sent_msat": "100001001msat",
"created_at": 1561395134,
"status": "failed",
"erring_index": 1,
"failcode": 16394,
"failcodename": "WIRE_UNKNOWN_NEXT_PEER",
"erring_node": "022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59",
"erring_channel": "103x2x1",
"erring_direction": 0
}
}
}
```
`sendpay` doesn't wait for the result of sendpay and `waitsendpay`
returns the result of sendpay in specified time or timeout, but
`sendpay_failure` will always return the result anytime when sendpay
fails if is was subscribed.
json_waitsendpay_on_resolve() has been replaced by wait_payment(), so correct it here.
trueptolemy
force-pushed
the
sendpay-notifications
branch
from
beb51b8 to
951fea4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Long long ago, @ZmnSCPxj mentioned
sendpay_successandsendpay_failurenotifications.My first thought was to combine these two case into one notification type,
sendpay_result.But I found the
jsonfields are two different betweensendpaysuccess and failure, so finally I dividedsendpay_resultintosendpay_successandsendpay_failureagain.For these two notification type, I still use the original notification interface, because I'm not sure whether #2944 is valid.
I pre-present these ideas here though I'll wait for 0.7.3 to addCHANGELOG,