Don't prevent detection when paying to self#625
Conversation
The amount detection is actually an exact match, computing the transaction amount by summing the recipient output and subtracting the sender inputs to get the (out - in) difference. When sending to self this computed value is zero so the amount is not matched (the tx is not even processed). This also fixes the edge case of a tx sending more coins to the recipient, e.g. via an extra output. Test Plan: yarn test Try sending to yourself and check detection now works.
📝 WalkthroughWalkthroughThe changes fix a payment validation issue where onSuccess callbacks weren't triggering for payments involving inputs and outputs from the same wallet. The amount calculation now uses total outputs instead of net (output minus input), and validation uses greater-than-or-equal comparison instead of strict equality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react/lib/util/chronik.ts (1)
116-138:⚠️ Potential issue | 🟡 Minor
Transaction.amountmay be inflated for self-payments that include change back to the same address.The fix correctly reports
totalOutputinstead of the previoustotalOutput − totalInput(which produced zero for self-payments). For the common self-payment scenario, however, an UTXO-spending transaction often sends change back to the same address:
- Input: 1000 XEC from Alice
- Output 1: 100 XEC to Alice ← intended payment
- Output 2: 895 XEC to Alice ← change
With the new logic,
totalOutput = 995 XECis recorded inTransaction.amount, not100 XEC. TheisLessThanOrEqualTocomparison invalidate.tsstill triggersonSuccesscorrectly (100 ≤ 995), but theamountsurfaced to merchant callbacks and the UI will be inflated. This is an inherent limitation of thetotalOutputapproach when change can't be distinguished from the intentional payment in a self-payment.Consider documenting this behaviour (e.g., a code comment on
getTransactionAmountAndDatanoting that for self-payments the amount reflects the sum of all outputs to the address, including change).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/util/chronik.ts` around lines 116 - 138, getTransactionAmountAndData currently sums all outputs to the given address (totalOutput) and therefore will report an inflated amount for self-payments that also return change to the same address; add a clear inline comment above the getTransactionAmountAndData function explaining that Transaction.amount reflects the sum of all outputs to the address (including change) in self-payments, describe the example case (payment + change) briefly, and note this limitation as intentional/current behavior and a future improvement area (e.g., require input ownership data to separate change from payment).
🧹 Nitpick comments (1)
react/lib/tests/util/validate.test.ts (1)
148-174:result1andresult2can beconst.Neither variable is reassigned after its initializer;
letoverstates mutability.♻️ Proposed fix
- let result1 = shouldTriggerOnSuccess( + const result1 = shouldTriggerOnSuccess( transaction, currency, price, false, false, expectedPaymentId, expectedAmount, expectedOpReturn ); expect(result1).toBe(true); expectedAmount = resolveNumber('101.00'); - let result2 = shouldTriggerOnSuccess( + const result2 = shouldTriggerOnSuccess( transaction, currency, price, false, false, expectedPaymentId, expectedAmount, expectedOpReturn );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/tests/util/validate.test.ts` around lines 148 - 174, The variables result1 and result2 in the test block are declared with let but never reassigned; change their declarations to const to reflect immutability where shouldTriggerOnSuccess(...) is called for each assertion (keep expectedAmount as let since it's reassigned before result2). Update the lines creating result1 and result2 to use const to improve clarity and satisfy lint rules.
🤖 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/util/validate.ts`:
- Around line 37-42: In the block where randomSatoshis is truthy you replaced
exact matching with isLessThanOrEqualTo and also skip isPaymentIdValid,
weakening the per-session uniqueness guarantee; update validate.ts by adding a
concise comment next to the randomSatoshis / isLessThanOrEqualTo branch (and
near the isPaymentIdValid skip) that states this use of isLessThanOrEqualTo is
intentional, documents that uniqueness is preserved by the paymentId /
isPaymentIdValid logic (or by the TX_ADDED_TO_MEMPOOL event semantics) rather
than exact amount matching (isEqualTo), and notes the trade-off/risk of
coincidental higher-value transactions triggering onSuccess — or alternatively
re-enable isPaymentIdValid for randomSatoshis flows if you want strict
uniqueness.
---
Outside diff comments:
In `@react/lib/util/chronik.ts`:
- Around line 116-138: getTransactionAmountAndData currently sums all outputs to
the given address (totalOutput) and therefore will report an inflated amount for
self-payments that also return change to the same address; add a clear inline
comment above the getTransactionAmountAndData function explaining that
Transaction.amount reflects the sum of all outputs to the address (including
change) in self-payments, describe the example case (payment + change) briefly,
and note this limitation as intentional/current behavior and a future
improvement area (e.g., require input ownership data to separate change from
payment).
---
Nitpick comments:
In `@react/lib/tests/util/validate.test.ts`:
- Around line 148-174: The variables result1 and result2 in the test block are
declared with let but never reassigned; change their declarations to const to
reflect immutability where shouldTriggerOnSuccess(...) is called for each
assertion (keep expectedAmount as let since it's reassigned before result2).
Update the lines creating result1 and result2 to use const to improve clarity
and satisfy lint rules.
| isAmountValid = resolveNumber(value).isLessThanOrEqualTo(amount) | ||
| }else { | ||
| isAmountValid = false | ||
| } | ||
| } else { | ||
| isAmountValid = expectedAmount.isEqualTo(amount); | ||
| isAmountValid = expectedAmount.isLessThanOrEqualTo(amount); |
There was a problem hiding this comment.
isLessThanOrEqualTo weakens the uniqueness guarantee when randomSatoshis is truthy.
The comparison direction (expectedAmount ≤ receivedAmount) is correct and logically consistent with the PR goal. However, a subtle regression:
When randomSatoshis is truthy and non-zero, the isPaymentIdValid check is entirely skipped (lines 48–53). Previously, isEqualTo enforced an exact match on the salt-modified amount, which implicitly gave each payment session uniqueness. With isLessThanOrEqualTo, any concurrent transaction to the monitored address with amount >= expectedAmount would trigger onSuccess.
In practice the WebSocket handler only processes TX_ADDED_TO_MEMPOOL events (new transactions), so a replay of an older larger transaction is not possible at the protocol level. The risk is therefore a coincidental same-block unrelated transaction to the same address that exceeds the threshold — low probability, but the uniqueness guarantee that randomSatoshis was intended to provide is no longer enforced by the amount check alone.
Consider adding a brief comment explaining that isLessThanOrEqualTo is intentional and that uniqueness for randomSatoshis sessions is instead provided by … (paymentId, or document the trade-off).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react/lib/util/validate.ts` around lines 37 - 42, In the block where
randomSatoshis is truthy you replaced exact matching with isLessThanOrEqualTo
and also skip isPaymentIdValid, weakening the per-session uniqueness guarantee;
update validate.ts by adding a concise comment next to the randomSatoshis /
isLessThanOrEqualTo branch (and near the isPaymentIdValid skip) that states this
use of isLessThanOrEqualTo is intentional, documents that uniqueness is
preserved by the paymentId / isPaymentIdValid logic (or by the
TX_ADDED_TO_MEMPOOL event semantics) rather than exact amount matching
(isEqualTo), and notes the trade-off/risk of coincidental higher-value
transactions triggering onSuccess — or alternatively re-enable isPaymentIdValid
for randomSatoshis flows if you want strict uniqueness.
3 participants
The amount detection is actually an exact match, computing the transaction amount by summing the recipient output and subtracting the sender inputs to get the (out - in) difference.
When sending to self this computed value is zero so the amount is not matched (the tx is not even processed).
This also fixes the edge case of a tx sending more coins to the recipient, e.g. via an extra output.
Closes #301.
Note that the exact same issue exists on server side.
Test Plan:
yarn test
Try sending to yourself and check detection now works.
Summary by CodeRabbit