feat(sdk): expose the label each contender actually requested - #4331
Conversation
DPNS keys a contest by the homograph-normalized label, so a UI that renders what Platform returns shows "p1zza" where the people involved typed "pizza". That reads as a typo to the very users whose name is being voted on, and the normalization is not reversible — `0`→`o` and `1`→`i`/`l` are ambiguous, so a client cannot recover the spelling by guessing. The spelling was already on the wire: each contender's `domain` document carries a `label` field, and both contested-resource queries were handing that document back without reading it — as hex in the generic vote-state JSON, and not at all in the DPNS list. - `dash_sdk_contested_resource_get_vote_state` now decodes each contender via `try_to_contender` against the queried contract's document type (generic, not DPNS-pinned) and adds `"label"` to each contender object. - `DashSDKContender` gains a `label` C string, populated in both `dash_sdk_dpns_get_contested_non_resolved_usernames` and `dash_sdk_dpns_get_contested_usernames_by_identity` from the baked-in DPNS system contract — a local load, no extra round trip. Freed alongside `identity_id` in `dash_sdk_contested_names_list_free`. - Swift: `DPNSContender.displayLabel` and `DPNSContest.requestedLabels` (de-duplicated, contender order). Decoding is best-effort throughout: a contract fetch or a single document that will not decode leaves `label` null for that row rather than failing the query, and callers fall back to the normalized label. Nothing is ever synthesized from the normalized form. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 17 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 (5)
📝 WalkthroughWalkthroughThe change propagates decoded DPNS contender labels from Rust queries through the FFI into Swift contest models. It adds FFI cleanup for label strings, identity-scoped contest retrieval, optional display labels, and ordered unique requested labels. ChangesContender label propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SwiftSDK
participant RustFFI
participant DPNSDocument
SwiftSDK->>RustFFI: Request unresolved contests
RustFFI->>DPNSDocument: Load type and decode contender document
DPNSDocument-->>RustFFI: Optional label
RustFFI-->>SwiftSDK: Contender data with optional label
SwiftSDK->>SwiftSDK: Build DPNSContender and requestedLabels
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The voting screens rendered Platform's homograph-normalized label, so a contest for "pizza" appeared as "p1zza" — which reads as a typo to the people whose name is being voted on. The normalization is not reversible client-side, so this needed the spelling to come off the wire; dashpay/platform#4331 decodes it from each contender's `domain` document. Contests now lead with what people actually typed, and keep the normalized form as supporting information rather than dropping it: - The list row shows the typed spelling, with the stored form beneath it in small monospace whenever the two differ. - The detail screen titles on the typed spelling and adds a "Stored as" row plus a one-line explanation of why they differ. - Contender rows show each requester's own spelling, falling back to the truncated identity id when their document could not be decoded. - When contenders typed *different* spellings that normalize to the same value — the situation a contest exists to resolve — all of them are shown joined by "or", rather than arbitrarily picking one. Search now matches both forms, so typing "pizza" finds the contest whether the row reads "pizza" or "p1zza". Nothing is reverse-engineered from the normalized label: `0`→`o` and `1`→`i`/`l` are ambiguous, so a contest with no decodable contender document keeps showing the normalized form rather than a guessed one. Requires dashpay/platform#4331 (adds `label` to `DashSDKContender`, so the xcframework must be rebuilt). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🕓 Ready for review — 3 ahead in queue (commit 275e48a) |
`cargo fmt --check` failed CI on the previous commit: the new `use` was in the wrong sort position and the two `load_system_data_contract` bindings exceeded the line width. Formatting only — no behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4331 +/- ##
============================================
- Coverage 87.78% 87.61% -0.17%
============================================
Files 2677 2704 +27
Lines 342371 345211 +2840
============================================
+ Hits 300551 302473 +1922
- Misses 41820 42738 +918
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR threads the requester's original DPNS label through both contested-resource query paths (generic vote-state JSON and the DPNS-specific C struct), with correct best-effort decoding that never synthesizes a label and is well covered by new Swift tests. Verification confirms one real defect: the new DashSDKContender.label field is freed only by dash_sdk_contested_names_list_free, while three sibling public destructors (dash_sdk_contender_free, dash_sdk_contest_info_free, dash_sdk_contested_name_free) still document that they free 'any heap-allocated strings owned by the structure' but only free identity_id — these three have no in-tree producer or caller today, so the practical blast radius is limited to external consumers of the generated C header. A minor perf nitpick (per-contender document-type lookup) and a Rust-side test-coverage gap for the new decoding logic round out the findings. No blocking issues; recommend COMMENT.
Source: reviewers codex-general (gpt-5.6-sol), codex-ffi-engineer (gpt-5.6-sol), sonnet-general (claude-sonnet-5), sonnet-ffi-engineer (claude-sonnet-5); final verifier claude-sonnet-5 (final-verifier-sonnet-1). openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— ffi-engineer (completed)
🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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-sdk-ffi/src/types.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/types.rs:1107-1166: New DashSDKContender.label field not freed by three sibling destructors
`DashSDKContender` gained a `label: *mut c_char` field (types.rs:1030-1040), heap-allocated via `CString::into_raw()` in both `dpns/queries/contested.rs` producers. Only `dash_sdk_contested_names_list_free` (types.rs:1174+, verified) was updated to also free `(*contender).label`. The other three exported `#[no_mangle]` destructors that also own a `DashSDKContender` — `dash_sdk_contender_free` (line 1107-1114), `dash_sdk_contest_info_free` (line 1123-1138), and `dash_sdk_contested_name_free` (line 1145-1166) — still free only `identity_id`, despite doc comments explicitly promising to free 'any heap-allocated strings owned by the structure.' Verified by grep that these three functions have no producer or in-tree caller today (only the `*_list` variants are exercised by `dpns/queries/contested.rs` and Swift), so the current shipped code paths are safe from this leak. However, all four functions are exported in the generated public C header, and any external consumer (mobile FFI, JNI, or a future producer of a standalone contender/contest-info struct) that frees through one of these three paths instead of the list free will leak the label string.
In `packages/rs-sdk-ffi/src/contested_resource/queries/vote_state.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/contested_resource/queries/vote_state.rs:201-253: New label-decoding logic has no Rust-side test coverage
Verified: the only tests in this file's `#[cfg(test)]` module (lines 306-347) cover null-handle and null-contract-id guard paths — nothing exercises the new label-decoding logic added in this PR (contract fetch, `document_type_for_name`, `try_to_contender`, JSON-embedded label extraction), nor the mirrored logic in `dpns/queries/contested.rs`. The new Swift tests inject already-decoded label JSON directly, so they validate JSON consumption but not the Rust-side production logic — e.g. that a document lacking a `label` field, or one that fails to deserialize against the fetched contract, is silently omitted rather than producing malformed JSON or panicking.
| } | ||
| // Add contenders | ||
| if result_type.has_documents() { | ||
| // Decode each contender's document so callers get the | ||
| // label the requester actually typed ("pizza") next to the | ||
| // homograph-normalized index value ("p1zza"). Without this | ||
| // a UI can only show the normalized form, which reads as a | ||
| // typo to the person who submitted it. | ||
| // | ||
| // Best-effort: the contract fetch or a single decode | ||
| // failing must not fail the whole query, so `label` is | ||
| // simply absent for rows that could not be decoded and the | ||
| // caller falls back to the normalized value. | ||
| let contract: Option<DataContract> = | ||
| DataContract::fetch(&sdk, contract_id).await.ok().flatten(); | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: New label-decoding logic has no Rust-side test coverage
Verified: the only tests in this file's #[cfg(test)] module (lines 306-347) cover null-handle and null-contract-id guard paths — nothing exercises the new label-decoding logic added in this PR (contract fetch, document_type_for_name, try_to_contender, JSON-embedded label extraction), nor the mirrored logic in dpns/queries/contested.rs. The new Swift tests inject already-decoded label JSON directly, so they validate JSON consumption but not the Rust-side production logic — e.g. that a document lacking a label field, or one that fails to deserialize against the fetched contract, is silently omitted rather than producing malformed JSON or panicking.
source: ['claude', 'codex']
| let label_json = contract | ||
| .as_ref() | ||
| .and_then(|contract| { | ||
| let doc_type = contract | ||
| .document_type_for_name(document_type_name_str) | ||
| .ok()?; | ||
| let decoded = contender | ||
| .try_to_contender(doc_type, sdk.version()) | ||
| .ok()?; | ||
| let label = decoded | ||
| .document() | ||
| .as_ref()? | ||
| .get("label")? | ||
| .as_str()? | ||
| .to_string(); | ||
| Some(format!( | ||
| r#","label":{}"#, | ||
| serde_json::to_string(&label).ok()? | ||
| )) | ||
| }) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
💬 Nitpick: document_type_for_name looked up per contender instead of once per query
contract.document_type_for_name(document_type_name_str) is called inside the per-contender .map() closure (verified at line ~264), repeating a name-keyed lookup into the contract's document type map for every contender instead of resolving it once before the loop. Contests are typically small so the practical cost is negligible, but hoisting it out avoids the repeated lookup.
source: ['claude']
… in Swift Review follow-up on #4331. **Three public destructors leaked the label.** `DashSDKContender.label` is allocated with `CString::into_raw`, but `dash_sdk_contender_free`, `dash_sdk_contest_info_free` and `dash_sdk_contested_name_free` each freed only `identity_id` — a consumer using any documented ownership path other than the list destructor leaked one allocation per decoded contender. The cause was duplication: the contender free-loop existed at four sites, so adding a field meant remembering all four, and I updated one. Collapsed into `free_contender_fields` / `free_contender_array`, with all four destructors — the list one included — routed through them. Adding the next field is now a one-line change rather than four. Covered by 7 new tests over the paths that leaked: each destructor with a label present and absent, plus the null-pointer and no-contender cases. **The Swift dictionary APIs discarded the decoded label.** `dpnsGetNonResolvedContestsForIdentity` and `dpnsGetContestedNonResolvedUsernames` copied only identity and votes before freeing the C list, so their callers still got `p1zza` with no way back to `pizza`. Both now carry `label` (omitted when null or empty), and point at the typed APIs for integer tallies. **The identity-scoped query had no typed replacement.** Added `SDK.dpnsContestsForIdentity(identityId:limit:)`, the "how are my own requests doing" counterpart to the network-wide `dpnsActiveContests`. Both FFIs return the same `DashSDKContestedNamesList`, so the decode is factored into one `consumeContestedNamesList` reader that takes ownership and frees on every path; a second hand-rolled reader would be one more place for the ownership rules and null-label handling to drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/swift-sdk/Sources/SwiftDashSDK/Voting/DPNSContest.swift`:
- Around line 108-115: Update the displayLabel documentation in DPNSContest to
remove the claim that active-contest listings omit documents; state that nil
means the label data was unavailable or decoding the contender’s label failed,
while preserving the existing guidance to fall back to identityId or the
normalized contest label.
- Around line 80-93: Move the ordered, de-duplicated label computation from the
Swift computed property requestedLabels into the Rust layer and expose the
resulting labels through FFI. Update requestedLabels to only decode and return
the FFI-provided result, preserving contender order and the empty-result
behavior when no contender document can be decoded; remove Swift-side iteration
and Set-based de-duplication.
🪄 Autofix
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: f9a08894-4372-4369-b25b-3c94718fc437
📒 Files selected for processing (7)
packages/rs-sdk-ffi/src/contested_resource/queries/vote_state.rspackages/rs-sdk-ffi/src/dpns/queries/contested.rspackages/rs-sdk-ffi/src/types.rspackages/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Voting/DPNSContest.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Voting/SDK+DPNSContests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/DPNSContestDecoderTests.swift
Review follow-up on #4331 (review 4885645372). **Ordered-unique label derivation moves to Rust.** `DPNSContest.requestedLabels` was a Swift computed property that iterated contenders and applied de-duplication policy. `packages/swift-sdk/CLAUDE.md:27` forbids exactly that — "No iteration / gap-limit walks / policy loops in Swift" — and the SDK is meant to be a thin bridge. I pushed back on this initially without checking the guideline; it is explicit and the reviewer was right. `DashSDKContestInfo` gains `requested_labels` / `requested_label_count`, derived at both DPNS construction sites, and Swift copies the array verbatim into a stored property. Which spellings to show is display policy, and it now has exactly one home. Freeing goes through a new `free_contest_info_fields`, so "what does a contest info own" is one function rather than three copies — the same consolidation that fixed the earlier `label` leak, applied before this field could repeat it. **`displayLabel`'s doc was stale.** It claimed `nil` meant the query carried no documents "(the active-contest listing does not)". That stopped being true in this very PR, which taught that listing to decode labels. It now says the label data was unavailable or decoding failed. Tests: two new Rust cases pin the de-dup policy where it now lives — a repeated spelling collapses to one entry, an undecodable contender is skipped, and a contest with nothing decodable yields a null array. `types::` suite 19/19. The Swift assertions that used to cover de-duplication are replaced by one that pins the value surviving the model boundary unchanged. Note: this is a second `#[repr(C)]` change to `DashSDKContestInfo`, so consumers rebuild the xcframework again. Done and verified for both slices. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#933) The voting screens rendered Platform's homograph-normalized label, so a contest for "pizza" appeared as "p1zza" — which reads as a typo to the people whose name is being voted on. The normalization is not reversible client-side, so this needed the spelling to come off the wire; dashpay/platform#4331 decodes it from each contender's `domain` document. Contests now lead with what people actually typed, and keep the normalized form as supporting information rather than dropping it: - The list row shows the typed spelling, with the stored form beneath it in small monospace whenever the two differ. - The detail screen titles on the typed spelling and adds a "Stored as" row plus a one-line explanation of why they differ. - Contender rows show each requester's own spelling, falling back to the truncated identity id when their document could not be decoded. - When contenders typed *different* spellings that normalize to the same value — the situation a contest exists to resolve — all of them are shown joined by "or", rather than arbitrarily picking one. Search now matches both forms, so typing "pizza" finds the contest whether the row reads "pizza" or "p1zza". Nothing is reverse-engineered from the normalized label: `0`→`o` and `1`→`i`/`l` are ambiguous, so a contest with no decodable contender document keeps showing the normalized form rather than a guessed one. Requires dashpay/platform#4331 (adds `label` to `DashSDKContender`, so the xcframework must be rebuilt). Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Why
DPNS keys a contest by the homograph-normalized label, so any UI that renders what Platform returns shows
p1zzawhere the people involved typedpizza. That reads as a typo to the very users whose username is being voted on.It cannot be fixed client-side: normalization maps
o→0andi/l→1, which is lossy and ambiguous, so a client that tried to reverse it would invent a name nobody asked for.The spelling was already on the wire the whole time. Each contender's
domaindocument carries alabelfield, and both contested-resource queries were handing that document back without reading it — as opaque hex in the generic vote-state JSON, and not at all in the DPNS list.What
dash_sdk_contested_resource_get_vote_statedecodes each contender viatry_to_contenderagainst the queried contract's document type (kept generic, not DPNS-pinned) and adds"label"to each contender object.DashSDKContendergains alabelC string, populated indash_sdk_dpns_get_contested_non_resolved_usernamesanddash_sdk_dpns_get_contested_usernames_by_identityfrom the baked-in DPNS system contract (load_system_data_contract) — a local load, no extra network round trip. Freed alongsideidentity_idindash_sdk_contested_names_list_free.DPNSContender.displayLabel, plusDPNSContest.requestedLabels(de-duplicated, contender order) for the case where contenders typed different spellings that normalize to the same value — which is precisely what a contest is.Honesty of the fallback
Decoding is best-effort at every step. A contract fetch that fails, or a single document that will not decode, leaves
labelnull for that row rather than failing the query; callers fall back to the normalized label or the identity id. Nothing is ever synthesized from the normalized form.Breaking change
DashSDKContendergains a field, so the generated C header changes and consumers must rebuild the xcframework. Done and verified here for both slices (ios-arm64,ios-arm64-simulator).Testing
cargo check -p rs-sdk-fficlean.SwiftDashSDKTests/DPNSContestDecoderTests: 24/24 pass against the rebuilt framework — 5 new cases covering a decoded label, a missing label (nil, not empty), an empty label treated as absent,requestedLabelsde-duplication and ordering, and the all-undecodable case asserting callers get an empty list to fall back from rather than a guess.🤖 Generated with Claude Code
Summary by CodeRabbit
requestedLabelsto provide unique contender labels in voting order.