backend payment optimism #1195
backend payment optimism #1195
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
api/resolvers/wallet.js
Outdated
| await models.$executeRaw` | ||
| INSERT INTO pgboss.job (name, data, retrylimit, retrybackoff, priority) | ||
| VALUES ('checkInvoice', jsonb_build_object('hash', ${hash}), 21, true, 100)` | ||
| return await models.invoice.findFirst({ where: { hash } }) |
There was a problem hiding this comment.
Why not import checkInvoice and run it ourselves?
Like this, this means that the invoice isn't actually cancelled yet when the mutation returns.
Even the response would indicate that. So a hypothetical user interface where we have a cancel button next to the invoice state, the invoice state wouldn't update on cancel (since it's not cancelled yet).
We don't have this anywhere yet afaict but I considered being able to cancel deposits manually (instead of letting them expire) in the past.
Co-authored-by: ekzyis <[email protected]>
Co-authored-by: ekzyis <[email protected]>
Co-authored-by: ekzyis <[email protected]>
|
Excellent review! You caught a lot of good stuff |
components/use-item-submit.js
Outdated
| sub: item?.subName || sub?.name, | ||
| boost: boost ? Number(boost) : undefined, | ||
| bounty: bounty ? Number(bounty) : undefined, | ||
| maxBid: maxBid || Number(maxBid) === 0 ? Number(maxBid) : undefined, |
There was a problem hiding this comment.
I'm not pointing out potential operator precedence confusion in less critical code, only where it's really important and thus warrants a double-check.
| let { data, ...rest } = await mutate(innerOptions) | ||
|
|
||
| // use the most inner callbacks/options if they exist | ||
| const { onPaid, onPayError, forceWaitForPayment, persistOnNavigate } = { ...options, ...innerOptions } |
There was a problem hiding this comment.
is forceWaitForPayment still used? I don't see it passed in anywhere.
There was a problem hiding this comment.
No, I don't think so. It's another solution to the problem persistOnNavigate solves.
| })) | ||
| // block until the invoice to be marked as paid | ||
| // for pessimisitic actions, they won't show up on navigation until they are marked as paid | ||
| await invoiceWaiter.waitUntilPaid(invoice, inv => inv?.actionState === 'PAID') |
There was a problem hiding this comment.
I understand the reason for this but these different definitions of "paid" depending on context are confusing. In the context of LND, "paid" means "LND settled the incoming payment" while in the context of actions, "paid" means "action completed successfully".
Maybe that's an indicator we shouldn't name the action state PAID but SUCCESS or something?
There was a problem hiding this comment.
LND doesn't call it paid. It calls it settled and held. We call it paid and held.
If we zoom out, afaict the confusing thing is the name of this function here which suggests that paid has multiple meanings. I didn't want to rename this function initially, but I can rename it.
components/invoice.js
Outdated
| useEffect(() => { | ||
| if (!invoice) { | ||
| return | ||
| } | ||
| if (waitFor && waitFor(invoice)) { | ||
| onPayment?.(invoice) | ||
| } | ||
| setExpired(new Date(invoice.expiredAt) <= new Date()) | ||
| }, [invoice, onPayment, setExpired]) |
Co-authored-by: ekzyis <[email protected]>
This is functionally done - all known problems, deficiencies have been completed afaik. All that's left is review, cleanup, and testing.Fully ready to review and deploy.
Regression warning:
The most important parts of this PR:
api/paidAction- has all the paid actions in their own filesapi/paidAction/index.js- drives all the paid actions determining whether to use fee credits, optimism, or pessimismcomponents/use-paid-mutation.js- wrapsuseMutationto handle responses of paid actions and pay any invoices returned and giveusePaidMutationcallers callbacks about payment progressworker/paidAction.js- handles state transitions of paid actions and their invoicesThe biggest concerns with the PR:
To achieve an ideal non-custodial UX, we are doing what we're calling 2-stage optimism:
PENDINGstate, returning success, an invoice, and appearing as-if the payment is confirmed to the actorWe then transition the action state to
PAIDorFAILEDdepending on the payment state. When the payment succeeds, we run the action side effects and the action is visible to everyone. When the payment fails, we report the failure in the stacker's notifications where they can retry the payment.Doing this requires an overhaul to all paid actions. The requirements on paid actions are summarized in the following table:
This pr intends to implement backend optimism for all actions. We aim to take a relatively declarative approach to creating paid actions, where paid actions implement a standard interface so that paid actions aren't directly concerned with managing the payments. So far, what this looks like is paid actions are implemented in a separate file in the
paidActionfolder with an interface that defines the following functionality:getCost- returns the cost of the action given the action's arguments so that an invoice can be createdperform- inserts the action's records leaving them in aPENDINGstate with reference to the correspondinginvoiceIdonPaid- transitions the records fromperformto aPAIDstate and does any side effects (e.g. denormalization)A lot of care will need to be taken wrt atomicity of state transitions and the performance of making
PENDINGactions appearPAIDselectively.To avoid having to serialize state transitions, we are using optimistic concurrency control.
This PR is being completed in stages:
PENDINGactions expect if the actor is viewing themedit XX:XXusually is we can have payment status)[ ] freebies with attached wallets?invoicePaidAtqueryRawinput typespaidActionstesting
Because this is such a behemoth, we need to test:
read committedisolation modeparity
This has the biggest impact on
... because we no longer do these in
plpgsql, or for the aggregates, they need to filter forinvoiceActionState. For all these, it'll help to enumerate everything that the old code does and make sure it still works.intentions
@anon@anon@anontesting TODOs