docs(platform-wallet): clarify broadcast retry contract

[thepastaclaw]

Follow-up for CodeRabbit comments on #3985.

Changes:

• Clarifies the TransactionBroadcaster::broadcast error contract so only pre-send/not-accepted errors are retry-safe and ambiguous errors keep reservations.
• Extends the send_to_addresses_releases_reservation_on_broadcast_failure test to cover both pre-send variants allowlisted by broadcast_failure_is_pre_send: TransactionBroadcast and SpvError, across BIP44 and BIP32 accounts.

Validation:

• cargo test -p platform-wallet --lib wallet::core::broadcast
• cargo fmt --check
• git diff --check HEAD~1..HEAD

Closes CodeRabbit follow-up comments on #3985.

Summary by CodeRabbit

• Documentation

  • Clarified the broadcast error behavior so callers know when a failed send is safe to retry versus when they should keep reservations to avoid double-spend risks.

• Tests

  • Expanded broadcast retry coverage to validate both supported pre-send failure cases.
  • Verified that a failed first attempt can be retried successfully across account types.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 07935fce-df42-4cf1-8021-ca1db1b647d0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Documentation for TransactionBroadcaster::broadcast is updated to classify TransactionBroadcast and SpvError as observed pre-send rejections versus ambiguous transport failures. The FailFirstBroadcaster test helper is parameterized by an error factory, and its reservation-release test now covers both error variants across account types.

Changes

Broadcast error contract and test coverage

Layer / File(s)	Summary
Error contract documentation packages/rs-platform-wallet/src/broadcaster.rs	Trait documentation for broadcast clarifies that TransactionBroadcast and SpvError represent observed pre-send rejections allowing retry, while other errors are ambiguous and require keeping reservations.
Parameterized test helper and expanded coverage packages/rs-platform-wallet/src/wallet/core/broadcast.rs	FailFirstBroadcaster accepts an error factory function instead of a hardcoded error, and the reservation-release test iterates over both pre-send error variants and account types to verify retry behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: clarifying the platform-wallet broadcast retry contract.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Review complete (commit 7958536)

[thepastaclaw]

Code Review

Docs+test-only follow-up that tightens the TransactionBroadcaster::broadcast contract and extends the reservation-release test to cover both allowlisted pre-send variants (TransactionBroadcast, SpvError) across BIP44/BIP32 accounts. Verified the doc language matches broadcast_failure_is_pre_send and both broadcaster impls only emit variants within the allowlist. One suggestion: the new contract states 'the network never accepted the transaction' more strongly than DapiBroadcaster/SpvBroadcaster actually guarantee, since both collapse transport-level errors (post-send timeouts, connection drops) into the same variants used for genuine pre-send rejections.

Source: reviewers opus/opus, sonnet5/claude-sonnet-5, codex/gpt-5.5[high] for general; verifier opus/opus. Failed lanes: codex/gpt-5.5[high] general.

🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/broadcaster.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/broadcaster.rs:23-29: Contract claims a pre-send-only guarantee stronger than the implementations enforce
  The revised doc says `TransactionBroadcast`/`SpvError` mean the submission was "rejected pre-send — the network never accepted the transaction", but both concrete implementations of this trait collapse post-send transport errors into these same variants:

  - `DapiBroadcaster::broadcast` (broadcaster.rs:66-68) maps *any* error from `self.sdk.execute(...).into_inner()` — including a gRPC timeout or connection reset that could occur after Core has already accepted/relayed the transaction — to `PlatformWalletError::TransactionBroadcast`.
  - `SpvRuntime::broadcast_transaction` (spv/runtime.rs:104-107) wraps *any* error from `client.broadcast_transaction(tx)` (multi-peer P2P send) as `PlatformWalletError::SpvError`, with no distinction between "rejected before propagation" and "lost the connection after some peers received it".

  Callers relying on this contract will release reservations and retry on a genuine post-send transport failure, opening a narrow double-spend window. Because the entire point of this PR is to formalize the pre-send/ambiguous distinction, softening the language (e.g., "the broadcaster observed a rejection before the transaction was propagated to the network; note that transport-level failures that occur after propagation are currently also reported through these variants — see follow-up") would better match reality. The underlying fix — richer error variants from the broadcasters — is out of scope here.

[thepastaclaw]

🟡 Suggestion: Contract claims a pre-send-only guarantee stronger than the implementations enforce

The revised doc says TransactionBroadcast/SpvError mean the submission was "rejected pre-send — the network never accepted the transaction", but both concrete implementations of this trait collapse post-send transport errors into these same variants:

• DapiBroadcaster::broadcast (broadcaster.rs:66-68) maps any error from self.sdk.execute(...).into_inner() — including a gRPC timeout or connection reset that could occur after Core has already accepted/relayed the transaction — to PlatformWalletError::TransactionBroadcast.
• SpvRuntime::broadcast_transaction (spv/runtime.rs:104-107) wraps any error from client.broadcast_transaction(tx) (multi-peer P2P send) as PlatformWalletError::SpvError, with no distinction between "rejected before propagation" and "lost the connection after some peers received it".

Callers relying on this contract will release reservations and retry on a genuine post-send transport failure, opening a narrow double-spend window. Because the entire point of this PR is to formalize the pre-send/ambiguous distinction, softening the language (e.g., "the broadcaster observed a rejection before the transaction was propagated to the network; note that transport-level failures that occur after propagation are currently also reported through these variants — see follow-up") would better match reality. The underlying fix — richer error variants from the broadcasters — is out of scope here.

_{source: ['claude']}

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[lklimek]

@thepastaclaw let's limit this only to doc changes, no code / test changes please.

[thepastaclaw]

Code Review

Docs-only follow-up that softens the TransactionBroadcaster::broadcast error contract from a factual claim about network state to a prescriptive requirement on implementations. This addresses the prior review's overclaim concern. No new defects; the pre-existing implementation gap in DapiBroadcaster/SpvBroadcaster is out of scope for a docs-only PR.

Source: reviewers opus/opus, sonnet5/claude-sonnet-5, codex/gpt-5.5[high] (failed: no JSON object with findings/summary found) for general; verifier opus/opus.

[thepastaclaw]

Adjusted this to docs-only per your request. The PR now only changes packages/rs-platform-wallet/src/broadcaster.rs; the wallet broadcast test changes were removed in 7958536.

[thepastaclaw]

Code Review

Correction: I over-scoped the first body for this review to a parent-branch diff artifact instead of the PR's actual base branch. Rechecked against the current PR diff for 7958536c, which only changes packages/rs-platform-wallet/src/broadcaster.rs.

No carried-forward prior findings: the previous ab8805fe review found none. The latest delta removes the code/test expansion and keeps the PR docs-only per maintainer request. I found no in-scope issues in the current head. The broader DAPI/SPV error-classification gap is an implementation follow-up outside this doc-only PR.

Source: reviewers opus/opus, sonnet5/claude-sonnet-5, codex/gpt-5.5[high] for general; verifier opus/opus.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

Closing this helper PR as superseded. The target branch now includes 314540296d from #3985, which replaces the old PlatformWalletError documentation convention with a typed BroadcastError::{Rejected, MaybeSent} contract and more precise broadcaster behavior. CodeRabbit had no actionable comments on the docs-only state; the remaining issue was just the stale/conflicting helper branch.