refactor: address claims accessor review follow-ups - #492
Open
SeanROlszewski wants to merge 1 commit into
Open
Conversation
Trims the claims() and fetch_credential doc comments to their summary lines, and removes claims_hex() so callers make a deliberate per-claim selection instead of grabbing a pre-encoded full vector. claims() keeps Vec<Arc<FieldElement>>: uniffi objects only implement Lower behind an Arc, so sequences of objects must be Vec<Arc<T>> — dropping the Arc fails scaffolding compilation. A code comment now records that.
SeanROlszewski
requested review from
Dzejkop,
Guardiola31337,
Takaros999,
danielle-tfh,
kilianglas,
murph,
paolodamico and
philsippl
as code owners
August 31, 2026 21:29
paolodamico
approved these changes
Aug 31, 2026
| /// serialize — it is an accessor, not a disclosure mechanism; whether and | ||
| /// which claims leave the device is entirely the host app's policy. | ||
| // | ||
| // `Arc` is load-bearing: uniffi objects only implement `Lower` behind an |
Contributor
There was a problem hiding this comment.
no need to have this. removing the Arc will immediately raise a compilation error to anyone confused like me
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the four review comments on #490:
claims()doc comment — reduced to the summary line.Arcinclaims() -> Vec<Arc<FieldElement>>— investigated and it's load-bearing: uniffi objects only implementLowerbehind anArc, so sequences of objects must beVec<Arc<T>>. Dropping it fails scaffolding compilation withthe trait bound 'FieldElement: uniffi::Lower<UniFfiTag>' is not satisfied(bare owned objects are accepted only as top-level return values, which is whysub()gets away without one). Left a code comment so the next reader doesn't retry it.claims_hex()— gone; callers now readclaims()and encode the specific elements they need, making per-claim selection the deliberate act.fetch_credentialdoc comment — reduced to the summary +# Errors(kept formissing_errors_doc).Testing:
cargo test -p walletkit-core --libgreen (tests updated for the removed method); clippy (pedantic) clean.Note
Low Risk
API surface and documentation cleanup only; behavior of claim access is unchanged aside from removing the convenience hex helper.
Overview
Follow-up on credential claims API review:
claims_hex()is removed so FFI callers useclaims()and encode only the field elements they need (e.g.to_hex_string()), instead of a bulk hex helper.claims()keeps returningVec<Arc<FieldElement>>; the long disclosure-oriented rustdoc is replaced with a short summary plus an inline note thatArcis required for UniFFI sequences of objects.fetch_credentialrustdoc is shortened (no tie-in to proof generation /claims_hex). Tests are updated accordingly.Reviewed by Cursor Bugbot for commit 700c632. Bugbot is set up for automated code reviews on this repo. Configure here.