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 payment hash to SendResponse #2033

Merged

Conversation

Projects
None yet
6 participants
@gitlikeagirl
Copy link

commented Oct 11, 2018

When SendPayment grpc call fails, there is currently no way of associating the failure with a payment.
Payment hash is already extracted by extractPaymentIntent, so it can just be added to SendResponse.
Did not add hash to successful sends, since they have the preimage anyway.

@gitlikeagirl gitlikeagirl changed the title lnd: Add payment hash to SendResponse lnrpc: Add payment hash to SendResponse Oct 11, 2018

@ccdle12
Copy link
Contributor

left a comment

Hi @gitlikeagirl

The PR has failed the Travis checks due to rpcserver.go not being correctly formatted. I think you will need to run go fmt. It's generally a good idea to run go fmt before pushing to a branch :)

@gitlikeagirl

This comment has been minimized.

Copy link
Author

commented Oct 12, 2018

ah, thanks @ccdle12 🙈 any idea what I can do about the timeout in TestLightningWallet/btcwallet/neutrino:reorg_wallet_balance ? Can't see my change affecting the test.

@wpaulino

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2018

@gitlikeagirl that is a known issue with our unit tests, I wouldn't worry about it for now. I've restarted the build.

@wpaulino
Copy link
Collaborator

left a comment

Changes LGTM! Could you squash the commits together?

@gitlikeagirl gitlikeagirl force-pushed the gitlikeagirl:ckc-hashinsendresponse branch from 86b1ec5 to 7a1b048 Oct 15, 2018

@joostjager

This comment has been minimized.

Copy link
Collaborator

commented Oct 15, 2018

There is one more stream.send. The success case. It does contain the preimage, so the hash can be computed. For consistency I'd think it is better to always supply the hash, as it is the main key to identify a payment.

@gitlikeagirl

This comment has been minimized.

Copy link
Author

commented Oct 18, 2018

Thanks @joostjager, I wasn't sure if it was necessary but it makes sense to add payment hash to success cases as well. Will do now.

@gitlikeagirl gitlikeagirl force-pushed the gitlikeagirl:ckc-hashinsendresponse branch from 7a1b048 to 11f4f37 Oct 18, 2018

@wpaulino

This comment has been minimized.

Copy link
Collaborator

commented Oct 27, 2018

Needs a rebase!

@halseth

This comment has been minimized.

Copy link
Collaborator

commented Oct 29, 2018

LGTM after rebase!

@gitlikeagirl gitlikeagirl force-pushed the gitlikeagirl:ckc-hashinsendresponse branch from 11f4f37 to c35465a Oct 30, 2018

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

Ready to land after a rebase, will have to synchronize with any other PRs that also modify the protos.

@@ -730,6 +730,7 @@ message SendResponse {
string payment_error = 1 [json_name = "payment_error"];
bytes payment_preimage = 2 [json_name = "payment_preimage"];
Route payment_route = 3 [json_name = "payment_route"];
bytes payment_hash = 4 [json_name ="payment_hash"];

This comment has been minimized.

Copy link
@halseth

halseth Dec 4, 2018

Collaborator

missing space after =

@gitlikeagirl gitlikeagirl force-pushed the gitlikeagirl:ckc-hashinsendresponse branch from c35465a to 2226695 Dec 4, 2018

@gitlikeagirl

This comment has been minimized.

Copy link
Author

commented Dec 4, 2018

Space added and rebase done. Just not sure how to address the merge conflict in rpc.pb.go, is it ok to leave as is?

@halseth

This comment has been minimized.

Copy link
Collaborator

commented Dec 4, 2018

To resolve the merge conflict, checkout the master version: git checkout master lnrcp/rpc.pb.go and regen the protos: make rpc.

@gitlikeagirl gitlikeagirl force-pushed the gitlikeagirl:ckc-hashinsendresponse branch from 2226695 to b2adfca Dec 5, 2018

@halseth
Copy link
Collaborator

left a comment

Great addition! LGTM 🍏

@halseth halseth merged commit 71444e7 into lightningnetwork:master Dec 11, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.005%) to 55.857%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.