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 1 commit 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: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 bb37a70) |
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']
| | 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']
| 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']
| | 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']
| 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']
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