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: add subscribesingleinvoice #2356

Merged
merged 11 commits into from Feb 2, 2019

Conversation

Projects
None yet
4 participants
@joostjager
Copy link
Collaborator

joostjager commented Dec 20, 2018

This PR allows listening to a notification stream for a single invoice.

Usage

SubscribeSingleInvoice is only available for applications integrating directly with the lnd invoices sub server grpc interface. It is not accessible through lncli.

After registering for single invoice notifications, the caller will immediately receive the current invoice state via the stream. From there onwards, lnd will continue pushing updates as they happen.

If the invoice that is subscribed to doesn't exist yet, the call will still block and start streaming when the invoice is created.

Build

Requires lnd to be built with the invoicesrpc build tag:

make tags="invoicesrpc"

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch 5 times, most recently from ff5e889 to 6225ced Dec 20, 2018

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch 4 times, most recently from 6154c5e to 6fe9e32 Dec 27, 2018

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch 3 times, most recently from 3340c88 to 3eedbcf Jan 3, 2019

@joostjager joostjager changed the title invoices: add subscribesingleinvoice [no review] invoices: add subscribesingleinvoice Jan 3, 2019

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch 4 times, most recently from 44baf90 to dedf7fe Jan 4, 2019

@Roasbeef Roasbeef requested a review from cfromknecht Jan 9, 2019

Show resolved Hide resolved lnrpc/invoicesrpc/config_default.go Outdated
Show resolved Hide resolved lnrpc/invoicesrpc/config_default.go Outdated
Show resolved Hide resolved lnrpc/invoicesrpc/invoices_server.go Outdated
Show resolved Hide resolved lnrpc/invoicesrpc/log.go Outdated
Show resolved Hide resolved invoices/invoiceregistry.go
Show resolved Hide resolved lnrpc/utils.go Outdated
Show resolved Hide resolved lnrpc/invoicesrpc/config_active.go

// Now that we know the full path of the invoices macaroon, we can
// check to see if we need to create it or not.
if !fileExists(macFilePath) && cfg.MacService != nil {

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 10, 2019

Collaborator

preferably this logic would also be made into a helper method in lnrpc as well. would you be opposed to doing so and then we can defer a refactoring of other subservers to a later PR?

This comment has been minimized.

Copy link
@joostjager

joostjager Jan 11, 2019

Author Collaborator

Gave it a try, but it gets many parameters. I think it may be better to, in a separate pr, have subservers expose some macaroon info and bake the macaroon somewhere else.

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 14, 2019

Collaborator

sounds good, thanks!

Show resolved Hide resolved cmd/lncli/invoicesrpc_default.go Outdated
Show resolved Hide resolved cmd/lncli/invoicesrpc_active.go Outdated

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch 3 times, most recently from 522ad59 to 970984a Jan 11, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Jan 11, 2019

@cfromknecht
Copy link
Collaborator

cfromknecht left a comment

Thanks @joostjager, few last nits. Otherwise i'd say this about ready to land! 🎉

Show resolved Hide resolved invoices/invoiceregistry.go
Show resolved Hide resolved invoices/invoiceregistry.go Outdated
Show resolved Hide resolved lnrpc/invoicesrpc/invoices_server.go Outdated
Show resolved Hide resolved lnrpc/invoicesrpc/invoices_server.go Outdated
//
// NOTE: This is part of the lnrpc.SubServer interface.
func (s *Server) Stop() error {
close(s.quit)

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 12, 2019

Collaborator

i think it'd be worth protecting this with a compare and swap to prevent us from accidentally closing the same channel twice, as it would lead us to panic during shutdown. Start should be fine though since it's a NOP

This comment has been minimized.

Copy link
@joostjager

joostjager Jan 14, 2019

Author Collaborator

Calling Stop() twice is invalid use of the subserver and adding the compare/swap would camouflage this. We would loose the opportunity to discover the bug of the caller. In general I'd like to avoid defensive code, unless we are dealing with legacy that has become too complicate to reason about and we want to take no risk.

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch from 970984a to 07b561e Jan 14, 2019

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch from df424fe to 0e291b2 Jan 27, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Jan 27, 2019

Leaktest removed. CLI command removed.

PTAL @halseth @Roasbeef @cfromknecht

@Roasbeef
Copy link
Member

Roasbeef left a comment

Needs a go mod tidy, but other than that, LGTM 🐣

Show resolved Hide resolved go.sum Outdated

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch from 0e291b2 to 5c73328 Jan 29, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Jan 29, 2019

Stale review dismissed, ptal

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch 2 times, most recently from 0f2ab5b to b640b48 Jan 29, 2019

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🌶

@cfromknecht
Copy link
Collaborator

cfromknecht left a comment

LGTM 📈

Show resolved Hide resolved invoices/invoiceregistry_test.go Outdated

joostjager added some commits Jan 15, 2019

lnhash: create Hash and Preimage types
This commit adds new hash and preimage types. These types are
similar to chainhash.Hash, except for that string representations
are not reversed.

The reason for adding dedicated types and not use [32]byte, is to
facilitate logging (%v displays as hex string) and have
standard methods to convert from byte slice and string with a
length check.
invoices: use lntypes.Hash and lntypes.Preimage
Previously chainhash.Hash was used, which converts to/from string in
reversed format. Payment hashes and preimages are supposed to be
non-reversed.
invoicesrpc: create sub server
Sub server implementation is still empty. This is a preparatory
step for adding invoice functionality.
@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Feb 1, 2019

Needs a rebase!

joostjager added some commits Dec 20, 2018

lnrpc: add search path to gen_protos.sh
Without this addition, sub servers cannot import google annotations
in their proto files.
channeldb: move idempotency up the call tree
As a preparation for subscribing to single invoices, InvoiceRegistry
needs to become aware of settling a settled invoice.
lnrpc: move invoice marshall code to package
As a preparation for reusing the marshall code in the invoices sub
server.

@joostjager joostjager dismissed stale reviews from cfromknecht and Roasbeef via c049173 Feb 1, 2019

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch from b640b48 to c049173 Feb 1, 2019

joostjager added some commits Jan 11, 2019

@joostjager joostjager force-pushed the joostjager:invoices-subserver branch from c049173 to b163571 Feb 1, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Feb 1, 2019

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🥘

@Roasbeef Roasbeef merged commit 4af857f into lightningnetwork:master Feb 2, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.4%) to 59.623%
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.