Skip to content

Commit

Permalink
Fix zap race condition
Browse files Browse the repository at this point in the history
If zapping too fast, item.meSats inside optimistic update could include pending sats from previous zap unlike item.meSats in calling context.
This is fixed by passing satsDelta instead of calculating it independently with readFragment.
This race condition lead to following unexpected behavior that is now fixed:

* pending sats could go negative causing items to falsely be marked as flagged

unrelated fix:

* fix zapUndoTrigger using total sats not satsDelta
  • Loading branch information
ekzyis committed May 15, 2024
1 parent cb9a7d5 commit d17d276
Showing 1 changed file with 10 additions and 23 deletions.
33 changes: 10 additions & 23 deletions components/item-act.js
Original file line number Diff line number Diff line change
Expand Up @@ -229,23 +229,7 @@ export function useAct () {
return act
}

const zapOptimisticUpdate = (cache, { id, path, act, sats }, { me }) => {
const readItemFragment = id => cache.readFragment({
id: `Item:${id}`,
fragment: gql`
fragment ItemMeSatsZap on Item {
meSats
}
`
})

// determine how much we increased existing sats
// by checking the difference between result sats and meSats
// if it's negative, skip the cache as it's an out of order update
// if it's positive, add it to sats and commentSats
const item = readItemFragment(id)
const satsDelta = sats - item.meSats

const zapOptimisticUpdate = (cache, { id, path, act, satsDelta }, { me }) => {
if (satsDelta > 0) {
updateItemSats(cache, { id, path, act, sats: satsDelta }, { me })
}
Expand Down Expand Up @@ -282,24 +266,27 @@ export function useZap () {

// add current sats to next tip since idempotent zaps use desired total zap not difference
const sats = meSats + nextTip(meSats, { ...me?.privates })
const satsDelta = sats - meSats

const act = 'TIP'
let cancel, revert, nid
try {
const variables = { path: item.path, id: item.id, sats, act }
revert = zapOptimisticUpdate(cache, variables, { me })
const variables = { id: item.id, path: item.path, act, sats }
revert = zapOptimisticUpdate(cache, { ...variables, satsDelta }, { me })
strike()
abortSignal?.start()
nid = notify(NotificationType.ZapPending, { amount: sats - meSats, itemId: item.id }, false)
if (zapUndoTrigger(me, sats)) {
nid = notify(NotificationType.ZapPending, { amount: satsDelta, itemId: item.id }, false)
if (zapUndoTrigger(me, satsDelta)) {
await zapUndo(abortSignal)
} else {
abortSignal?.done()
}
let hash, hmac;
[{ hash, hmac }, cancel] = await payment.request(sats - meSats)
[{ hash, hmac }, cancel] = await payment.request(satsDelta)
await zap({ variables: { ...variables, hash, hmac } })
if (me) persistItemPendingSats({ ...item, act, sats: -(sats - meSats) })
// in the success case, we need to remove pending sats
// since they will now be included in item.meSats of server responses
if (me && satsDelta > 0) persistItemPendingSats({ ...item, act, sats: -satsDelta })
} catch (error) {
revert?.()
if (error instanceof InvoiceCanceledError || error instanceof ActCanceledError) {
Expand Down

0 comments on commit d17d276

Please sign in to comment.