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.

Sign up for GitHub

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

Jump to bottom

LN payment flow hardening #1892

Open
7 of 11 tasks
danieldaquino opened this issue Jan 19, 2024 · 7 comments
Open
7 of 11 tasks

LN payment flow hardening #1892

danieldaquino opened this issue Jan 19, 2024 · 7 comments
Labels
purple Damus purple membership technical
Milestone
"Madeira" AppStor...

Comments

@danieldaquino
Copy link
Contributor

danieldaquino commented Jan 19, 2024 •
edited
Loading

This is a placeholder task for further improvements/hardening of the LN payment flow.

Continuation of #1754 and #1827

Acceptance criteria

  • Add unit tests to damus-api that covers the LN flow as well as edge cases
  • Add [email protected] email on purple website and checkout
  • Lightning flow: Better integrate lightning expiry into the code as a mechanism to prevent stale/unmonitored invoices and improve robustness
  • Make sure expiry bumping logic handles edge cases
  • Save checkout objects into a DB for better crash resistance
  • Add more timestamps and data to the DBs, as well as server logs to help us diagnose problems and manually fix things in case issues arise
  • LN flow: Handle edge cases better (e.g. How does the UI display an expired invoice? How does the UI handle when the server is having issues? Does it displays errors nicely?)
  • Add more guidance on the UI for certain edge cases (e.g. What if the user is running an incompatible version of Damus and the Verify npub link does not work? There should be some message in the website mentioning that if the link fails they might need to update the app)
  • Better safeguards against unauthorized use of damus:purple links
  • Add monitoring for oom? issues @jb55 Checkinvoice API codepath status LN node waitinvoice. Call over LNsocket. If issue, send notification.
  • Add special endpoint for staging server to allow an account to be reset (for testing purposes)
  • On iOS, make "Verify Npub" screen raise a session flag indicating that a checkout is in progress, and detect if account was created when app enters foreground, to show welcome sheet right away Moved to Handle LN checkout cases where user does not click on "continue on app" in the last step #2021

Note: This ticket is generic, it can be split into more tickets if convenient.
@danieldaquino danieldaquino added the purple Damus purple membership label Jan 19, 2024
@danieldaquino danieldaquino mentioned this issue Jan 19, 2024
Closed
16 tasks
@danieldaquino danieldaquino changed the title LN payment flow hardening (Placeholder) LN payment flow hardening Jan 22, 2024
@danieldaquino danieldaquino mentioned this issue Jan 22, 2024
Closed
2 tasks
@danieldaquino
Copy link
Contributor Author

@jb55, since the recent damus-api changes might still be fresh in your mind, what do you think of the acceptance criteria here? Any other suggestions on things we can do to improve robustness?

To save you a trip to Github, I will list them here so you can stay in your email client lol:

  • Add unit tests to damus-api that covers the LN flow as well as edge cases
  • Add [email protected] email on purple website and checkout
  • Lightning flow: Better integrate lightning expiry into the code as a mechanism to prevent stale/unmonitored invoices and improve robustness
  • Make sure expiry bumping logic handles edge cases
  • Save checkout objects into lmdb for better crash resistance
  • Add more timestamps and data to the DBs, as well as server logs to help us diagnose problems and manually fix things in case issues arise
  • LN flow: Handle edge cases better (e.g. How does the UI display an expired invoice? How does the UI handle when the server is having issues? Does it displays errors nicely?)
  • Add more guidance messages on the UI for certain edge cases (e.g. What if the user is running an incompatible version of Damus and the Verify npub link does not work? There should be some message in the website mentioning that if the link fails they might need to update the app)
  • Better safeguards against unauthorized use of damus:purple links

@danieldaquino
Copy link
Contributor Author

Save checkout objects into a DB for better crash resistance

Patch sent: https://groups.google.com/a/damus.io/g/patches/c/eT2BINfOH0Q

@danieldaquino
Copy link
Contributor Author

@jb55, @alltheseas, I wrote some code to better handle checkout errors (if they occur)

I went on each function, each step of the checkout process, and added error handlers with a detailed message.

  • Each message contains tailored action tips for the user on what to do if they encounter such error
  • The popup shows the user info on how to contact support
  • The popup gives users the checkout reference code that they can copy and save, to facilitate troubleshooting for us

Here is one example:

Screenshot 2024-01-30 at 17 09 02

@danieldaquino
Copy link
Contributor Author

@jb55, don't know if this is the right ticket to send this, but here is a preview of the discount label I just wrote (I did not send this patch yet)

Screenshot 2024-01-30 at 18 17 44

@alltheseas
Copy link
Collaborator

Figure out which are critical items

@alltheseas
Copy link
Collaborator

What should expiry invoice duration be? @danieldaquino suggests short invoice, so LN node does not have to keep checking status for invoices.

Core LN default is one (1) week.

There remains one unsolved checkout/subscription request.

@danieldaquino
Copy link
Contributor Author

On iOS, make "Verify Npub" screen raise a session flag indicating that a checkout is in progress, and detect if account was created when app enters foreground, to show welcome sheet right away

Moved this to #2021

jb55 pushed a commit that referenced this issue Feb 26, 2024
@danieldaquino @jb55
purple: show welcome sheet after ln payment
bdc811a 
Automatically show welcome sheet for people who completed a Lightning
checkout but did not click on the "Continue" button

Some people get confused in the last step of the lightning checkout and
they do not click on the "Continue in app", which causes the welcome
screen to not show up and the translation setting to not be setup.

This ticket addresses this issue to ensure they get the welcome screen
in such cases.

This is how it works:

- When they go through the mandatory checkout verification step, the
  verify screen registers that there is a checkout in progress (and
  which checkout is in progress) on the DamusPurple struct

- Every time the app enters the foreground, it checks for that flag:

  - If there are no checkouts in progress, it does nothing

  - If there is one or multiple checkouts in progress, it checks the
    checkout status for those. If any checkout is freshly completed and
    successful, and the account is now active, it shows the welcome
    screen and onboarding

- Once the welcome screen is shown, those checkouts are marked
  internally as handled (so that we only show it once)

===========
Testing
===========

PASS

Damus: This commit
Device: iPhone 13 Mini (real physical device)
iOS: 17.3.1
damus-api: 044150fedddc5ba4135a80579e41e9c1c5743fc0
damus-website: af8089128159e25df31141be624b4090c66d6ddf
Setup:
- Local test Setup
- Clean db before starting

PART 1: LN flow without clicking on the link
---------------------------------------------

Steps:

1. Go through the normal LN flow, EXCEPT at the last step. On the last
   step, instead of clicking "continue in the app", just switch to the
   app.

2. Ensure the welcome screen and onboarding shows up, and the purple
   screen updates to show account info. PASS

3. Switch out and into the app again. Welcome screen should NOT show up
   again. PASS

4. Close the app completely and open the app again. Purple welcome
   screen should NOT show up again. PASS

PART 2: Normal LN flow
---------------------------------------------
Steps:

1. Reset the entire test setup

2. Go through the normal LN flow (this time click on "continue in app").
   The welcome screen should show up without issues or glitches. PASS

PART 3: Crazy flow with multiple incomplete checkouts
---------------------------------------------
(This is to simulate a confused user who accidentally opens multiple new
checkouts, but in the end only completes one of them)

Steps:
1. Reset the entire test setup

2. Open 5 LN checkouts and verify npub with all of them.

3. Only pay one of those checkouts (to make it more confusing, don't pay
   the first or last one, but a random one in between)

4. Go to the app directly (Do not use the link, just go directly to the app)

Related: #1892
Changelog-Fixed: Fix welcome screen not showing if the user enters the app directly after a successful checkout without going through the link
Closes: #2021
Signed-off-by: Daniel D’Aquino <[email protected]>
Link: [email protected]
Signed-off-by: William Casarin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
purple Damus purple membership technical
Projects
None yet
Milestone
"Madeira" AppStore 1.7 purple follow ...
Development

No branches or pull requests
2 participants
@danieldaquino @alltheseas