search

stackernews / stacker.news

Internet communities that pay you Bitcoin
https://stacker.news
MIT License
423 stars 106 forks source link

Replace toasts - be more optimistic - prepaid and postpaid #1184

Closed ekzyis closed 4 months ago

ekzyis commented 4 months ago

Description

Using undocumented Apollo API for optimistic behavior for zaps, poll votes and bounty payments

Every action which does not need a database id in the client now updates the UI optimistically even if payment is required. This includes (and is limited to) zaps, poll votes and bounty payments. This leads to a not-custodial UX that is equal to the familiar speed of the custodial UX since all payment delays are moved to the background, out of view from the user. When the invoice was paid, the action is submitted to the server as usual with payment hash + hmac.

To support this, the form component now supports optimisticUpdate. When this property is passed a function, its return value will be passed to the following function which calls QueryManager.markOptimisticMutation and cache.removeOptimistic to manually trigger the optimistic cache update and its revert:

export function optimisticUpdate ({ mutation, variables, optimisticResponse, update }) {
  const { cache, queryManager } = getApolloClient()

  const mutationId = String(queryManager.mutationIdCounter)
  queryManager.markMutationOptimistic(optimisticResponse, {
    mutationId,
    document: mutation,
    variables,
    update
  })

  return () => cache.removeOptimistic(mutationId)
}

We call this function before triggering a payment. On payment or submit error, we rollback the optimistic update. The user is only informed in this case via notifications that are stored in window.localStorage.

During the changes, it became clear that the property invoiceable should actually be called prepaid. This makes it purpose more clear.

Optimistic replies and posts via post-paid support

For mutations where the id is required to update the UI in a sane way, the form component now supports a new property: postpaid.

If this property is passed, the form component will check the return value of the submit handler for an invoice. If one was found, it will try to pay it.

This allows us to use the server response to update the UI as fast as if it was paid via the custodial wallet. In the case a external payment is required, the item is inserted as "pending" which makes them only visible to the author and the server response will include an invoice. Since anons are not logged in and thus don't support this optimistic behavior, they don't have access to pending items but need to pay first. Therefore, forms and mutations support prepaid and postpaid simultaneously.

When the invoice is paid, we update the item status to active which makes it visible to everyone.

Based on and thus includes #1071 closes #849 closes #848 supersedes #1161.

TODO

Prepaid

Postpaid

TODO after review

Editting a pending comment made a modal popup telling me the payment failed. Is that expected?

When a top level comment failed, I clicked on the notification, then submitted the form to retry to the comment, it succeeded but the comment field did not clear

Videos

TBD

Additional Context

Checklist

Are your changes backwards compatible? Please answer below:

Did you QA this? Could we deploy this straight to production? Please answer below:

For frontend changes: Tested on mobile? Please answer below:

Did you introduce any new environment variables? If so, call them out explicitly here:

coderabbitai[bot] commented 4 months ago

Walkthrough

The changes span across multiple files, focusing on enhancing the payment processing capabilities, refining the user experience for pending payments, and optimizing the handling of external payments. Key additions include new hooks for managing invoices and payments, updates to form components to handle query parameters and invoicing, and modifications to various components to support optimistic UI updates for pending payments. The database schema and worker functions have also been updated to support these changes.

Changes

Files/Path Change Summary
components/payment.js Added hooks for managing invoices, WebLN payments, and QR payments.
components/poll-form.js Added query parameters handling, updated upsertPoll mutation, and adjusted initial form values.
components/poll.js Updated imports to use new payment hooks and added toast notifications.
components/qr.js Simplified payment logic.
components/reply.js Added query parameters for pre-filling and expanded GraphQL query for invoices.
components/territory-form.js, components/territory-payment-due.js, pages/rewards/index.js Changed invoiceable prop to prepaid.
components/toast.js, components/toast.module.css Removed unused functions and styles, updated toast handling logic.
components/upvote.js, components/upvote.module.css Updated ItemAct component calls, made handleShortPress async, and added CSS for pending state.
components/use-crossposter.js Removed nested function, imported determineItemType from @/lib/item.
components/webln/index.js, components/webln/nwc.js Removed unused imports, added constants and errors, refactored context value creation.
fragments/notifications.js Added FailedItem fragment with additional fields.
lib/apollo.js Added optimisticUpdate function.
lib/constants.js Added constants JIT_INVOICE_TIMEOUT_MS, DEFAULT_INVOICE_TIMEOUT_MS, and ZAP_UNDO_DELAY_MS.
lib/item.js Introduced determineItemType function.
pages/_app.js Added ClientNotificationProvider before LoggerProvider.
pages/api/lnurlp/[username]/pay.js Modified create_invoice call with additional parameters.
pages/invoices/[id].js Changed Invoice component import to default import.
pages/settings/index.js Added ZAP_UNDO_DELAY_MS to INVOICE_RETENTION_DAYS and updated button display duration.
prisma/migrations/.../migration.sql Added enum types, columns, and functions for handling invoices and items.
worker/action.js, worker/index.js, worker/wallet.js Added functions for handling actions related to invoices and items, updated handling of canceled invoices.
api/resolvers/user.js Added statusClause to various query clauses.

Assessment against linked issues

Objective Addressed Explanation
Refine external pending payment UX (#849) ❓ The changes improve the UX for pending payments, but it is unclear if they fully replace toasts as suggested.
Make actions dependent on external payments complete optimistically (#848) ✅ The changes support optimistic UI updates for pending payments and store action data on the server.
Handle payment failures and retries (#848) ✅ The system now supports handling payment failures and retries through notifications and updated database functions.

[!TIP]

Early Access Features - `gpt-4o` model for chat

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to full the review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
ekzyis commented 4 months ago

Regarding persistence of optimistic updates for prepaid stuff: I noticed we really don't need it. In 8e6f7bac, I (re)implemented client notifications and realized that if something is still pending when we reload the page, we can immediately show it as failed since this means the payment flow was interrupted. So there is never the case of something being pending but the UI not showing this.

huumn commented 4 months ago

The zapping experience seems to work really well. I didn't notice an issue there yet or encounter an error. It appears to just work.

Things noticed after a few minutes of QA:

  1. I boosted a post and it's stuck as PENDING
  2. When a top level comment failed, I clicked on the notification, then submitted the form to retry to the comment, it succeeded but the comment field did not clear
  3. Editting a pending comment made a modal popup telling me the payment failed. Is that expected?
  4. When I have an autowithdrawal method attached, paying for things races with autowithdraw (after I paid for the post in (1), it autowithdrew 51 sats)

(2, 3, 4) aren't really regressions or "big deals" but (1) is pretty nasty as it looks to the person that created it that it succeeded but it didn't and it's unclear what they should do now. I'd guess there's a problem in the failure finalize code but I don't know. It might be easier to recreate if you add an autowithdraw method.

That's after just a few minutes. There's a lot going on that's changed in this code, so it's hard for me to know what is causing what.

I'll rebase #1178 on this and begin mending them/refactoring together in the meantime. I'll let you know if anything comes up.

Edit: it looks like the payment in (1) was confirmed but the Item is still pending.

stackernews=# select * from "Invoice" where "actionId" = 459394;
   id   |       created_at        |       updated_at        | userId |                               hash                               |                        
                                                                                                                              bolt11                             
                                                                                                                          |       expiresAt        |       confir
medAt       | cancelled | msatsRequested | msatsReceived |             desc              | preimage | isHeld | comment | lud18Data | confirmedIndex | actionId | 
actionType |           actionData            
--------+-------------------------+-------------------------+--------+------------------------------------------------------------------+------------------------
-----------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------+------------------------+-------------
------------+-----------+----------------+---------------+-------------------------------+----------+--------+---------+-----------+----------------+----------+-
-----------+---------------------------------
 159722 | 2024-05-23 19:38:43.926 | 2024-05-23 19:38:57.794 |    616 | 1918ea20fa668e98a6b87943d168369b3334f2164c0bf01aa35e8d4a523520b4 | lnbcrt510n1pnylx6rpp5ry
vw5g86v68f3f4c09paz6pknvenfuskfs9lqx4rt6x55534yz6qdp0gdex2ct5d9hxwgrfw3jk6gr0dcs8xarpvd4k2u3wdejhwuccqzzsxqzftsp595gdhx2smesau4l3ea4sq7muvf5725pugdfr09fsk8ymnfzf
azhq9qyyssqc6jrd4e50t54gkzf3hqw2v28xkqz8vu25gp35cq4at5x3t0n4y69hdpc348rgppqfkyvl9tr73nftqrl8h5jp6qzarxxgnqq6ul902cp3sq0lp | 2024-05-23 19:43:43.91 | 2024-05-23 1
9:38:57.599 | f         |          51000 |         51000 | Creating item on stacker.news |          |        |         |           |             43 |   459394 | 
ITEM       | {"cost": 51000, "credits": 510}
(1 row)

stackernews=# select * from "Item" where status <> 'ACTIVE';
   id   |       created_at        |       updated_at        |   title   |              text               | url | userId | parentId |     path      | pinId | lat
itude | location | longitude | maxBid | maxSalary | minSalary | remote | subName | statusUpdatedAt | status  | company | weightedVotes | boost | uploadId | pollC
ost | paidImgLink | commentMsats | lastCommentAt | ncomments | msats | weightedDownVotes | bio | freebie | deletedAt | otsFile | otsHash | bounty | rootId | boun
tyPaidTo | upvotes | weightedComments | imgproxyUrls | noteId | outlawed | pollExpiresAt | lastZapAt 
--------+-------------------------+-------------------------+-----------+---------------------------------+-----+--------+----------+---------------+-------+----
------+----------+-----------+--------+-----------+-----------+--------+---------+-----------------+---------+---------+---------------+-------+----------+------
----+-------------+--------------+---------------+-----------+-------+-------------------+-----+---------+-----------+---------+---------+--------+--------+-----
---------+---------+------------------+--------------+--------+----------+---------------+-----------
 459389 | 2024-05-23 19:29:39.594 | 2024-05-23 19:29:51.687 |           | fdasjldjlajlfdasfdsaeaefwa33333 |     |    616 |   458433 | 458433.459389 |       |    
      |          |           |        |           |           |        |         |                 | FAILED  |         |             0 |     0 |          |      
    | f           |            0 |               |         0 |     0 |                 0 | f   | f       |           |         |         |        | 458433 |     
         |       0 |                0 | {}           |        | f        |               | 
 459394 | 2024-05-23 19:38:43.926 | 2024-05-23 19:38:49.129 | reqwwfasd | fdsafdsafdsa                    |     |    616 |          | 459394        |       |    
      |          |           |        |           |           |        | bitcoin |                 | PENDING |         |             0 | 50000 |          |      
    | f           |            0 |               |         0 |     0 |                 0 | f   | f       |           |         |         |        |        |     
         |       0 |                0 | {}           |        | f        |               | 
(2 rows)
huumn commented 4 months ago

Thinking through my own changes in #1178 alongside this makes me think we are going to need a refactor to make our planned changes robust.

If we solve the problem of (1) in a robust way and we can reason through the robustness of all the states of payments<->actions in this pr, we can ship this before the refactor. Bugs like this are evidence the code has become too much to handle.

ekzyis commented 4 months ago
  1. When a top level comment failed, I clicked on the notification, then submitted the form to retry to the comment, it succeeded but the comment field did not clear

Sounds like a missing resetForm call, should be easy to fix

  1. Editting a pending comment made a modal popup telling me the payment failed. Is that expected?

No :eyes: I actually didn't test this, since I didn't change how items are updated and edits should have no cost associated to them so it shouldn't trigger any payment code. But will test now

  1. I boosted a post and it's stuck as PENDING

  2. When I have an autowithdrawal method attached, paying for things races with autowithdraw (after I paid for the post in (1), it autowithdrew 51 sats)

If we solve the problem of (1) in a robust way and we can reason through the robustness of all the states of payments<->actions in this pr, we can ship this before the refactor.

One thing I wanted to do (and I think you also mentioned this) was to run the action in the same tx as the invoice gets confirmed. Currently, it's not since the state transition is not idempotent due to item side effects so I used the return value of confirm_invoice to see if it's the first confirmation.

Running both actions in the same tx should fix the race condition with autowithdrawals. I could do this by creating a postgres function that runs the code in handleAction since then I can rely on serialized transactions to check if it's the first confirmation (just like confirm_invoice).

We don't run into this problem with prepaid because the hash+hmac approach does that: creating and thus paying for the item is in the same tx as confirming the invoice.

Edit: it looks like the payment in (1) was confirmed but the Item is still pending.

There is a worker job which runs finalizeAction after the invoice is expired. It's there to make sure all state transitions are guaranteed to happen eventually (when the invoice expires). However, if the state transition to ACTIVE also fails there, it will be pending forever. That's a bug, I should just mark the item as FAILED then.

So I think the most important remaining TODO here is to create handle_action as a postgres function that we can run in the same tx as confirm_invoice. This should fix conflicts with autowithdrawals and thus 1) and 4) (and possibly other payment problems)

huumn commented 4 months ago

FYI any of the code here might end up getting completely rewritten in the refactor and we need to do the refactor soon. All of our payment stuff was complicated before these changes we are making, and now it's even more so (ie there are three ways to pay for something now vs two). The code on the frontend got a lot better, but the backend code got a lot worse.

Just wanted to caution against getting too attached to any particular bug fix when we really need a systemic fix. Still, happy to ship this once it's deterministic and bug-free.

For a hint of how complicated this is, here's the abstraction requirements table from my notes:

anonable optimistic qr payable p2p wrapped side effects fee credits payable
posts x x x x x
comments x x x x x
zaps x x x x x x
downzaps x x x x
poll votes x x x
territory purchases x x
donations x x x x
update posts x? x x x
update comments x? x x x
ekzyis commented 4 months ago

I realized that fixing edits of pending items would also require changes to update_item (a similar flow as with create_item essentially) that will be overwritten by the systemic fix we discussed.

However, I think even the systemic fix we discussed wouldn't handle edits properly. Edits of pending items means that we now potentially have two invoices that could be paid separately but they require payment in order since it doesn't make sense to pay for an edit when the item itself wasn't paid yet and thus could still fail. Additionally, edits are the only actions that we have that mutate data instead of creating a row that we could mark as pending.

I see a few ways to approach this problem:

1) edits of pending items aren't allowed. you must first pay for the item before you can edit it. an edit that wasn't paid yet marks the item as pending again. 2) items never fail but can always be paid later via a new invoice 3) edits cancel the existing invoice for the pending item and create a new invoice that pays for the item + edit

(or a combination of them; 2) is something we discussed already iirc)

However, as mentioned, I consider this to be better done in the systemic fix instead of me messing with the backend more in this PR to get pending items working without bugs.

So I decided to indeed create a PR with only the frontend changes. I think we should merge them even if we'll also move zaps etc. to the server as pending acts since they include important changes to cleanup frontend code (removal of toast flows). They also already improve UX even if there are only few stackers who use attached wallets like me.

ekzyis commented 4 months ago

Superseded by #1194 and #1195