Create ExternalPreimage flag on Invoices#3
Conversation
This modifies the Invoice schema to include an indicator of whether the preimage for a given invoice is stored locally or if it is stored externally. For externally stored preimages, the Payment Hash of the Invoice is persisted with it, since it needs to be accessed by other users and it can no longer be generated from the PaymentPreimage.
Adds a boolean indicator ExternalPreimage to the AddInvoice RPC to allow for consumers to create invoices that reference external preimages. Also adds ExternalPreimage to Invoices that are sent in response to various RPC calls.
| "format": "boolean", | ||
| "description": "/ Whether this invoice should include routing hints for private channels." | ||
| }, | ||
| "external_preimage": { |
There was a problem hiding this comment.
Just so we know, was this updated from the cli util? #7 in usage https://github.com/grpc-ecosystem/grpc-gateway#usage
There was a problem hiding this comment.
yes it was - the only file I updated directly was rpc.proto
| } | ||
| if !reflect.DeepEqual(fakeExternalPreimageInvoice, dbExternalPreimageInvoice) { | ||
| t.Fatalf("invoice fetched from db doesn't match original %v vs %v", | ||
| spew.Sdump(fakeExternalPreimageInvoice), spew.Sdump(dbExternalPreimageInvoice)) |
There was a problem hiding this comment.
Does this log show up correctly if its wrong? (id imagine this is a huge log)
There was a problem hiding this comment.
Yes, but only for failed tests cases.
| } else { | ||
| if bytes.Equal(i.Terms.PaymentPreimage[:], zeroPreimage[:]) { | ||
| return zeroHash, fmt.Errorf("Invoices must have a preimage or" + | ||
| "use ExternalPreimages") |
There was a problem hiding this comment.
[nit] This message is clear if you have a context. A clearer message could be "Invoices must have a preimage or ExternalPreimages must be set to true"
| return err | ||
| } | ||
|
|
||
| paymentHash, err := getPaymentHash(i) |
There was a problem hiding this comment.
Did you see any good reason for why err is in the second position for these returned values? I was surprised that these were switched?
There was a problem hiding this comment.
It's just a golang convention: https://gobyexample.com/errors
|
@treygriffith this LGTM. Did we want to setup CI for this repo? |
This change adds a gRPC protobuf file and client to lnd for retrieving external preimages (see #3). It also adds a thin wrapper around the gRPC client in order to lazily connect to the server and reuse connections where possible. It also wraps the streaming API of the request feel more like a request/response pattern. It initializes this client in the server initialization, and provides it to each created Link as part of the ChannelLinkConfig, in anticipation of its use in the exitHop.
2 participants
This change adds an ExternalPreimage indicator on Invoices, and updates the AddInvoice RPC to be able to set that flag.
When this indicator is set, the user must include an
r_hash, which will now be persisted with the Invoice. ExternalPreimage invoices are meant for invoices where the preimage is not stored locally in the LND instance's storage, but offsite in another system.Actually using these invoices in a payment flow is forthcoming in another change.