Repository navigation
fix: check a DashPay invitation claim against Platform before broadcasting it - #5536
romchornyi wants to merge 7 commits into
Conversation
…re broadcasting it The claim used to broadcast blindly. When a submitted key was already registered to another identity, Platform refused it with 40207/10501 and charged the voucher on every attempt. Hosts also could not tell their own landed claim from someone else's. - rs-sdk: before signing, look up the identity at the invitation's id and under every submitted key hash, and return ClaimedByThisWallet / AlreadyClaimed / KeysAlreadyRegistered / NotReady without broadcasting. Map 40207/10501 after a broadcast to KeysAlreadyRegistered, and classify a consumed-outpoint refusal by whose identity is there. One shared identity_holds_master_key predicate. - rs-platform-wallet: typed error variants; invitation_claim_status_for_identity with a claimed_by_this_wallet tri-state, resolving the master key only when the invitation was claimed; an identity the wallet already records counts as its own. - FFI: codes 63-66 (JSON detail on 63/64), AlreadyClaimed -> 24, and platform_wallet_invitation_claim_status_for_identity. - Swift/Kotlin mirrors, example-app message for 64, parity manifest gap for the missing Kotlin status binding. Refs dashpay/dashwallet-ios#1224 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit da374fc) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The PR's preflight and typed error plumbing are generally sound, and the available Rust, Swift, Kotlin, and relevant JavaScript checks pass; PR Hygiene is still pending. One blocking classification bug remains for valid BLS master keys, while the mixed-key precedence, consensus-error preservation, lock scope, and protocol-version comments need correction.
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The substantial changes to Sdk::claim_invitation in packages/rs-sdk/src/platform/dashpay/invitation.rs alter key-ownership validation and the decision to broadcast a voucher-consuming identity creation, directly affecting key handling and funds movement. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/src/platform/dashpay/invitation.rs`:
- [BLOCKING] packages/rs-sdk/src/platform/dashpay/invitation.rs:1077-1081: Recognize landed claims made with a BLS master key
`preflight_keys_map` accepts any MASTER/AUTHENTICATION key, and identity-create signing and Platform validation support BLS12_381 keys with proof of possession. However, `identity_holds_master_key` returns false unless the submitted master key is ECDSA_SECP256K1. If a valid BLS-backed claim lands and its result is lost, a retry finds the invitation identity but classifies it as `AlreadyClaimed` instead of `ClaimedByThisWallet`; the same predicate also prevents the wallet-aware status path from recognizing that claim. This tells the caller that another party claimed the voucher and blocks the intended recovery classification.
- [SUGGESTION] packages/rs-sdk/src/platform/dashpay/invitation.rs:1129-1140: Check all key-hash results for the invitation identity before reporting taken keys
When the lookup by invitation id is absent or fails, the loop returns the first successful key-hash identity. Because `join_all` preserves `key_hashes` order, a foreign identity found for an earlier hash can produce `KeysAlreadyRegistered` before a later hash identifies `prospective_identity_id`. The documented precedence requires the invitation identity to win regardless of submitted-key order, so retain the first foreign owner, continue scanning all results, and return the foreign-key outcome only after every successful result has been checked for the prospective identity.
- [SUGGESTION] packages/rs-sdk/src/platform/dashpay/invitation.rs:1184-1186: Correct the duplicate-key refusal version threshold
The comment says Basic error 10501 applies before protocol v7, but the version tables select validation generation V5 for protocol v7 and V6 for protocol v10, both of which still return the Basic error. Validation generation V7 is first selected by protocol v11 and changes the result to State error 40207. The same incorrect threshold appears in the `preflight_keys_map` comment near line 757 and in the PR description. The behavior is unaffected, but the documentation gives future version-gated changes the wrong activation point.
- [SUGGESTION] packages/rs-sdk/src/platform/dashpay/invitation.rs:1194-1197: Preserve the consensus rejection when mapping taken keys
`keys_taken_on_broadcast` converts the original `InvitationError::Sdk(error)` into `KeysAlreadyRegistered { identity_id: None }` and discards the consensus error. As a result, native callers lose the duplicate-key details, and the FFI conversion reports `consensus_code = 0` and `consensus_kind = None` even when Platform rejected the broadcast with 40207 or 10501. Preserve the original error as optional source metadata on this outcome and carry that metadata through the wallet and FFI conversions while retaining the typed invitation result code.
In `packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs:494-517: Do not hold the wallet-manager read guard during key derivation
`claimant_master_public_key` keeps the `wallet_manager.read()` guard alive while deriving the keypair. The derivation is synchronous CPU work and does not need the manager lock for the master-key path, so it unnecessarily extends the critical section and can delay writers such as identity recording. Extract the required network and wallet-derived inputs before derivation, or otherwise scope the manager guard to the minimum access needed; retain the lock only where the resident-wallet derivation genuinely requires the wallet object.
- [NITPICK] packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs:491-500: Remove the redundant Unavailable match arm
The early `matches!` check returns `Ok(None)` for `InvitationClaimantKey::Unavailable`, so the identical arm inside the following exhaustive match can never execute. Keep the exhaustive match arm and remove the separate early return, or remove the match arm and structure the branches so the case is handled only once.
- Recognize a landed claim whose master key is BLS: Platform signs BLS keys into an identity like ECDSA ones. - Check every key-hash result for the invitation's identity before reporting a taken key, whatever order the keys were submitted in. - Keep Platform's 40207/10501 refusal on KeysAlreadyRegistered and stamp its consensus code on FFI code 63. - Classify an unverifiable claim-status lookup as NotReady, as the claim does. - Release the wallet storage before the claim's network round, and the wallet-manager guard before deriving from a resolved master key. - Correct the protocol version the duplicate-key refusal changed in (v11), narrow the wallet-internal claimant API to pub(crate), fix doc placement. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 7863d1f confirms that all six prior findings are fixed, with no remaining blockers identified. One nonblocking issue remains: Swift's new typed invitation error discards the consensus metadata preserved by Rust and FFI. Current CI shows successful JavaScript build/tests and Swift frozen-schema validation, while Rust workspace tests and Kotlin SDK build/tests remain pending; no local builds or tests were run.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate changes to Sdk::claim_invitation in packages/rs-sdk/src/platform/dashpay/invitation.rs alter key-ownership validation and the decision to sign and broadcast voucher-funded identity creation, directly affecting key handling and voucher expenditure. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:977-979: Preserve consensus metadata in Swift's typed invitation error
A broadcast rejection for duplicate keys now retains Platform's original error through rs-sdk and rs-platform-wallet, and FFI returns code 63 alongside consensus metadata such as 40207/State or 10501/Basic. `init(result:)` reads that metadata, but this conversion constructs a case containing only `identityId`; the public `consensusError` getter consequently returns nil. Although the existing Swift adapter deliberately exposes consensus metadata only for catch-all errors, this new typed outcome drops information explicitly preserved by the Rust implementation, and the same rejection previously reached Swift through `.consensusRejection`. Claim safety is unaffected, but Swift callers lose the authoritative rejection code for diagnostics. Retain an optional `PlatformConsensusError` alongside the typed invitation case and expose it through `consensusError`, without replacing the invitation classification. Add conversion tests for both rejection codes and a preflight outcome without consensus metadata.
…tered Code 63 refused after the broadcast now carries Platform's 40207/10501 consensus verdict; the typed Swift case keeps it beside the classification and `consensusError` returns it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at 5be60e7 and independently verified all seven prior findings as fixed. One non-blocking correctness issue remains in wallet-aware status recognition for unrecorded BLS claims; the proposed non-unique-index blocker does not establish the claimed voucher-consuming failure. This was a static review: current CI passes Rust workspace tests, Kotlin tests, JS builds/tests, and the Swift frozen-schema check; Swift build/tests remain pending.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large cross-language change that alters invitation claims involving voucher charging and identity key handling, including protocol-version-specific consensus error behavior in packages/rs-sdk/src/platform/dashpay/invitation.rs and packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs:530-537: Wallet-aware status reports BLS-owned invitation claims as not ours
Both derivation branches produce an ECDSA key, and this construction always labels it ECDSA_SECP256K1. The claim API also accepts BLS master keys, so an earlier claim made with this wallet's BLS key can land without being recorded locally. When that identity has no matching ECDSA signing key, the new wallet-aware status returns Some(false), which reaches Swift as claimedByThisWallet == false, even though retrying the claim with the original BLS keys correctly returns ClaimedByThisWallet. The shared predicate's BLS support does not fix this because it never receives a BLS candidate here. Derive and compare the supported master-key types in the Rust owner, or return None when an unsupported derivation prevents a definitive ownership answer. Add a regression test for a landed, unrecorded BLS-backed claim.
|
/skip-bots |
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
/self-reviewed |
|
Missing human approval. Automatic reviewer requests are paused and this PR is excluded from reviewers' queues. Merge, close, or draft another active PR by this author to free a review slot. Required human review can be satisfied without a slot; once all merge requirements are met, PR Hygiene passes. |
github-actions
Bot
added
too-many-open-prs
|
Reviewed at
No blockers from me. CI is green and all bot threads are resolved. Non-blocking:
A trade-off to be aware of, not a request: the preflight fails closed. A lookup failure that is not a quorum error ( |
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
LGTM. Non-blocking notes in #5536 (comment)
…on code 64 - Test `Sdk::claim_invitation` against a mock SDK: our identity found by id and a taken key found by the last key's hash both end the claim before signing; with nothing registered it reaches the signer. `MockDashPlatformSdk::expect_request` (test-only) answers the Core funding-transaction request. - `adopt_already_landed_claim` takes the master key from its caller, dropping an arm that could not be reached. - Both example apps continue with the identity a code 64 names: the one the wallet records, or the one loaded from Platform when its id matches; otherwise they show its id and point to the re-scan screen. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Addressed in b96690e:
On fail-closed: agreed, that is intended, given what the voucher costs. |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The invitation preflight and typed error propagation are correctly wired, but the final host recovery paths still mishandle recoverable identity state. Code 64 recovery collapses identity-load failures into “not listed,” and code 63 is not recovered when the colliding identity belongs to this wallet; the wallet-aware status API also still misclassifies unrecorded BLS-backed claims. The Rust workspace failure appears to be unrelated base drift, while one browser check remains in progress.
🔴 1 blocking | 🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to invitation claiming that affects voucher-funded identity creation, key registration and cryptographic key handling across SDK, wallet, FFI, Swift, and Kotlin boundaries, specifically in packages/rs-sdk/src/platform/dashpay/invitation.rs and packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ClaimInvitationSheet.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ClaimInvitationSheet.swift:238-245: Do not collapse failed identity recovery into 'not listed'
`loadIdentity(atIndex:)` returns an optional identity on success but throws when Keychain access, DAPI/quorum lookup, or persistence fails. `try?` collapses those failures with a successful `nil`, so a code-64 recovery failure is presented as though the identity were absent and the user is directed to rescan. Kotlin has the same defect: `runCatching { discoverIdentities(...) }.getOrNull()` turns discovery errors into an empty result. Preserve the error and show the “not listed” message only after a successful lookup that finds no matching identity; otherwise surface a retryable failure. Unknown wallet state must not become definitive absence.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ClaimInvitationSheet.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ClaimInvitationSheet.kt:175-191: Handle code 63 when the collided key belongs to this wallet
The code-63 result includes the identity holding a submitted key when Platform refuses the claim. If that identity was created by this wallet at the candidate index but is missing from the local `walletOwned` snapshot, both example apps handle only code 64, leave the derived keys persisted, and keep the same index as unused. The next attempt derives the same keys and receives code 63 again. For a named code-63 identity, probe or discover the candidate index and recover only when the loaded identity exactly matches the error's identity; retain the typed error for a foreign owner, an absent identity, or an unknown lookup failure. The Swift example has the same omission.
In `packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs:495-539: Wallet-aware status reports BLS-owned invitation claims as not ours
(existing thread: https://github.com/dashpay/platform/pull/5536#discussion_r4349701624)
`identity_holds_master_key` correctly recognizes both ECDSA and BLS keys, and the SDK claim path can therefore classify a landed BLS-backed claim as belonging to this wallet. However, `claimant_master_public_key` always derives a 33-byte secp256k1 public key and labels it `ECDSA_SECP256K1`. If a BLS-backed claim is made and local recording is lost, `invitation_claim_status_for_identity` compares the claimed identity against the wrong key and returns `Some(false)`, which crosses the FFI as a definitive “not this wallet” result. Derive and compare the claimant's supported key type in the Rust wallet layer, or return `None` when the key type cannot be checked instead of reporting a definitive false result. The limitation is tracked by #5543, but this status API introduced the incorrect classification.
| } else if let loaded = try? await wallet.loadIdentity(atIndex: identityIndex), | ||
| loaded.toBase58String() == claimedId { | ||
| newIdentityId = loaded | ||
| } else { | ||
| errorMessage = "This invitation already created identity \(claimedId) " | ||
| + "in this wallet. If it is not listed, try Re-scan for Identities." | ||
| return | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Do not collapse failed identity recovery into 'not listed'
loadIdentity(atIndex:) returns an optional identity on success but throws when Keychain access, DAPI/quorum lookup, or persistence fails. try? collapses those failures with a successful nil, so a code-64 recovery failure is presented as though the identity were absent and the user is directed to rescan. Kotlin has the same defect: runCatching { discoverIdentities(...) }.getOrNull() turns discovery errors into an empty result. Preserve the error and show the “not listed” message only after a successful lookup that finds no matching identity; otherwise surface a retryable failure. Unknown wallet state must not become definitive absence.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
There was a problem hiding this comment.
Resolved (re-reviewed at a51289bc): The Swift recovery path now distinguishes a failed identity load from an absent or mismatched identity and reports the load failure as retryable instead of treating it as 'not listed'.
There was a problem hiding this comment.
Fixed in a51289b. A failed load no longer reads as "not listed". In Swift, loadIdentity(atIndex:) errors are reported as a failure to retry, on code 64 and on code 63 alike. In Kotlin, the discovery walk is replaced by a new bridge to platform_wallet_load_identity_at_index, so both apps probe exactly the claim's index and surface its errors the same way. "Not listed" is shown only after a load that succeeded without finding the named identity.
There was a problem hiding this comment.
The a51289b changes address this: both recovery paths distinguish a failed loadIdentity(atIndex:) from a successful lookup that found no identity. The rescan/not-listed message is now reached only after a successful absent or mismatched result, while load failures remain retryable.
The finding stands on this commit until that change is pushed; the push gets a full review.
| } catch (e: DashSdkError.PlatformWallet.InvitationClaimedByThisWallet) { | ||
| // The invitation's identity is this wallet's: one it | ||
| // records, or one an earlier claim created without | ||
| // recording it, which discovery loads like any | ||
| // identity of this wallet. Only the identity the | ||
| // error names counts. | ||
| val claimedId = Base58.decodeIdentifier(e.identityId) ?: throw e | ||
| walletOwned.firstOrNull { it.identityId.contentEquals(claimedId) } | ||
| ?.identityId | ||
| ?: runCatching { | ||
| mgr.identityRegistration.discoverIdentities( | ||
| walletHandle = wallet.handle, | ||
| mnemonicResolverHandle = mgr.mnemonicResolverHandle, | ||
| ) | ||
| }.getOrNull() | ||
| ?.firstOrNull { it.contentEquals(claimedId) } | ||
| ?: throw e |
There was a problem hiding this comment.
🟡 Suggestion: Handle code 63 when the collided key belongs to this wallet
The code-63 result includes the identity holding a submitted key when Platform refuses the claim. If that identity was created by this wallet at the candidate index but is missing from the local walletOwned snapshot, both example apps handle only code 64, leave the derived keys persisted, and keep the same index as unused. The next attempt derives the same keys and receives code 63 again. For a named code-63 identity, probe or discover the candidate index and recover only when the loaded identity exactly matches the error's identity; retain the typed error for a foreign owner, an absent identity, or an unknown lookup failure. The Swift example has the same omission.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at a51289bc): The Kotlin sheet now loads the identity at the claimed index on code 63 and accepts it only when the loaded identifier matches the owner named by Platform, preventing the repeated collision loop.
There was a problem hiding this comment.
Done in a51289b. On code 63 with a named owner, both apps load the identity at the claim's index. If it is the owner, it is now recorded, and the user is told to claim again, which takes the next index. A foreign or absent owner keeps the typed refusal. A failed load is reported as a failure to retry, not as a refusal.
There was a problem hiding this comment.
The a51289b recovery now handles code 63 in both example apps: it loads the candidate index, adopts the identity only when its ID matches Platform's named owner, and preserves the typed refusal for foreign, absent, or failed lookups.
The finding stands on this commit until that change is pushed; the push gets a full review.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
…n the claim sheets On code 64 a failed load of the claimed identity is now reported as a failure to retry instead of "not listed". On code 63 both example apps load the identity at the claim's index; when it is the key owner, it is recorded and the user claims again at the next index. Kotlin gains a bridge to platform_wallet_load_identity_at_index, which the Swift app already calls, so both apps probe exactly the claim's index. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The claim preflight, recovery handling, and consensus-error propagation are correctly implemented, and the previously reported claim-safety issues are fixed or tracked. However, the change enlarges the existing C ABI output structure without size/version negotiation, so an updated native library can write beyond an older host's allocation; this requires an ABI-compatible V2 entry point or bounded versioned output. Additional non-blocking issues affect negative JNI indices, multi-wallet identity shortcuts, and public enum API versioning. Rust workspace tests and one browser suite were still running at the captured CI snapshot.
🔴 1 blocking | 🟡 4 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to invitation claiming and identity/key handling that directly affects voucher-funded asset-lock spending, claim broadcasting, cryptographic key matching, and consensus-error interpretation in packages/rs-sdk/src/platform/dashpay/invitation.rs and related wallet/FFI layers. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/src/invitation.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/invitation.rs:451-472: Version the invitation claim status ABI before adding a field
`InvitationClaimStatusFFI` is a `#[repr(C)]` structure returned through the existing `platform_wallet_invitation_claim_status` function, whose out-parameter carries no size or ABI version. Adding `claimed_by_this_wallet` means a new native producer can write past the allocation of an older host compiled against the previous header, while an old producer paired with a new host cannot provide the field reliably. The PR's instruction that hosts must rebuild does not protect independently versioned binaries. Preserve the existing structure and function and add a V2 structure/entry point, or introduce a size/version-tagged output parameter that bounds writes. The repository already uses this pattern for `platform_wallet_manager_list_masternodes_v2`.
In `packages/rs-unified-sdk-jni/src/identity.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/identity.rs:942-950: Reject negative identity indices before narrowing to u32
`loadIdentityAtIndex` accepts a signed JNI `jint` and casts it directly to `u32`, so `-1` becomes `4_294_967_295` and probes an unrelated DIP-9 slot instead of reporting invalid input. The Kotlin wrapper also accepts a signed `Int` without the non-negative guard already used by sibling registration APIs. Reject the value in the JNI bridge before the cast and mirror that validation in `IdentityRegistration.loadIdentityAtIndex`.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ClaimInvitationSheet.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ClaimInvitationSheet.swift:231-237: Scope the recorded-identity shortcut to the selected wallet
The `identities` query is filtered only by network, but `PersistentIdentity.wallet` is the ownership source of truth and the sheet already knows the selected `walletId`. On code 64, an identity row belonging to another wallet—or a merely observed identity—can therefore be accepted as the selected wallet's recorded identity, bypassing `loadIdentity(atIndex:)`. The selected Rust wallet then remains unrecovered and later operations can fail or repeat code 64. Filter the shortcut by the selected wallet relationship, or defer the decision to the Rust wallet load path.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ClaimInvitationSheet.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ClaimInvitationSheet.kt:176-184: Scope the Kotlin recorded-identity shortcut to the selected wallet
`walletOwned` comes from `observeWalletOwnedByNetwork` and includes identities associated with every loaded wallet on that network. The code-64 shortcut checks only the identity bytes, so an identity belonging to another wallet can be accepted for `claimWallet`, bypassing `identityAt` and leaving the selected Rust wallet without the recovered identity. Match the identity's `walletId` to the selected wallet before using the row as the shortcut.
In `packages/rs-sdk/src/platform/dashpay/invitation.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/dashpay/invitation.rs:145-210: Declare the new public invitation error variants as a breaking API
`InvitationError` is a public exported Rust enum and this change adds four variants without `#[non_exhaustive]`; `PlatformWalletError` adds corresponding public variants. Downstream users with exhaustive matches will stop compiling. The reviewed range contains no package release metadata or breaking-change commit/title marker, even though the repository uses `!` for breaking changes. Record this API break at the crate release boundary and in release notes, or use an explicitly extensible result surface.
| /// was claimed. `false` does NOT prove the voucher is unspent (see | ||
| /// [`platform_wallet_invitation_prospective_identity_id`]). | ||
| pub already_claimed: bool, | ||
| /// Whether this wallet made the claim, as | ||
| /// [`platform_wallet_invitation_claim_status_for_identity`] decides it: | ||
| /// [`INVITATION_CLAIMED_BY_THIS_WALLET_NO`] (also whenever | ||
| /// `already_claimed` is false), [`INVITATION_CLAIMED_BY_THIS_WALLET_YES`], | ||
| /// or [`INVITATION_CLAIMED_BY_THIS_WALLET_UNKNOWN`] (the wallet's master | ||
| /// key could not be derived). [`platform_wallet_invitation_claim_status`] | ||
| /// does not decide it and reports `UNKNOWN` for a claimed invitation. | ||
| pub claimed_by_this_wallet: u8, |
There was a problem hiding this comment.
✅ Withdrawn at
da374fc0; see the replies below.
🔴 Blocking: Version the invitation claim status ABI before adding a field
InvitationClaimStatusFFI is a #[repr(C)] structure returned through the existing platform_wallet_invitation_claim_status function, whose out-parameter carries no size or ABI version. Adding claimed_by_this_wallet means a new native producer can write past the allocation of an older host compiled against the previous header, while an old producer paired with a new host cannot provide the field reliably. The PR's instruction that hosts must rebuild does not protect independently versioned binaries. Preserve the existing structure and function and add a V2 structure/entry point, or introduce a size/version-tagged output parameter that bounds writes. The repository already uses this pattern for platform_wallet_manager_list_masternodes_v2.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Not adding a V2 here, because the old layout never shipped. InvitationClaimStatusFFI and platform_wallet_invitation_claim_status came in with #4997 on v5.1-dev. No release tag carries them: git show v5.0.0-rc.8:packages/rs-platform-wallet-ffi/src/invitation.rs has neither, and neither does v5.0.0 or v5.0-dev. So no independently versioned binary was built against the struct without claimed_by_this_wallet. platform_wallet_manager_list_masternodes_v2 versions a function that had already shipped. Added this to Breaking Changes.
There was a problem hiding this comment.
Given that the invitation status structure and entry point were introduced after the latest released v5.0 artifacts and are absent from the cited release tags, this is not an old-host/new-producer compatibility break. Recording the layout change under Breaking Changes is sufficient here, so I withdraw this finding.
Withdrawing this finding.
| guard(&mut env, ptr::null_mut(), |env| { | ||
| let mut found = false; | ||
| let mut id = [0u8; 32]; | ||
| let result = unsafe { | ||
| platform_wallet_ffi::platform_wallet_load_identity_at_index( | ||
| wallet_handle as Handle, | ||
| resolver_handle as *mut MnemonicResolverHandle, | ||
| identity_index as u32, | ||
| &mut found as *mut bool, |
There was a problem hiding this comment.
🟡 Suggestion: Reject negative identity indices before narrowing to u32
loadIdentityAtIndex accepts a signed JNI jint and casts it directly to u32, so -1 becomes 4_294_967_295 and probes an unrelated DIP-9 slot instead of reporting invalid input. The Kotlin wrapper also accepts a signed Int without the non-negative guard already used by sibling registration APIs. Reject the value in the JNI bridge before the cast and mirror that validation in IdentityRegistration.loadIdentityAtIndex.
| guard(&mut env, ptr::null_mut(), |env| { | |
| let mut found = false; | |
| let mut id = [0u8; 32]; | |
| let result = unsafe { | |
| platform_wallet_ffi::platform_wallet_load_identity_at_index( | |
| wallet_handle as Handle, | |
| resolver_handle as *mut MnemonicResolverHandle, | |
| identity_index as u32, | |
| &mut found as *mut bool, | |
| guard(&mut env, ptr::null_mut(), |env| { | |
| if identity_index < 0 { | |
| throw_sdk_exception(env, 1, "identityIndex must be non-negative"); | |
| return ptr::null_mut(); | |
| } | |
| let mut found = false; | |
| let mut id = [0u8; 32]; | |
| let result = unsafe { | |
| platform_wallet_ffi::platform_wallet_load_identity_at_index( | |
| wallet_handle as Handle, | |
| resolver_handle as *mut MnemonicResolverHandle, | |
| identity_index as u32, | |
| &mut found as *mut bool, | |
| id.as_mut_ptr(), | |
| ) | |
| }; |
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in da374fc. The JNI bridge rejects a negative identityIndex before the cast, and IdentityRegistration.loadIdentityAtIndex checks it the same way as its sibling registration APIs.
There was a problem hiding this comment.
The da374fc changes reject negative indices in both the JNI bridge and IdentityRegistration.loadIdentityAtIndex before any narrowing or lookup, matching the existing validation used by sibling registration APIs.
The finding stands on this commit until that change is pushed; the push gets a full review.
| } catch PlatformWalletError.invitationClaimedByThisWallet(let claimedId) { | ||
| // The invitation's identity is this wallet's: one it | ||
| // records, or one an earlier claim at this index created | ||
| // without recording it, which is loaded like any identity | ||
| // of this wallet. Only the identity the error names counts. | ||
| if let recorded = identities.first(where: { $0.identityIdBase58 == claimedId }) { | ||
| newIdentityId = recorded.identityId |
There was a problem hiding this comment.
🟡 Suggestion: Scope the recorded-identity shortcut to the selected wallet
The identities query is filtered only by network, but PersistentIdentity.wallet is the ownership source of truth and the sheet already knows the selected walletId. On code 64, an identity row belonging to another wallet—or a merely observed identity—can therefore be accepted as the selected wallet's recorded identity, bypassing loadIdentity(atIndex:). The selected Rust wallet then remains unrecovered and later operations can fail or repeat code 64. Filter the shortcut by the selected wallet relationship, or defer the decision to the Rust wallet load path.
| } catch PlatformWalletError.invitationClaimedByThisWallet(let claimedId) { | |
| // The invitation's identity is this wallet's: one it | |
| // records, or one an earlier claim at this index created | |
| // without recording it, which is loaded like any identity | |
| // of this wallet. Only the identity the error names counts. | |
| if let recorded = identities.first(where: { $0.identityIdBase58 == claimedId }) { | |
| newIdentityId = recorded.identityId | |
| if let recorded = identities.first(where: { | |
| $0.identityIdBase58 == claimedId && $0.wallet?.walletId == walletId | |
| }) { | |
| newIdentityId = recorded.identityId | |
| } else { |
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
There was a problem hiding this comment.
Fixed in da374fc. The recorded-identity shortcut now also requires wallet?.walletId == walletId. Otherwise the sheet goes through loadIdentity(atIndex:) on the selected wallet.
There was a problem hiding this comment.
The da374fc change now requires the recorded Swift identity to belong to the selected wallet before using it as the code-64 shortcut. Identities from another wallet fall through to the selected wallet's load path.
The finding stands on this commit until that change is pushed; the push gets a full review.
| } catch (e: DashSdkError.PlatformWallet.InvitationClaimedByThisWallet) { | ||
| // The invitation's identity is this wallet's: one it | ||
| // records, or one an earlier claim at this index | ||
| // created without recording it, which is loaded like | ||
| // any identity of this wallet. Only the identity the | ||
| // error names counts. | ||
| val claimedId = Base58.decodeIdentifier(e.identityId) ?: throw e | ||
| walletOwned.firstOrNull { it.identityId.contentEquals(claimedId) } | ||
| ?.identityId |
There was a problem hiding this comment.
🟡 Suggestion: Scope the Kotlin recorded-identity shortcut to the selected wallet
walletOwned comes from observeWalletOwnedByNetwork and includes identities associated with every loaded wallet on that network. The code-64 shortcut checks only the identity bytes, so an identity belonging to another wallet can be accepted for claimWallet, bypassing identityAt and leaving the selected Rust wallet without the recovered identity. Match the identity's walletId to the selected wallet before using the row as the shortcut.
| } catch (e: DashSdkError.PlatformWallet.InvitationClaimedByThisWallet) { | |
| // The invitation's identity is this wallet's: one it | |
| // records, or one an earlier claim at this index | |
| // created without recording it, which is loaded like | |
| // any identity of this wallet. Only the identity the | |
| // error names counts. | |
| val claimedId = Base58.decodeIdentifier(e.identityId) ?: throw e | |
| walletOwned.firstOrNull { it.identityId.contentEquals(claimedId) } | |
| ?.identityId | |
| walletOwned.firstOrNull { | |
| it.walletId?.contentEquals(wallet.walletId) == true && | |
| it.identityId.contentEquals(claimedId) | |
| }?.identityId |
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
There was a problem hiding this comment.
Fixed in da374fc. The shortcut now matches the row's walletId against the selected wallet before using it.
There was a problem hiding this comment.
The da374fc change applies the same wallet relationship check in Kotlin: the recorded shortcut requires both the claimed identity and the selected wallet ID to match.
The finding stands on this commit until that change is pushed; the push gets a full review.
| #[error("{0}")] | ||
| NotChainLocked(String), | ||
|
|
||
| /// The invitation's identity already exists and holds the claimant's | ||
| /// master key: an earlier claim of the claimant landed (for example one | ||
| /// whose result was lost when the app stopped). Usually found before the | ||
| /// broadcast; Platform's consumed-outpoint refusal also reports it when a | ||
| /// node that lagged behind let the claim through and the identity's keys | ||
| /// are not exactly the submitted ones. | ||
| /// The claim does not take the identity over; the caller loads it the | ||
| /// way it loads any identity of its own. | ||
| #[error("invitation was already claimed by this claimant as identity {identity_id}")] | ||
| ClaimedByThisWallet { identity_id: Identifier }, | ||
|
|
||
| /// Someone else claimed the invitation: an identity exists at its id and | ||
| /// does not hold the claimant's master key. Usually found before the | ||
| /// broadcast, else from Platform's consumed-outpoint refusal. | ||
| #[error("invitation was already claimed by identity {identity_id}")] | ||
| AlreadyClaimed { identity_id: Identifier }, | ||
|
|
||
| /// A submitted key is already registered to another identity, so | ||
| /// Platform would refuse the claim and charge the voucher for it. | ||
| /// `identity_id` is that identity when the claim's own lookup found it, | ||
| /// and `None` when the refusal came from Platform after the broadcast | ||
| /// (a node that had not seen the identity yet answered the lookup). | ||
| /// `rejection` is then Platform's refusal itself (consensus error 40207, | ||
| /// or 10501 before protocol v11), kept for its consensus code. | ||
| #[error( | ||
| "a key of this claim is already registered to {}", | ||
| key_owner(identity_id) | ||
| )] | ||
| KeysAlreadyRegistered { | ||
| identity_id: Option<Identifier>, | ||
| #[source] | ||
| rejection: Option<Error>, | ||
| }, | ||
|
|
||
| /// The claim's identity lookups could not be verified yet: the trusted | ||
| /// quorum source is not ready (see [`Error::is_quorum_source_unavailable`]). | ||
| /// Nothing was broadcast; retry once the wallet has synced. | ||
| #[error("cannot verify Platform's answer yet, retry once synced: {0}")] | ||
| NotReady(#[source] Error), | ||
|
|
||
| /// An SDK call failed. | ||
| #[error(transparent)] | ||
| Sdk(#[from] Error), |
There was a problem hiding this comment.
✅ Withdrawn at
da374fc0; see the replies below.
🟡 Suggestion: Declare the new public invitation error variants as a breaking API
InvitationError is a public exported Rust enum and this change adds four variants without #[non_exhaustive]; PlatformWalletError adds corresponding public variants. Downstream users with exhaustive matches will stop compiling. The reviewed range contains no package release metadata or breaking-change commit/title marker, even though the repository uses ! for breaking changes. Record this API break at the crate release boundary and in release notes, or use an explicitly extensible result surface.
source: gpt-6.1-sol (phase2-reviewer: platform-versioning)
There was a problem hiding this comment.
InvitationError and the invitation variants of PlatformWalletError are unreleased: packages/rs-sdk/src/platform/dashpay/invitation.rs doesn't exist at v5.0.0-rc.8, and that tag's rs-platform-wallet/src/error.rs has no invitation variants. So no published version is broken. The variants are listed under Breaking Changes, and the release notes for the version that first ships them will cover them.
There was a problem hiding this comment.
Not changing the title to fix!. The ! marks a break against a released version, and nothing here has shipped: InvitationError and the invitation variants of PlatformWalletError don't exist at v5.0.0-rc.8, and the claim behaviour this PR changes came in with #4997, which no tag contains either. The first release that ships these types ships them already in this form. Breaking Changes in the PR body lists them so the release notes can pick them up.
There was a problem hiding this comment.
I agree that these enum variants are unreleased and therefore do not break an already published API. Listing them under Breaking Changes for the release that first ships these types is sufficient; changing the title to fix! is not required for this case, so I withdraw the finding.
Withdrawing this finding.
…e claim shortcut to its wallet The JNI bridge and the Kotlin wrapper refuse a negative identity index instead of probing the slot the u32 cast lands on. On code 64 both example apps take a recorded identity as the claimed one only when it is recorded under the selected wallet; otherwise they load it from that wallet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓, 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Updated after discussion: 2 finding(s) of this review were settled in the inline threads.
- ✅ Withdrawn: 🟡 Do not classify uncompressed ECDSA claims as definitively not owned
- ✅ Withdrawn: 🟡 Declare the new public invitation error variants as a breaking API
The invitation preflight, consensus-error propagation, wallet-scoped recovery, and negative-index fixes are correct at this head. Three non-blocking issues remain: the public API break is not marked in the PR title, the new C status entry point can abort the host on a Rust panic, and wallet-aware status cannot distinguish uncompressed ECDSA claims. Rust tests and several build jobs are still in progress; no completed code check is failing.
🟡 3 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate cross-language change that alters invitation claim authorization, key handling, Platform preflight behavior, and voucher-spending failure paths in rs-sdk and wallet/FFI layers. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/src/invitation.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/invitation.rs:593-648: Guard the new invitation-status C entry point against Rust panics
The new `platform_wallet_invitation_claim_status_for_identity` export calls `block_on_worker`, whose `JoinHandle` handling uses `expect("tokio worker panicked")`. A panic in the status future or resolver path therefore panics inside the `extern "C"` function and can abort an unwinding host instead of returning a `PlatformWalletFFIResult`. The JNI panic guard cannot protect the Swift caller because it is on the far side of this C boundary. Wrap the exported body with the crate's panic-to-result mechanism and retain the already-zeroed out-parameter when a panic is converted to an error.
In `packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs`:
- [SUGGESTION → WITHDRAWN in discussion; do not fix] packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs:495-539: Do not classify uncompressed ECDSA claims as definitively not owned
`claimant_master_public_key` always derives the ECDSA candidate as a 33-byte compressed key and `identity_holds_master_key` compares key data byte-for-byte. The claim API accepts the submitted `IdentityPublicKey` supplied by the caller, so a valid claim whose master authentication key is the same ECDSA point in uncompressed 65-byte encoding can land successfully, but wallet-aware status returns `Some(false)` because it compares only the compressed representation. That makes the status API report the wallet's own claim as another party's claim. Compare the supported ECDSA encodings by public-key point, derive and check both representations, or return `None` when the available derivation cannot establish ownership. The separate BLS variant of this ownership gap remains tracked by #5543.
In `packages/rs-sdk/src/platform/dashpay/invitation.rs`:
- [SUGGESTION → WITHDRAWN in discussion; do not fix] packages/rs-sdk/src/platform/dashpay/invitation.rs:145-210: Declare the new public invitation error variants as a breaking API
(existing thread: https://github.com/dashpay/platform/pull/5536#discussion_r4376363050)
The PR body now documents these variants under Breaking Changes, but the PR title remains `fix:` even though the repository conventions require `!` for breaking changes. `InvitationError` and `PlatformWalletError` gain non-`non_exhaustive` variants, so downstream Rust callers with exhaustive matches will fail to compile when the first release containing these APIs ships. Mark the release-boundary change explicitly, for example by changing the title to `fix!:` and ensuring the release metadata carries the same breaking-change marker.
| pub unsafe extern "C" fn platform_wallet_invitation_claim_status_for_identity( | ||
| wallet_handle: Handle, | ||
| mnemonic_resolver_handle: *mut MnemonicResolverHandle, | ||
| identity_index: u32, | ||
| uri: *const c_char, | ||
| out_status: *mut InvitationClaimStatusFFI, | ||
| ) -> PlatformWalletFFIResult { | ||
| check_ptr!(out_status); | ||
| unsafe { | ||
| *out_status = InvitationClaimStatusFFI::zeroed(); | ||
| } | ||
| check_ptr!(uri); | ||
|
|
||
| let uri = unwrap_result_or_return!(unsafe { CStr::from_ptr(uri) }.to_str()); | ||
| let invitation = match parse_invitation_for_query(uri) { | ||
| Ok(invitation) => invitation, | ||
| Err(e) => return e, | ||
| }; | ||
|
|
||
| let resolver_addr = mnemonic_resolver_handle as usize; | ||
| // Take what the call needs and release the wallet storage before the | ||
| // network calls and the resolver callback run. | ||
| let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { | ||
| ( | ||
| wallet.identity().clone(), | ||
| wallet.wallet_id(), | ||
| wallet.network(), | ||
| ) | ||
| }); | ||
| let (identity_wallet, wallet_id, network) = unwrap_option_or_return!(option); | ||
| let result = block_on_worker(async move { | ||
| identity_wallet | ||
| .invitation_claim_status_for_identity(&invitation, identity_index, || { | ||
| let resolver = resolver_addr as *mut MnemonicResolverHandle; | ||
| if resolver.is_null() { | ||
| return None; | ||
| } | ||
| // SAFETY: the caller keeps the resolver valid for the | ||
| // whole call, which blocks until this future completes. | ||
| match unsafe { resolve_master_from_resolver(resolver, &wallet_id, network) } { | ||
| Ok(master) => Some(master), | ||
| Err(failure) => { | ||
| tracing::warn!( | ||
| code = ?failure.code, | ||
| "could not resolve the wallet's mnemonic to tell whose \ | ||
| invitation claim it was" | ||
| ); | ||
| None | ||
| } | ||
| } | ||
| }) | ||
| .await | ||
| }) | ||
| .map(|(status, claimed_by_this_wallet)| { | ||
| InvitationClaimStatusFFI::new(status, claimed_by_this_wallet) | ||
| }); |
There was a problem hiding this comment.
🟡 Suggestion: Guard the new invitation-status C entry point against Rust panics
The new platform_wallet_invitation_claim_status_for_identity export calls block_on_worker, whose JoinHandle handling uses expect("tokio worker panicked"). A panic in the status future or resolver path therefore panics inside the extern "C" function and can abort an unwinding host instead of returning a PlatformWalletFFIResult. The JNI panic guard cannot protect the Swift caller because it is on the far side of this C boundary. Wrap the exported body with the crate's panic-to-result mechanism and retain the already-zeroed out-parameter when a panic is converted to an error.
source: gpt-6.1-sol (phase2-reviewer: ffi-engineer)
There was a problem hiding this comment.
Leaving this export without a guard, for two reasons. The only caller is Swift. iOS builds with panic = "abort" (dev-ios / release-ios), and catch_panic_to_code documents that on iOS a panic aborts before the guard can see it, so for this caller the guard would change nothing. Kotlin doesn't bridge the export. Also, none of the other invitation exports (platform_wallet_claim_invitation, platform_wallet_invitation_claim_status, platform_wallet_invitation_prospective_identity_id) have a guard; only the shielded spend exports do. Guarding every block_on_worker export is a crate-wide change and belongs in its own PR.
There was a problem hiding this comment.
I agree that a guard cannot change the behavior of the panic = abort iOS profiles. It still matters for the host/test profiles, where panic unwinding remains enabled: block_on_worker turns a worker panic into an expect, and this new extern "C" entry point can then abort across the boundary. Since this export is introduced here, I still consider the finding valid rather than merely a crate-wide cleanup request.
There was a problem hiding this comment.
Agreed that a guard matters where unwinding is on. I tried adding one to this export and backed it out, because it can't be done properly inside this PR:
catch_panic_to_codelives inshielded_send, which compiles only with--features shielded. Importing it frominvitation.rsbreaks the crate's default build (default = []). Using it here means first moving the helper into an always-compiled module.- The panic comes from
block_on_worker's.expect("tokio worker panicked"). Under unwind, the siblingplatform_wallet_invitation_claim_status,platform_wallet_claim_invitation,platform_wallet_create_invitationandplatform_wallet_invitation_prospective_identity_idhit the same abort at theirextern "C"frames. Guarding only the new export would make the two status calls fail differently on the same panic, although this one documents its errors as those of its sibling. - For the only caller, Swift, iOS builds with
panic = "abort", so nothing changes there either way.
The fix that covers all of them is a fallible block_on_worker, or a shared guard used by every export in the crate. That deserves its own PR, so I'm keeping this one out of scope.
There was a problem hiding this comment.
I agree that a shared guard or a fallible block_on_worker is preferable to making this one export behave differently from its siblings, and that the guard must move out of the shielded-gated module. However, that scope concern does not remove the underlying issue: this newly added export can still re-panic at the extern "C" boundary in host/test builds when its worker future or resolver panics. Since the shared fix has not been pushed here, I’m keeping the suggestion open on this commit.
There was a problem hiding this comment.
This is already tracked crate-wide: #3805 covers block_on_worker's .expect re-panicking across extern "C" at every call site, and #4437 covers uncontained panics hard-aborting the host. A complete fix for #4437 sits on fix/ffi-panic-unwind-guard, from the closed #4424, and it covers the invitation exports, including this one. So I'm leaving this export to that change instead of adding a one-off guard here.
There was a problem hiding this comment.
Thanks for pointing to #3805 and #4437; they are relevant tracking for the shared FFI panic-boundary fix. They do not change the current state, though: this newly added export still re-panics at the extern "C" boundary in unwind-enabled host/test builds, and the proposed shared fix has not been included here. I’m keeping the suggestion open on this commit rather than treating a future follow-up as a fix.
| let public_key: [u8; 33] = match key { | ||
| InvitationClaimantKey::Unavailable => return Ok(None), | ||
| InvitationClaimantKey::Master(master) => { | ||
| // Only the network comes from the manager; the derivation | ||
| // runs after its read guard is released. | ||
| let network = { | ||
| let wm = self.wallet_manager.read().await; | ||
| wm.get_wallet(&self.wallet_id) | ||
| .ok_or_else(wallet_not_found)? | ||
| .network | ||
| }; | ||
| derive_ecdsa_identity_auth_keypair_from_master( | ||
| master, | ||
| network, | ||
| identity_index, | ||
| MASTER_KEY_INDEX, | ||
| )? | ||
| .public_key | ||
| } | ||
| // The resident key material lives in the wallet the manager | ||
| // holds, so this derivation runs under its read guard. | ||
| InvitationClaimantKey::ResidentWallet => { | ||
| let wm = self.wallet_manager.read().await; | ||
| let wallet = wm | ||
| .get_wallet(&self.wallet_id) | ||
| .ok_or_else(wallet_not_found)?; | ||
| let (_path, _private, public_key) = derive_identity_auth_keypair( | ||
| wallet, | ||
| wallet.network, | ||
| identity_index, | ||
| MASTER_KEY_INDEX, | ||
| )?; | ||
| public_key.to_compressed() | ||
| } | ||
| }; | ||
| Ok(Some(IdentityPublicKey::V0(IdentityPublicKeyV0 { | ||
| id: 0, | ||
| purpose: Purpose::AUTHENTICATION, | ||
| security_level: SecurityLevel::MASTER, | ||
| contract_bounds: None, | ||
| key_type: KeyType::ECDSA_SECP256K1, | ||
| read_only: false, | ||
| data: BinaryData::new(public_key.to_vec()), | ||
| disabled_at: None, | ||
| }))) |
There was a problem hiding this comment.
✅ Withdrawn at
da374fc0; see the replies below.
🟡 Suggestion: Do not classify uncompressed ECDSA claims as definitively not owned
claimant_master_public_key always derives the ECDSA candidate as a 33-byte compressed key and identity_holds_master_key compares key data byte-for-byte. The claim API accepts the submitted IdentityPublicKey supplied by the caller, so a valid claim whose master authentication key is the same ECDSA point in uncompressed 65-byte encoding can land successfully, but wallet-aware status returns Some(false) because it compares only the compressed representation. That makes the status API report the wallet's own claim as another party's claim. Compare the supported ECDSA encodings by public-key point, derive and check both representations, or return None when the available derivation cannot establish ownership. The separate BLS variant of this ownership gap remains tracked by #5543.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Leaving the byte comparison as is. Two keys with the same point but different encodings are different keys to the wallet and to Platform:
- The wallet finds its identities by the hash160 of the compressed master key (
load_identity_by_indexand discovery fetch by thatPublicKeyHash). An identity holding our master key only in 65-byte form is invisible to them. Reporting it as this wallet's would end in code 64, telling the host to load an identity it cannot find. - Platform's unique-key index hashes the encoding too, so the compressed and uncompressed forms are separate unique keys.
Our hosts never submit an uncompressed master key: claimant_master_public_key derives the 33-byte form, and the claim submits that. So Some(false) here means "not a claim this wallet can use", which matches what the wallet can load. If cross-encoding identity is ever wanted, it belongs in rs-dpp next to public_key_hash_for_key_data, and the wallet's key lookups would need it too.
There was a problem hiding this comment.
The distinction is valid: Platform and the wallet's key lookup hash the submitted key encoding, not merely the elliptic-curve point, and this wallet's claim path derives the compressed form. An identity holding only the uncompressed encoding is therefore not a claim this wallet can identify or load through its normal derivation path, so Some(false) is appropriate for this API; I withdraw the finding.
Withdrawing this finding.
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
Approving. I reviewed the rs-sdk preflight, the wallet and FFI layers, and the Swift/Kotlin mirrors.
- The preflight matches Drive.
validate_unique_identity_public_key_hashes_not_in_state(v0 and v1) hashes every submitted key against the unique-key index and refuses with 10501 or 40207. So looking up the derived id and thePublicKeyHashof every key before signing covers exactly the refusals that charge the voucher. Tests pin the precedence (the invitation's identity, then a taken key, thenNotReady, thenSdk), including the cross-node case where only a key hash finds our identity. - The slot check in
preflight_keys_mapcan't trip FFI callers, becausedecode_identity_pubkeyskeys the map byrow.key_id. - Releasing
PLATFORM_WALLET_STORAGEbefore the network calls, in the claim and in the new status export, is a good change. - The red
Rust workspace testsjob isdrive-abci …contract_structure_error_tests::should_admit_in_check_tx_a_ref_to_a_keyword_a_shorthand_stands_for. It fails the same way onv5.1-dev(run 40455165768), so it is unrelated. Nextest stopped there, and "Rust wallet tests" was skipped, so I ran the platform-wallet and platform-wallet-ffi invitation tests locally atda374fc: 22 and 21 pass.
Non-blocking:
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
Issue being fixed or feature implemented
Part of dashpay/dashwallet-ios#1224 (item 8): a DashPay invitation claim can end up on an identity the voucher did not fund, or burn the inviter's voucher for nothing.
Sdk::claim_invitation(moved into rs-sdk by #5471) broadcasts the claim without looking at Platform first. When one of the submitted keys is already registered to another identity (typically an identity this wallet registered before at the same index), Drive refuses the IdentityCreate withDuplicatedIdentityPublicKeyIdStateError(40207, or 10501 before protocol v11) and charges the voucher throughPartiallyUseAssetLockAction(10M credits plus the fee) on every attempt. The inviter pays, and the invitee gets an opaque consensus error.Hosts also cannot tell an invitation claimed by someone else from one their own earlier claim already used (a claim that landed while the app was killed). The pre-claim status only says "an identity exists".
What was done?
rs-sdk (
platform/dashpay/invitation.rs)ClaimedByThisWallet { identity_id }: the invitation's identity holds the claim's master key (key 0). The preflight does not adopt or record it.AlreadyClaimed { identity_id }: someone else's identity is there.KeysAlreadyRegistered { identity_id }: another identity holds a submitted key.NotReady: a lookup could not be verified yet (Error::is_quorum_source_unavailable). Any other lookup failure isSdk.NotReadypreferred.identity_holds_master_keyis one predicate shared by the preflight and the claim status. It is a key-type and key-data match for ECDSA and BLS keys, which nobody else can forge, because Platform requires a signature for every such key added to an identity.KeysAlreadyRegistered { identity_id: None, rejection }, which keeps Platform's refusal; the FFI reports it inconsensus_code/consensus_kindbeside code 63. Other refusals that charge the voucher (contract bounds and similar) are not mapped.adopt_already_landed_claimstill adopts an identity that carries exactly the submitted keys. Any other identity behind a consumed outpoint is now classified asClaimedByThisWalletorAlreadyClaimedinstead of the raw rejection.preflight_keys_maprefuses a key filed under another id's slot. Two keys with one id would otherwise be refused by Platform with 10501, which the backstop reads as "taken".invitation_claim_status_with_identityalso returns the claimed identity.invitation_claim_statusreturns the same status; a lookup the quorum source cannot verify yet is nowNotReady(FFI 65, previously 99), as in the claim.rs-platform-wallet
PlatformWalletErrorvariants:InvitationClaimedByThisWallet,InvitationAlreadyClaimed,InvitationKeysAlreadyRegistered,InvitationNotReady.IdentityWallet::invitation_claim_status_for_identity(invitation, identity_index, resolve_master)returns the status plusclaimed_by_this_wallet: Option<bool>:identity_indexdecides;claim_invitationreportsAlreadyClaimedfor an identity this wallet already records asInvitationClaimedByThisWallet.rs-platform-wallet-ffi
ERROR_CODE_REGISTRY.md(61/62 are held by feat(platform-wallet): spend only final coins and report funds waiting on the network #5395/fix(platform-wallet): keep the cached identity in step with the chain around updates #5487):ErrorInvitationKeysAlreadyRegistered(63) andErrorInvitationClaimedByThisWallet(64), each with a JSON detail{"identityId": ...};ErrorInvitationNotReady(65);ErrorAssetLockNotChainLocked(66), which used to flatten to 99;InvitationAlreadyClaimedmaps to 24.platform_wallet_invitation_claim_status_for_identity(wallet, resolver, identity_index, uri, out). It forwards to the wallet method and releases the wallet storage before the network calls and the resolver callback run.InvitationClaimStatusFFIgains aclaimed_by_this_wallettri-state (INVITATION_CLAIMED_BY_THIS_WALLET_NO/YES/UNKNOWN). The out-param is zeroed to UNKNOWN before any fallible work.Swift / Kotlin
PlatformWalletErrorcases andPlatformWalletResultCoderaw cases with theirinit(ffi:)arms.InvitationClaimStatus.claimedByThisWallet: Bool?. A new overloadinvitationClaimStatus(uri:identityIndex:storage:); the oldinvitationClaimStatus(uri:)is kept.PlatformWallet.InvitationKeysAlreadyRegistered/InvitationClaimedByThisWallet/InvitationNotReady/AssetLockNotChainLocked(the last two retryable).loadIdentity(atIndex:), KotlindiscoverIdentities) and accept only that id. When it cannot be loaded, they show its id and point to the identity re-scan screen.sdk-parity-manifest.json/PARITY_SUMMARY.md.How Has This Been Tested?
cargo test -p dash-sdk -p platform-wallet -p platform-wallet-ffi --all-features: all green. New tests cover:Sdk::claim_invitationagainst a mock SDK. Our identity found by id ends the claim before anything is signed. So does a taken key found by the hash of the last submitted key. With nothing registered, the claim reaches the submission;m/9'/1'/5'/0'/0'/0'/0');cargo clippy -p dash-sdk -p platform-wallet -p platform-wallet-ffi --all-features --all-targets -- -D warnings: clean../build_ios.sh --target sim(SwiftDashSDK with-warnings-as-errors, plus SwiftExampleApp).xcodebuild test -only-testing:SwiftDashSDKTests/ErrorHandlingTests: green../gradlew :sdk:testDebugUnitTest(InvitationClaimErrorTest,DashSdkErrorTest,DpnsMarketplaceErrorTest) and:app:compileDebugKotlin: green.python3 scripts/check_sdk_parity_manifest.py: OK.Breaking Changes
No consensus change.
Host-visible behaviour changes:
InvitationClaimedByThisWallet(64) and records nothing. The host loads the identity the way it loads any of its own, which also fetches its DPNS names; claim-side adoption recorded only the identity and its keys. Only a node that lags behind and lets the retry through still reaches the old adopt path.AssetLockNotChainLockednow arrives as code 66 instead of 99.InvitationClaimStatusFFIgainsclaimed_by_this_wallet: u8, soplatform_wallet_invitation_claim_statuswrites a larger struct. The struct and the function exist only on v5.1-dev (no tag up to v5.0.0-rc.8 carries them), so no released binary uses the old layout and no_v2entry point is added. Hosts rebuild against the generated header.InvitationErrorgains variants, andpreflight_keys_maprefuses a key whose id differs from its map slot.InvitationErrorand the invitation variants ofPlatformWalletErrorare also unreleased (absent from v5.0.0-rc.8).IdentityRegistration.loadIdentityAtIndex, a bridge toplatform_wallet_load_identity_at_index, mirroring SwiftloadIdentity(atIndex:).Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
da374fc/self-revieweddocs/sdk/sdk-parity-manifest.json,packages/rs-unified-sdk-jni/src/identity.rs) — QuantumExplorer or shumkovkotlin-sdk(packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ClaimInvitationSheet.kt,packages/kotlin-sdk/PARITY_SUMMARY.md,packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktand 4 more) — HashEngineeringrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md,packages/rs-platform-wallet-ffi/src/error.rs,packages/rs-platform-wallet-ffi/src/invitation.rs) — approved by llbartekllrs-platform-wallet(packages/rs-platform-wallet/src/error.rs,packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs) — approved by llbartekllrust-sdk(packages/rs-sdk/src/mock/sdk.rs,packages/rs-sdk/src/platform/dashpay/invitation.rs,packages/rs-sdk/src/platform/dashpay/mod.rs) — lklimek or shumkovswift-sdk— you own itWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.