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

Invoice: Improve expiry display #1534

Merged
merged 2 commits into from Dec 30, 2023

Conversation

myxmaster
Copy link
Contributor

@myxmaster myxmaster commented Jul 2, 2023

Description

Instead of displaying the initially defined time until expiration, the remaining time is now displayed in a human readable form. Also made the layout more consistent by putting labels on the left and values on the right.

grafik

grafik

Before:

grafik

grafik

This fixes #1509.

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Configuration change
  • Locales update
  • Quality assurance
  • Other

Checklist

  • I’ve run yarn run tsc and made sure my code compiles correctly
  • I’ve run yarn run lint and made sure my code didn’t contain any problematic patterns
  • I’ve run yarn run prettier and made sure my code is formatted correctly
  • I’ve run yarn run test and made sure all of the tests pass

Testing

If you modified or added a utility file, did you add new unit tests?

  • No, I’m a fool
  • Yes
  • N/A

I have tested this PR on the following platforms (please specify OS version and phone model/VM):

  • Android
  • iOS

I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):

  • LND (REST)
  • LND (Lightning Node Connect)
  • Core Lightning (c-lightning-REST)
  • Core Lightning (Spark)
  • Eclair
  • LndHub

Locales

  • I’ve added new locale text that requires translations
  • I’m aware that new translations should be made on the Zeus Transfix page and not directly to this repo

Third Party Dependencies and Packages

  • Contributors will need to run yarn after this PR is merged in
  • 3rd party dependencies have been modified:
    • verify that package.json and yarn.lock have been properly updated
    • verify that dependencies are installed for both iOS and Android platforms

Other:

  • Changes were made that require an update to the README
  • Changes were made that require an update to onboarding

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2023-08-03 at 8 29 20 PM
We are getting expiration time as 0 seconds in LNDHub. I am using Alby instance in this one

@kaloudis
Copy link
Contributor

kaloudis commented Aug 4, 2023

Screenshot 2023-08-03 at 8 29 20 PM We are getting expiration time as 0 seconds in LNDHub. I am using Alby instance in this one

Yeah, as suspected we need to test this against all the backends

@myxmaster
Copy link
Contributor Author

@shubhamkmr04 Kind of fixing this blindly, please check if it's working now.

@kaloudis
Copy link
Contributor

kaloudis commented Sep 6, 2023

@myxmaster can you fix the conflicts here?

This PR will pave the way for this to work with LNDHub and all the other backends #1652

@myxmaster
Copy link
Contributor Author

It works now... Maybe something was changed?

grafik

@kaloudis kaloudis changed the title Improve expiry display Invoice: Improve expiry display Sep 7, 2023
@kaloudis
Copy link
Contributor

kaloudis commented Oct 1, 2023

Instead of saying 4 days, 2 hours and it refers to the time to expiration and not the absolute time it is valid for, perhaps it should say 4 days, 2 hours from now

@myxmaster
Copy link
Contributor Author

myxmaster commented Oct 1, 2023

I understand you want to make clear that it is the time as of now and not as of invoice creation time.

The problem is, I can already see trouble for german translation with your suggestion, plus I like to have 'values only' on the right (its very clear structured / easy to read).

What about making it clear on the left, where we have text already:
Time until expiry 4 days, 2 hours

Edit:
Or maybe even better: lets put an explainer behind "Expiration"!

@myxmaster myxmaster force-pushed the improve-expiry-display branch 2 times, most recently from bc7ac62 to 788c8c6 Compare October 24, 2023 21:29
@myxmaster
Copy link
Contributor Author

myxmaster commented Oct 24, 2023

  • Renamed "Expiration" --> "Time until Expiry" as discussed with @kaloudis
  • Improved activity layout by putting expiry time to the bottom (lines up with left side now) and displaying memo in italic (easier to distinguish from other context)
    grafik

Invoice details displays both now, "Time until Expiry" and "Original Expiration":
grafik

As you can see in the screenshot above, there is just one last issue with the calculation of expiry time: 53 years is not correct :) This happens when using LNDhub (via lnbits); LND, CLN, getAlby is fine.
I think this is because of this (from PR #1652):

if (expiry && new BigNumber(expiry).gte(1600000000)) {

@myxmaster myxmaster marked this pull request as draft October 24, 2023 21:40
@kaloudis
Copy link
Contributor

@myxmaster what's the raw formatting of that timestamp above?

@myxmaster
Copy link
Contributor Author

myxmaster commented Oct 25, 2023

A fresh lnbits invoice created from Zeus while connected via LNDhub extension has "expiry = 1698222471".
When I create an invoice in the web interface of legend.lnbits.com, and scan it with Zeus, it displays "600".

Current unix time is ~1698222471, that is the problem (https://www.unixtimestamp.com/). So we have to make clear cases for which Backends use unix time and which expiry in seconds.

@myxmaster myxmaster force-pushed the improve-expiry-display branch 2 times, most recently from cb920b0 to 4e75cec Compare November 15, 2023 21:36
@myxmaster
Copy link
Contributor Author

myxmaster commented Nov 15, 2023

We now take the expiry and timestamp from the bolt11 payment request as the format is standardized - with one exception: Core Lightning (expiry is missing in payment request here).

Now displaying original expiration also as time span instead of timestamp:

grafik

@myxmaster myxmaster marked this pull request as ready for review November 15, 2023 21:39
@@ -0,0 +1,223 @@
// based on light-bolt11-decoder: Copyright (c) 2021 bitcoinjs contributors, fiatjaf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what enhancements have been made here that make it faster than the original library?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the docs of the lib:

It doesn't recover payee from signature, doesn't check signature, doesn't parse fallback addresses and doesn't do any encoding

Since the signature wasn't checked before, this is no security downgrade. And checking the signature worsens the performance so much (about 20 times) that the usability would not be acceptable.

Furthermore we removed almost all the parts we don't need for this PR.

@kaloudis kaloudis added this to the v0.8.1 milestone Nov 28, 2023
@kaloudis
Copy link
Contributor

Looking good, tested on LND and CLN so far.

Do we need to display expiry on paid invoices?

@kaloudis
Copy link
Contributor

Screenshot 2023-12-22 at 11 21 44

Furthermore, on an expired request, maybe we remove the third line and replace the title with 'Expired request' or 'Expired invoice'?

@shubhamkmr04
Copy link
Contributor

Tested for LND and LNDHub in both iOS and Android. Working fine.

@myxmaster
Copy link
Contributor Author

Good ideas, done. Displaying original expiry for paid invoices works for LND and CLN but does not work with lndhub.

grafik

grafik

Copy link
Contributor

@kaloudis kaloudis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

@kaloudis kaloudis merged commit 62396da into ZeusLN:master Dec 30, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display expiry time of invoices in minutes, hours or days if useful
3 participants