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

Invoices with --private marked as such on LookupInvoice #2222

Merged
merged 1 commit into from Dec 18, 2018

Conversation

4 participants
@ErikEk
Copy link
Contributor

ErikEk commented Nov 26, 2018

When looking at invoices that were created with the private option, they do not reflect their private status correctly. This PR propose a fix for that.

Fixes for #2179.

@wpaulino
Copy link
Collaborator

wpaulino left a comment

One minor inconsistency is that a private invoice can be created but there are no eligible private channels to be used as routing hints for the invoice. In this case, the invoice was intended to be "private", but this change won't reflect that. IMO, this isn't really a big deal though.

@ErikEk

This comment has been minimized.

Copy link
Contributor Author

ErikEk commented Nov 26, 2018

Thx for the review. I thought about that, but since I didn't want to change the structure for saved invoices I guess this is good enough for now.

@Roasbeef Roasbeef added this to the 0.5.2 milestone Nov 27, 2018

@alexbosworth

This comment has been minimized.

Copy link
Contributor

alexbosworth commented Dec 3, 2018

What if private were just renamed to something like include_hop_hints and then the response did not mirror the creation arguments?

@ErikEk

This comment has been minimized.

Copy link
Contributor Author

ErikEk commented Dec 4, 2018

I don't think we really need to store or print this value at all. The routing hits themself would be adequate.

@Roasbeef Roasbeef added this to In progress in High Priority via automation Dec 18, 2018

@Roasbeef Roasbeef assigned wpaulino and unassigned wpaulino Dec 18, 2018

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 💎

High Priority automation moved this from In progress to Final Testing -- Ready For Merge Dec 18, 2018

@Roasbeef Roasbeef merged commit a9cba33 into lightningnetwork:master Dec 18, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.02%) to 55.973%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

High Priority automation moved this from Final Testing -- Ready For Merge to Done Dec 18, 2018

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.