Trigger on confirmed transactions#622
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughHandleNewTransaction now skips processing if a prior payment succeeded and invokes handlePayment for any transaction with a positive amount regardless of confirmation state. WebSocket parsing now treats Changes
Sequence Diagram(s)sequenceDiagram
participant WS as "WebSocket"
participant Chronik as "chronik util"
participant Widget as "WidgetContainer"
WS->>Chronik: receive TX_ADDED_TO_MEMPOOL / TX_CONFIRMED msg
Chronik->>Chronik: chronik.tx(txid) -> getTransactionFromChronikTransaction
Chronik->>Widget: setNewTx([transaction])
Widget->>Widget: handleNewTransaction(transaction)
alt success already true
Widget-->>Widget: return early (skip processing)
else amount > 0
Widget->>Widget: handlePayment(transaction)
Widget->>Widget: (fire onTransaction only if unconfirmed)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 282-284: The onTransaction callback can fire twice for the same
txid because when shouldTriggerOnSuccess(tx) is false on mempool arrival we call
onTransaction(tx) and then call handlePayment again when TX_CONFIRMED triggers;
update handleNewTransaction/handlePayment logic to deduplicate by txid (or check
a success flag) before calling onTransaction: e.g., track seen txids for pending
transactions or check shouldTriggerOnSuccess(tx) and a stored seenTransactions
set before invoking onTransaction(tx), ensuring onTransaction is only called
once per txid (refer to functions handleNewTransaction, handlePayment,
shouldTriggerOnSuccess, onTransaction, resolveNumber, isGreaterThanZero).
- Around line 278-287: The callback that responds to transaction events
currently only early-returns on success, so shifts already completed via the
altpayment path (shiftCompleted) can trigger handlePayment and onSuccess again;
update the early-return in the useCallback that processes
TX_CONFIRMED/TX_ADDED_TO_MEMPOOL to check both success and shiftCompleted (e.g.,
if (success || shiftCompleted) return;) and add shiftCompleted to the dependency
array alongside handlePayment and success so the callback sees updates to that
flag.
| if (success) { | ||
| return; | ||
| } | ||
|
|
||
| if (isGreaterThanZero(resolveNumber(tx.amount))) { | ||
| handlePayment(tx); | ||
| } | ||
| }, | ||
| [handlePayment], | ||
| [handlePayment, success], | ||
| ); |
There was a problem hiding this comment.
shiftCompleted is not guarded — altpayment onSuccess and success sound can fire twice.
The early-return at Line 278 only checks success, but the altpayment path in handlePayment sets shiftCompleted and calls onSuccess without setting success. This means when TX_CONFIRMED arrives after TX_ADDED_TO_MEMPOOL for an already-settled shift, success is still false, the guard is bypassed, and handlePayment runs again — re-playing the sound and re-invoking onSuccess.
🐛 Proposed fix
- if (success) {
+ if (success || shiftCompleted) {
return;
}And add shiftCompleted to the useCallback dependency array:
- [handlePayment, success],
+ [handlePayment, success, shiftCompleted],📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (success) { | |
| return; | |
| } | |
| if (isGreaterThanZero(resolveNumber(tx.amount))) { | |
| handlePayment(tx); | |
| } | |
| }, | |
| [handlePayment], | |
| [handlePayment, success], | |
| ); | |
| if (success || shiftCompleted) { | |
| return; | |
| } | |
| if (isGreaterThanZero(resolveNumber(tx.amount))) { | |
| handlePayment(tx); | |
| } | |
| }, | |
| [handlePayment, success, shiftCompleted], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react/lib/components/Widget/WidgetContainer.tsx` around lines 278 - 287, The
callback that responds to transaction events currently only early-returns on
success, so shifts already completed via the altpayment path (shiftCompleted)
can trigger handlePayment and onSuccess again; update the early-return in the
useCallback that processes TX_CONFIRMED/TX_ADDED_TO_MEMPOOL to check both
success and shiftCompleted (e.g., if (success || shiftCompleted) return;) and
add shiftCompleted to the dependency array alongside handlePayment and success
so the callback sees updates to that flag.
So far only unconfirmed (aka mempool) transactions are parsed. This means that any transaction that is mined before it reaches chronik's mempool will be ignored and the payment missed, which is obviously very bad. The transaction confirmed message is added to the list of managed chronik messages, and the success is checked before processing to avoid triggering the payment success twice. Note that the filter exists in the socket.ts txsListener but this is dead code and is removed in PayButton#619. Test Plan: yarn test Check with a widget test, pay for it, check the success and wait for the block to be mined: the payment animation is not replayed.
Fabcien
force-pushed
the
process_confirmed_txs
branch
from
822a061 to
d17f9d2
Compare
|
As per the rabbit feedback, filter the onTransaction() callback to retain the existing behavior |
3 participants
So far only unconfirmed (aka mempool) transactions are parsed. This means that any transaction that is mined before it reaches chronik's mempool will be ignored and the payment missed, which is obviously very bad.
The transaction confirmed message is added to the list of managed chronik messages, and the success is checked before processing to avoid triggering the payment success twice.
Note 1: the filter exists in the socket.ts txsListener but this is dead code and is removed in #619.
Note 2: there should be an option for waiting for finalization but this is out of scope.
Test Plan:
yarn test
Check with a widget test, pay for it, check the success and wait for the block to be mined: the payment animation is not replayed.
Summary by CodeRabbit
New Features
Bug Fixes