Reject payment started without seller account - #8061
Conversation
CoreTradesService.confirmPaymentStarted checks only that the trade has not failed and that its deposit tx is confirmed. Neither says anything about the seller's payment account payload, which reaches the buyer with the DepositTxAndDelayedPayoutTxMessage and can be absent on a trade whose deposit the buyer confirmed from the wallet alone. None of the five tasks in BuyerProtocol.onPaymentStarted reads that payload: ApplyFilter treats it as nullable, the fee verification tasks are no-ops, and the payout tasks use the contract only for addresses and multisig keys. So the call succeeds, the trade moves to FIAT_SENT and the payment started message goes to the seller for a payment the buyer had no details to make. FIAT_SENT is also past every phase in which the buyer still accepts the DepositTxAndDelayedPayoutTxMessage, so the confirm closes the last window in which the missing payload could still arrive. Reject the call while the contract carries no seller payment account payload. The check sits before the counter currency tx proof is stored, so a rejected confirmpaymentstartedxmr leaves no proof data on the trade. The desktop client refuses the same state since bisq-network#7955, which disables the confirm button on the same contract payload. Part of bisq-network#7945
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesPayment-start validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change rejects payment confirmation when seller payment details are absent, before trade state or proof data is updated. No actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
CoreTradesService.confirmPaymentStarted checks only that the trade has
not failed and that its deposit tx is confirmed. Neither says anything
about the seller's payment account payload, which reaches the buyer
with the DepositTxAndDelayedPayoutTxMessage and can be absent on a
trade whose deposit the buyer confirmed from the wallet alone.
None of the five tasks in BuyerProtocol.onPaymentStarted reads that
payload: ApplyFilter treats it as nullable, the fee verification tasks
are no-ops, and the payout tasks use the contract only for addresses
and multisig keys. So the call succeeds, the trade moves to FIAT_SENT
and the payment started message goes to the seller for a payment the
buyer had no details to make.
FIAT_SENT is also past every phase in which the buyer still accepts
the DepositTxAndDelayedPayoutTxMessage, so the confirm closes the last
window in which the missing payload could still arrive.
Reject the call while the contract carries no seller payment account
payload. The check sits before the counter currency tx proof is
stored, so a rejected confirmpaymentstartedxmr leaves no proof data on
the trade.
The desktop client refuses the same state since #7955, which disables
the confirm button on the same contract payload.
Both rejection tests fail on master: without the guard the call reaches
onPaymentStarted and stores the tx proof. The third test covers the
normal path, so the guard cannot be widened unnoticed.
A precondition in BuyerProtocol would not work for this: FluentProtocol
returns before the task runner is built, so the gRPC caller would still
get an OK. A first task in onPaymentStarted would cover both clients,
but it runs after the phase is set and its failure writes an error
message on the trade, so it is a protocol change rather than the guard
this follow-up was about.
No specification change: this is a client-side precondition, not a rule
of the trade protocol, and neither of the two merged PRs for this issue
carried one.
Part of #7945
Summary by CodeRabbit