fix(platform-wallet): deliver typed AddressNonceMismatch error across wallet, FFI, and Swift host#4047
Merged
Conversation
…llers
The optimistic address-nonce path submits `fetched + 1` by design and
relies on the caller retrying when Platform rejects a stale/replayed
value under a lagging DAPI replica read (Found-033). rs-sdk carries the
typed `AddressInvalidNonceError` (consensus code 40603) end-to-end
intact, but platform-wallet flattened it to a string at its
error-mapping seams, so callers could not recover `expected_nonce` to
honor their half of the optimistic-concurrency contract.
Add a public `as_address_invalid_nonce` extractor (mirroring
`as_asset_lock_proof_cl_height_too_low`, matching both the
`Protocol(ConsensusError)` and `StateTransitionBroadcastError` SDK
shapes), a first-class `PlatformWalletError::AddressNonceMismatch`
variant carrying `{address, provided_nonce, expected_nonce}`, and a
`promote_address_nonce_error` helper. Rewire the lossy string-collapse
seams (shield `enrich`, `broadcast_shielded_spend`,
`classify_spend_wait_failure`, identity `top_up_from_addresses`) to
promote via the helper when it matches, else keep today's exact string
fallback.
rs-sdk is untouched. The plain transfer/withdrawal paths already
preserve the typed error via `PlatformWalletError::Sdk`, from which the
now-public extractor can recover the nonce.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ecurse nonce extractor QA-001 (HIGH): the AddressNonceMismatch variant added at the wallet layer had no FFI mapping, so it both regressed and under-delivered at the C boundary hosts actually consume. - Regression: a shield nonce-rejection that used to surface as `ErrorShieldedBroadcastFailed` (code 16 — definitively failed, notes released, safe to retry) fell through `map_spend_result`'s catch-all to `ErrorWalletOperation` (code 6), dropping the retry-safety contract. - Fix: add a dedicated `ErrorAddressNonceMismatch = 21` result code (an additive enum slot) carrying the SAME definitively-failed / notes- released / safe-to-retry contract, and map to it in BOTH `map_spend_result` (covers shield/unshield/transfer/withdraw) AND the blanket `From<PlatformWalletError>` (covers identity `top_up_from_addresses`, which propagates via `?`/`.into()`). Hosts can now recognize the self- healing nonce race and retry — the retry re-fetches the address nonce. Nonce values are NOT exposed as structured out-fields: the shared `PlatformWalletFFIResult` is returned by value and mirrored by every language binding, so adding fields (or send-function out-params) would be an ABI-breaking change for all consumers. The submitted/expected nonces still travel in the result `message` (typed Display), and an FFI retry re-fetches the nonce anyway, so the practical loss is nil. QA-002 (LOW): `as_address_invalid_nonce` now recurses through `dash_sdk::Error::NoAvailableAddressesToRetry(inner)`, matching its sibling predicate `broadcast_definitely_failed` so the two stay in lockstep (latent today — the retry envelope never wraps a ConsensusError yet). Tests: AddressNonceMismatch -> ErrorAddressNonceMismatch via both the blanket From and map_spend_result; extractor unwraps the retry envelope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… test assertions QA-R2 #1 (MEDIUM): the Swift host binding — the only in-repo host — stopped at result code 20, so the new ErrorAddressNonceMismatch (=21) collapsed to `.errorUnknown` → `.unknown`, and the typed nonce-race fix never reached the shipped host. Mirror it in PlatformWalletResult.swift: add `case errorAddressNonceMismatch = 21` to `PlatformWalletResultCode`, its `init(ffi:)` arm (matching the cbindgen constant `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_ADDRESS_NONCE_MISMATCH`), a dedicated `PlatformWalletError.addressNonceMismatch(String)` case, and the `init(result:)` mapping — mirroring `errorShieldedBroadcastFailed`'s definitively-failed / notes-released / safe-to-retry semantics. Swift compilation was NOT verified: the cbindgen-generated header + DashSDKFFI module are produced by build_ios.sh (iOS toolchain, unavailable here); the change is pattern-exact against the existing arms and the `prefix_with_name + ScreamingSnakeCase` cbindgen config. QA-R2 #2 (LOW): the two new nonce-value FFI tests asserted bare digits (`contains('1') && contains('2')`), which a provided/expected transposition would pass. Pin the exact rendered substrings instead (`submitted nonce 1` / `Platform expected 2`) so a transposition fails. Per the ABI decision (keep dedicated-code-only), document at the blanket `From` mapping arm that structured provided/expected nonce out-fields are intentionally out of scope — PlatformWalletFFIResult is by-value/ABI-frozen; the values travel in the message and an FFI retry re-fetches the nonce. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claudius-Maginificent
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Contributor
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a new ChangesAddress Nonce Mismatch Error Handling
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Collaborator
|
✅ Review complete (commit 61d95f2) |
thepastaclaw
suggested changes
Jul 9, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers: claude opus general (failed), codex gpt-5.5 general (ok), claude opus security-auditor (failed), codex gpt-5.5 security-auditor (ok), claude opus rust-quality (failed), codex gpt-5.5 rust-quality (ok), claude opus ffi-engineer (failed), codex gpt-5.5 ffi-engineer (ok). Verifier: codex gpt-5.5.
The nonce-mismatch error plumbing is directionally correct, but it is incomplete across the address-funded wallet surface. Several public FFI-backed paths can still flatten the same retryable consensus error into generic wallet errors, and one shielded wait-side classifier fails to unwrap the retry envelope that the new extractor explicitly supports.
🔴 1 blocking | 🟡 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/wallet/platform_addresses/transfer.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:228-230: Preserve nonce mismatch errors across all address-funded paths
This transfer path still calls the auto-nonce SDK helper and propagates its error with bare `?`, so an `AddressInvalidNonceError` from `transfer_address_funds` is converted through `PlatformWalletError::Sdk` instead of the new `AddressNonceMismatch` variant. The same problem remains in withdrawal (`packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs:125`) and identity registration from addresses (`packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs:100`, where it is wrapped as `InvalidIdentityData`). These paths use the same `fetch_inputs_with_nonce` plus `nonce_inc` optimistic nonce flow that this PR is making retryable for shield/top-up, and they are exposed through FFI/Swift entry points, so callers still receive `ErrorUnknown` or a generic Swift error for the same retryable race. Promote `AddressInvalidNonceError` at these SDK error seams too, including the explicit-nonce variants where Platform can reject a caller-supplied stale nonce.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2295-2301: Unwrap retry envelope before classifying nonce rejection
`as_address_invalid_nonce` now recurses through `dash_sdk::Error::NoAvailableAddressesToRetry`, but shield wait-side handling only calls `promote_address_nonce_error` after `carries_consensus_rejection` returns true. Since this predicate does not unwrap `NoAvailableAddressesToRetry`, a wait error shaped like `NoAvailableAddressesToRetry(Protocol(ConsensusError::AddressInvalidNonceError(_)))` is treated as ambiguous `ShieldedSpendUnconfirmed` instead of the new retryable `AddressNonceMismatch`. Make the classifier compositional over the same wrapper so the new extractor can actually run on wrapped consensus rejections.
…ddress-funded call sites Deliver Platform's typed AddressInvalidNonceError (consensus 40603) as PlatformWalletError::AddressNonceMismatch consistently, matching the shield/top-up path, so callers can retry with expected_nonce instead of parsing a flattened string. - transfer.rs / withdrawal.rs: promote the nonce rejection at all six Explicit / ExplicitWithNonces / Auto call sites via a shared promote_address_nonce_error_or_sdk helper (Sdk fallback preserved). - register_from_addresses.rs: promote on top of the existing InvalidIdentityData fallback (matching its top_up sibling). - operations.rs: carries_consensus_rejection now recurses through NoAvailableAddressesToRetry, in lockstep with as_address_invalid_nonce, so a nonce mismatch in a retry envelope promotes instead of falling through to the ambiguous ShieldedSpendUnconfirmed bucket. Adds unit tests for the helper (both branches + retry envelope) and the classifier recursion (wrapped consensus / transport / nonce shapes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
@thepastaclaw — fixed in d3103c8. Ripped out the last few spots where a 40603 nonce rejection was still leaking out as a flat string instead of the typed
New unit tests cover the helper (both branches + retry envelope) and the classifier recursion (wrapped consensus / transport / nonce shapes). |
…ngth budget Apply coding-best-practices comment-length rules to the error.rs items and the operations.rs classifier note touched by the promotion fix: tighten the AddressNonceMismatch variant doc and promote_address_nonce_error_or_sdk to the 5-10 line pub-API budget, cut as_address_invalid_nonce from ~18 lines to essentials, and shrink the carries_consensus_rejection recursion note plus the inline retry-envelope comment to the internal budget. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Trim the private helper's recursion note from 3 to 2 lines (internal-comment budget), dropping the outcome-narrative tail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
|
Verified at current head What I checked:
Local verification:
|
QuantumExplorer
approved these changes
Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Extracted from #3692 (
feat/platform-wallet-rehydration) at the user's request: these three commits fix address-nonce mismatch error handling and are unrelated to watch-only wallet rehydration, so they belong in their own PR on top ofv4.1-dev.The optimistic address-nonce path submits
fetched + 1by design and relies on the caller retrying when Platform rejects a stale/replayed value under a lagging DAPI replica read (Found-033). The typedAddressInvalidNonceError(consensus code 40603) was getting flattened to a string at several mapping seams, so callers (including the FFI boundary and the Swift host binding) lost the ability to recognize this self-healing race and retry correctly.What was done?
packages/rs-platform-wallet: added a first-classPlatformWalletError::AddressNonceMismatch { address, provided_nonce, expected_nonce }variant, a publicas_address_invalid_nonceextractor, and apromote_address_nonce_errorhelper. Rewired the lossy string-collapse seams (shieldenrich,broadcast_shielded_spend,classify_spend_wait_failure, identitytop_up_from_addresses) to promote via the helper.packages/rs-platform-wallet-ffi: added a dedicatedErrorAddressNonceMismatch = 21FFI result code (additive), mapped in bothmap_spend_resultand the blanketFrom<PlatformWalletError>, preserving the definitively-failed / notes-released / safe-to-retry contract.as_address_invalid_noncenow also recurses throughdash_sdk::Error::NoAvailableAddressesToRetry.packages/swift-sdk: mirrored the new code 21 inPlatformWalletResult.swift(PlatformWalletResultCode.errorAddressNonceMismatch,init(ffi:),PlatformWalletError.addressNonceMismatch(String),init(result:)), and pinned the FFI nonce tests to exact rendered substrings instead of bare-digit substring checks.Nonce values are intentionally NOT exposed as structured FFI out-fields (ABI-frozen by-value result struct); they travel in the typed
Displaymessage, and a retry re-fetches the nonce anyway.How Has This Been Tested?
cargo check -p platform-wallet -p platform-wallet-fficargo fmt --check -p platform-wallet -p platform-wallet-fficargo clippy -p platform-wallet -p platform-wallet-ffi --all-targetsAddressNonceMismatchpromotion through both the blanketFromandmap_spend_result, and extractor recursion through the retry envelope.errorShieldedBroadcastFailedarm.Breaking Changes
None. The new FFI result code is an additive enum slot; existing codes are unchanged.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes