Skip to content

Lock the send slider once a broadcast has been attempted - #6190

Merged
peachbits merged 1 commit into
developfrom
matthew/send-broadcast-lock
Sep 14, 2026
Merged

peachbits merged 1 commit into
developfrom
matthew/send-broadcast-lock

Conversation

@peachbits

@peachbits peachbits commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

  • Tested on iOS device
  • Tested on Android device
  • Tested on small-screen device (iPod Touch)
  • Tested on large-screen device (tablet)

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 saveTx then threw, and handleSliderComplete treated that throw like any other failure: a generic network error card plus a finally that re-armed the 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 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 broadcastTx is called. Once set, the finally block 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.

The card is sticky and the scene does not call onDone with the error. Once a broadcast has been attempted the quote effect is frozen, the address-reset handler no longer clears error, and the quote-expiry callback is a no-op, so editing the amount or address, or the quote timing out, cannot remove the only explanation the user has for the locked slider. For gift cards onExpired navigates back, so without that guard expiry would also have popped the scene. The locked branch deliberately does not call onDone(error): the ramp launchers (paybis, moonpay) navigation.pop() the send scene when that promise rejects and show a generic failure, which would hide the card and return the user to a flow that can start another real payment. Those flows still terminate through the existing onBack-on-unmount path when the user leaves, as on develop.

Pre-broadcast failures are unchanged. PIN, the beforeTransaction hook, 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 calls resetSlider() instead of leaving the slider spinning, which was a pre-existing bug.

SafeSlider gains an explicit lockedText prop for this. Overloading the existing disabled prop was the first attempt and was wrong: a completed slider that a parent then disables would swap its spinner for disabledText, which defaults to "Enter an Amount". That would have affected the other fourteen scenes using SafeSlider whenever a parent disables mid-callback, which SendScene2 itself can do via processingAmountChanged and hasPendingTx.

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. 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. Run twice: once on the original commit, and again on an iPhone 17 Pro Max after the review fixups.

Scenario Result
Broadcast succeeds, then saveTx throws "No addresses to process" (the incident) Slider locks, post-broadcast copy shown, card auto-scrolled fully above the slider
Broadcast itself throws (ambiguous) Slider locks, ambiguous-failure copy shown
Broadcast throws an engine-named rejection (ErrorEosInsufficientCpu) Slider locks, the EOS-specific copy shows instead of "status unknown"
Signing throws before any broadcast Slider re-arms to "Slide to Confirm"; the same scene then locks on the next attempt
Editing the amount on a locked scene Card stays, lock stays, fee tile stays static (a fee-tile spinner found here is fixed in the last fixup)
Dragging the locked slider across the full track, both directions No movement, no second send

Existing jest suite: 722 passing, including SendScene2.ui.test.tsx. tsc and eslint clean.

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.


Note

Medium Risk
Changes core send UX and error handling where mistaken retries can cause duplicate payments, though scope is mostly SendScene2 plus an optional SafeSlider prop.

Overview
Prevents duplicate sends when the UI reports failure but a transaction may already be on the network (e.g. broadcast succeeds then saveTx throws).

Send confirmation is one-shot per scene visit. After signing, the scene marks a broadcast boundary before calling broadcastTx. The confirm slider stays completed and shows "Send Attempted" via a new SafeSlider lockedText prop—it no longer re-arms on success or ambiguous failure. Pre-broadcast errors (PIN, hooks, signing, deterministic node rejections like EOS CPU) still reset the slider as before; beforeTransaction hook failures now also reset the slider instead of leaving it spinning.

Messaging and sticky state. Ambiguous failures at or after broadcast use new "Transaction Status Unknown" copy (explorer / confirmation email, duplicate-payment warning), with separate strings for broadcast-time vs post-broadcast errors. Named engine rejections keep their specific messages. The scene freezes re-quoting, ignores quote expiry, and does not clear the error when resetting address fields; it avoids calling ramp onDone(error) so launchers do not pop the scene and hide the warning.

New locale strings and a CHANGELOG entry document the behavior.

Reviewed by Cursor Bugbot for commit 908d8a9. Bugbot is set up for automated code reviews on this repo. Configure here.

@j0ntz j0ntz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/components/scenes/SendScene2.tsx
Comment thread src/components/scenes/SendScene2.tsx Outdated
Comment thread src/components/scenes/SendScene2.tsx Outdated
// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed against the four callers (moonpay, banxa, fiatPlugin, GiftCardPurchaseScene). Fixed in 7dc8632: the locked branch now calls onDone(errorCasted) and only logs the callback's own rejection, since the scene already shows the error. The scene itself stays put so the honest message remains visible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reversing this in 39644cc, and re-opening the thread so you see why. Bugbot flagged it after the fixup and it checks out: paybis and moonpay both navigation.pop() the send scene when their onDone promise rejects, then reopen the provider webview with a generic showError. So onDone(error) here dismisses the Transaction Status Unknown card and drops the user back into the sell flow, free to start another payment, which is the MoonPay case from the incident notes. Your underlying point stands, the success callback is unreachable after a lock, but the flows are not stranded: backing out of the scene fires onBack on unmount, the launchers reject with SendErrorBackPressed and wind down quietly, exactly as on develop. A cleaner ending would be for the launchers to receive the real error when the user leaves rather than BackPressed, but that changes each launcher's post-pop behavior and needs per-flow testing, so I have left it as a follow-up rather than fold it in here.

@j0ntz j0ntz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@peachbits
peachbits force-pushed the matthew/send-broadcast-lock branch from b231419 to 7dc8632 Compare September 7, 2026 18:39

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.

Tip: disable this comment in your organization's Code Review settings.

@peachbits

Copy link
Copy Markdown
Contributor Author

Subject reworded to Lock the send slider after a broadcast attempt (46 chars). The three inline findings are addressed in fixup 7dc8632; details on each thread.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread src/components/scenes/SendScene2.tsx Outdated
Comment thread src/components/scenes/SendScene2.tsx
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.
@peachbits
peachbits force-pushed the matthew/send-broadcast-lock branch from c6d526c to 908d8a9 Compare September 14, 2026 17:52

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.

Tip: disable this comment in your organization's Code Review settings.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 908d8a9. Configure here.

Comment thread src/components/scenes/SendScene2.tsx
@peachbits
peachbits merged commit 327fc75 into develop Sep 14, 2026
7 checks passed
@peachbits
peachbits deleted the matthew/send-broadcast-lock branch September 14, 2026 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants