docs(platform-wallet): error-code registry for the FFI result space - #4261
docs(platform-wallet): error-code registry for the FFI result space#4261bfoss765 wants to merge 8 commits into
Conversation
Four open PRs allocate discriminants into PlatformWalletFFIResultCode in parallel, with no shared source of truth. A duplicate discriminant does not produce a textual merge conflict, so the second merge silently misclassifies errors on every host — one such collision (code 29) has already required a renumber directive in review. Adds packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md: the merged allocation table (0-26, 98, 99), the proposed allocations with owning PRs (27-33), the rule for claiming the next free integer, the ABI-stability rule (never renumber after release, deprecate instead), and the currently contested allocations. Links it from the crate README's error-handling section. Docs only — no enum, mapping, or mirror is changed. Renumbering remains the owning PRs' work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds an FFI error-code registry and documents result-code ABI stability in the wallet FFI README. The registry records assigned, reserved, proposed, conflicting, and historical code values, plus required SDK mirror updates. ChangesFFI ABI documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
|
⛔ Blockers found — Sonnet deferred (commit 7802342) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The registry captures several surveyed allocations correctly, but it is not yet safe to use as the authoritative source it claims to be. It omits active incompatible allocations and simultaneously marks code 30 both free and assigned; the Swift update rule, code-13 provenance, and duplicate-discriminant rationale also need factual corrections.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(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-ffi/ERROR_CODE_REGISTRY.md`:
- [BLOCKING] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:97-98: Registry omits incompatible allocations from active PRs
The open-PR inventory and survey provenance omit branches that modify this exact ABI enum. At current head `5931df745a`, #3968 assigns `ErrorPersisterTransient = 26`, `ErrorPersisterFatal = 27`, and renumbers the already-shipped `ErrorTransactionBroadcastRejected` from 26 to 28. At head `93d0bd49b`, #3954 assigns `ErrorShutdownIncomplete = 27`. This conflicts with shipped code 26 and with #4185's code-27 claim, while #3968 and #3954 also assign different meanings to 27. An existing Swift host would classify #3968's persister-transient code 26 as `.errorTransactionBroadcastRejected`, and its actual rejection code 28 would fall through `init(ffi:)` to `.errorUnknown`. #4259 at `64146a2bb6` should also be recorded as carrying the same code-31 allocation as #4183. The supposedly complete no-new-code inventory additionally omits open PRs #3417, #3549, #3992, and #4243. Reconcile the incompatible claims and recompute the next-free value before presenting this file as the source of truth.
- [BLOCKING] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:90-120: Code 30 is marked free even though the registry assigns it to #4185
The table marks 30 as `Unallocated`, and the later section explicitly calls it free, while the resolution of record assigns that value to #4185's `ErrorReservationWalletMismatch`; the stated next-free value of 34 also assumes that ownership. Rule 1 permits contributors to claim a gap when this file marks it free, so another PR could legitimately take 30 while #4185 follows the recorded renumber directive. Record 30 as allocated to #4185 and describe its current use of 29 only as stale surveyed-head state.
- [SUGGESTION] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:32-38: Require the actual C-to-Swift result-code switch
The generated C enum is first converted by `PlatformWalletResultCode.init(ffi:)` at `PlatformWalletResult.swift:75-138`. That switch has a default that maps an omitted native constant to `.errorUnknown`. The registry instead names a nonexistent `PlatformWalletResultCode.init(result:)`; the actual `init(result:)` belongs to the downstream `PlatformWalletError` conversion. A contributor could therefore add the Swift raw case and downstream error handling but omit `init(ffi:)`, losing the native code's identity before typed handling sees it. Require all three Swift locations explicitly.
| Open PRs that touch `rs-platform-wallet-ffi` but claim **no** new code: #4186, | ||
| #4191, #4194, #4195, #4240, #4251, #4258. |
There was a problem hiding this comment.
🔴 Blocking: Registry omits incompatible allocations from active PRs
The open-PR inventory and survey provenance omit branches that modify this exact ABI enum. At current head 5931df745a, #3968 assigns ErrorPersisterTransient = 26, ErrorPersisterFatal = 27, and renumbers the already-shipped ErrorTransactionBroadcastRejected from 26 to 28. At head 93d0bd49b, #3954 assigns ErrorShutdownIncomplete = 27. This conflicts with shipped code 26 and with #4185's code-27 claim, while #3968 and #3954 also assign different meanings to 27. An existing Swift host would classify #3968's persister-transient code 26 as .errorTransactionBroadcastRejected, and its actual rejection code 28 would fall through init(ffi:) to .errorUnknown. #4259 at 64146a2bb6 should also be recorded as carrying the same code-31 allocation as #4183. The supposedly complete no-new-code inventory additionally omits open PRs #3417, #3549, #3992, and #4243. Reconcile the incompatible claims and recompute the next-free value before presenting this file as the source of truth.
source: ['codex']
There was a problem hiding this comment.
Resolved in 690d464 — Registry omits incompatible allocations from active PRs no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Fixed in 9f3dab1 — the last outstanding part of this finding.
Re-checked each sub-claim against 2d2c6c8 rather than trusting the earlier auto-resolve:
- feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration #3968's 26/27/28 — already recorded in the non-conforming table (rows for
ErrorPersisterTransient = 26,ErrorPersisterFatal = 27, and the shippedErrorTransactionBroadcastRejectedrenumbered 26 → 28). Landed earlier; still correct at head5931df745a. - feat(platform-wallet)!: shared ThreadRegistry for coordinator lifecycle + shutdown UAF/data-loss fixes #3954's
ErrorShutdownIncomplete = 27— recorded, and resolved the other way: feat(platform-wallet)!: shared ThreadRegistry for coordinator lifecycle + shutdown UAF/data-loss fixes #3954 was closed and its work merged as feat(platform-wallet): registry-owned coordinator lifecycle with Rust-owned FFI callback contexts #4268, which is why 27 is in the merged table. - feat(platform-wallet): classic Dash message signing (signMessage) over FFI, JNI, Kotlin, and Swift #4259's carried code 31 — recorded in the inherited-code table. Head refreshed to
9336bdbb71. - The no-new-code inventory omitting feat(swift-sdk): use SPV-synced quorums for Platform proof verification #3417, test(platform-wallet): e2e framework + full test suite — triage pins, Found-*/PA-* guards, fail-closed persist, Stage-2 merge #3549, feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up) #3992 and fix(sdk)!: complete encrypted txMetadata parity #4243 — this part was not fixed. It is now.
The inventory was rebuilt on 2026-08-03 from each PR's actual file list plus the error.rs at its head, and it now reads #3417, #3549, #3992, #4186, #4191, #4194, #4195, #4243. Four entries were also removed, each recorded with its reason so they are not silently re-added: #4240 and #4251 touch no file under this crate at their heads; #4258 merged into v4.2-dev on 2026-08-03; #4264 is closed, with its error.rs change carried by #4243. #4243 is called out explicitly — it does modify error.rs, but only to map new wallet errors onto the existing ErrorInvalidParameter, and this list tracks integer claims, not file touches.
| | 29 | `ErrorReservationWalletMismatch` | #4185 | **Collision** — see below | | ||
| | 29 | `ErrorAssetLockInsufficientFunds` | #4184 | **Collision** — see below | | ||
| | 30 | — | — | **Unallocated.** Reserved in sibling comments only; see below | | ||
| | 31 | `ErrorSigningKeyUnavailable` | #4183 | In review (also carried by #4204) | | ||
| | 32 | `ErrorTransactionBuild` | #4247 | In review (also carried by #4256) | | ||
| | 33 | `ErrorTransactionSigning` | #4256 | In review | | ||
|
|
||
| Open PRs that touch `rs-platform-wallet-ffi` but claim **no** new code: #4186, | ||
| #4191, #4194, #4195, #4240, #4251, #4258. | ||
|
|
||
| ## Contested and pending | ||
|
|
||
| ### 29 — `ErrorReservationWalletMismatch` (#4185) vs `ErrorAssetLockInsufficientFunds` (#4184) | ||
|
|
||
| Both PR heads define code 29. This is the known collision: review on #4185 | ||
| directed that PR to keep #4184's `29 = ErrorAssetLockInsufficientFunds` and move | ||
| `ErrorReservationWalletMismatch` to 30. That renumber has not landed on #4185's | ||
| head, and #4256 (stacked downstream) carries the pre-renumber `29`. | ||
|
|
||
| Resolution of record: **#4184 keeps 29; #4185 moves to 30**, propagated through | ||
| the Rust enum, the FFI `From` mapping, Swift `PlatformWalletResult`, Kotlin | ||
| `DashSdkError` (+ `DashSdkErrorTest`), and the JNI rustdoc — plus #4256, which | ||
| inherits the value. | ||
|
|
||
| ### 30 — reserved in comments for a variant that no longer exists | ||
|
|
||
| `ErrorAssetLockCrossDomainConsentRequired` is named as the holder of 30 in | ||
| in-tree comments on #4183, #4204, and #4247/#4256's numbering rationale. It is | ||
| **not defined anywhere** — #4184, the PR that would have introduced it, does not | ||
| contain it after a re-scope. 30 is therefore free, and is the slot the #4185 | ||
| renumber above should take. The stale "reserved for the consent code" comments |
There was a problem hiding this comment.
🔴 Blocking: Code 30 is marked free even though the registry assigns it to #4185
The table marks 30 as Unallocated, and the later section explicitly calls it free, while the resolution of record assigns that value to #4185's ErrorReservationWalletMismatch; the stated next-free value of 34 also assumes that ownership. Rule 1 permits contributors to claim a gap when this file marks it free, so another PR could legitimately take 30 while #4185 follows the recorded renumber directive. Record 30 as allocated to #4185 and describe its current use of 29 only as stale surveyed-head state.
source: ['codex']
There was a problem hiding this comment.
Resolved in 690d464 — Code 30 is marked free even though the registry assigns it to #4185 no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Fixed in 9f3dab1.
The contradiction was real and still live at 2d2c6c8, despite the earlier auto-resolve on this thread. The table rows and the frontier paragraph said RESERVED, but two other places said the opposite:
- the code-30 section: "30 is free once more, and is deliberately not being reissued"
- the collision history: "28 and 30 stay free. Do not reissue them in this review cycle"
Under rule 1 ("Do not reuse a gap unless this file marks it free") those two sentences licensed exactly the claim the rest of the file forbids, so the document carried two allocation frontiers at once.
All four places now say reserved-not-free and say it the same way. The code-30 heading is now "30 — vacated, then RESERVED (not free)", the body distinguishes vacated from freed, and the collision-history paragraph ends with an explicit pointer back to rule 1: reusing a gap requires this file to mark it free, and this file marks neither 28 nor 30 free. The frontier paragraph makes the same statement rather than just asserting 38.
| 5. **Update the mirrors in the same PR**: the Rust enum, the Swift | ||
| `PlatformWalletResultCode` + its `init(result:)` switch, and — where the code | ||
| deserves typed handling — the Kotlin `fromPlatformWalletNative` mapping and | ||
| `DashSdkErrorTest`. Kotlin is allowed to be non-exhaustive: unmapped codes | ||
| fall through to `PlatformWallet.Generic(code, …)`, which preserves the | ||
| integer. Swift is exhaustive; an unmirrored code surfaces as | ||
| `.errorUnknown` there and loses its identity. |
There was a problem hiding this comment.
🟡 Suggestion: Require the actual C-to-Swift result-code switch
The generated C enum is first converted by PlatformWalletResultCode.init(ffi:) at PlatformWalletResult.swift:75-138. That switch has a default that maps an omitted native constant to .errorUnknown. The registry instead names a nonexistent PlatformWalletResultCode.init(result:); the actual init(result:) belongs to the downstream PlatformWalletError conversion. A contributor could therefore add the Swift raw case and downstream error handling but omit init(ffi:), losing the native code's identity before typed handling sees it. Require all three Swift locations explicitly.
| 5. **Update the mirrors in the same PR**: the Rust enum, the Swift | |
| `PlatformWalletResultCode` + its `init(result:)` switch, and — where the code | |
| deserves typed handling — the Kotlin `fromPlatformWalletNative` mapping and | |
| `DashSdkErrorTest`. Kotlin is allowed to be non-exhaustive: unmapped codes | |
| fall through to `PlatformWallet.Generic(code, …)`, which preserves the | |
| integer. Swift is exhaustive; an unmirrored code surfaces as | |
| `.errorUnknown` there and loses its identity. | |
| 5. **Update the mirrors in the same PR**: the Rust enum; the Swift | |
| `PlatformWalletResultCode`, its `init(ffi:)` switch that maps the generated C | |
| constants, and `PlatformWalletError` + its `init(result:)` switch; and — where | |
| the code deserves typed handling — the Kotlin `fromPlatformWalletNative` | |
| mapping and `DashSdkErrorTest`. Kotlin is allowed to be non-exhaustive: | |
| unmapped codes fall through to `PlatformWallet.Generic(code, …)`, which | |
| preserves the integer. Swift's `init(ffi:)` has an unknown-value fallback; | |
| omitting its mapping surfaces the new code as `.errorUnknown` and loses its | |
| identity. |
source: ['codex']
There was a problem hiding this comment.
Fixed in 7802342.
You're right that rule 5 named a PlatformWalletResultCode.init(result:) that does not exist — init(result:) belongs to the downstream PlatformWalletError. The gap you describe is real and #4204 is sitting in it right now: at d78b940a03 it has the raw case and the init(ffi:) arm but no PlatformWalletError case, so the exhaustive init(result:) no longer compiles.
Rather than list the three locations, rule 5 now enumerates them and states how each one fails, since the two failure modes are opposite and that is the part worth remembering:
PlatformWalletResultCode— the raw case.PlatformWalletResultCode.init(ffi:)— the arm mapping the generated C constant. Has adefault:yielding.errorUnknown, so omitting it compiles fine and silently loses the code's identity before typed handling sees it.PlatformWalletError— the typed case and itsinit(result:)arm. That switch is exhaustive with nodefault:, so adding (1) without this makes it non-exhaustive and the Swift package stops compiling.
Kotlin's non-exhaustive Generic(code, …) fallback is unchanged and now sits after the three Swift sites rather than being interleaved with them.
There was a problem hiding this comment.
Resolved in 690d464 — Require the actual C-to-Swift result-code switch no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| | 10 | `ErrorInvalidIdentifier` | | | ||
| | 11 | `ErrorMemoryAllocation` | | | ||
| | 12 | `ErrorUtf8Conversion` | | | ||
| | 13 | `ErrorArithmeticOverflow` | Reserved slot — declared, no in-tree producer; holds the number for the mapping arriving via #3549 | |
There was a problem hiding this comment.
💬 Nitpick: ErrorArithmeticOverflow already has an in-tree producer
The exact PR base already produces this code in packages/rs-platform-wallet-ffi/src/shielded_send.rs:204-213: platform_wallet_shielded_estimate_fee maps shielded fee-formula failures to ErrorArithmeticOverflow. That producer entered through commit c9f8ef57925 and is present on v4.1-dev; the current #3549 diff does not add this mapping. Calling the slot producerless and awaiting #3549 gives incorrect provenance for an ABI registry.
| | 13 | `ErrorArithmeticOverflow` | Reserved slot — declared, no in-tree producer; holds the number for the mapping arriving via #3549 | | |
| | 13 | `ErrorArithmeticOverflow` | Produced by `platform_wallet_shielded_estimate_fee` when shielded fee computation overflows | |
source: ['codex']
There was a problem hiding this comment.
Fixed in 9f3dab1.
Confirmed on v4.2-dev at 5d68612a45: packages/rs-platform-wallet-ffi/src/shielded_send.rs:210 returns PlatformWalletFFIResultCode::ErrorArithmeticOverflow, with the contract described at line 180. So the row's "declared, no in-tree producer" was wrong, and crediting #3549 with the eventual mapping was misleading.
The row now names shielded_send.rs as the producer. It also records that the variant's own rustdoc in error.rs still calls itself a reserved slot with no producer — that comment is stale for the same reason, and should be corrected by whichever PR touches it next. Left as a note rather than a code change since this PR is documentation-only.
There was a problem hiding this comment.
Resolved in 690d464 — ErrorArithmeticOverflow already has an in-tree producer no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| This file exists because several feature branches allocate into the same | ||
| integer range in parallel. A duplicate discriminant in two branches does **not** | ||
| produce a textual merge conflict — the second merge silently misclassifies | ||
| errors on every host — so allocations have to be reconciled here, in one place, | ||
| rather than in each branch's diff. |
There was a problem hiding this comment.
💬 Nitpick: Duplicate Rust discriminants do not silently reach runtime
A combined Rust enum containing two variants with the same explicit value fails compilation with E0081, and Swift rejects duplicate raw enum values as well. A textual merge can therefore leave an invalid tree, but that tree cannot successfully build and silently misclassify runtime errors. The actual ABI hazard is that independent branches or releases can reuse or renumber an integer relative to hosts already compiled against another meaning, as #3968 currently does with shipped code 26. State that cross-branch and cross-version failure mode instead.
| This file exists because several feature branches allocate into the same | |
| integer range in parallel. A duplicate discriminant in two branches does **not** | |
| produce a textual merge conflict — the second merge silently misclassifies | |
| errors on every host — so allocations have to be reconciled here, in one place, | |
| rather than in each branch's diff. | |
| This file exists because several feature branches allocate into the same | |
| integer range in parallel. A combined Rust enum with duplicate discriminants | |
| fails to compile, but independent branches can still reuse or renumber an ABI | |
| value relative to a host that has already compiled the other meaning. Such a | |
| version mismatch silently misclassifies errors on that host, so allocations | |
| have to be reconciled here, in one place, rather than in each branch's diff. |
source: ['codex']
There was a problem hiding this comment.
Fixed in 9f3dab1.
You are right, and the file contradicted itself on it: the preamble said a duplicate discriminant "silently misclassifies errors on every host", while the code-32 section said the opposite — "this one was not a paper conflict… produced a hard error[E0081]: discriminant value 32 assigned more than once".
The preamble now splits the two shapes explicitly:
- Two different variant names on one integer — the merged enum fails to compile with E0081. Loud, but only after someone merges both branches into one tree; neither branch's own CI sees it, because neither branch contains both variants. This is how the code-32 collision was caught.
- The same meaning moving to a different integer, or a host mirror left un-updated — nothing fails to compile, and a shipped host silently reads the new integer as whatever the old one meant. This is the failure the file mainly exists to prevent.
The collision-history paragraph that repeated the old claim was corrected the same way: it now says neither compiler ever sees the E0081 because neither tree contains both variants, rather than implying duplicates are inherently silent.
There was a problem hiding this comment.
Resolved in 690d464 — Duplicate Rust discriminants do not silently reach runtime no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
bfoss765
changed the title
…t + finalize (dashpay#4185 review) Wallet removal was not linearized with the deferred-payment registry, so a retained handle could push a removed wallet's payment onto the network. Two independent windows: 1. remove-then-sweep. `platform_wallet_manager_remove_wallet` called `manager.remove_wallet` and only afterwards swept the registry, with no shared lock spanning the two — and the removal's own awaits (shielded coordinator + identity-sync unregistration) sat in the gap. A concurrent `core_wallet_signed_payment_broadcast` in that window passed every guard: `is_same_generation` compares two handles, so a removed generation matches itself; `last_processed_height` is `None` once the wallet is gone and `reservation_expired` maps `None` to "not expired"; and `broadcast_payment_releasing_reservation` has no wallet-existence gate. 2. in-flight finalizer. `finalize_transaction` drops the manager write lock before awaiting the signer, and `register` only validates the payment against its finalizing generation — never that the generation still exists. A removal during the signer await swept the registry, then the finalizer inserted a fresh token no later sweep would catch, contradicting the documented teardown invariant that dropping tokens makes stale handles inert. Remedies: * `SignedPaymentRegistry` gains a lifecycle gate (`tokio::RwLock`). Teardown takes the exclusive side across BOTH the manager removal and the sweep, making them one linearization point; broadcast and release take the shared side for their whole duration. The existing `entries` mutex cannot do this — it is dropped before every await by design. Lock order is always gate then manager. * Broadcast rejects an absent current generation via the new `CoreWallet::is_current_generation`, returning `SignedPaymentError:: WalletRemoved` instead of silently proceeding to the broadcaster. * `core_wallet_signed_payment_finalize` holds the shared gate across its liveness check and the synchronous `register`, abandoning the payment (reconciling its reservation) if the wallet went away during signing. The gate is taken after the signer await, not around it, so an open signing prompt cannot stall teardown. No new FFI error code: the wallet-removed case is reported as the existing `NotFound` (98), which both hosts already map. Deliberately avoids the 29/30 renumbering contested in dashpay#4261. Swift/Kotlin/Rust docs updated to record that 98 now also carries this case, and how it differs from `ErrorReservationWalletMismatch` (29). Adds three FFI regression tests. All three fail against the pre-fix code — the race test reports a payment reaching the broadcaster after teardown completed. Also serializes the registry-count-asserting tests, which the new tests would otherwise race in the shared process-global registry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ollisions The 29 collision is resolved and the renumber has now landed on dashpay#4185's branch: dashpay#4184 keeps 29 (ErrorAssetLockInsufficientFunds), dashpay#4185 takes 30 (ErrorReservationWalletMismatch). Table rows updated to match the code. Fixes the "30 is both free and assigned" inconsistency: the next-free line claimed 27-33 were claimed while the table showed 30 unallocated. 30 is now genuinely allocated to dashpay#4185, so the two agree. Adds allocations the survey had omitted, verified 2026-08-01 by reading error.rs at the head of all 62 open PRs: - dashpay#3968 numbers 26/27/28 (Persister* + a pre-merge TransactionBroadcastRejected) -> contradicts merged ABI at 26 and collides with dashpay#4185 at 27 and 28 - dashpay#3954 numbers ErrorShutdownIncomplete = 27 -> collides with dashpay#4185 at 27 - dashpay#4259 carries ErrorSigningKeyUnavailable = 31, inherited from dashpay#4183 rather than a new allocation The same sweep confirms no open PR anywhere defines a code 30.
…30 (dashpay#4185 review) Code 29 collided with `ErrorAssetLockInsufficientFunds` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and this PR moves to 30. Verified 30 was genuinely free by reading `rs-platform-wallet-ffi/src/error.rs` at the head of all 62 open PRs: no PR defines a code 30. The `ErrorAssetLockCrossDomainConsentRequired` that in-tree comments name as 30's holder does not exist anywhere after dashpay#4184's re-scope. The discriminant is public ABI, so every mirror moves together: - Rust enum + its three rustdoc cross-references (error.rs) - two doc references in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value + doc - Kotlin fromPlatformWalletNative branch, class KDoc, code-98 comment, WalletManagerNative KDoc, and the DashSdkErrorTest offset assertion Both Swift switches are symbolic (cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants), so only the enum raw value carried the number. Also disarms the NativeCleaner backstop in SignedCoreTransactionTest by closing the SignedCoreTransaction, so the armed native release cannot fire from the cleaner thread in a pure-JVM test. Note: dashpay#4256 is stacked downstream and still carries the pre-renumber 29; it must adopt 30 on rebase.
…cord dashpay#4196 scope Clears the two review blockers on dashpay#4261 and re-syncs the registry with what the code on each branch actually does, re-read at every head rather than trusted from this file. Blocker (a) — dashpay#3968 / dashpay#3954 / dashpay#4259 were described in prose but had no rows, which is exactly what rule 2 forbids. They now have them: - A "Non-conforming allocations" table for dashpay#3968 (26/27/28) and dashpay#3954 (27). These are deliberately kept out of the proposed table: each row is a claim to be withdrawn and reissued, not an allocation of record. - An inherited-code table for the 31 that dashpay#4204 and dashpay#4259 carry but did not allocate (dashpay#4183 owns it), so it is not double-counted. - dashpay#4196 is recorded as claiming no integer at all: it routes a new token-less `StaleReservation` variant through the existing `ErrorStaleReservationToken`. The dashpay#3968 half is the serious one and is called out as such. Its 28 is not a new claim — it *moves the already-shipped* `ErrorTransactionBroadcastRejected` off 26 to make room for its own persister code. Rule 3 forbids that: a host compiled against merged ABI returns 26 for a broadcast rejection, and after dashpay#3968 the same condition returns 28 while 26 means a transient persister failure. Neither branch's diff shows the contradiction. Blocker (b) — 30 marked both free and assigned was already resolved by the preceding commit; verified consistent here (30 is allocated to dashpay#4185 throughout, frontier is 34, and the one remaining "genuinely free" is past tense explaining why dashpay#4185 could take it). Also corrected, all verified against the branches: - Survey provenance had dashpay#4185 at `0b0d5c76d6` labelled "(post-renumber)". Wrong twice: that commit is the *parent* of the renumber `d854debb`, and the head has since moved to `6c37e8679e`. dashpay#4184, dashpay#4247 and dashpay#4256 SHAs refreshed too. - dashpay#4256 has now taken 30 (`9481e5783b`) and dropped its stale "30 is reserved for the consent code" rationale; the equivalent comments on dashpay#4183 and dashpay#4204 are flagged as still present. - dashpay#4184 has a comment-only drift: it reserves "Codes 27-28" but names three codes. Correct when the trio was 27/28/29; it is now 27/28/30. Its discriminant is right and is the resolution of record — only the prose is stale, and dashpay#4184 is left untouched. - The dashpay#4196 section now records why the restack has not happened: its three own commits conflict in 3 files / 10 hunks against dashpay#4185's head, and the registry redesign underneath it (mandatory `registered_height`, new `WalletRemoved` variant, owner-stamped funding token) makes it author work rather than conflict resolution. Its trio numbers come from the dashpay#4185 copy it carries, so the restack fixes 28 -> 30 for free; the number dashpay#4196 itself must chase is 27, not 30. Verified: cargo fmt --all -- --check clean; cargo test -p platform-wallet-ffi -p platform-wallet = 738 passed / 0 failed. Docs-only change.
…30 (dashpay#4256) dashpay#4256 is stacked on dashpay#4185 and still carried the pre-renumber `29`, which now collides with `ErrorAssetLockInsufficientFunds = 29` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and the `ErrorReservationWalletMismatch` family moves to 30; dashpay#4185 already made that move in `d854debb`. This brings dashpay#4256 in line. CI could not have caught this: dashpay#4256 and dashpay#4184 are both MERGEABLE with green checks, because two branches assigning the same discriminant produce no textual conflict. It surfaces only as an E0081 after a textual merge, or silently as a wrong error code on the host. The discriminant is public ABI, so every mirror moves together: - Rust enum + its rustdoc cross-reference (error.rs) - doc reference in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value - Kotlin fromPlatformWalletNative branch, class KDoc, WalletManagerNative KDoc, and the DashSdkErrorTest offset assertion Both Swift switches are symbolic (cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants), so only the enum raw value carried the number. Also corrects this PR's own numbering rationale on `ErrorTransactionSigning` (33), which claimed 30 was "reserved for dashpay#4184's ErrorAssetLockCrossDomainConsentRequired". That code does not exist on any branch — dashpay#4184 dropped it in a re-scope — and 30 is now ErrorReservationWalletMismatch. dashpay#4256's codes are unchanged otherwise: it keeps 32 (ErrorTransactionBuild, shared with dashpay#4247) and 33. Verified: cargo fmt --all -- --check clean; cargo test -p platform-wallet-ffi -p platform-wallet = 805 passed / 0 failed; :sdk:test BUILD SUCCESSFUL with DashSdkErrorTest 9/9. Swift is unverified — it cannot be compiled here.
|
Both blockers addressed in (b) "code 30 marked both free and assigned" — resolved. The contradiction was internal to the doc: the next-free line claimed 27–33 were taken while the table row showed 30 unallocated. 30 is now recorded as allocated to the reservation-mismatch code, which is where it actually landed (#4185 (a) omitted allocations — added rows for #3968, #3954 and #4259, with corrected provenance. Verified by reading Recorded but not fixed, since they belong to other PRs:
Structural note worth capturing somewhere more durable than a doc: duplicate discriminants across branches produce no textual conflict, so two PRs can both be MERGEABLE and green while claiming the same integer — #4256 and #4184 were in exactly that state until today. This registry only helps if it is consulted; nothing enforces it.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The two former blocking allocation defects are fixed: the registry now records the incompatible and inherited open-PR allocations, and code 30 is consistently assigned to #4185. No blocking issue remains, but the document still contains three procedural or inventory inaccuracies and four smaller provenance or rationale errors that should be corrected before it serves as the authoritative ABI ledger.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed)
🟡 3 suggestion(s) | 💬 1 nitpick(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-ffi/ERROR_CODE_REGISTRY.md`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:114-119: Do not classify #3968 as a stale-base rebase
The surveyed #3968 head `5931df745a` already contains the registry's exact base `ed4116b26c`: `git merge-base` returns `ed4116b26c`, and merge commit `debf67bdae` brought that base into the branch. Its diff from that base explicitly adds persister codes 26 and 27 and moves `ErrorTransactionBroadcastRejected` from 26 to 28. The section heading, stale-base explanation, and instruction at lines 224-225 therefore prescribe a rebase that has already occurred. The substantive remedy stated elsewhere is correct: #3968 must edit the enum to restore broadcast rejection to 26 and assign fresh values to both persister codes.
- [SUGGESTION] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:235-241: List every numeric reference owned by #4196
Comparing #4196's three own commits using `911c8f7264..ea4f783490` shows three changed or added references to native code 26 that must become 27 during the restack: the mapping-test rustdoc in `rs-platform-wallet-ffi/src/error.rs`, the rewritten `StaleReservationToken` KDoc in `DashSdkError.kt`, and the public V2 broadcast KDoc in `ManagedCoreWallet.kt`. Conversely, the `PlatformWalletError::StaleReservation` variant only refers to the FFI code symbolically and contains no number. The current two-reference description can leave public Kotlin boundary documentation advertising code 26 after the runtime mapping moves to 27.
- [SUGGESTION] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:98-99: Make the no-new-code PR inventory complete and accurate
The list does not match the open PRs that modify this crate. GitHub's exact PR file lists show that #3417, #3549, #3992, #4243, and #4264 modify `packages/rs-platform-wallet-ffi` without adding a result-code integer, but they are omitted. #4243 and #4264 even modify `error.rs`, intentionally mapping new wallet errors to the existing `ErrorInvalidParameter` code. Conversely, the exact surveyed heads for listed PRs #4240 and #4251 contain no file under this crate. These discrepancies do not affect the next-free integer, but they make the registry's open-PR survey incomplete and inaccurate.
| ### Non-conforming allocations (rebase required) | ||
|
|
||
| These branches allocate into the same range from a stale base. They are listed | ||
| here rather than in the proposed table because their numbers cannot stand as | ||
| written — each row is a claim to be **withdrawn and reissued**, not an | ||
| allocation of record. |
There was a problem hiding this comment.
🟡 Suggestion: Do not classify #3968 as a stale-base rebase
The surveyed #3968 head 5931df745a already contains the registry's exact base ed4116b26c: git merge-base returns ed4116b26c, and merge commit debf67bdae brought that base into the branch. Its diff from that base explicitly adds persister codes 26 and 27 and moves ErrorTransactionBroadcastRejected from 26 to 28. The section heading, stale-base explanation, and instruction at lines 224-225 therefore prescribe a rebase that has already occurred. The substantive remedy stated elsewhere is correct: #3968 must edit the enum to restore broadcast rejection to 26 and assign fresh values to both persister codes.
source: ['codex']
There was a problem hiding this comment.
Resolved in e915e5c — Do not classify #3968 as a stale-base rebase no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Addressed in 9f3dab1, with a correction to the premise.
Your merge-base check is right and I reproduced it: git merge-base 5931df745a ed4116b26c returns ed4116b26c, so #3968's head does contain the base this file was originally compiled against. Calling it a stale-base branch on that basis was wrong.
But the base has since moved. git merge-base --is-ancestor 5d68612a45 5931df745a fails — #3968 does not contain the current v4.2-dev head, which is where #4268's merged ErrorShutdownIncomplete = 27 lives. So a rebase is genuinely outstanding; it just is not the remedy.
The section heading is now "Non-conforming allocations (withdraw and reissue)" instead of "(rebase required)", and it opens with exactly that distinction: the rebase is a precondition, #3968 is behind the current base rather than un-rebased, and rebasing resolves nothing on its own because git sees no conflict in any of it — the branch has to edit its own enum. Which is the substantive remedy you identified, and it is unchanged.
| **All three of those numbers come from the copy of #4185 that #4196 carries, not | ||
| from #4196's own commits.** Restacking onto #4185's head therefore fixes the | ||
| trio for free — including `ErrorReservationWalletMismatch` 28 → 30, which #4196 | ||
| never had to move itself. The one number #4196 does own is a doc reference: its | ||
| `StaleReservation` variant and the matching Kotlin KDoc both cite | ||
| `ErrorStaleReservationToken` as **26**, and that becomes **27** post-restack. | ||
| So the number #4196 must chase is 27, not 30. |
There was a problem hiding this comment.
🟡 Suggestion: List every numeric reference owned by #4196
Comparing #4196's three own commits using 911c8f7264..ea4f783490 shows three changed or added references to native code 26 that must become 27 during the restack: the mapping-test rustdoc in rs-platform-wallet-ffi/src/error.rs, the rewritten StaleReservationToken KDoc in DashSdkError.kt, and the public V2 broadcast KDoc in ManagedCoreWallet.kt. Conversely, the PlatformWalletError::StaleReservation variant only refers to the FFI code symbolically and contains no number. The current two-reference description can leave public Kotlin boundary documentation advertising code 26 after the runtime mapping moves to 27.
| **All three of those numbers come from the copy of #4185 that #4196 carries, not | |
| from #4196's own commits.** Restacking onto #4185's head therefore fixes the | |
| trio for free — including `ErrorReservationWalletMismatch` 28 → 30, which #4196 | |
| never had to move itself. The one number #4196 does own is a doc reference: its | |
| `StaleReservation` variant and the matching Kotlin KDoc both cite | |
| `ErrorStaleReservationToken` as **26**, and that becomes **27** post-restack. | |
| So the number #4196 must chase is 27, not 30. | |
| **All three reservation-trio values come from the copy of #4185 that #4196 | |
| carries, not from #4196's own commits.** Restacking onto #4185's head therefore | |
| fixes the trio for free — including `ErrorReservationWalletMismatch` 28 → 30. | |
| The numeric references in #4196's own changed hunks still need explicit updates: | |
| the mapping-test rustdoc in `rs-platform-wallet-ffi/src/error.rs`, the rewritten | |
| `StaleReservationToken` KDoc in `DashSdkError.kt`, and the public V2 broadcast | |
| KDoc in `ManagedCoreWallet.kt` all cite **26** and must become **27**. The | |
| `PlatformWalletError::StaleReservation` variant itself contains no numeric | |
| reference. Thus #4196 allocates nothing, but it must update all three documents | |
| to code 27 during the restack. |
source: ['codex']
There was a problem hiding this comment.
Addressed in 7802342, though the finding has been overtaken by events in the best way.
#4196 has restacked. Its head is now 12492e8c54, not ea4f783490. Verified: ErrorStaleReservationToken = 34, ErrorReservationTokenConsumed = 35, ErrorReservationWalletMismatch = 36, ErrorShutdownIncomplete = 27 present from the merged base, #4185's head 8813e98533 is an ancestor, and the PR is MERGEABLE against v4.2-dev.
So the target number is 34, not the 27 the suggestion assumed — that was written before the trio's third move — and the references you identified were carried along with the restack rather than needing separate updates. Confirmed at 12492e8c54:
DashSdkError.kt—StaleReservationTokenKDoc reads "native code 34";fromPlatformWalletNativemaps34 -> PlatformWallet.StaleReservationToken.ManagedCoreWallet.kt— V2 broadcast KDoc reads "native code 34, shared with the deferred-token surface"; the remaining mentions are symbolic[StaleReservationToken]links with no number.PlatformWalletError::StaleReservation— symbolic only, no number, exactly as you noted.
Your substantive point stands and is recorded: the file previously described two references when #4196 owned three (including the ManagedCoreWallet.kt public boundary KDoc), and named the variant as one of them when it carries no number. That inventory is corrected, and the section is now a resolution rather than an open item. The account of why the restack was hard — #4185's registered_height, WalletRemoved, and the in-place rewrite of RESERVATION_MAX_AGE_BLOCKS — is kept, since that was the substance of the delay.
| Open PRs that touch `rs-platform-wallet-ffi` but claim **no** new code: #4186, | ||
| #4191, #4194, #4195, #4240, #4251, #4258. |
There was a problem hiding this comment.
🟡 Suggestion: Make the no-new-code PR inventory complete and accurate
The list does not match the open PRs that modify this crate. GitHub's exact PR file lists show that #3417, #3549, #3992, #4243, and #4264 modify packages/rs-platform-wallet-ffi without adding a result-code integer, but they are omitted. #4243 and #4264 even modify error.rs, intentionally mapping new wallet errors to the existing ErrorInvalidParameter code. Conversely, the exact surveyed heads for listed PRs #4240 and #4251 contain no file under this crate. These discrepancies do not affect the next-free integer, but they make the registry's open-PR survey incomplete and inaccurate.
| Open PRs that touch `rs-platform-wallet-ffi` but claim **no** new code: #4186, | |
| #4191, #4194, #4195, #4240, #4251, #4258. | |
| Open PRs that touch `rs-platform-wallet-ffi` but claim **no** new code: #3417, | |
| #3549, #3992, #4186, #4191, #4194, #4195, #4243, #4258, #4264. |
source: ['claude', 'codex']
There was a problem hiding this comment.
Fixed in 9f3dab1.
Rebuilt the inventory on 2026-08-03 from each PR's file list and the error.rs at its head rather than applying the suggestion verbatim, because two of its entries had moved since it was written:
#4264is closed. Itserror.rschange — mapping new wallet errors onto the existingErrorInvalidParameter— is carried by#4243, which is open and is in the list.#4258has merged intov4.2-dev(ce8233edb7). It claimed no code, so the merged table is unchanged, but it is no longer an open PR.
The list is now #3417, #3549, #3992, #4186, #4191, #4194, #4195, #4243. Your two removals are applied as written: #4240 and #4251 touch no file under this crate at their heads. All four removals are recorded in a short table with the reason for each, so a later pass does not re-add them from an older revision of this file. #4243's error.rs involvement is called out explicitly for the same reason — touching error.rs is not the same as claiming an integer.
| `ErrorAssetLockCrossDomainConsentRequired` is named as the holder of 30 in | ||
| in-tree comments on #4183, #4204, and #4247/#4256's numbering rationale. It is | ||
| **not defined anywhere** — #4184, the PR that would have introduced it, does not | ||
| contain it after a re-scope. | ||
|
|
||
| Verified 2026-08-01 by reading `packages/rs-platform-wallet-ffi/src/error.rs` at | ||
| the head of **every one of the 62 open PRs**: no PR anywhere defines a code 30. | ||
| 30 was therefore genuinely free, and #4185 has taken it. The stale "reserved for |
There was a problem hiding this comment.
💬 Nitpick: Qualify the code-30 sweep as pre-allocation
The exact surveyed heads later cited for #4185 (6c37e8679e) and #4256 (9481e5783b) both define ErrorReservationWalletMismatch = 30, so the unqualified statement that no open PR defines code 30 is false for the recorded survey. The cited #4247 head 0dcdc743e7 also does not mention ErrorAssetLockCrossDomainConsentRequired; the stale reservation appears on #4183, #4204, and #4256's pre-renumber rationale. The intended conclusion remains valid—no unrelated PR competes with #4185 for 30—but the provenance needs to distinguish the stale historical reservation from the post-allocation surveyed state.
| `ErrorAssetLockCrossDomainConsentRequired` is named as the holder of 30 in | |
| in-tree comments on #4183, #4204, and #4247/#4256's numbering rationale. It is | |
| **not defined anywhere** — #4184, the PR that would have introduced it, does not | |
| contain it after a re-scope. | |
| Verified 2026-08-01 by reading `packages/rs-platform-wallet-ffi/src/error.rs` at | |
| the head of **every one of the 62 open PRs**: no PR anywhere defines a code 30. | |
| 30 was therefore genuinely free, and #4185 has taken it. The stale "reserved for | |
| `ErrorAssetLockCrossDomainConsentRequired` is named as the holder of 30 in | |
| in-tree comments on #4183 and #4204, and in #4256's pre-renumber numbering | |
| rationale. It is **not defined anywhere** — #4184, the PR that would have | |
| introduced it, does not contain it after a re-scope. | |
| At the surveyed heads, #4185 and its downstream #4256 define | |
| `ErrorReservationWalletMismatch = 30`; no unrelated open PR defines code 30. | |
| The stale consent-code reservation therefore does not conflict with #4185's | |
| allocation. |
source: ['codex']
There was a problem hiding this comment.
Fixed in 7802342.
Correct on both counts, and the first one is the kind of overstatement that undermines the rest of the file. "No PR anywhere defines a code 30" was false for the very heads the provenance cites — #4185 and its downstream #4256 both defined ErrorReservationWalletMismatch = 30 at the surveyed heads. That was the allocation, not a competing claim, but the sentence did not say so.
It now reads: no PR unrelated to #4185 defines a code 30 — which is the claim that actually supports the conclusion, stated as a qualification rather than left implicit. The sweep result is unchanged: nothing contested 30, #4185's claim stood, and the stale consent-code reservation never conflicted with it.
The list of branches carrying that stale reservation is corrected to #4183 and #4204, plus #4256's pre-renumber rationale. #4247 was never one of them.
One update since you wrote this: #4183's stale comment is gone as of its 2026-08-03 rebase, so #4204 is now the only branch still carrying one. That is recorded in the same section.
There was a problem hiding this comment.
Resolved in e915e5c — Qualify the code-30 sweep as pre-allocation no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev ABI on 2026-08-02, taking the number dashpay#4185 had held. dashpay#4185 and dashpay#4256 moved the deferred-token trio to the contiguous block 34-36 in response. Registry changes: * 27 enters the merged table, owned by dashpay#4268. * The proposed table moves the trio to 34/35/36 and marks 28 and 30 free but deliberately not reissued. Next free integer is now 37. * New "Collision history" section records all three numberings of the trio (26/27/28 -> 27/28/30 -> 34/35/36) and, more usefully, corrects this file's own reasoning: on 2026-08-01 it recorded dashpay#3954's `ErrorShutdownIncomplete = 27` as a non-conforming claim that had to be withdrawn because dashpay#4185's claim was older. Seniority among open PRs does not decide an ABI number — merging does. dashpay#3954 was closed, its work landed as dashpay#4268, and 27 is now merged ABI. The trio therefore moved above every claimed number rather than into the next free gap, so nothing currently in flight can hit it again. * dashpay#3968's 27 is re-characterised: it was a proposed-vs-proposed collision, and is now a contradiction of merged ABI. Its frontier is 37+. * dashpay#4196 is now two moves behind at 26/27/28; the doc reference it owns has to chase 34, not 27. * Records a mirror gap found while grepping for this move: dashpay#4256 declares `ErrorTransactionBuild` (32) and `ErrorTransactionSigning` (33) in Rust and maps both in Kotlin, but declares neither in Swift, so both reach Swift hosts as `.errorUnknown`. Rule 5's Swift clause; left for that PR's author. * Provenance re-verified against v4.2-dev `5d68612a45`, including the check that 32 and 33 were already taken — which is why the trio went to 34-36 and not 32-34. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Updated for #4268's merge, and corrected this file's own reasoning#4268 merged
The substantive change is a new Collision history section, which corrects something this file got wrong. On 2026-08-01 it listed #3954's That is also why the trio took 34–36 instead of the next free gap. 32 and 33 turned out to be already claimed by #4247/#4256, so the obvious 32/33/34 was not available; going above every claimed number means nothing currently in flight can displace it again. Knock-on corrections:
One new row that isn't a numbering issue: #4256 declares |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta correctly records #4268's merged code 27 and the reservation trio's move to 34–36, but it introduces a blocking contradiction: codes 28 and 30 are marked free under the allocation rule while the registry simultaneously directs new allocations to 37 and forbids reissuing those gaps. Six carried-forward documentation findings remain valid; two prior blockers remain fixed, and the former #3968 stale-base finding is now outdated because #4268 advanced the base after the prior review.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-ffi/ERROR_CODE_REGISTRY.md`:
- [BLOCKING] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:80-102: Reserve 28 and 30 before declaring 37 next
Rule 1 directs contributors to claim the first value not merged, proposed, or reserved and explicitly permits reuse when the registry marks a gap free. This section and the table mark 28 and 30 as free, making 28 the next allocatable value under the authoritative rule, while simultaneously declaring 37 next and instructing readers not to reissue either gap during this review cycle. Mark 28 and 30 temporarily reserved so the registry gives one unambiguous allocation frontier.
| **Next free integer: 37** — 27–36 are claimed (27 merged; 29, 31–36 in the | ||
| proposed table below). **28 and 30 are free**: #4185 and #4256 vacated them when | ||
| the reservation trio moved to 34–36, but they are deliberately left unclaimed | ||
| rather than back-filled, so that the trio stays contiguous and no number is | ||
| reused within a single review cycle. A new code should take 37 unless it has a | ||
| reason to sit next to something. | ||
|
|
||
| ## Proposed allocations (open PRs) | ||
|
|
||
| Not yet ABI. Numbers here may still move; they move by agreement recorded in | ||
| this file. | ||
|
|
||
| | Code | Name | Owning PR | Status | | ||
| | ---: | --- | --- | --- | | ||
| | 28 | *(free)* | — | Vacated by #4185/#4256 on 2026-08-02; not reissued | | ||
| | 29 | `ErrorAssetLockInsufficientFunds` | #4184 | In review — **keeps 29** (collision resolved) | | ||
| | 30 | *(free)* | — | Vacated by #4185/#4256 on 2026-08-02; not reissued | | ||
| | 31 | `ErrorSigningKeyUnavailable` | #4183 | In review (also carried by #4204, #4259) | | ||
| | 32 | `ErrorTransactionBuild` | #4247 | In review (also carried by #4256) | | ||
| | 33 | `ErrorTransactionSigning` | #4256 | In review | | ||
| | 34 | `ErrorStaleReservationToken` | #4185 | In review — **moved 27 → 34** (also carried by #4256; #4196 inherits on restack) | | ||
| | 35 | `ErrorReservationTokenConsumed` | #4185 | In review — **moved 28 → 35** (also carried by #4256) | | ||
| | 36 | `ErrorReservationWalletMismatch` | #4185 | In review — **moved 30 → 36** (also carried by #4256) | |
There was a problem hiding this comment.
🔴 Blocking: Reserve 28 and 30 before declaring 37 next
Rule 1 directs contributors to claim the first value not merged, proposed, or reserved and explicitly permits reuse when the registry marks a gap free. This section and the table mark 28 and 30 as free, making 28 the next allocatable value under the authoritative rule, while simultaneously declaring 37 next and instructing readers not to reissue either gap during this review cycle. Mark 28 and 30 temporarily reserved so the registry gives one unambiguous allocation frontier.
| **Next free integer: 37** — 27–36 are claimed (27 merged; 29, 31–36 in the | |
| proposed table below). **28 and 30 are free**: #4185 and #4256 vacated them when | |
| the reservation trio moved to 34–36, but they are deliberately left unclaimed | |
| rather than back-filled, so that the trio stays contiguous and no number is | |
| reused within a single review cycle. A new code should take 37 unless it has a | |
| reason to sit next to something. | |
| ## Proposed allocations (open PRs) | |
| Not yet ABI. Numbers here may still move; they move by agreement recorded in | |
| this file. | |
| | Code | Name | Owning PR | Status | | |
| | ---: | --- | --- | --- | | |
| | 28 | *(free)* | — | Vacated by #4185/#4256 on 2026-08-02; not reissued | | |
| | 29 | `ErrorAssetLockInsufficientFunds` | #4184 | In review — **keeps 29** (collision resolved) | | |
| | 30 | *(free)* | — | Vacated by #4185/#4256 on 2026-08-02; not reissued | | |
| | 31 | `ErrorSigningKeyUnavailable` | #4183 | In review (also carried by #4204, #4259) | | |
| | 32 | `ErrorTransactionBuild` | #4247 | In review (also carried by #4256) | | |
| | 33 | `ErrorTransactionSigning` | #4256 | In review | | |
| | 34 | `ErrorStaleReservationToken` | #4185 | In review — **moved 27 → 34** (also carried by #4256; #4196 inherits on restack) | | |
| | 35 | `ErrorReservationTokenConsumed` | #4185 | In review — **moved 28 → 35** (also carried by #4256) | | |
| | 36 | `ErrorReservationWalletMismatch` | #4185 | In review — **moved 30 → 36** (also carried by #4256) | | |
| **Next allocatable integer: 37** — 27 is merged; 29 and 31–36 are claimed | |
| in the proposed table below. **28 and 30 are temporarily reserved**: #4185 and | |
| #4256 vacated them when the reservation trio moved to 34–36, but they must not | |
| be reissued during this review cycle. A new code must therefore take 37. | |
| ## Proposed allocations (open PRs) | |
| Not yet ABI. Numbers here may still move; they move by agreement recorded in | |
| this file. | |
| | Code | Name | Owning PR | Status | | |
| | ---: | --- | --- | --- | | |
| | 28 | *(temporarily reserved)* | — | Vacated by #4185/#4256 on 2026-08-02; do not reissue during this review cycle | | |
| | 29 | `ErrorAssetLockInsufficientFunds` | #4184 | In review — **keeps 29** (collision resolved) | | |
| | 30 | *(temporarily reserved)* | — | Vacated by #4185/#4256 on 2026-08-02; do not reissue during this review cycle | | |
| | 31 | `ErrorSigningKeyUnavailable` | #4183 | In review (also carried by #4204, #4259) | | |
| | 32 | `ErrorTransactionBuild` | #4247 | In review (also carried by #4256) | | |
| | 33 | `ErrorTransactionSigning` | #4256 | In review | | |
| | 34 | `ErrorStaleReservationToken` | #4185 | In review — **moved 27 → 34** (also carried by #4256; #4196 inherits on restack) | | |
| | 35 | `ErrorReservationTokenConsumed` | #4185 | In review — **moved 28 → 35** (also carried by #4256) | | |
| | 36 | `ErrorReservationWalletMismatch` | #4185 | In review — **moved 30 → 36** (also carried by #4256) | |
source: ['codex']
There was a problem hiding this comment.
Fixed in 9f3dab1.
The contradiction was real and still live at 2d2c6c8, despite the earlier auto-resolve on this thread. The table rows and the frontier paragraph said RESERVED, but two other places said the opposite:
- the code-30 section: "30 is free once more, and is deliberately not being reissued"
- the collision history: "28 and 30 stay free. Do not reissue them in this review cycle"
Under rule 1 ("Do not reuse a gap unless this file marks it free") those two sentences licensed exactly the claim the rest of the file forbids, so the document carried two allocation frontiers at once.
All four places now say reserved-not-free and say it the same way. The code-30 heading is now "30 — vacated, then RESERVED (not free)", the body distinguishes vacated from freed, and the collision-history paragraph ends with an explicit pointer back to rule 1: reusing a gap requires this file to mark it free, and this file marks neither 28 nor 30 free. The frontier paragraph makes the same statement rather than just asserting 38.
There was a problem hiding this comment.
Resolved in c6fb09a — Reserve 28 and 30 before declaring 37 next no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
… allocation frontier
|
@thepastaclaw fixed the contradiction you flagged: 28 and 30 are now recorded as reserved (vacated, not reissuable) rather than "free", so the next-free frontier (37) is the single allocation source and the claim-first-free rule no longer conflicts with the table. Please re-review at the new head. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md`:
- Line 105: Update the leading PR references in ERROR_CODE_REGISTRY.md,
including the listed lines, so each `#number` token is wrapped in backticks or has
its hash escaped; preserve the reference text and surrounding content while
eliminating markdownlint MD018 warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b2b53665-d224-4a5a-972f-f16be1174445
📒 Files selected for processing (2)
packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.mdpackages/rs-platform-wallet-ffi/README.md
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta partially addresses the allocation-frontier blocker by reserving codes 28 and 30 in the summary and table, but two later passages still call them free, so all seven carried-forward findings remain valid at exact head c6fb09a. No genuinely new latest-delta defect was found beyond that incomplete fix; separately, CodeRabbit correctly identified pre-existing MD018 warnings throughout this PR's new registry document.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 💬 1 nitpick(s)
7 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
| Open PRs that touch `rs-platform-wallet-ffi` but claim **no** new code: #4186, | ||
| #4191, #4194, #4195, #4240, #4251, #4258. |
There was a problem hiding this comment.
💬 Nitpick: Fix the MD018 warnings for leading PR references
Lines beginning directly with #<number> are interpreted by markdownlint as malformed ATX headings and trigger MD018. Wrap each leading PR reference in backticks or escape its hash. This affects lines 105, 115, 133, 137, 180, 185, 225, 231, 235, 247, 248, 283, 297, 359, 361, and 365–367.
source: ['coderabbit']
There was a problem hiding this comment.
Resolved in 2d2c6c8 — Fix the MD018 warnings for leading PR references no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Fixed in 9f3dab1.
Flagging that the auto-resolve on this thread was premature: markdownlint-cli2 still reported 18 MD018 violations at 2d2c6c8. The lines beginning with a bare #<number> were still there — 106, 116, 134, 138, 215, 220, 260, 266, 270, 282, 283, 318, 332, 394, 396, 400, 401, 402.
Fixed by rewording so the PR reference is no longer the first token on the line (PR #3968 must keep 26…, the #4185 numbering of…), rather than by escaping or code-fencing. That keeps GitHub's PR autolinks intact, which \#4196 or `#4196` would have broken.
markdownlint-cli2 now reports 0 MD018 on this file. MD004 also went to 0 in the same pass — the file had mixed */- top-level bullets, now all *. MD013 is down from 19 to 18, all of them long table rows.
…-> 37 and mirror it (dashpay#4204) 32 is allocated to `ErrorTransactionBuild` (dashpay#4247, also carried by dashpay#4256) in ERROR_CODE_REGISTRY.md (dashpay#4261). This variant took 32 without a registry row, so the two collide as a hard `E0081: discriminant value 32 assigned more than once` the moment both land — reproduced on a real integration merge, not hypothetical. 27-36 are all claimed (27 ErrorShutdownIncomplete via the merged dashpay#4268; 29 dashpay#4184; 31 dashpay#4183; 32/33 dashpay#4247/dashpay#4256; 34-36 the dashpay#4185 trio) and 28/30 are vacated-but-RESERVED, so 37 is the allocation frontier. The code was also unmirrored on BOTH hosts, which is the more dangerous half: Swift is exhaustive, so it surfaced as .errorUnknown and lost its identity; Kotlin fell through to Generic(32), and in any tree carrying dashpay#4185's ErrorReservationWalletMismatch = 32 it actively MISCLASSIFIED "shielded invite already claimed" as "reservation wallet mismatch". That matters on the claim-recovery path specifically — the error is raised from four sites in shielded/operations.rs, three inside the recovery function. Adds the typed Kotlin PlatformWallet.ShieldedInviteAlreadyClaimed (terminal, inherited isRetryable = false), the Swift enum case + init(ffi:) arm, a DashSdkErrorTest assertion pinning 37, and refreshes the stale Swift reservation comment the registry asked the next toucher to drop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e 32 collision dashpay#4204 allocated 32 without a row here, colliding with dashpay#4247's ErrorTransactionBuild. Caught for real during the v41int13 integration as an E0081, not on paper — the first collision this file has actually stopped. dashpay#4204 moves to 37; frontier advances to 38. Also records that the code was unmirrored on both hosts, which had Kotlin misclassifying it as ReservationWalletMismatch wherever dashpay#4185's 32 was present. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta correctly allocates code 37 and advances the frontier to 38, but it introduces stale #3968 allocation guidance, overstates the completeness of #4204's Swift mirror, and leaves the supporting provenance on a pre-allocation head. Seven carried-forward findings remain valid, including the contradictory free/reserved labels for codes 28 and 30; the two blocking allocation contradictions require changes. One additional valid CodeRabbit MD018 style finding was omitted from the canonical findings because the 10-comment budget is exhausted.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 2 suggestion(s)
7 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-ffi/ERROR_CODE_REGISTRY.md`:
- [BLOCKING] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:138-143: Advance #3968's reissue frontier to 38
Introduced by the latest delta. Code 37 is now allocated to #4204 and lines 80–85 correctly declare 38 as the next allocatable integer, but this paragraph still directs #3968 to obtain its two persister codes from `37+`. Lines 260–262 repeat the stale instruction. Following that branch-specific guidance would allow #3968 to claim 37 and recreate the collision with `ErrorShieldedInviteAlreadyClaimed`; both references must advance to `38+`.
- [SUGGESTION] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:172-175: Do not mark #4204's Swift mirror fixed yet
Introduced by the latest delta. At current #4204 head `d78b940a03`, Swift contains `errorShieldedInviteAlreadyClaimed = 37` and the corresponding `PlatformWalletResultCode.init(ffi:)` arm, but `PlatformWalletError` has no typed case and its exhaustive `init(result:)` switch has no `.errorShieldedInviteAlreadyClaimed` arm. The switch is therefore non-exhaustive after adding the raw case, so the Swift package cannot compile as written. Record the Swift work as incomplete until #4204 adds the downstream typed error conversion required by rule 5.
- [SUGGESTION] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:398-404: Record the post-renumber #4204 head in provenance
Introduced by the latest delta. The code-37 row and resolved-collision section describe #4204 state introduced by `d78b940a03`, but the provenance still names `7bc8a845c6` and says the rows reflect the listed heads. That recorded head contains no `ErrorShieldedInviteAlreadyClaimed` variant at all, so it cannot substantiate either the temporary code-32 allocation or the move to 37 and its host mappings. Add a 2026-08-03 revalidation and record `d78b940a03` as #4204's post-renumber head.
| #3968 is the serious one: rule 3 forbids renumbering a code that has shipped, | ||
| and `ErrorTransactionBroadcastRejected = 26` is merged ABI. Moving it to 28 | ||
| would silently reinterpret every 26 an already-compiled host returns. #3968 must | ||
| keep 26 where it is and take fresh integers from the frontier (37+) for its two | ||
| persister codes. Its 27 is now doubly wrong: 27 is merged ABI | ||
| (`ErrorShutdownIncomplete`), so rule 3 protects it too. |
There was a problem hiding this comment.
🔴 Blocking: Advance #3968's reissue frontier to 38
Introduced by the latest delta. Code 37 is now allocated to #4204 and lines 80–85 correctly declare 38 as the next allocatable integer, but this paragraph still directs #3968 to obtain its two persister codes from 37+. Lines 260–262 repeat the stale instruction. Following that branch-specific guidance would allow #3968 to claim 37 and recreate the collision with ErrorShieldedInviteAlreadyClaimed; both references must advance to 38+.
source: ['codex']
There was a problem hiding this comment.
Fixed in 9f3dab1.
Confirmed both references were stale. #3968 must keep 26 where it is and take fresh integers from the frontier (37+) in the non-conforming section, and the same 37+ in the 27/28 detail section, both now read 38+ — matching the frontier paragraph, which is unchanged at 38.
Two additions beyond the literal text of the finding, because "38+" alone would still have left an ambiguity for that branch: both places now also state that 28 and 30 are reserved rather than free, so #3968 cannot read the gaps as available. The free-vs-reserved contradiction that made that reading possible is fixed in the same commit (see the code-30 section and the collision history).
There was a problem hiding this comment.
Resolved in 9f3dab1 — Advance #3968's reissue frontier to 38 no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| Fixed on #4204 together with the renumber: typed | ||
| `PlatformWallet.ShieldedInviteAlreadyClaimed` (terminal, `isRetryable = false`), | ||
| the Swift case and its `init(ffi:)` arm, and a `DashSdkErrorTest` assertion that | ||
| pins 37 so a future move off the frontier fails the suite instead of the hosts. |
There was a problem hiding this comment.
🟡 Suggestion: Do not mark #4204's Swift mirror fixed yet
Introduced by the latest delta. At current #4204 head d78b940a03, Swift contains errorShieldedInviteAlreadyClaimed = 37 and the corresponding PlatformWalletResultCode.init(ffi:) arm, but PlatformWalletError has no typed case and its exhaustive init(result:) switch has no .errorShieldedInviteAlreadyClaimed arm. The switch is therefore non-exhaustive after adding the raw case, so the Swift package cannot compile as written. Record the Swift work as incomplete until #4204 adds the downstream typed error conversion required by rule 5.
| Fixed on #4204 together with the renumber: typed | |
| `PlatformWallet.ShieldedInviteAlreadyClaimed` (terminal, `isRetryable = false`), | |
| the Swift case and its `init(ffi:)` arm, and a `DashSdkErrorTest` assertion that | |
| pins 37 so a future move off the frontier fails the suite instead of the hosts. | |
| Partially fixed on #4204 together with the renumber: typed | |
| `PlatformWallet.ShieldedInviteAlreadyClaimed` (terminal, `isRetryable = false`), | |
| the Swift raw case and its `init(ffi:)` arm, and a `DashSdkErrorTest` assertion | |
| that pins 37. The Swift `PlatformWalletError` case and its `init(result:)` arm | |
| are still missing, so typed Swift error handling remains pending. |
source: ['codex']
There was a problem hiding this comment.
Fixed in 9f3dab1.
Verified against #4204 at d78b940a03 before changing the text. PlatformWalletResult.swift there has case errorShieldedInviteAlreadyClaimed = 37 (line 96) and the matching init(ffi:) arm (line 159), but PlatformWalletError has no .shieldedInviteAlreadyClaimed case, and its init(result:) switches over PlatformWalletResultCode with no default: — so adding the raw case makes that switch non-exhaustive and the Swift package does not build as written.
The section now says partially fixed, lists what actually landed (typed Kotlin case, the Swift raw case + init(ffi:) arm, the DashSdkErrorTest pin on 37) separately from what is missing, and states that rule 5's Swift clause is not yet satisfied. The proposed-table row for 37 is unchanged — the number is settled; it is the mirror that is incomplete.
There was a problem hiding this comment.
Resolved in 9f3dab1 — Do not mark #4204's Swift mirror fixed yet no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| PR heads of record: #3954 `93d0bd49b7` (closed), #3968 `5931df745a`, #4183 | ||
| `2cd948331b`, #4184 `bd19a3e020`, **#4185 `3dec774929`** (post-34/35/36 move), | ||
| #4186 `6f7abbadc1`, #4191 `8acb0bd14c`, #4194 `9efc0b7e3a`, #4195 `4f2eb06d64`, | ||
| #4196 `ea4f783490`, #4204 `7bc8a845c6`, #4240 `9328609a16`, #4247 `0dcdc743e7`, | ||
| #4251 `176f8ed3eb`, **#4256 `8febac177c`** (post-34/35/36 move), #4258 | ||
| `5adfc40032`, #4259 `4270d827c2`. Rows describing open PRs reflect those heads | ||
| and go stale as the PRs are updated; the merged table does not. |
There was a problem hiding this comment.
🟡 Suggestion: Record the post-renumber #4204 head in provenance
Introduced by the latest delta. The code-37 row and resolved-collision section describe #4204 state introduced by d78b940a03, but the provenance still names 7bc8a845c6 and says the rows reflect the listed heads. That recorded head contains no ErrorShieldedInviteAlreadyClaimed variant at all, so it cannot substantiate either the temporary code-32 allocation or the move to 37 and its host mappings. Add a 2026-08-03 revalidation and record d78b940a03 as #4204's post-renumber head.
source: ['codex']
There was a problem hiding this comment.
Fixed in 9f3dab1.
Rather than adding a single #4204 line, the whole provenance block was re-read from GitHub on 2026-08-03 and rewritten as a dated table, because several other heads had moved too (#4185, #4186, #4194, #4195, #4240, #4247, #4256 and #4259 were all stale, and #4258 had merged).
#4204 is recorded at d78b940a03 with the note "post-renumber 32 → 37; pre-#4268 base". The 2026-08-03 pass also confirms what the code-37 row and the resolved-collision section actually rest on: 32/33 are still #4247/#4256's, 37 is #4204's post-renumber code, nothing in flight has taken 28, 30 or 38, and #4247's head now carries #4185's 34/35/36 trio by inheritance rather than as a second claim.
The closing paragraph now states that every head in the table was read from GitHub on the date given rather than carried forward from a previous revision of this file — which is the mistake that produced the #4185 0b0d5c76d6 (post-renumber) entry the note already documents.
There was a problem hiding this comment.
Resolved in 9f3dab1 — Record the post-renumber #4204 head in provenance no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…lt space `ErrorTransactionBuild = 32` was added to the Rust FFI enum and to the Kotlin decoder by this PR, but not to the Swift mirror. Swift decodes an unlisted raw value through the `default:` arm of `PlatformWalletResultCode .init(ffi:)`, so every typed build rejection this PR introduced — an unresolvable or watch-only `fundingPath`, a breached monetary bound, a malformed recipients blob — reached iOS as `errorUnknown` (99) with only the message string to distinguish it. That is the exact failure the code was split out of `ErrorUnknown` to end, reintroduced on the other host. Adds the enum case, the `init(ffi:)` mapping, and the `PlatformWalletError.transactionBuild` arm. There is no compile-time check that the raw values match Rust, so the numbering comment is kept in sync with the registry (dashpay#4261) and now records only the codes this PR does NOT own. Verified against the cbindgen header: PLATFORM_WALLET_FFI_RESULT_CODE_ ERROR_TRANSACTION_BUILD = 32. No other exhaustive switch over PlatformWalletResultCode / PlatformWalletError exists in the Swift SDK.
…sult space Same gap this stack closed for 32 one PR down: the code was added to the Rust FFI enum and the Kotlin decoder here but not to the Swift mirror, so a signing failure reached iOS through `init(ffi:)`'s `default:` arm as `errorUnknown` (99). That misclassification is worse for 33 than for most codes because the two errors carry OPPOSITE advice. 32 means "the request is at fault, a verbatim retry fails identically"; 33 means "the request is fine, the reservation was released, resubmit the identical request once the signer is usable". Collapsing 33 into the unknown bucket loses the one distinction the code exists to make, and a locked Keychain is the common case on iOS. Adds the enum case, the `init(ffi:)` mapping, the `PlatformWalletError.transactionSigning` arm and its `errorDescription` entry. Verified against the cbindgen header: PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_TRANSACTION_SIGNING = 33. Registry state after this stack (dashpay#4261): 32 ErrorTransactionBuild, 33 ErrorTransactionSigning, 34/35/36 the deferred-token trio — identical in Rust, Kotlin and Swift.
Keeps this branch on top of dashpay#4247, which now carries current v4.2-dev (post-dashpay#4268). Merged cleanly — dashpay#4268's coordinator-lifecycle rework and this branch's funding-path finalize touch disjoint code, and the error space was already reconciled to the registry (dashpay#4261): 27 ErrorShutdownIncomplete (dashpay#4268), 32 ErrorTransactionBuild (dashpay#4247), 33 ErrorTransactionSigning (this PR), 34-36 the dashpay#4185 deferred-token trio.
Resolves the two blocking review findings and the live suggestions on dashpay#4261. Blocking — advance dashpay#3968's reissue frontier to 38. Code 37 is allocated to dashpay#4204, so the branch-specific guidance in the non-conforming section and in the 27/28 detail section could still have sent dashpay#3968 to 37 and recreated the collision with ErrorShieldedInviteAlreadyClaimed. Both references now say 38+, and both state that the reserved 28 and 30 are not available either. Blocking — 28 and 30 were labelled RESERVED in the table and the frontier paragraph but "free" in the code-30 section and the collision history. Rule 1 lets a contributor claim a gap this file marks free, so the two labels gave two different allocation frontiers. Everything now says reserved-not-free and cites rule 1 explicitly. Open-PR inventory: verified 2026-08-03 against each PR's file list and the error.rs at its head. Adds dashpay#3417, dashpay#3549, dashpay#3992 and dashpay#4243 (all touch the crate without claiming an integer; dashpay#4243 maps new wallet errors onto the existing ErrorInvalidParameter). Removes dashpay#4240 and dashpay#4251, whose heads touch no file under this crate; removes dashpay#4258, merged on 2026-08-03; records dashpay#4264 as closed with its work carried by dashpay#4243. Also in this pass: - dashpay#4204's Swift mirror is recorded as PARTIALLY fixed. At d78b940 the raw case and its init(ffi:) arm exist, but PlatformWalletError has no typed case and its exhaustive init(result:) has no arm, so the Swift package does not compile as written. Rule 5's Swift clause is not yet satisfied there. - The preamble no longer claims duplicate discriminants are always silent. Two different names on one integer is an E0081 after a merge (that is how the code-32 collision surfaced); the silent case is a meaning moving to a new integer, or an un-updated host mirror. - Code 13 ErrorArithmeticOverflow does have an in-tree producer (shielded_send.rs); the row said it had none. - dashpay#3968 is no longer described as simply un-rebased: its head does contain the 2026-08-01 base, it is behind the current base, and a rebase alone fixes nothing because git sees no conflict — the branch must edit its own enum. - Provenance re-read on 2026-08-03 from GitHub rather than carried forward, now as a table with a note per PR. dashpay#4183 and dashpay#4184 were rebased onto 5d68612 today and keep 31 and 29 respectively; their stale in-enum reservation comments were corrected in the same rebase, so the code-30 section now lists dashpay#4204 as the only branch still carrying one. - markdownlint: MD018 (18 occurrences) and MD004 (mixed bullet styles) are both clean; MD013 is down to long table rows only.
…-> 37 and mirror it (dashpay#4204) 32 is allocated to `ErrorTransactionBuild` (dashpay#4247, also carried by dashpay#4256) in ERROR_CODE_REGISTRY.md (dashpay#4261). This variant took 32 without a registry row, so the two collide as a hard `E0081: discriminant value 32 assigned more than once` the moment both land — reproduced on a real integration merge, not hypothetical. 27-36 are all claimed (27 ErrorShutdownIncomplete via the merged dashpay#4268; 29 dashpay#4184; 31 dashpay#4183; 32/33 37 is the allocation frontier. The code was also unmirrored on BOTH hosts, which is the more dangerous half: Swift is exhaustive, so it surfaced as .errorUnknown and lost its identity; Kotlin fell through to Generic(32), and in any tree carrying "shielded invite already claimed" as "reservation wallet mismatch". That matters on the claim-recovery path specifically — the error is raised from four sites in shielded/operations.rs, three inside the recovery function. Adds the typed Kotlin PlatformWallet.ShieldedInviteAlreadyClaimed (terminal, inherited isRetryable = false), the Swift enum case + init(ffi:) arm, a DashSdkErrorTest assertion pinning 37, and refreshes the stale Swift reservation comment the registry asked the next toucher to drop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… sites Three more live review findings, all verified against current PR heads. Rule 5 named a `PlatformWalletResultCode.init(result:)` that does not exist — `init(result:)` belongs to the downstream `PlatformWalletError`. As written, a contributor could add the Swift raw case and the typed error handling and still omit `PlatformWalletResultCode.init(ffi:)`, which is where the generated C constant is recognised; that switch has a `default:` yielding `.errorUnknown`, so the omission compiles and silently loses the code's identity before typed handling sees it. Rule 5 now enumerates all three Swift sites and says how each one fails: (1) the raw case, (2) the `init(ffi:)` arm — silent, and (3) `PlatformWalletError` + its `init(result:)` arm — a hard compile error, since that switch is exhaustive with no `default:`. That third failure is exactly what dashpay#4204 is sitting on at `d78b940a03`. dashpay#4196 is no longer blocked. Its head moved to `12492e8c54`, the restack onto dashpay#4185 is done, dashpay#4185's head `8813e98533` is an ancestor, the trio reads 34/35/36, and the PR is MERGEABLE against v4.2-dev. Verified the numeric references it owns were carried too: the `StaleReservationToken` KDoc and `fromPlatformWalletNative` mapping in `DashSdkError.kt` both read 34, and the V2 broadcast KDoc in `ManagedCoreWallet.kt` reads 34 with the rest symbolic. `PlatformWalletError::StaleReservation` refers to the code symbolically and never carried a number. The section is now a resolution rather than an open item; the account of why the restack was hard is kept, since that was the substance of the delay. The code-30 sweep was overstated. "No PR anywhere defines a code 30" is false for the surveyed heads — dashpay#4185 and dashpay#4256 both did; that was the allocation, not a competing claim. It now reads "no PR unrelated to dashpay#4185 defines a code 30", which is the claim that actually supports the conclusion. The list of branches carrying the stale consent-code reservation is corrected to dashpay#4183, dashpay#4204 and dashpay#4256's pre-renumber rationale (dashpay#4247 was never one of them). Provenance and the proposed table pick up dashpay#4196's new head. markdownlint MD018/MD004 remain at 0.
…-> 37 and mirror it (dashpay#4204) 32 is allocated to `ErrorTransactionBuild` (dashpay#4247, also carried by dashpay#4256) in ERROR_CODE_REGISTRY.md (dashpay#4261). This variant took 32 without a registry row, so the two collide as a hard `E0081: discriminant value 32 assigned more than once` the moment both land — reproduced on a real integration merge, not hypothetical. 27-36 are all claimed (27 ErrorShutdownIncomplete via the merged dashpay#4268; 29 dashpay#4184; 31 dashpay#4183; 32/33 37 is the allocation frontier. The code was also unmirrored on BOTH hosts, which is the more dangerous half: Swift is exhaustive, so it surfaced as .errorUnknown and lost its identity; Kotlin fell through to Generic(32), and in any tree carrying "shielded invite already claimed" as "reservation wallet mismatch". That matters on the claim-recovery path specifically — the error is raised from four sites in shielded/operations.rs, three inside the recovery function. Adds the typed Kotlin PlatformWallet.ShieldedInviteAlreadyClaimed (terminal, inherited isRetryable = false), the Swift enum case + init(ffi:) arm, a DashSdkErrorTest assertion pinning 37, and refreshes the stale Swift reservation comment the registry asked the next toucher to drop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t + finalize (dashpay#4185 review) Wallet removal was not linearized with the deferred-payment registry, so a retained handle could push a removed wallet's payment onto the network. Two independent windows: 1. remove-then-sweep. `platform_wallet_manager_remove_wallet` called `manager.remove_wallet` and only afterwards swept the registry, with no shared lock spanning the two — and the removal's own awaits (shielded coordinator + identity-sync unregistration) sat in the gap. A concurrent `core_wallet_signed_payment_broadcast` in that window passed every guard: `is_same_generation` compares two handles, so a removed generation matches itself; `last_processed_height` is `None` once the wallet is gone and `reservation_expired` maps `None` to "not expired"; and `broadcast_payment_releasing_reservation` has no wallet-existence gate. 2. in-flight finalizer. `finalize_transaction` drops the manager write lock before awaiting the signer, and `register` only validates the payment against its finalizing generation — never that the generation still exists. A removal during the signer await swept the registry, then the finalizer inserted a fresh token no later sweep would catch, contradicting the documented teardown invariant that dropping tokens makes stale handles inert. Remedies: * `SignedPaymentRegistry` gains a lifecycle gate (`tokio::RwLock`). Teardown takes the exclusive side across BOTH the manager removal and the sweep, making them one linearization point; broadcast and release take the shared side for their whole duration. The existing `entries` mutex cannot do this — it is dropped before every await by design. Lock order is always gate then manager. * Broadcast rejects an absent current generation via the new `CoreWallet::is_current_generation`, returning `SignedPaymentError:: WalletRemoved` instead of silently proceeding to the broadcaster. * `core_wallet_signed_payment_finalize` holds the shared gate across its liveness check and the synchronous `register`, abandoning the payment (reconciling its reservation) if the wallet went away during signing. The gate is taken after the signer await, not around it, so an open signing prompt cannot stall teardown. No new FFI error code: the wallet-removed case is reported as the existing `NotFound` (98), which both hosts already map. Deliberately avoids the 29/30 renumbering contested in dashpay#4261. Swift/Kotlin/Rust docs updated to record that 98 now also carries this case, and how it differs from `ErrorReservationWalletMismatch` (29). Adds three FFI regression tests. All three fail against the pre-fix code — the race test reports a payment reaching the broadcaster after teardown completed. Also serializes the registry-count-asserting tests, which the new tests would otherwise race in the shared process-global registry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…30 (dashpay#4185 review) Code 29 collided with `ErrorAssetLockInsufficientFunds` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and this PR moves to 30. Verified 30 was genuinely free by reading `rs-platform-wallet-ffi/src/error.rs` at the head of all 62 open PRs: no PR defines a code 30. The `ErrorAssetLockCrossDomainConsentRequired` that in-tree comments name as 30's holder does not exist anywhere after dashpay#4184's re-scope. The discriminant is public ABI, so every mirror moves together: - Rust enum + its three rustdoc cross-references (error.rs) - two doc references in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value + doc - Kotlin fromPlatformWalletNative branch, class KDoc, code-98 comment, WalletManagerNative KDoc, and the DashSdkErrorTest offset assertion Both Swift switches are symbolic (cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants), so only the enum raw value carried the number. Also disarms the NativeCleaner backstop in SignedCoreTransactionTest by closing the SignedCoreTransaction, so the armed native release cannot fire from the cleaner thread in a pure-JVM test. Note: dashpay#4256 is stacked downstream and still carries the pre-renumber 29; it must adopt 30 on rebase.
…lt space `ErrorTransactionBuild = 32` was added to the Rust FFI enum and to the Kotlin decoder by this PR, but not to the Swift mirror. Swift decodes an unlisted raw value through the `default:` arm of `PlatformWalletResultCode .init(ffi:)`, so every typed build rejection this PR introduced — an unresolvable or watch-only `fundingPath`, a breached monetary bound, a malformed recipients blob — reached iOS as `errorUnknown` (99) with only the message string to distinguish it. That is the exact failure the code was split out of `ErrorUnknown` to end, reintroduced on the other host. Adds the enum case, the `init(ffi:)` mapping, and the `PlatformWalletError.transactionBuild` arm. There is no compile-time check that the raw values match Rust, so the numbering comment is kept in sync with the registry (dashpay#4261) and now records only the codes this PR does NOT own. Verified against the cbindgen header: PLATFORM_WALLET_FFI_RESULT_CODE_ ERROR_TRANSACTION_BUILD = 32. No other exhaustive switch over PlatformWalletResultCode / PlatformWalletError exists in the Swift SDK.
…sult space Same gap this stack closed for 32 one PR down: the code was added to the Rust FFI enum and the Kotlin decoder here but not to the Swift mirror, so a signing failure reached iOS through `init(ffi:)`'s `default:` arm as `errorUnknown` (99). That misclassification is worse for 33 than for most codes because the two errors carry OPPOSITE advice. 32 means "the request is at fault, a verbatim retry fails identically"; 33 means "the request is fine, the reservation was released, resubmit the identical request once the signer is usable". Collapsing 33 into the unknown bucket loses the one distinction the code exists to make, and a locked Keychain is the common case on iOS. Adds the enum case, the `init(ffi:)` mapping, the `PlatformWalletError.transactionSigning` arm and its `errorDescription` entry. Verified against the cbindgen header: PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_TRANSACTION_SIGNING = 33. Registry state after this stack (dashpay#4261): 32 ErrorTransactionBuild, 33 ErrorTransactionSigning, 34/35/36 the deferred-token trio — identical in Rust, Kotlin and Swift.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Eight of the ten prior findings are fixed at the reviewed head; the incomplete #4196 numeric-reference list and no-new-code PR inventory remain as carried-forward suggestions. A current-base resurvey also shows two new stale-status issues: code 31 has merged into v4.2-dev but remains classified as movable proposed ABI, and the documented Swift mirror gaps for #4204 and #4256 have since been closed. The merged-ABI misclassification is blocking, so this review requests changes.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-ffi/ERROR_CODE_REGISTRY.md`:
- [BLOCKING] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:124: Record code 31 as merged ABI
New current-base finding. Current v4.2-dev at `f53e5eef0a` contains `ErrorSigningKeyUnavailable = 31`, merged by #4183 in `189a3abb1c`, together with the Rust C-facing discriminant and complete Swift and Kotlin mirrors. This row still places 31 under “Proposed allocations,” whose introduction explicitly permits renumbering, contradicting rule 3 for a value now present on v4.2-dev. Move code 31 into the merged table and update the proposed, inherited-code, collision-history, open-PR, and provenance text that still treats #4183 as open.
- [SUGGESTION] packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:231-238: Close the mirror gaps fixed on current PR heads
New current-head drift. At #4204 head `4efecd5b71`, Swift now has the raw `errorShieldedInviteAlreadyClaimed = 37` case, generated-C `init(ffi:)` arm, typed `PlatformWalletError.shieldedInviteAlreadyClaimed` case, and exhaustive `init(result:)` arm, so this paragraph no longer describes the current branch. Likewise, #4256 head `862036b18d` mirrors codes 32 and 33 through all three Swift locations, contrary to the known-gap section at lines 440–448. Mark both gaps resolved and refresh the recorded PR-head SHAs.
| | 28 | *(reserved — vacated)* | — | Vacated by #4185/#4256 on 2026-08-02; RESERVED, not reissuable — the next-free frontier is the only allocation source | | ||
| | 29 | `ErrorAssetLockInsufficientFunds` | #4184 | In review — **keeps 29** (collision resolved) | | ||
| | 30 | *(reserved — vacated)* | — | Vacated by #4185/#4256 on 2026-08-02; RESERVED, not reissuable — the next-free frontier is the only allocation source | | ||
| | 31 | `ErrorSigningKeyUnavailable` | #4183 | In review (also carried by #4204, #4259) | |
There was a problem hiding this comment.
🔴 Blocking: Record code 31 as merged ABI
New current-base finding. Current v4.2-dev at f53e5eef0a contains ErrorSigningKeyUnavailable = 31, merged by #4183 in 189a3abb1c, together with the Rust C-facing discriminant and complete Swift and Kotlin mirrors. This row still places 31 under “Proposed allocations,” whose introduction explicitly permits renumbering, contradicting rule 3 for a value now present on v4.2-dev. Move code 31 into the merged table and update the proposed, inherited-code, collision-history, open-PR, and provenance text that still treats #4183 as open.
source: ['codex']
| **Still missing on #4204, and it does not compile without it:** Swift's | ||
| `PlatformWalletError` has no `.shieldedInviteAlreadyClaimed` case, and its | ||
| `init(result:)` switches exhaustively over `PlatformWalletResultCode` with no | ||
| `default:`. Adding the raw case without the matching `init(result:)` arm makes | ||
| that switch non-exhaustive, so the Swift package fails to build at | ||
| `d78b940a03`. Rule 5's Swift clause is therefore not yet satisfied — treat the | ||
| Swift mirror as incomplete until #4204 adds the typed error case and its | ||
| conversion arm. |
There was a problem hiding this comment.
🟡 Suggestion: Close the mirror gaps fixed on current PR heads
New current-head drift. At #4204 head 4efecd5b71, Swift now has the raw errorShieldedInviteAlreadyClaimed = 37 case, generated-C init(ffi:) arm, typed PlatformWalletError.shieldedInviteAlreadyClaimed case, and exhaustive init(result:) arm, so this paragraph no longer describes the current branch. Likewise, #4256 head 862036b18d mirrors codes 32 and 33 through all three Swift locations, contrary to the known-gap section at lines 440–448. Mark both gaps resolved and refresh the recorded PR-head SHAs.
source: ['codex']
2 participants
Problem
PlatformWalletFFIResultCode(packages/rs-platform-wallet-ffi/src/error.rs) is public ABI —cbindgenemits the integers into the generated C header, Swift mirrors them inPlatformWalletResultCode, and Kotlin branches on them infromPlatformWalletNative. Four open PRs are currently allocating adjacent integers in that enum with no shared source of truth.A duplicate discriminant in two branches produces no textual merge conflict. The second merge just silently misclassifies errors on every host. That is not hypothetical here:
29is already claimed by two PRs, and review on #4185 had to issue a renumber directive (ErrorReservationWalletMismatch29 → 30) to resolve it. Three separate PRs now carry hand-written comments trying to reserve ranges on each other's behalf, and one of those reservations is for a variant that no longer exists.This PR adds a registry so the allocation is decided in one file instead of in four diffs.
What's here
packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md:Plus a six-line pointer from the crate README's Error Handling section.
Docs-only. Zero code risk. No enum, no
Frommapping, no Swift/Kotlin mirror is touched, and nothing is renumbered — that stays the owning PRs' work. This PR only writes down what the branches already say.Codes claimed by the in-flight PRs
Reconciling in one place, so this is the table to correct:
ErrorStaleReservationTokenErrorReservationTokenConsumedErrorReservationWalletMismatchErrorAssetLockInsufficientFundsErrorSigningKeyUnavailableErrorTransactionBuildErrorTransactionSigningOpen PRs touching this crate that claim no new code: #4186, #4191, #4194, #4195, #4240, #4251, #4258. Next free integer: 34.
rs-sdk-ffi'sDashSDKErrorCodeis a separate integer space and is not modified by any of these PRs.Surveying the branches turned up a second collision that does not appear to have been raised in review.
#4196 branched before
26 = ErrorTransactionBroadcastRejectedmerged tov4.2-dev(in9302c62e8b), and its head numbers the reservation trio 26 / 27 / 28:So as it stands, #4196 would give
26two meanings on merge, and it also contradicts #4185 — the PR it is stacked on — which numbers those same three names 27 / 28 / 29. The fix is a rebase onto currentv4.2-devplus adopting whatever numbering #4185 lands with; no new integers are needed. Flagging rather than fixing, per the docs-only scope.Code 30 is free, despite what three branches say
ErrorAssetLockCrossDomainConsentRequiredis named as the holder of30in in-tree comments on #4183, #4204, and in #4247/#4256's numbering rationale. It is not defined anywhere — #4184, the PR that would have introduced it, dropped it in a re-scope.30is therefore genuinely free, and is the slot the #4185 renumber should take. Those stale comments should be dropped by whichever PR touches them next.Ambiguity left open for maintainers
Review on #4256 suggested mapping its signing failure onto
31; #4256 declined and took33, arguing that31(ErrorSigningKeyUnavailable, #4183) asserts a narrower contract — the signer holds no usable private key for a requested public key — whileBuilderError::SigningFailedalso covers unresolved derivation paths, sighash failures, and malformed signature encodings. Both are currently allocated. Collapsing them is a reasonable call, but it belongs to #4183 and #4256 jointly; the registry records the split as-is rather than pre-judging it.Please correct the table
The merged rows were read straight from
v4.2-devated4116b26c. The proposed rows were read from each PR's head commit (SHAs listed in the doc's provenance section) and will go stale as those PRs are updated. If a maintainer has a different intent for any allocation — particularly30, and the31/33split — please say so on this PR and I'll amend, or edit the file directly. The point is that there is now one place where that correction lands.🤖 Generated with Claude Code
Summary by CodeRabbit