Skip to content

wait for RPC hash before showing success screen#524

Merged
n13 merged 1 commit into
mainfrom
send_flow_improvements
Jun 14, 2026
Merged

wait for RPC hash before showing success screen#524
n13 merged 1 commit into
mainfrom
send_flow_improvements

Conversation

@n13

@n13 n13 commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator
  • send without waiting for server was an over-optimization which led to bad edge cases - if a tx hash is returned, the tx will most likely work
  • show full address in send confirmation - this is a security feature
Simulator Screenshot - iPhone 17e - 2026-06-14 at 11 09 31

this over optimization led to bad edge cases - if a tx hash is returned, the tx will most likely work
show full address in send confirmation - this is a security feature

@dewabisma dewabisma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@n13 n13 merged commit 2d8a7ee into main Jun 14, 2026
1 check passed
@n13

n13 commented Jun 14, 2026

Copy link
Copy Markdown
Collaborator Author

I've reviewed PR #524 against the actual code. Here's my assessment.

Summary

The PR makes two scoped changes:

  1. submitAndTrackTransaction now awaits the submission and the returned extrinsic hash before completing (previously fire-and-forget via unawaited(_submitAndTrackBackground(...))), and rethrows on failure so the caller surfaces the error instead of optimistically navigating to the success screen.
  2. review_send_screen.dart shows the full recipient address in the summary "To" row instead of the truncated form.

Overall this is a sound, well-reasoned change. A few things worth flagging.

What's good

  • Aligns with fail-early: errors now propagate to _confirmSend's catch and show sendReviewSubmitFailed, instead of being swallowed in a background future. This matches the project's "no silent failure" rule better than the old behavior.
  • Dead-code cleanup: the old failure branch did updateState(..., failed, ...) immediately followed by remove(...), so the failed-state write was pointless. The new code just remove(...) + rethrow. Cleaner.
  • The double-spend / single-sign retry safety comment is preserved, and the docstring is updated accurately.

Issues

1. Unguarded setState on the success path (should fix) — review_send_screen.dart:86

    try {
      ...
      await submissionService.balanceTransfer(...);
      unawaited(RecentAddressesService()...);
      setState(() {            // <-- no `mounted` check
        _submitting = false;
        _errorMessage = null;
      });

      if (mounted) {
        Navigator.push(...);
      }

Previously the await returned almost instantly, so unmount-during-await was effectively impossible. Now the await genuinely blocks on an RPC round-trip (plus up to 3 retries with backoff). That widens the unmount window a lot. If the widget unmounts during submission, this setState throws, gets caught by the catch at line 104, which logs Transfer failed and would show the failure message — i.e. a successful send gets routed into the failure path and the success screen is never shown. Recommend if (!mounted) return; before the success setState (the catch already guards its setState with mounted).

2. No bounded timeout now that the UI blocks on submit (consideration)

submitExtrinsic retries up to 3× with 500ms * attempt backoff across redundant endpoints (substrate_service.dart:181), relying on the underlying HTTP client's timeout. Since the confirm button now blocks on this, a hung/slow endpoint means the spinner can hang for a while. Given the "fail early" rule, consider a bounded .timeout(...) so the user gets a clear failure rather than an indefinite spinner.

3. Optimistic pattern still used elsewhere (consistency / out of scope?)

The multisig flows (proposeTransfer, approveProposal, executeProposal, cancelProposal, and multisig_submission_service._submitAndTrackBackground) still use the unawaited(...) fire-and-forget pattern this PR is explicitly moving away from. The same "over-optimization → bad edge cases" rationale applies to them. Fine to leave out of scope, but worth a follow-up so the codebase converges on one pattern.

4. Two address representations on one screen (minor/UI)

On the review screen the hero card renders a truncated address via AddressCheckphraseWithInitial (address_checkphrase_with_initial.dart:46 calls formatAddress), while the summary row now shows the full address. So the same address appears in two forms on the same screen, and the full SS58/qz… address will wrap to multiple lines in the right-aligned Flexible. The screenshot suggests this is acceptable, just confirm it's intentional rather than also updating the hero card.

Verdict

Approve with one recommended fix (the unguarded success-path setState in #1), and consider the timeout in #2. The behavior change itself is the right call.

Want me to apply the mounted guard fix (and optionally a submit timeout)?

n13 added a commit that referenced this pull request Jun 15, 2026
The blocking submit (#524) widens the unmount window while awaiting the
RPC. If the screen unmounts mid-submission, the unguarded setState threw
and was caught by the failure handler, reporting a successful send as
failed and skipping navigation to the success screen.
@dewabisma dewabisma deleted the send_flow_improvements branch June 22, 2026 03:42
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