-
Notifications
You must be signed in to change notification settings - Fork 210
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
fix(client): missing expiry time #3955
Conversation
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.
Wooops.
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.
Maybe I need more coffee but since it had a bug and I still has trouble reading maybe we should either add a test, or figure out some way to make this code obviously correct.
let invoice_since_epoch = invoice.duration_since_epoch() + TOLERANCE; | ||
if now_since_epoch > invoice_since_epoch { | ||
let invoice_expiration_epoch = | ||
invoice.duration_since_epoch() + invoice.expiry_time() + TOLERANCE; |
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.
thought: So here we add "duration" + "time" + "tolerance (duration)", so what's confusing is that (I guess, expiry_time
is "duration", while duration_since_epoch
is actually "(absolute) time"). :D.
if now_since_epoch > invoice_since_epoch { | ||
let invoice_expiration_epoch = | ||
invoice.duration_since_epoch() + invoice.expiry_time() + TOLERANCE; | ||
if now_since_epoch > invoice_expiration_epoch { |
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.
thought: And now we call on3e thin _since_epoch
and the other _epoch
which also seems meh. :D
I guess we'll want a quick backport and release of this, as is makes all ln payments have 60s deadlines? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3955 +/- ##
==========================================
- Coverage 57.07% 57.03% -0.05%
==========================================
Files 193 193
Lines 43041 43042 +1
==========================================
- Hits 24566 24547 -19
- Misses 18475 18495 +20 ☔ View full report in Codecov by Sentry. |
let invoice_since_epoch = invoice.duration_since_epoch() + TOLERANCE; | ||
if now_since_epoch > invoice_since_epoch { | ||
let invoice_expiration_epoch = | ||
invoice.duration_since_epoch() + invoice.expiry_time() + TOLERANCE; |
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.
thought: BTW. If this 3 lines were in a method, than I think we could have a really tiny unit test that crafts a fake invoice and then checks against values against and after the timeout.
Successfully created backport PR for |
No description provided.