-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Use the BOLT-11 invoice format within lnd #317
Use the BOLT-11 invoice format within lnd #317
Conversation
bef79d5
to
ba5efe0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! Looking forward to finally switching over to using BOLT 11 internally within lnd
.
Most of my comments are pretty minor, so we shouldn't be held up on getting this in for long. Once this PR has been rebased to the latest version of master, I'll be able to do another pass of review over the PR and also test it out locally.
rpcserver.go
Outdated
} | ||
return | ||
} | ||
nextPayment.Amt = int64(payReq.MilliSat.ToSatoshis()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect. All payment amounts should be handled as mSAT
internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Changed to always sending msat over payChan
rpcserver.go
Outdated
} | ||
nextPayment.Amt = int64(payReq.MilliSat.ToSatoshis()) | ||
|
||
if payReq.PaymentHash == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this logic be already pulled into the Decode
function of zpay32
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, checked in the validateInvoice
method within Decode
. Removed check here and one other place in the file.
rpcserver.go
Outdated
return nil, fmt.Errorf("payment requests with no " + | ||
"amount specified not currently supported") | ||
} | ||
amt = payReq.MilliSat.ToSatoshis() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment here w.r.t ensuring all payment amounts internally are properly handled using mSAT
.
// list of options to be added to the encoded payment request. For now | ||
// we only support the required fields description/description_hash, | ||
// expiry, fallback address, and the amount field. | ||
var options []func(*zpay32.Invoice) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice usage of the functional API!
aa8a0c7
to
4cf4d43
Compare
Rebased and corrected according to review comments. PTAL. |
4cf4d43
to
227af89
Compare
rpcserver.go
Outdated
options = append(options, zpay32.FallbackAddr(addr)) | ||
} | ||
|
||
// If expiry is set, specify it. If it is not provided, then expiry will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be missing something, but, I don't see the location that the default expiration is enforced. Also note that the expiration should always be in absolute terms. Not relative as it seems to be now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is not specified, it will not be encoded in the payment request. The receiver will know that a missing expiry means 3600 seconds. I updated the comment to make this more clear.
Expiration is set as timestamp + expiry
, so expiry should in this case be relative.
rpcserver.go
Outdated
RHash: rHash[:], | ||
RPreimage: paymentPreimge, | ||
Value: int64(invoiceAmount), | ||
Settled: dbInvoice.Terms.Settled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the instances where an invoice is returned from the database, the following fields are never populated:
FallbackAddr
Expiry
DescriptionHash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right... Added :)
} | ||
p.msat = *payReq.MilliSat | ||
p.pHash = payReq.PaymentHash[:] | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When sending (both here and within SendPaymentSync
), if a full invoice was passed in, then the expiration of the invoice is not being properly observed. If the payment request has expired, then the payment attempt should be rejected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a check for this.
While doing this I realized time.Time
is not the best way of expressing timespans in Go, so I had to cast payReq.Expiry
to seconds, and then to time.Duration
. We might consider changing the zpay32.Invoice
struct to express Expiry
directly in time.Duration
. I will also consider moving the "default to 3600 seconds logic" within zpay32
in the same change.
rpcserver.go
Outdated
// second expiry time. | ||
expiry := 3600 * time.Second | ||
if payReq.Expiry != nil { | ||
expiry = time.Duration(payReq.Expiry.Unix()) * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unix
returns of the number of seconds since January 1st 1970. time.Duration
itself defaults to units of ns
if it isn't scaled at all. As a result, I believe this computation should actually be:
time.Second(payReq.Expiry.Unix())
Otherwise, this will always scale up the expiration time in seconds by a few orders of magnitude.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This aside, I like the suggestion of instead natively moving the expiration validation within zpay32
itself (along with all the other invoice related validation).
This commit adds fields that are supported by the BOLT-11 invoice format to the Invoice and PayReq protos. These fields are timestamp, expiry, fallback address, description and description hash.
5b3eb9c
to
80b703b
Compare
This commit renames the invoice field Expiry to expiry, and changes the type from time.Time to time.Duration. Getting the value of the field will now have to be done using the getter Expiry(), which will also return the default invoice expiry (3600s) if it is not set explicitly by the the invoice.
This commit changes the rpcserver to rely on the new zpay32 package, and support the new payment request options available in the BOLT-11 invoice format.
The new BOLT-11 compliant zpay32 package offers a few new available options when creating invoices. This commit adds those options to lncli, such that callers can specify these when creating payment requests.
80b703b
to
ed8dd0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
This follows #268, which added the BOLT-11 parsing package. This PR moves this package into the
zpay32
namespace and makes use of it withinlnd
. These changes mainly affect thelncli
command line tool, and therpcserver
, in addition to the protos these use to do RPC.channeldb
A new field
PaymentRequest
is added to theInvoice
struct inchanneldb
, to allow easy listing of the payment request created when the invoice was added. Adding this field involved less changes than adding all the fields necessary to deterministically recreate the payment request.nodesigner
The
nodesigner
gets a new utility method for signing hashed data. This is needed to sign a single SHA-256 hash when creating the payment request, which was not supported before.lnrpc
New BOLT-11 fields such as
expiry
,timestamp
,fallback_addr
, anddescription_hash
added to the protos.rpcserver and lncli
Support for the proto changes and extra fields added with the new format.
Note: BOLT-11 payment requests contain a mandatory
description
field. We reuse thememo
argument already present inlncli
/lnrpc
for this purpose.