Lock the send slider once a broadcast has been attempted - #6190
Conversation
One intended Bitcoin send became five real payments (Asana 1217135300337949). Every broadcast succeeded over the HTTP fallback, the engine's saveTx then threw, and the send scene treated that throw as a failed send: a generic network error card plus a re-armed slider. Because saveTx marks the inputs spent before it throws, each retry re-quoted on the remaining UTXOs, so every slide was a fresh real payment. A failure reported by broadcastTx does not prove the transaction is absent from the network. A server can relay it and still fail to answer, and the network conditions that broke the broadcast also make any immediate "did it land?" check unreliable. So the scene now treats the broadcast call as a one-way boundary: - A ref is set immediately before broadcastTx is called. Once set, the finally block never re-arms the slider, on success or on failure, and the slider renders locked as "Send Attempted" for the life of the scene. The ref (rather than state) is what the handler reads, so the FIO no-bundled retry recursion cannot re-arm it either. - Failures at or after that boundary no longer show the generic error. They show "Transaction Status Unknown" telling the user the transaction may have reached the network, to check a block explorer or wait for a confirmation email, and that sending again risks a duplicate payment. Separate copy distinguishes a broadcast that reported failure from an error after a successful one. - Failures before the boundary (PIN, the beforeTransaction hook, the FIO fee check, signing) still show the existing errors and still re-arm the slider, because nothing can have been sent. The hook failure also now resets the slider instead of leaving it spinning. SafeSlider gains an explicit `lockedText` prop for this. Overloading the existing `disabled` prop would have shown a completed slider's `disabledText` (which defaults to "Enter an Amount") in place of the spinner whenever any parent disabled a slider mid-callback, affecting the other fourteen scenes that use it. The locked-state card is longer than a normal error and the slider floats over the bottom of the scroll view, so the existing needsScrollToEnd mechanism scrolls it into view. Verified on the iOS simulator against the real send scene with the broadcast stubbed out, for a post-broadcast throw, an ambiguous broadcast failure, and a pre-broadcast signing failure.
j0ntz
left a comment
j0ntz
left a comment
There was a problem hiding this comment.
Reviewed alongside EdgeApp/edge-currency-plugins#458; the findings are inline.
Nit: the commit subject Lock the send slider once a broadcast has been attempted is 56 characters, over Edge's 50-character limit. Lock the send slider after a broadcast attempt is 46.
| // the slider for the life of this scene no matter what happens next. | ||
| // The render-side flag is set in the finally block, so the slider | ||
| // keeps its spinner while the attempt is in flight. | ||
| broadcastAttemptedRef.current = true |
There was a problem hiding this comment.
Setting the ref here is right for the lock. Using the same ref at line 1530 to choose the message is what causes trouble: deterministic rejections thrown from inside broadcastTx now read as ambiguous.
EosEngine.broadcastTx maps tx_cpu_usage_exceeded / tx_net_usage_exceeded / ram_usage_exceeded onto ErrorEosInsufficientCpu / Net / Ram after the node has explicitly refused the transaction. Those now hit the early return, so an EOS user short on CPU is told the transaction "may still have gone through" and that "sending again could result in a duplicate payment", and the slider locks for the life of the scene. Nothing was accepted, and the actual fix (stake more CPU) is not reachable without backing out of the whole send flow.
Splitting the two decisions keeps the design intact: lock unconditionally, as you do now, but let the existing errorCasted.name branches supply the copy when the engine has named a deterministic rejection, and fall through to Transaction Status Unknown for everything else.
For the record on the neighbouring branches: ErrorAlgoRecipientNotActivated is unaffected (Algorand throws it from signTx, before this line), and the 504 branch is correctly superseded, since a 504 during a broadcast is genuinely ambiguous.
| logActivity( | ||
| `Error ${ | ||
| broadcastSucceeded ? 'after' : 'during' | ||
| } broadcastTx (txid ${edgeTransaction.txid}): ${String(err)}` |
There was a problem hiding this comment.
edgeTransaction.txid is empty here for UTXO sends, which is the incident this PR exists for. makeSpend returns txid: '' (UtxoEngine.ts), and the real txid is assigned during signTx inside the engine, on the far side of the core bridge, so the scene's copy never receives it. signedTx holds it but is const-scoped inside the try.
The log line the error card sends the user hunting for therefore records no txid at all. Hoisting let signedTx: EdgeTransaction | undefined above the try and logging signedTx?.txid ?? edgeTransaction.txid fixes it.
| // slider floats over the bottom of the scroll view. Scroll it into | ||
| // view so the whole message is readable without scrolling by hand. | ||
| needsScrollToEnd.current = true | ||
| return |
There was a problem hiding this comment.
This path returns without calling onDone, so a send launched from a ramp sell or a gift-card purchase ends with no completion callback, and the locked slider means the user cannot reach one by retrying either. The flow is simply stranded on the send scene.
onDone already takes an error first (moonpayRampPlugin rethrows it, banxaRampPlugin and GiftCardPurchaseScene also accept it), so onDone?.(errorCasted) here would let the plugin flow terminate on its own terms while the scene keeps its honest messaging.
Not a regression against develop, where the generic error card skipped onDone too, but locking the slider is what makes it terminal.
j0ntz
left a comment
j0ntz
left a comment
There was a problem hiding this comment.
Correcting the verdict on my earlier review: those findings are blocking, not advisory.
The EOS path and the stranded onDone both change what a user sees after a send that failed, which is the behavior this PR exists to get right.
2 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
none
Requirements
If you have made any visual changes to the GUI. Make sure you have:
Description
GUI half of the "send failed in the UI but the money moved" incident. Asana: https://app.asana.com/1/9976422036640/project/1213880789473005/task/1217135300337949
Engine companion (independent, neither blocks the other): EdgeApp/edge-currency-plugins#458
One intended Bitcoin send became five real payments. Every broadcast succeeded over the HTTP fallback, the engine's
saveTxthen threw, andhandleSliderCompletetreated that throw like any other failure: a generic network error card plus afinallythat re-armed the slider. BecausesaveTxmarks the inputs spent before it throws, each retry re-quoted on the remaining UTXOs, so every slide was a fresh real payment.A failure reported by
broadcastTxdoes not prove the transaction is absent from the network. A server can relay it and still fail to answer, and the network conditions that broke the broadcast also make an immediate "did it land?" check unreliable. Polling was considered and rejected: it blocks the flow and any timeout is arbitrary. So the scene treats the broadcast call as a one-way boundary instead.The slider is now idempotent. A ref is set immediately before
broadcastTxis called. Once set, thefinallyblock never re-arms the slider, on success or failure, and the slider renders locked as "Send Attempted" for the life of the scene. The ref rather than state is what the handler reads, so the FIO no-bundled retry recursion cannot re-arm it either.Honest messaging replaces the generic error. Failures at or after the boundary show "Transaction Status Unknown", telling the user the transaction may have reached the network, to check a block explorer or wait for a confirmation email, and that sending again could produce a duplicate payment. Separate copy distinguishes a broadcast that reported failure from an error after a successful one. There is no fake happy path and no silent success.
Pre-broadcast failures are unchanged. PIN, the
beforeTransactionhook, the FIO fee check and signing all still show their existing errors and still re-arm the slider, because nothing can have been sent. The hook failure also now callsresetSlider()instead of leaving the slider spinning, which was a pre-existing bug.SafeSlidergains an explicitlockedTextprop for this. Overloading the existingdisabledprop was the first attempt and was wrong: a completed slider that a parent then disables would swap its spinner fordisabledText, which defaults to "Enter an Amount". That would have affected the other fourteen scenes usingSafeSliderwhenever a parent disables mid-callback, whichSendScene2itself can do viaprocessingAmountChangedandhasPendingTx.The locked-state card is longer than a normal error and the slider floats over the bottom of the scroll view, so the existing
needsScrollToEndmechanism scrolls it into view. That is a one-line reuse of the pattern four other handlers already use.Testing
Driven on the iOS simulator against the real send scene, with the broadcast stubbed so nothing could reach the network and using a zero-balance wallet:
saveTxthrows "No addresses to process" (the incident)The third and first cases were exercised on the same scene instance in sequence, so the scene re-armed after the pre-broadcast failure and then locked after the broadcast attempt.
Existing jest suite: 722 passing, including
SendScene2.ui.test.tsx.tscandeslintclean.Known gap
The lock is per scene instance. Backing out to the wallet and tapping Send again gives a fresh slider. That matches the direction agreed for this task, where the friction of re-entering the flow is the point, but it is not a hard guarantee against a determined second send.