Replace toasts - be more optimistic - prepaid and postpaid #1184
Conversation
WalkthroughThe 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
Assessment against linked issues
Tip Early Access Features
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
components/invoice.js (1)
63-67: HandleWebLnNotEnabledErrorspecifically to improve user feedback on payment failures.Consider enhancing the error message to provide more actionable information to the user.
ekzyis
force-pushed
the
wallet-ux-2
branch
2 times, most recently
from
b6f62a0 to
3988a00
Compare
worker/wallet.js
Outdated
| await models.$transaction(async (tx) => { | ||
| const item = await tx.item.update({ | ||
| where: { id: itemId }, | ||
| data: { status: 'ACTIVE' } | ||
| }) | ||
|
|
||
| await tx.user.update({ | ||
| where: { | ||
| id: item.userId | ||
| }, | ||
| data: { | ||
| msats: { | ||
| decrement: cost - msatsReceived - locked | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| // run skipped item queries | ||
| await tx.itemAct.create({ | ||
| data: { msats: cost, itemId: item.id, userId: item.userId, act: 'FEE' } | ||
| }) | ||
| if (item.boost > 0) { | ||
| await tx.$executeRaw(`SELECT item_act(${item.id}::INTEGER, ${item.userId}::INTEGER, 'BOOST'::"ItemActType", ${item.boost}::INTEGER)`) | ||
| } | ||
| if (item.maxBid) { | ||
| await tx.$executeRaw(`SELECT run_auction(${item.id}::INTEGER);`) | ||
| } | ||
| }, { isolationLevel: Prisma.TransactionIsolationLevel.Serializable }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Just a heads up (I know this is in draft and this might not be what you intend to ship): worst case, this is 5 roundtrips to the db (because it's interactive and not pipelined), and if during that time any of the modified rows are updated by another transaction, this and the other tx will fail.
In general, the longer a serializable tx takes to run, the more likely the failure. Everywhere in the code so far, we only do one roundtrip for serializable txs (and we wrap them all in a retry in serializable)
There was a problem hiding this comment.
worst case, this is 5 roundtrips to the db (because it's interactive and not pipelined) [...] Everywhere in the code so far, we only do one roundtrip for serializable txs (and we wrap them all in a retry in serializable)
Oh, I didn't realize that the serialize calls with an array of queries weren't multiple round trips.
But I agree with you, roundtrips are bad, especially when it's in a serialized tx. I didn't think too much about it here for now, it was just too convenient to use Prisma's API.
But I noticed now that the only reason I use an interactive tx was to fetch the item author. I can do this outside of the tx and then use serialize.
| -- here, we immediately deduct as many of the sats that are required for the payment | ||
| -- to effectively "lock" them for it. the remainder will be paid via invoice. | ||
| -- if the payment fails, we release the locked sats by adding them to the balance again. | ||
| UPDATE users SET msats = GREATEST(msats - cost_msats - item.boost, 0) WHERE id = item."userId"; |
There was a problem hiding this comment.
This pr is a big lift even without this partial payment change, but you might have this working well enough it's okay to add. But if it is overwhelming getting this extra done, I'd recommend postponing the partial payments until the primary objective of this PR is pristine.
It's usually a lot easier to add extras when the foundation is settled.
There was a problem hiding this comment.
Mhh yes, but so far, I think it's only a matter of adding them back in case of failure which I have to handle anyway. So let's see
There was a problem hiding this comment.
Ok, since deducting user balance is not idempotent and even though LND subscriptions don't trigger on invoice expiration (so idempotency shouldn't be necessary), I removed partial payments in 2c1807d to be sure.
|
Regarding persistence of optimistic updates for prepaid stuff: I noticed we really don't need it. In 8e6f7ba, 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. |
| <SubmitButton variant={down ? 'danger' : 'success'} className='ms-auto mt-1 px-4' value='TIP'>{down && 'down'}zap</SubmitButton> | ||
| </div> | ||
| </Form> | ||
| <ClientNotifyProvider additionalProps={{ itemId: item.id }}> |
There was a problem hiding this comment.
I hope I'll still like this approach to pass something to notify that the form component can't down the road
Can't tell yet if this is simple clever or confusing clever
There was a problem hiding this comment.
Actionable comments posted: 18
Outside diff range and nitpick comments (9)
worker/wallet.js (2)
69-70: Clarify the comment regarding invoice notifications.The comment about
SubscribeToInvoicesonly handling invoice creation and settlement transitions could be expanded to explain the implications more clearly. Consider adding examples or scenarios where this limitation might affect the application's behavior.
247-248: Clarify the handling of withdrawal confirmations.The handling of the first confirmation for withdrawals could use more detailed logging or comments to explain what happens when
code === 0and why this condition is important.api/resolvers/notifications.js (1)
154-155: Ensure proper indexing for notification queries.Given the complexity and potential size of the data involved in the notification queries, ensure that the database is properly indexed on columns used in
WHEREclauses to optimize performance.components/notifications.js (2)
Line range hint
77-95: TheNotificationLayoutcomponent is well-implemented with appropriate routing logic. Consider adding a comment explaining the purpose of theoverrideLinksstyle application for clarity.+ // Apply zIndex style to override links when necessary style={overrideLinks ? { zIndex: 1 } : undefined}
Line range hint
551-617: TheNotificationAlertcomponent is well-crafted, effectively managing notification settings and user preferences. Consider adding detailed comments explaining the logic within theuseEffecthooks for better maintainability.+ // Check support for service worker features and manage notification settings useEffect(() => { const isSupported = sw.support.serviceWorker && sw.support.pushManager && sw.support.notification ... }, [sw])api/resolvers/wallet.js (3)
16-16: Consider enhancing the error message "not ur invoice" to be more professional and informative, such as "You do not have permission to access this invoice."
331-331: Improve the clarity and detail of error messages in thecreateWithdrawalfunction to enhance user understanding and troubleshooting.
[!TIP]
Codebase VerificationThe
cancelInvoicefunction inapi/resolvers/wallet.jsdoes handle HMAC validation securely. The function generates an HMAC using thecreateHmacfunction and compares it with the provided HMAC. If they do not match, it throws aGraphQLErrorwith aFORBIDDENcode.
- The
createHmacfunction is defined in the same file and uses thecrypto.createHmacmethod with a SHA-256 algorithm and a key derived from an environment variable.- The comparison is done using a simple equality check, which might be susceptible to timing attacks. However, in
api/resolvers/serial.js, a more securetimingSafeEqualmethod is used for HMAC comparison.To enhance security, consider using
timingSafeEqualfor HMAC comparison in thecancelInvoicefunction as well.
- Update the HMAC comparison in
cancelInvoiceto usetimingSafeEqual.Category:
Analysis chain
Line range hint
384-395: Ensure the security and correctness of HMAC generation and comparison in thecancelInvoicefunction to prevent unauthorized access.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify HMAC security in `cancelInvoice` function. # Test: Search for HMAC usage in the function. Expect: Secure HMAC generation and comparison. ast-grep --lang javascript --pattern $'function cancelInvoice($_, $_, $_) { $$$ const hmac2 = createHmac($_) if (hmac !== hmac2) { $$$ } $$$ }'Length of output: 158
Script:
#!/bin/bash # Description: Locate and inspect the `cancelInvoice` function for HMAC handling. # Step 1: Locate the `cancelInvoice` function definition. ast-grep --lang javascript --pattern 'function cancelInvoice($_, $_, $_) { $$$ }' # Step 2: Search for HMAC usage within the located function. rg 'createHmac' -A 5 -B 5Length of output: 5558
pages/settings/index.js (1)
1010-1010: Clarify the behavior of theZAP_UNDO_DELAY_MSconstant.The comment on line 1010 mentions that the undo button is shown for a specific duration based on
ZAP_UNDO_DELAY_MS. It would be helpful to add a comment explaining whereZAP_UNDO_DELAY_MSis defined and its typical value to make the code more understandable for new developers or during maintenance.
| let filter = 'AND "Item"."status" <> \'FAILED\'' | ||
| if (me) { | ||
| filter += `AND ("Item"."userId" = ${me.id} OR "Item"."status" <> 'PENDING')` | ||
| const [{ item_comments_zaprank_with_me: comments }] = await models.$queryRawUnsafe( | ||
| 'SELECT item_comments_zaprank_with_me($1::INTEGER, $2::INTEGER, $3::INTEGER, $4::INTEGER, $5, $6)', Number(id), GLOBAL_SEED, Number(me.id), COMMENT_DEPTH_LIMIT, filter, orderBy) | ||
| return comments | ||
| } | ||
|
|
||
| filter += 'AND "Item"."status" <> \'PENDING\'' |
There was a problem hiding this comment.
Optimize the SQL query to prevent potential performance issues with large datasets.
Consider using more efficient SQL constructs or adding appropriate indexes to improve the performance of this query.
| const setItemMeAnonSats = ({ id, amount }) => { | ||
| const storageKey = `TIP-item:${id}` | ||
| const existingAmount = Number(window.localStorage.getItem(storageKey) || '0') | ||
| window.localStorage.setItem(storageKey, existingAmount + amount) |
There was a problem hiding this comment.
Ensure proper error handling and validation for local storage operations.
const setItemMeAnonSats = ({ id, amount }) => {
const storageKey = `TIP-item:${id}`
const existingAmount = Number(window.localStorage.getItem(storageKey) || '0')
+ if (isNaN(existingAmount)) {
+ throw new Error('Invalid amount in local storage');
+ }
window.localStorage.setItem(storageKey, existingAmount + amount)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const setItemMeAnonSats = ({ id, amount }) => { | |
| const storageKey = `TIP-item:${id}` | |
| const existingAmount = Number(window.localStorage.getItem(storageKey) || '0') | |
| window.localStorage.setItem(storageKey, existingAmount + amount) | |
| const setItemMeAnonSats = ({ id, amount }) => { | |
| const storageKey = `TIP-item:${id}` | |
| const existingAmount = Number(window.localStorage.getItem(storageKey) || '0') | |
| if (isNaN(existingAmount)) { | |
| throw new Error('Invalid amount in local storage'); | |
| } | |
| window.localStorage.setItem(storageKey, existingAmount + amount) | |
| } |
| @@ -1007,7 +1007,7 @@ const ZapUndosField = () => { | |||
| <Info> | |||
| <ul className='fw-bold'> | |||
| <li>An undo button is shown after every zap that exceeds or is equal to the threshold</li> | |||
| <li>The button is shown for 5 seconds</li> | |||
| <li>The button is shown for {ZAP_UNDO_DELAY_MS / 1000} seconds</li> | |||
There was a problem hiding this comment.
Consider improving user feedback for the ZapUndosField component.
The ZapUndosField component could benefit from enhanced user feedback. For instance, when the undo button is not shown due to the conditions not being met, displaying a brief message explaining why can help improve user understanding and interaction.
| export function statusClause (me) { | ||
| let filter = '"Item"."status" <> \'FAILED\' AND ' | ||
|
|
||
| if (me) { | ||
| filter += `("Item"."userId" = ${me.id} OR "Item"."status" <> 'PENDING')` | ||
| } else { | ||
| filter += '"Item"."status" <> \'PENDING\'' | ||
| } | ||
|
|
||
| return filter | ||
| } |
There was a problem hiding this comment.
Refactor the statusClause function to improve readability and maintainability.
Consider breaking down the function into smaller, more manageable parts or using helper functions to handle different conditions.
| const newFailedItems = await models.item.findFirst({ | ||
| where: { | ||
| userId: me.id, | ||
| status: 'FAILED', | ||
| updatedAt: { | ||
| gt: lastChecked | ||
| } | ||
| } | ||
| }) | ||
| if (newFailedItems) { | ||
| foundNotes() | ||
| return true | ||
| } |
There was a problem hiding this comment.
Consider optimizing the query for checking new failed items.
The current implementation performs a query for each type of notification. This could potentially be optimized by combining some of these queries into a single query, especially if they are checking similar conditions or can be grouped logically.
There was a problem hiding this comment.
This comment reminds me that I probably need to manually update updatedAt in raw queries since it's not based on a trigger.
There was a problem hiding this comment.
@ekzyis, you're absolutely right about the need to manually update the updatedAt field in raw queries, especially since it's not managed by a database trigger in this context. Manually managing this field can ensure that your data remains consistent and accurate, which is crucial for features that rely on the timeliness of data updates.
If you're updating multiple records or have multiple places where this needs to be handled, you might consider abstracting this logic into a helper function or middleware to keep your code DRY and maintainable. Here’s a quick example of how you might implement such a function in JavaScript:
const updateWithTimestamp = async (model, where, data) => {
const updatedAt = new Date();
const updatedData = { ...data, updatedAt };
return await model.update({ where, data: updatedData });
};This function takes the model, the conditions (where), and the data to be updated, and it automatically appends the current timestamp to the updatedAt field before executing the update.
Let me know if you need further assistance with this or any other part of your implementation!
There was a problem hiding this comment.
Mhh, even though I don't see a trigger that does this, updatedAt gets updated after a raw query updates an item 🤔
These are the existing item triggers:
Triggers:
index_item AFTER INSERT OR UPDATE ON "Item" FOR EACH ROW EXECUTE FUNCTION index_item()
ncomments_after_comment_trigger AFTER INSERT ON "Item" FOR EACH ROW EXECUTE FUNCTION ncomments_after_comment()
path_tgr BEFORE INSERT OR UPDATE ON "Item" FOR EACH ROW EXECUTE FUNCTION update_item_path()
timestamp_item_on_insert AFTER INSERT ON "Item" FOR EACH ROW EXECUTE FUNCTION timestamp_item_on_insert()
There was a problem hiding this comment.
@ekzyis, it's interesting to see that the updatedAt field is being updated automatically, even though there isn't a specific trigger listed for this action. This behavior might be due to a couple of reasons:
-
Database Abstraction or ORM Behavior: If the project uses an ORM (Object-Relational Mapping) or any database abstraction layer, it's common for these tools to manage
updatedAtfields automatically. They often do this during any update operation without needing explicit triggers in the database. -
Existing Trigger Handling More Than Its Name Suggests: The trigger
timestamp_item_on_insertmight be slightly misleading by its name. It's possible that this trigger is configured to handle updates as well as inserts, despite what the name suggests. This would allow it to update theupdatedAtfield on any modification to the row.
If you need to confirm or inspect this behavior further, you might consider checking the definition of the timestamp_item_on_insert trigger to see if it indeed handles updates. Alternatively, reviewing the ORM or database layer's configuration or documentation could provide insights into this automatic handling of the updatedAt field.
Let me know if you need more detailed information on this or any other queries!
| @@ -327,7 +328,7 @@ export default { | |||
| } | |||
| }, | |||
| Mutation: { | |||
| createInvoice: async (parent, { amount, hodlInvoice = false, expireSecs = 3600 }, { me, models, lnd, headers }) => { | |||
| createInvoice: async (parent, { amount, hodlInvoice = false, expireSecs = (DEFAULT_INVOICE_TIMEOUT_MS / 1000) }, { me, models, lnd, headers }) => { | |||
There was a problem hiding this comment.
Review and optimize transaction management in the upsertWallet function to ensure data consistency and error resilience.
| if (data) { | ||
| invoice = data.invoice | ||
| } | ||
|
|
||
| if (error) { | ||
| return <div>{error.toString()}</div> | ||
| } |
There was a problem hiding this comment.
Good error handling in the component. Displaying the error message directly can help in debugging, but consider sanitizing or formatting the error for end users.
components/notifications.js
Outdated
| const onClickFailedItem = ({ n }) => { | ||
| const { parentId, title, url, text, subName: sub, boost, bounty } = n.item | ||
| const isPost = !!n.item.title | ||
|
|
||
| if (isPost) { | ||
| const type = determineItemType(n.item) | ||
| const query = { type, title, text, sub } | ||
| if (boost > 0) { | ||
| query.boost = boost | ||
| } | ||
| if (type === 'link') { | ||
| query.url = url | ||
| } | ||
| if (type === 'bounty' && bounty > 0) { | ||
| query.bounty = bounty | ||
| } | ||
| return { | ||
| href: { | ||
| pathname: '/post', | ||
| query | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const rootId = commentSubTreeRootId(n.item) | ||
| const query = { id: rootId, text } | ||
| if (Number(rootId) !== Number(parentId)) { | ||
| query.commentId = parentId | ||
| } | ||
| return { | ||
| href: { | ||
| pathname: '/items/[id]', | ||
| query | ||
| }, | ||
| as: `/items/${rootId}` | ||
| } | ||
| } |
There was a problem hiding this comment.
The onClickFailedItem function correctly handles link generation for failed items. Consider adding error handling or a default case for unexpected item types to enhance robustness.
+ if (!['link', 'bounty', 'post', 'comment'].includes(type)) {
+ console.error('Unexpected item type:', type);
+ return {}; // Return empty to prevent navigation
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const onClickFailedItem = ({ n }) => { | |
| const { parentId, title, url, text, subName: sub, boost, bounty } = n.item | |
| const isPost = !!n.item.title | |
| if (isPost) { | |
| const type = determineItemType(n.item) | |
| const query = { type, title, text, sub } | |
| if (boost > 0) { | |
| query.boost = boost | |
| } | |
| if (type === 'link') { | |
| query.url = url | |
| } | |
| if (type === 'bounty' && bounty > 0) { | |
| query.bounty = bounty | |
| } | |
| return { | |
| href: { | |
| pathname: '/post', | |
| query | |
| } | |
| } | |
| } | |
| const rootId = commentSubTreeRootId(n.item) | |
| const query = { id: rootId, text } | |
| if (Number(rootId) !== Number(parentId)) { | |
| query.commentId = parentId | |
| } | |
| return { | |
| href: { | |
| pathname: '/items/[id]', | |
| query | |
| }, | |
| as: `/items/${rootId}` | |
| } | |
| } | |
| const onClickFailedItem = ({ n }) => { | |
| const { parentId, title, url, text, subName: sub, boost, bounty } = n.item | |
| const isPost = !!n.item.title | |
| if (isPost) { | |
| const type = determineItemType(n.item) | |
| const query = { type, title, text, sub } | |
| if (boost > 0) { | |
| query.boost = boost | |
| } | |
| if (type === 'link') { | |
| query.url = url | |
| } | |
| if (type === 'bounty' && bounty > 0) { | |
| query.bounty = bounty | |
| } | |
| if (!['link', 'bounty', 'post', 'comment'].includes(type)) { | |
| console.error('Unexpected item type:', type); | |
| return {}; // Return empty to prevent navigation | |
| } | |
| return { | |
| href: { | |
| pathname: '/post', | |
| query | |
| } | |
| } | |
| } | |
| const rootId = commentSubTreeRootId(n.item) | |
| const query = { id: rootId, text } | |
| if (Number(rootId) !== Number(parentId)) { | |
| query.commentId = parentId | |
| } | |
| return { | |
| href: { | |
| pathname: '/items/[id]', | |
| query | |
| }, | |
| as: `/items/${rootId}` | |
| } | |
| } |
| function FailedItem ({ n }) { | ||
| const isPost = !!n.item.title | ||
| const type = isPost ? determineItemType(n.item) : undefined | ||
| const message = `failed to ${type ? `create ${type}` : 'reply'}: invoice was not paid` | ||
| return ( | ||
| <div className='ms-2'> | ||
| <small className='fw-bold text-danger'> | ||
| {n.reason ? `${message}: ${n.reason}` : message} | ||
| <small className='text-muted ms-1 fw-normal' suppressHydrationWarning>{timeSince(new Date(n.sortTime))}</small> | ||
| </small> | ||
| {isPost | ||
| ? <Item item={n.item} /> | ||
| : ( | ||
| <div className='pb-2'> | ||
| <RootProvider root={n.item.root}> | ||
| <Comment item={n.item} noReply includeParent noComments clickToContext /> | ||
| </RootProvider> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
The FailedItem component effectively communicates failure details to the user. Consider using template literals for constructing the failure message to improve readability.
- const message = `failed to ${type ? `create ${type}` : 'reply'}: invoice was not paid`
+ const action = type ? `create ${type}` : 'reply';
+ const message = `failed to ${action}: invoice was not paid`;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| function FailedItem ({ n }) { | |
| const isPost = !!n.item.title | |
| const type = isPost ? determineItemType(n.item) : undefined | |
| const message = `failed to ${type ? `create ${type}` : 'reply'}: invoice was not paid` | |
| return ( | |
| <div className='ms-2'> | |
| <small className='fw-bold text-danger'> | |
| {n.reason ? `${message}: ${n.reason}` : message} | |
| <small className='text-muted ms-1 fw-normal' suppressHydrationWarning>{timeSince(new Date(n.sortTime))}</small> | |
| </small> | |
| {isPost | |
| ? <Item item={n.item} /> | |
| : ( | |
| <div className='pb-2'> | |
| <RootProvider root={n.item.root}> | |
| <Comment item={n.item} noReply includeParent noComments clickToContext /> | |
| </RootProvider> | |
| </div> | |
| )} | |
| </div> | |
| ) | |
| } | |
| function FailedItem ({ n }) { | |
| const isPost = !!n.item.title | |
| const type = isPost ? determineItemType(n.item) : undefined | |
| const action = type ? `create ${type}` : 'reply'; | |
| const message = `failed to ${action}: invoice was not paid`; | |
| return ( | |
| <div className='ms-2'> | |
| <small className='fw-bold text-danger'> | |
| {n.reason ? `${message}: ${n.reason}` : message} | |
| <small className='text-muted ms-1 fw-normal' suppressHydrationWarning>{timeSince(new Date(n.sortTime))}</small> | |
| </small> | |
| {isPost | |
| ? <Item item={n.item} /> | |
| : ( | |
| <div className='pb-2'> | |
| <RootProvider root={n.item.root}> | |
| <Comment item={n.item} noReply includeParent noComments clickToContext /> | |
| </RootProvider> | |
| </div> | |
| )} | |
| </div> | |
| ) | |
| } |
| invoice { | ||
| id | ||
| bolt11 | ||
| hash | ||
| hmac | ||
| expiresAt | ||
| satsRequested | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure alignment of invoice object structure with backend schema.
The invoice object structure in the upsertLink mutation should match the backend schema definitions. Here are the relevant structures:
-
Frontend (components/link-form.js):
invoice { id bolt11 hash hmac expiresAt satsRequested }
-
Backend (prisma/schema.prisma):
model Invoice { id Int @id @default(autoincrement()) createdAt DateTime @default(now()) @map("created_at") updatedAt DateTime @default(now()) @updatedAt @map("updated_at") userId Int hash String @unique(map: "Invoice.hash_unique") preimage String? @unique(map: "Invoice.preimage_unique") isHeld Boolean? bolt11 String expiresAt DateTime confirmedAt DateTime? }
-
Backend (api/typeDefs/wallet.js):
type Invoice { id: ID! createdAt: Date! hash: String! bolt11: String! expiresAt: Date! cancelled: Boolean! confirmedAt: Date satsReceived: Int satsRequested: Int! nostr: JSONObject }
The invoice object in the frontend is missing some fields present in the backend schema. Ensure that the frontend structure includes all necessary fields expected by the backend.
- Add missing fields such as
createdAt,confirmedAt,cancelled,satsReceived, andnostrif they are required by the backend.
Analysis chain
Ensure that the invoice object structure in the upsertLink mutation aligns with backend expectations.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the `invoice` object structure in the `upsertLink` mutation.
# Test: Search for the backend schema for `invoice`. Expect: Alignment with the frontend structure.
rg --type js "invoice" api/schema.js
Length of output: 90
Script:
#!/bin/bash
# Description: Locate the backend schema file by searching for common schema-related keywords.
# Search for files containing the keyword 'schema' which might indicate the backend schema.
rg --files-with-matches "schema"
# Search for files containing the keyword 'invoice' to locate the relevant schema definition.
rg --files-with-matches "invoice"
Length of output: 3570
Script:
#!/bin/bash
# Description: Verify the `invoice` object structure in the backend schema files.
# Search for the `invoice` object definition in `prisma/schema.prisma`.
rg "invoice" prisma/schema.prisma
# Search for the `invoice` object definition in `api/typeDefs/wallet.js`.
rg "invoice" api/typeDefs/wallet.js
Length of output: 262
Script:
#!/bin/bash
# Description: Extract the full definition of the `Invoice` object from the backend schema files.
# Extract the full definition of the `Invoice` object from `prisma/schema.prisma`.
rg -A 10 "model Invoice" prisma/schema.prisma
# Extract the full definition of the `Invoice` object from `api/typeDefs/wallet.js`.
rg -A 10 "type Invoice" api/typeDefs/wallet.js
Length of output: 781
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
api/resolvers/user.js (3)
Line range hint
2-3: Update import syntax for Node.js built-in modules.- import { readFile } from 'fs/promises' - import { join, resolve } from 'path' + import { readFile } from 'node:fs/promises' + import { join, resolve } from 'node:path'This change ensures compatibility with the latest Node.js module resolution enhancements, which recommend specifying the
node:protocol when importing built-in modules.
Line range hint
21-24: Consider usingfor...ofinstead offorEachfor better error handling and async support.- fileContent.split('\n') - .map(line => line.trim()) - .filter(line => !!line) - .forEach(name => set.add(name)) + for (const line of fileContent.split('\n').map(line => line.trim()).filter(line => !!line)) { + set.add(line); + }Using
for...ofallows for easier integration of asynchronous operations and better handling of exceptions within loops.
Line range hint
628-630: Remove unnecessaryelseclause after early return.- } else { - return await models.user.update({ where: { id: me.id }, data: { ...data, nostrRelays: { deleteMany: {} } } }) - } + return await models.user.update({ where: { id: me.id }, data: { ...data, nostrRelays: { deleteMany: {} } } })Since the previous branches in the conditional structure use
return, theelseclause is redundant and can be omitted for cleaner code.
|
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:
(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 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 |
| JSON.stringify({ ...item, status: 'PENDING' }), JSON.stringify(fwdUsers), JSON.stringify(options), uploadIds)) | ||
|
|
||
| // set required invoice data to update item on payment | ||
| const actionData = { cost: err.cost, credits: err.balance }; |
There was a problem hiding this comment.
If these fields need to be saved, might we do better storing them as columns with types and naming them in the db layer?
|
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. |
Sounds like a missing
No 👀 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
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 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 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.
There is a worker job which runs So I think the most important remaining TODO here is to create |
|
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:
|
|
I realized that fixing edits of pending items would also require changes to 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:
(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. |
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 callsQueryManager.markOptimisticMutationandcache.removeOptimisticto manually trigger the optimistic cache update and its revert: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
invoiceableshould actually be calledprepaid. 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
optimisticUpdatepersistence of optimistic update while payment is pending viawindow.localStoragewe'll do this after not-custodial zap beta #1178 since we need to be aware of pending stuff on the server anyway if proxy is used
optimisticUpdatepersistence of optimistic update while payment is pending viawindow.localStoragewe'll do this after not-custodial zap beta #1178 since we need to be aware of pending stuff on the server anyway if proxy is used
optimisticUpdatepersistence of optimistic update while payment is pending viawindow.localStoragewe'll do this after not-custodial zap beta #1178 since we need to be aware of pending stuff on the server anyway if proxy is used
related to persistence to some degree since payment should also be considered "failed" if payment + associated action was lost due to page refresh, but we'll skip this case for now as mentioned above
error handling if payment failed (reverting persistence)see comments above
Postpaid
payment.sendnext topayment.requestfrom Replace useInvoiceable with usePayment #1071postpaidto form which checks for returned invoices and tries to pay themACTIVEeven if transition query failed after invoice confirmationupdatefor some reason, this wasn't necessaryupdatedAtmanually in raw queries for notification red square to workTODO after review
confirm_invoiceto fix race conditions if autowithdrawals are enabledresetForm: 669ad67Videos
TBD
Additional Context
mutationIdCounterand notmutationIdCounter++as in the library source code was that the mutation will also call an optimistic update with the same id to make sure the UI updates and overwrite the previous layer. If I recall correctly, I found out it actually creates its own layer with its own (automated) revert. Therefore, I considered it not only be redundant but also a possible source of bugs. However, as mentioned, not sure, would need to check this behavior again.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: